Repository: getsops/sops (language: python)

Issue:
shamir treshold breaks env files (panic: interface conversion: interface {} is float64, not string)
sops: 3.7.1 (ubuntu)

.sops.yaml
```
creation_rules:
- shamir_threshold: 2
  key_groups:
  - pgp:
    - XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
  - pgp:
    - YYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYY
```

command:
`sops example.env`

produces:
```
panic: interface conversion: interface {} is float64, not string

goroutine 1 [running]:
go.mozilla.org/sops/v3/stores/dotenv.(*Store).EmitPlainFile(0x1a17488, 0xc0005d24e0, 0x1, 0x1, 0x3, 0xc0005af040, 0x8, 0x10, 0x0)
	/Users/ajvb/gocode/src/go.mozilla.org/sops/stores/dotenv/store.go:122 +0x3f6
go.mozilla.org/sops/v3/stores/dotenv.(*Store).EmitEncryptedFile(0x1a17488, 0x21816938, 0xed80b80e1, 0x0, 0x11adb59, 0xc, 0x0, 0x0, 0x0, 0x0, ...)
	/Users/ajvb/gocode/src/go.mozilla.org/sops/stores/dotenv/store.go:107 +0x2fa
main.editTree(0x137d960, 0xc000490fc0, 0x138f380, 0x1a17488, 0x138f380, 0x1a17488, 0xc0001a4840, 0x5c, 0x0, 0xc00048d1d0, ...)
	/Users/ajvb/gocode/src/go.mozilla.org/sops/cmd/sops/edit.go:164 +0xa35
main.editExample(0x137d960, 0xc000490fc0, 0x138f380, 0x1a17488, 0x138f380, 0x1a17488, 0xc0001a4840, 0x5c, 0x0, 0xc00048d1d0, ...)
	/Users/ajvb/gocode/src/go.mozilla.org/sops/cmd/sops/edit.go:84 +0x51b
main.main.func8(0xc0001e4dc0, 0x0, 0x0)
	/Users/ajvb/gocode/src/go.mozilla.org/sops/cmd/sops/main.go:943 +0x24bc
gopkg.in/urfave/cli%2ev1.HandleAction(0xfdafa0, 0x11f8d28, 0xc0001e4dc0, 0x0, 0x0)
	/Users/ajvb/gocode/src/go.mozilla.org/sops/vendor/gopkg.in/urfave/cli.v1/app.go:490 +0x82
gopkg.in/urfave/cli%2ev1.(*App).Run(0xc00047c4e0, 0xc00018c000, 0x2, 0x2, 0x0, 0x0)
	/Users/ajvb/gocode/src/go.mozilla.org/sops/vendor/gopkg.in/urfave/cli.v1/app.go:264 +0x5f5
main.main()
	/Users/ajvb/gocode/src/go.mozilla.org/sops/cmd/sops/main.go:987 +0x3885

```
Serialize using mapstructure conversion instead of JSON marshalling.
These changes remove the JSON encoding/decoding steps that are performed during the serialization of ini and dotenv files. This roundtrip loses type information during the transformation which causes values to be incorrectly converted to the JSON marshaller defaults (int becomes float64, bool becomes string, etc, etc). In place of this JSON encoding, the mapstructure library allows for a direct conversion between the Metadata struct and
map[string]interface{} needed to leverage the stores.Flatten and stores.Unflatten functions.

In addition this adds mapstructure tags to the metadata structures to allow backwards compatibility with the JSON encoding.

Resolves #879 & resolves #857

This is a re-submission of @acastle's #1009 with mapstructure updated to v1.5.0 and unused imports removed applied to getsops/sops/main HEAD.


Discussion / hints:
👋🏻 Hello folks,

I've also been hitting this exact issue, and can add some color to the configuration that causes it.

This first configuration defines multiple `key_groups`, and will `panic` when running `sops updatekeys`. Additionally, it there is **no** panic when running `sops --encrypt` or `sops --decrypt`.

```yaml
creation_rules:
  - path_regex: .*
    key_groups:
      - pgp:
          - XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
      - pgp:
          - YYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYY
```

Whereas this second configuration a single `key_groups` with multiple keys, and will **not** panic under any `sops` operation.

```yaml
creation_rules:
  - path_regex: .*
    key_groups:
      - pgp:
          - XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
          - YYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYY
```

Hopefully this help to resolve this bug. Cheers!

---

Additionally, this observation holds even if you're mixing `pgp` and `kms` (which I was). This configuration will panic:

```yaml
key_groups:
  - kms:
      - arn:aws:kms:XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
  - pgp:
      - YYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYY
```

But this configuration will not:

```yaml
key_groups:
  - kms:
      - arn:aws:kms:XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
    pgp: # <--- No dash before, so it's the same list item.
      - YYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYY
```
Hi,
Small tip, if you want this issue to close 2 issues automatically, you need to repeat resolves just before the second issue number, like :
Resolves #879 & resolves #857
> Hi, Small tip, if you want this issue close to issues automatically, you need to repeat resolves just before the second issue number, like : Resolves #879 & resolves #857

Nice.  The commit message actually belongs to @acastle, so hopefully we can both benefit!  Thank you.

Fix patch:
diff --git a/stores/dotenv/store.go b/stores/dotenv/store.go
index 58d7c6d53..9433322ee 100644
--- a/stores/dotenv/store.go
+++ b/stores/dotenv/store.go
@@ -49,7 +49,10 @@ func (store *Store) LoadEncryptedFile(in []byte) (sops.Tree, error) {
 	}
 
 	stores.DecodeNewLines(mdMap)
-	stores.DecodeNonStrings(mdMap)
+	err = stores.DecodeNonStrings(mdMap)
+	if err != nil {
+		return sops.Tree{}, err
+	}
 	metadata, err := stores.UnflattenMetadata(mdMap)
 	if err != nil {
 		return sops.Tree{}, err
diff --git a/stores/flatten.go b/stores/flatten.go
index c894e6e29..fe6e981fd 100644
--- a/stores/flatten.go
+++ b/stores/flatten.go
@@ -236,13 +236,28 @@ func EncodeNewLines(m map[string]interface{}) {
 }
 
 // DecodeNonStrings will look for known metadata keys that are not strings and decode to the appropriate type
-func DecodeNonStrings(m map[string]interface{}) {
+func DecodeNonStrings(m map[string]interface{}) error {
 	if v, ok := m["mac_only_encrypted"]; ok {
 		m["mac_only_encrypted"] = false
 		if v == "true" {
 			m["mac_only_encrypted"] = true
 		}
 	}
+	if v, ok := m["shamir_threshold"]; ok {
+		switch val := v.(type) {
+			case string:
+				vInt, err := strconv.Atoi(val)
+				if err != nil {
+					return fmt.Errorf("shamir_threshold is not an integer: %s", err.Error())
+				}
+				m["shamir_threshold"] = vInt
+			case int:
+				m["shamir_threshold"] = val
+			default:
+				return fmt.Errorf("shamir_threshold is neither a string nor an integer, but %T", val)
+		}
+	}
+	return nil
 }
 
 // EncodeNonStrings will look for known metadata keys that are not strings and will encode it to strings
@@ -255,4 +270,9 @@ func EncodeNonStrings(m map[string]interface{}) {
 			}
 		}
 	}
+	if v, found := m["shamir_threshold"]; found {
+		if vInt, ok := v.(int); ok {
+			m["shamir_threshold"] = fmt.Sprintf("%d", vInt)
+		}
+	}
 }
diff --git a/stores/ini/store.go b/stores/ini/store.go
index 12bd17ce1..dd1a77c63 100644
--- a/stores/ini/store.go
+++ b/stores/ini/store.go
@@ -188,7 +188,10 @@ func (store *Store) iniSectionToMetadata(sopsSection *ini.Section) (stores.Metad
 		metadataHash[k] = v
 	}
 	stores.DecodeNewLines(metadataHash)
-	stores.DecodeNonStrings(metadataHash)
+	err := stores.DecodeNonStrings(metadataHash)
+	if err != nil {
+		return stores.Metadata{}, err
+	}
 	return stores.UnflattenMetadata(metadataHash)
 }
 


Test patch:
diff --git a/stores/flatten_test.go b/stores/flatten_test.go
index 4d11dd1ad..6c75da3d1 100644
--- a/stores/flatten_test.go
+++ b/stores/flatten_test.go
@@ -222,14 +222,35 @@ func TestDecodeNonStrings(t *testing.T) {
 		{map[string]interface{}{"mac_only_encrypted": "false"}, map[string]interface{}{"mac_only_encrypted": false}},
 		{map[string]interface{}{"mac_only_encrypted": "true"}, map[string]interface{}{"mac_only_encrypted": true}},
 		{map[string]interface{}{"mac_only_encrypted": "something-else"}, map[string]interface{}{"mac_only_encrypted": false}},
+		{map[string]interface{}{"shamir_threshold": "2"}, map[string]interface{}{"shamir_threshold": 2}},
+		{map[string]interface{}{"shamir_threshold": "002"}, map[string]interface{}{"shamir_threshold": 2}},
+		{map[string]interface{}{"shamir_threshold": "123"}, map[string]interface{}{"shamir_threshold": 123}},
+		{map[string]interface{}{"shamir_threshold": 123}, map[string]interface{}{"shamir_threshold": 123}},
 	}
 
 	for _, tt := range tests {
-		DecodeNonStrings(tt.input)
+		err := DecodeNonStrings(tt.input)
+		assert.Nil(t, err)
 		assert.Equal(t, tt.want, tt.input)
 	}
 }
 
+func TestDecodeNonStringsErrors(t *testing.T) {
+	tests := []struct {
+		input map[string]interface{}
+		want  string
+	}{
+		{map[string]interface{}{"shamir_threshold": "foo"}, "shamir_threshold is not an integer: strconv.Atoi: parsing \"foo\": invalid syntax"},
+		{map[string]interface{}{"shamir_threshold": true}, "shamir_threshold is neither a string nor an integer, but bool"},
+	}
+
+	for _, tt := range tests {
+		err := DecodeNonStrings(tt.input)
+		assert.NotNil(t, err)
+		assert.Equal(t, tt.want, err.Error())
+	}
+}
+
 func TestEncodeNonStrings(t *testing.T) {
 	tests := []struct {
 		input map[string]interface{}
@@ -237,6 +258,8 @@ func TestEncodeNonStrings(t *testing.T) {
 	}{
 		{map[string]interface{}{"mac_only_encrypted": false}, map[string]interface{}{"mac_only_encrypted": "false"}},
 		{map[string]interface{}{"mac_only_encrypted": true}, map[string]interface{}{"mac_only_encrypted": "true"}},
+		{map[string]interface{}{"shamir_threshold": 2}, map[string]interface{}{"shamir_threshold": "2"}},
+		{map[string]interface{}{"shamir_threshold": 123}, map[string]interface{}{"shamir_threshold": "123"}},
 	}
 
 	for _, tt := range tests {