Repository: getsops/sops (language: python)

Issue:
SOPS panics when input type is not binary but output type is and input contains a 'data' mapping key.
Sops (latest version) panics, when decrypting yaml file with explicit type specification. Same file with .yaml extension is decrypted successfully.

```
[anton@vm ]$ diff auth.yaml.enc2 auth.enc.yaml

[anton@vm ]$ sops --version
sops 3.5.0 (latest)

[anton@vm ]$ sops -d auth.enc.yaml >/dev/null ; echo $?
0

[anton@vm ]$ sops --input-type yaml auth.yaml.enc2
panic: interface conversion: interface {} is sops.TreeBranch, not string

goroutine 1 [running]:
go.mozilla.org/sops/v3/stores/json.BinaryStore.EmitPlainFile(0xc00036b100, 0x1, 0x1, 0x242, 0x1b6, 0xc000010030, 0x0, 0x0)
	/home/ajvb/gocode/src/go.mozilla.org/sops/stores/json/store.go:50 +0x156
main.editTree(0x1211f80, 0xc000370ba0, 0x1226bc0, 0x190eba8, 0x1226b00, 0x190eba8, 0xc00010e640, 0x41, 0x0, 0xc00036c120, ...)
	/home/ajvb/gocode/src/go.mozilla.org/sops/cmd/sops/edit.go:126 +0xbbe
main.edit(0x1211f80, 0xc000370ba0, 0x1226bc0, 0x190eba8, 0x1226b00, 0x190eba8, 0xc00010e640, 0x41, 0x0, 0xc00036c120, ...)
	/home/ajvb/gocode/src/go.mozilla.org/sops/cmd/sops/edit.go:105 +0x2c5
main.main.func8(0xc000128dc0, 0x0, 0x0)
	/home/ajvb/gocode/src/go.mozilla.org/sops/cmd/sops/main.go:767 +0x23c7
gopkg.in/urfave/cli%2ev1.HandleAction(0xe62ca0, 0x10603e0, 0xc000128dc0, 0x0, 0x0)
	/home/ajvb/gocode/pkg/mod/gopkg.in/urfave/cli.v1@v1.20.0/app.go:490 +0xc8
gopkg.in/urfave/cli%2ev1.(*App).Run(0xc0002db860, 0xc0000d4000, 0x4, 0x4, 0x0, 0x0)
	/home/ajvb/gocode/pkg/mod/gopkg.in/urfave/cli.v1@v1.20.0/app.go:264 +0x58c
main.main()
	/home/ajvb/gocode/src/go.mozilla.org/sops/cmd/sops/main.go:823 +0x2de4

```

```
cat auth.enc.yaml 
apiVersion: ENC[AES256_GCM,data:igo=,iv:2TZajuaEplwhxOLoc2wo/T9CbPip1cVh1J/uw5mcUR8=,tag:6Np390u1MMouj4n3TyhCww==,type:str]
data:
    auth: ENC[AES256_GCM,data:Ghp/DXmLL98ujqDJ1rJMyOlVdGL9KtUPigpfOzzpRBxBiF0lGv3aCZQOyLAfZGdi9UwZAUJomo4=,iv:OdglsUCjRmW9nZ7BK+DIj0tgipD+UwFLzeeUo6TtWqg=,tag:jpdAL3JCTPSMpZCtg8/Qww==,type:str]
kind: ENC[AES256_GCM,data:H16uOwEp,iv:WYnXYG0/AheXt7mWB33idNl1VAy7VF3pmpRPaiZd634=,tag:ThEFBFmTYZylv27QokOhKw==,type:str]
metadata:
    creationTimestamp: null
    name: ENC[AES256_GCM,data:uitF6Yx0lSs=,iv:4wZfhMtOdvET4q8hcWXpVfil2piv6UX4kK2OOlj/ekY=,tag:QHWcgibKq2sgMjPXFqV/RA==,type:str]
sops:
    kms: []
    gcp_kms:
    -   resource_id: xxx
        created_at: '2019-12-18T21:17:49Z'
        enc: CiQA8Xd3G6oq6KSE0uLyRP2trk24LeMP7GhcINOf3oYcffYGggUSTBJKCgwV1Dc3s3RQrBnikj0SKD35HZm0UxNj/ubXrA7HipNIabReUTRHFm19ghVNuEhEJzwCkVWxvSkaEP1kDwpX7TBPoejP4VW2Eso=
    azure_kv: []
    lastmodified: '2019-12-18T21:17:49Z'
    mac: ENC[AES256_GCM,data:w1BLQbnPT0tHDElqsrCMC5NICG/rbkxVGxZgDFmjgFbFkWFOg6A3UoqhXMHTiiAmyBEDXLHk4pPtzRX7O/67+GRR3DwaM5Lt8AaYB0AkT7O60TCWx23IzCu/jdnrj9K7rCrqNh+Fbdmihv6g2U6wjG0GuTx2dsDTRdxPB15juvE=,iv:ushcpUzLozcklWBQYPNtHq1ZltPzr994nch5yQ0FmMY=,tag:nF85o6scy4krdQiNIjzmGQ==,type:str]
    pgp: []
    unencrypted_suffix: _unencrypted
    version: 3.4.0
```


Discussion / hints:
I think your issue is that while you've specified an --input-type, you haven't specified an --output-type. So SOPS is loading the file as YAML (as you told it to), but then tries to emit it as binary. Usually this isn't a problem, but unfortunately your YAML has a top-level 'data' key, which is the key that SOPS uses to stuff binary data into.

Panicking definitely isn't the right thing to do here, though :). In general, outputting as binary when the input is not binary should probably be disallowed.
Hi Adrian,

Yes, specifying *--input-type *and leaving *out --output-type *is a
precondition for the crash. I understand now after your explanation what
has happened. From the user perspective, I'd suggest, that if output type
is not specified, it shall default to input-type (e.g. yaml in, yaml out).
User do not expect format conversion, it is save to assume that output
format is the same as input, if not otherwise specified. I missed out
output-type not by mistake, I thought that SOPS it is not needed for normal
operations. The crash surprised me and I didn't connected to missed output
format specification at the first place.

Hope this input helps to improve wonderful tool further.

Thank you for your work and Happy New Year,
Anton


On Thu, 26 Dec 2019 at 15:18, Adrian Utrilla <notifications@github.com>
wrote:

> I think your issue is that while you've specified an --input-type, you
> haven't specified an --output-type. So SOPS is loading the file as YAML (as
> you told it to), but then tries to emit it as binary. Usually this isn't a
> problem, but unfortunately your YAML has a top-level 'data' key, which is
> the key that SOPS uses to stuff binary data into.
>
> Panicking definitely isn't the right thing to do here, though :). In
> general, outputting as binary when the input is not binary should probably
> be disallowed.
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/mozilla/sops/issues/596?email_source=notifications&email_token=AEECBZDK6A5LIMKZW2G2PEDQ2S4NDA5CNFSM4J4SZBQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHVUF2Q#issuecomment-569066218>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AEECBZFQR55WAQVU3RL4GH3Q2S4NDANCNFSM4J4SZBQQ>
> .
>

I came across this exact same issue with `sops` version `3.7.1`. @autrilla's answer explaining the reason behind it and the "workaround" to set an explicit `output-type` helped me out.

Also, I second @0anton's point of view in that by default, SOPS should use the input type for output as well, since this is what most users would want I reckon.

But I think that if this isn't agreed upon and if SOPS was to maintain the current approach, it should probably mention it in the README (for example when the `--input-type` option is presented, in the [Important information on types](https://github.com/mozilla/sops#important-information-on-types) chapter).

How do the collaborators feel about the "default output type being the same as the input type"? Because if this isn't going to change anytime soon, this is a very slight change to a README that I could propose in a tiny PR, just to help future users :)

I really enjoy using this tool, and thank you all for your hard work!
Merry Christmas and have a Happy New Year everyone!

P.S.: just fun to see that this answer is almost exactly 2 years after your previous Happy New Year wishes @0anton 😄 ⛄ 
> Also, I second @0anton's point of view in that by default, SOPS should use the input type for output as well, since this is what most users would want I reckon.

This would be a breaking change. There probably are many uses already which only work because of the current behavior, and will break once it is changed.

For example, think of someone who wants sops encrypted data to always use YAML, even for binary file. They do `sops --encrypt --output-type yaml binary-file > binary-file.sops`, and then want to decode it with `sops --decrypt --input-type yaml binary-file.sops > binary-file`. With your change, they wouldn't get the binary file back, but a YAML file with a key `data` and the binary file as the value for that key.

Fix patch:
diff --git a/cmd/sops/decrypt.go b/cmd/sops/decrypt.go
index 680af4cad..037e77f04 100644
--- a/cmd/sops/decrypt.go
+++ b/cmd/sops/decrypt.go
@@ -1,14 +1,19 @@
 package main
 
 import (
+	"errors"
 	"fmt"
 
 	"github.com/getsops/sops/v3"
 	"github.com/getsops/sops/v3/cmd/sops/codes"
 	"github.com/getsops/sops/v3/cmd/sops/common"
 	"github.com/getsops/sops/v3/keyservice"
+	"github.com/getsops/sops/v3/stores/json"
 )
 
+const notBinaryHint = ("This is likely not an encrypted binary file?" +
+	" If not, use --output-type to select the correct output type.")
+
 type decryptOpts struct {
 	Cipher      sops.Cipher
 	InputStore  sops.Store
@@ -45,6 +50,9 @@ func decrypt(opts decryptOpts) (decryptedFile []byte, err error) {
 		return extract(tree, opts.Extract, opts.OutputStore)
 	}
 	decryptedFile, err = opts.OutputStore.EmitPlainFile(tree.Branches)
+	if errors.Is(err, json.BinaryStoreEmitPlainError) {
+		err = fmt.Errorf("%s\n\n%s", err.Error(), notBinaryHint)
+	}
 	if err != nil {
 		return nil, common.NewExitError(fmt.Sprintf("Error dumping file: %s", err), codes.ErrorDumpingTree)
 	}
@@ -59,6 +67,9 @@ func extract(tree *sops.Tree, path []interface{}, outputStore sops.Store) (outpu
 	if newBranch, ok := v.(sops.TreeBranch); ok {
 		tree.Branches[0] = newBranch
 		decrypted, err := outputStore.EmitPlainFile(tree.Branches)
+		if errors.Is(err, json.BinaryStoreEmitPlainError) {
+			err = fmt.Errorf("%s\n\n%s", err.Error(), notBinaryHint)
+		}
 		if err != nil {
 			return nil, common.NewExitError(fmt.Sprintf("Error dumping file: %s", err), codes.ErrorDumpingTree)
 		}
diff --git a/stores/json/store.go b/stores/json/store.go
index 1b18300f7..81b8bfead 100644
--- a/stores/json/store.go
+++ b/stores/json/store.go
@@ -3,6 +3,7 @@ package json //import "github.com/getsops/sops/v3/stores/json"
 import (
 	"bytes"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 
@@ -42,15 +43,24 @@ func (store BinaryStore) EmitEncryptedFile(in sops.Tree) ([]byte, error) {
 	return store.store.EmitEncryptedFile(in)
 }
 
+var BinaryStoreEmitPlainError = errors.New("error emitting binary store")
+
 // EmitPlainFile produces plaintext json file's bytes from its corresponding sops.TreeBranches object
 func (store BinaryStore) EmitPlainFile(in sops.TreeBranches) ([]byte, error) {
+	if len(in) != 1 {
+		return nil, fmt.Errorf("%w: there must be exactly one tree branch", BinaryStoreEmitPlainError)
+	}
 	// JSON stores a single object per file
 	for _, item := range in[0] {
 		if item.Key == "data" {
-			return []byte(item.Value.(string)), nil
+			if value, ok := item.Value.(string); ok {
+				return []byte(value), nil
+			} else {
+				return nil, fmt.Errorf("%w: 'data' key in tree does not have a string value", BinaryStoreEmitPlainError)
+			}
 		}
 	}
-	return nil, fmt.Errorf("No binary data found in tree")
+	return nil, fmt.Errorf("%w: no binary data found in tree", BinaryStoreEmitPlainError)
 }
 
 // EmitValue extracts a value from a generic interface{} object representing a structured set


Test patch:
diff --git a/stores/json/store_test.go b/stores/json/store_test.go
index d9dd82733..be5e30be3 100644
--- a/stores/json/store_test.go
+++ b/stores/json/store_test.go
@@ -3,8 +3,8 @@ package json
 import (
 	"testing"
 
-	"github.com/stretchr/testify/assert"
 	"github.com/getsops/sops/v3"
+	"github.com/stretchr/testify/assert"
 )
 
 func TestDecodeJSON(t *testing.T) {
@@ -320,6 +320,75 @@ func TestLoadJSONFormattedBinaryFile(t *testing.T) {
 	assert.Equal(t, "data", branches[0][0].Key)
 }
 
+func TestEmitBinaryFile(t *testing.T) {
+	store := BinaryStore{}
+	data, err := store.EmitPlainFile(sops.TreeBranches{
+		sops.TreeBranch{
+			sops.TreeItem{
+				Key:   "data",
+				Value: "foo",
+			},
+		},
+	})
+	assert.Nil(t, err)
+	assert.Equal(t, []byte("foo"), data)
+}
+
+func TestEmitBinaryFileWrongBranches(t *testing.T) {
+	store := BinaryStore{}
+	data, err := store.EmitPlainFile(sops.TreeBranches{
+		sops.TreeBranch{
+			sops.TreeItem{
+				Key:   "data",
+				Value: "bar",
+			},
+		},
+		sops.TreeBranch{
+			sops.TreeItem{
+				Key:   "data",
+				Value: "bar",
+			},
+		},
+	})
+	assert.Nil(t, data)
+	assert.Contains(t, err.Error(), "there must be exactly one tree branch")
+
+	data, err = store.EmitPlainFile(sops.TreeBranches{})
+	assert.Nil(t, data)
+	assert.Contains(t, err.Error(), "there must be exactly one tree branch")
+}
+
+func TestEmitBinaryFileNoData(t *testing.T) {
+	store := BinaryStore{}
+	data, err := store.EmitPlainFile(sops.TreeBranches{
+		sops.TreeBranch{
+			sops.TreeItem{
+				Key:   "foo",
+				Value: "bar",
+			},
+		},
+	})
+	assert.Nil(t, data)
+	assert.Contains(t, err.Error(), "no binary data found in tree")
+}
+
+func TestEmitBinaryFileWrongDataType(t *testing.T) {
+	store := BinaryStore{}
+	data, err := store.EmitPlainFile(sops.TreeBranches{
+		sops.TreeBranch{
+			sops.TreeItem{
+				Key: "data",
+				Value: sops.TreeItem{
+					Key:   "foo",
+					Value: "bar",
+				},
+			},
+		},
+	})
+	assert.Nil(t, data)
+	assert.Contains(t, err.Error(), "'data' key in tree does not have a string value")
+}
+
 func TestEmitValueString(t *testing.T) {
 	bytes, err := (&Store{}).EmitValue("hello")
 	assert.Nil(t, err)