Repository: getsops/sops (language: python)

Issue:
MAC only over encrypted values
Our use case is more that we have a YAML file with many settings, where only few of them have to be encrypted. We want to store that file in public open source git repository, so that everyone can see the configuration. The issue that sops require one to be able to decrypt the file to be able to edit it (and re-encrypt it, updating per-file MAC). That is great for some use cases, but in our case we would want external collaborators to be able to edit settings (at least the non-encrypted ones) and make PRs to change them, both the code and configuration (as code). E.g., somebody adds a new configuration option and also wants to add the value for that configuration option for our deployment. During PR review process those changes would be reviewed and if approved would be merged to the main branch.

I think this would work out if MAC would be stored together with encrypted value itself, per encrypted value (and path to it in YAML).

This might solve issues with merging others have reported, too: #52

I think the issue is that SOPS started with the design which had all values encrypted (where MAC per-file makes complete sense) and now added more features to enable only some values to be encrypted, which opens more use cases. For some (where both encrypted and non-encrypted settings have tight coupling and one does not want to open doors for somebody to change any of them) this is a great fit, but I am seeing that this is not always the case.


Discussion / hints:
I see that encrypted values are already encrypted using AES256 GCM, which is authenticated encryption, so it already includes kinda MAC.

Maybe the alternative could be to compute MAC only over encrypted values? This could improve things for collaboration while for editing of any encrypted value it would still be required to be able to decrypt all values (to compute MAC over them).

I see a use case where external collaborator might want to encrypt a value using the public key only and would be prevented from doing so because they cannot compute MAC over all files (lacking the private key), but I am not completely sure this use case makes much sense.

Fix patch:
diff --git a/README.rst b/README.rst
index 2400172c7..e1b206193 100644
--- a/README.rst
+++ b/README.rst
@@ -1427,6 +1427,9 @@ to any key of a file. When set, all values underneath the key that set the
 Note that, while in cleartext, unencrypted content is still added to the
 checksum of the file, and thus cannot be modified outside of SOPS without
 breaking the file integrity check.
+This behavior can be modified using ``--mac-only-encrypted`` flag or ``.sops.yaml``
+config file which makes SOPS compute a MAC only over values it encrypted and
+not all values.
 
 The unencrypted suffix can be set to a different value using the
 ``--unencrypted-suffix`` option.
@@ -1539,6 +1542,9 @@ In addition to authenticating branches of the tree using keys as additional
 data, SOPS computes a MAC on all the values to ensure that no value has been
 added or removed fraudulently. The MAC is stored encrypted with AES_GCM and
 the data key under tree -> ``sops`` -> ``mac``.
+This behavior can be modified using ``--mac-only-encrypted`` flag or ``.sops.yaml``
+config file which makes SOPS compute a MAC only over values it encrypted and
+not all values.
 
 Motivation
 ----------
diff --git a/cmd/sops/edit.go b/cmd/sops/edit.go
index 4ac92e487..d71906f1a 100644
--- a/cmd/sops/edit.go
+++ b/cmd/sops/edit.go
@@ -35,6 +35,7 @@ type editExampleOpts struct {
 	EncryptedSuffix   string
 	UnencryptedRegex  string
 	EncryptedRegex    string
+	MACOnlyEncrypted  bool
 	KeyGroups         []sops.KeyGroup
 	GroupThreshold    int
 }
@@ -65,6 +66,7 @@ func editExample(opts editExampleOpts) ([]byte, error) {
 			EncryptedSuffix:   opts.EncryptedSuffix,
 			UnencryptedRegex:  opts.UnencryptedRegex,
 			EncryptedRegex:    opts.EncryptedRegex,
+			MACOnlyEncrypted:  opts.MACOnlyEncrypted,
 			Version:           version.Version,
 			ShamirThreshold:   opts.GroupThreshold,
 		},
diff --git a/cmd/sops/encrypt.go b/cmd/sops/encrypt.go
index f5b770e7a..826fa496a 100644
--- a/cmd/sops/encrypt.go
+++ b/cmd/sops/encrypt.go
@@ -23,6 +23,7 @@ type encryptOpts struct {
 	EncryptedSuffix   string
 	UnencryptedRegex  string
 	EncryptedRegex    string
+	MACOnlyEncrypted  bool
 	KeyGroups         []sops.KeyGroup
 	GroupThreshold    int
 }
@@ -82,6 +83,7 @@ func encrypt(opts encryptOpts) (encryptedFile []byte, err error) {
 			EncryptedSuffix:   opts.EncryptedSuffix,
 			UnencryptedRegex:  opts.UnencryptedRegex,
 			EncryptedRegex:    opts.EncryptedRegex,
+			MACOnlyEncrypted:  opts.MACOnlyEncrypted,
 			Version:           version.Version,
 			ShamirThreshold:   opts.GroupThreshold,
 		},
diff --git a/cmd/sops/main.go b/cmd/sops/main.go
index 2d5cea4db..c78b51478 100644
--- a/cmd/sops/main.go
+++ b/cmd/sops/main.go
@@ -668,6 +668,10 @@ func main() {
 			Name:  "ignore-mac",
 			Usage: "ignore Message Authentication Code during decryption",
 		},
+		cli.BoolFlag{
+			Name:  "mac-only-encrypted",
+			Usage: "compute MAC only over values which end up encrypted",
+		},
 		cli.StringFlag{
 			Name:  "unencrypted-suffix",
 			Usage: "override the unencrypted key suffix.",
@@ -738,6 +742,7 @@ func main() {
 		encryptedSuffix := c.String("encrypted-suffix")
 		encryptedRegex := c.String("encrypted-regex")
 		unencryptedRegex := c.String("unencrypted-regex")
+		macOnlyEncrypted := c.Bool("mac-only-encrypted")
 		conf, err := loadConfig(c, fileName, nil)
 		if err != nil {
 			return toExitError(err)
@@ -756,6 +761,9 @@ func main() {
 			if unencryptedRegex == "" {
 				unencryptedRegex = conf.UnencryptedRegex
 			}
+			if !macOnlyEncrypted {
+				macOnlyEncrypted = conf.MACOnlyEncrypted
+			}
 		}
 
 		cryptRuleCount := 0
@@ -806,6 +814,7 @@ func main() {
 				EncryptedSuffix:   encryptedSuffix,
 				UnencryptedRegex:  unencryptedRegex,
 				EncryptedRegex:    encryptedRegex,
+				MACOnlyEncrypted:  macOnlyEncrypted,
 				KeyServices:       svcs,
 				KeyGroups:         groups,
 				GroupThreshold:    threshold,
@@ -963,6 +972,7 @@ func main() {
 					EncryptedSuffix:   encryptedSuffix,
 					UnencryptedRegex:  unencryptedRegex,
 					EncryptedRegex:    encryptedRegex,
+					MACOnlyEncrypted:  macOnlyEncrypted,
 					KeyGroups:         groups,
 					GroupThreshold:    threshold,
 				})
diff --git a/config/config.go b/config/config.go
index 6f34e0066..67ddea1bb 100644
--- a/config/config.go
+++ b/config/config.go
@@ -123,6 +123,7 @@ type creationRule struct {
 	EncryptedSuffix   string     `yaml:"encrypted_suffix"`
 	UnencryptedRegex  string     `yaml:"unencrypted_regex"`
 	EncryptedRegex    string     `yaml:"encrypted_regex"`
+	MACOnlyEncrypted  bool       `yaml:"mac_only_encrypted"`
 }
 
 // Load loads a sops config file into a temporary struct
@@ -142,6 +143,7 @@ type Config struct {
 	EncryptedSuffix   string
 	UnencryptedRegex  string
 	EncryptedRegex    string
+	MACOnlyEncrypted  bool
 	Destination       publish.Destination
 	OmitExtensions    bool
 }
@@ -265,6 +267,7 @@ func configFromRule(rule *creationRule, kmsEncryptionContext map[string]*string)
 		EncryptedSuffix:   rule.EncryptedSuffix,
 		UnencryptedRegex:  rule.UnencryptedRegex,
 		EncryptedRegex:    rule.EncryptedRegex,
+		MACOnlyEncrypted:  rule.MACOnlyEncrypted,
 	}, nil
 }
 
diff --git a/sops.go b/sops.go
index ae0ecfc24..827a5ee5b 100644
--- a/sops.go
+++ b/sops.go
@@ -70,6 +70,12 @@ const MacMismatch = sopsError("MAC mismatch")
 // MetadataNotFound occurs when the input file is malformed and doesn't have sops metadata in it
 const MetadataNotFound = sopsError("sops metadata not found")
 
+// MACOnlyEncryptedInitialization is a constant and known sequence of 32 bytes used to initialize
+// MAC which is computed only over values which end up encrypted. That assures that a MAC with the
+// setting enabled is always different from a MAC with this setting disabled.
+// The following numbers are taken from the output of `echo -n sops | sha256sum` (shell) or `hashlib.sha256(b'sops').hexdigest()` (Python).
+var MACOnlyEncryptedInitialization = []byte{0x8a, 0x3f, 0xd2, 0xad, 0x54, 0xce, 0x66, 0x52, 0x7b, 0x10, 0x34, 0xf3, 0xd1, 0x47, 0xbe, 0xb, 0xb, 0x97, 0x5b, 0x3b, 0xf4, 0x4f, 0x72, 0xc6, 0xfd, 0xad, 0xec, 0x81, 0x76, 0xf2, 0x7d, 0x69}
+
 var log *logrus.Logger
 
 func init() {
@@ -291,22 +297,21 @@ func (branch TreeBranch) walkBranch(in TreeBranch, path []string, onLeaves func(
 // is provided (by default it is not), those not matching EncryptedRegex,
 // if EncryptedRegex is provided (by default it is not) or those matching
 // UnencryptedRegex, if UnencryptedRegex is provided (by default it is not).
-// If encryption is successful, it returns the MAC for the encrypted tree.
+// If encryption is successful, it returns the MAC for the encrypted tree
+// (all values if MACOnlyEncrypted is false, or only over values which end
+// up encrypted if MACOnlyEncrypted is true).
 func (tree Tree) Encrypt(key []byte, cipher Cipher) (string, error) {
 	audit.SubmitEvent(audit.EncryptEvent{
 		File: tree.FilePath,
 	})
 	hash := sha512.New()
+	if tree.Metadata.MACOnlyEncrypted {
+		// We initialize with known set of bytes so that a MAC with this setting
+		// enabled is always different from a MAC with this setting disabled.
+		hash.Write(MACOnlyEncryptedInitialization)
+	}
 	walk := func(branch TreeBranch) error {
 		_, err := branch.walkBranch(branch, make([]string, 0), func(in interface{}, path []string) (interface{}, error) {
-			// Only add to MAC if not a comment
-			if _, ok := in.(Comment); !ok {
-				bytes, err := ToBytes(in)
-				if err != nil {
-					return nil, fmt.Errorf("Could not convert %s to bytes: %s", in, err)
-				}
-				hash.Write(bytes)
-			}
 			encrypted := true
 			if tree.Metadata.UnencryptedSuffix != "" {
 				for _, v := range path {
@@ -344,6 +349,16 @@ func (tree Tree) Encrypt(key []byte, cipher Cipher) (string, error) {
 					}
 				}
 			}
+			if !tree.Metadata.MACOnlyEncrypted || encrypted {
+				// Only add to MAC if not a comment
+				if _, ok := in.(Comment); !ok {
+					bytes, err := ToBytes(in)
+					if err != nil {
+						return nil, fmt.Errorf("Could not convert %s to bytes: %s", in, err)
+					}
+					hash.Write(bytes)
+				}
+			}
 			if encrypted {
 				var err error
 				pathString := strings.Join(path, ":") + ":"
@@ -371,13 +386,20 @@ func (tree Tree) Encrypt(key []byte, cipher Cipher) (string, error) {
 // those not ending with EncryptedSuffix, if EncryptedSuffix is provided (by default it is not),
 // those not matching EncryptedRegex, if EncryptedRegex is provided (by default it is not),
 // or those matching UnencryptedRegex, if UnencryptedRegex is provided (by default it is not).
-// If decryption is successful, it returns the MAC for the decrypted tree.
+// If decryption is successful, it returns the MAC for the decrypted tree
+// (all values if MACOnlyEncrypted is false, or only over values which end
+// up decrypted if MACOnlyEncrypted is true).
 func (tree Tree) Decrypt(key []byte, cipher Cipher) (string, error) {
 	log.Debug("Decrypting tree")
 	audit.SubmitEvent(audit.DecryptEvent{
 		File: tree.FilePath,
 	})
 	hash := sha512.New()
+	if tree.Metadata.MACOnlyEncrypted {
+		// We initialize with known set of bytes so that a MAC with this setting
+		// enabled is always different from a MAC with this setting disabled.
+		hash.Write(MACOnlyEncryptedInitialization)
+	}
 	walk := func(branch TreeBranch) error {
 		_, err := branch.walkBranch(branch, make([]string, 0), func(in interface{}, path []string) (interface{}, error) {
 			encrypted := true
@@ -441,13 +463,15 @@ func (tree Tree) Decrypt(key []byte, cipher Cipher) (string, error) {
 			} else {
 				v = in
 			}
-			// Only add to MAC if not a comment
-			if _, ok := v.(Comment); !ok {
-				bytes, err := ToBytes(v)
-				if err != nil {
-					return nil, fmt.Errorf("Could not convert %s to bytes: %s", in, err)
+			if !tree.Metadata.MACOnlyEncrypted || encrypted {
+				// Only add to MAC if not a comment
+				if _, ok := v.(Comment); !ok {
+					bytes, err := ToBytes(v)
+					if err != nil {
+						return nil, fmt.Errorf("Could not convert %s to bytes: %s", in, err)
+					}
+					hash.Write(bytes)
 				}
-				hash.Write(bytes)
 			}
 			return v, nil
 		})
@@ -490,6 +514,7 @@ type Metadata struct {
 	UnencryptedRegex          string
 	EncryptedRegex            string
 	MessageAuthenticationCode string
+	MACOnlyEncrypted          bool
 	Version                   string
 	KeyGroups                 []KeyGroup
 	// ShamirThreshold is the number of key groups required to recover the
diff --git a/stores/stores.go b/stores/stores.go
index 420c115c7..e4b17289d 100644
--- a/stores/stores.go
+++ b/stores/stores.go
@@ -51,6 +51,7 @@ type Metadata struct {
 	EncryptedSuffix           string      `yaml:"encrypted_suffix,omitempty" json:"encrypted_suffix,omitempty"`
 	UnencryptedRegex          string      `yaml:"unencrypted_regex,omitempty" json:"unencrypted_regex,omitempty"`
 	EncryptedRegex            string      `yaml:"encrypted_regex,omitempty" json:"encrypted_regex,omitempty"`
+	MACOnlyEncrypted          bool        `yaml:"mac_only_encrypted,omitempty" json:"mac_only_encrypted,omitempty"`
 	Version                   string      `yaml:"version" json:"version"`
 }
 
@@ -114,6 +115,7 @@ func MetadataFromInternal(sopsMetadata sops.Metadata) Metadata {
 	m.UnencryptedRegex = sopsMetadata.UnencryptedRegex
 	m.EncryptedRegex = sopsMetadata.EncryptedRegex
 	m.MessageAuthenticationCode = sopsMetadata.MessageAuthenticationCode
+	m.MACOnlyEncrypted = sopsMetadata.MACOnlyEncrypted
 	m.Version = sopsMetadata.Version
 	m.ShamirThreshold = sopsMetadata.ShamirThreshold
 	if len(sopsMetadata.KeyGroups) == 1 {
@@ -270,6 +272,7 @@ func (m *Metadata) ToInternal() (sops.Metadata, error) {
 		EncryptedSuffix:           m.EncryptedSuffix,
 		UnencryptedRegex:          m.UnencryptedRegex,
 		EncryptedRegex:            m.EncryptedRegex,
+		MACOnlyEncrypted:          m.MACOnlyEncrypted,
 		LastModified:              lastModified,
 	}, nil
 }


Test patch:
diff --git a/config/config_test.go b/config/config_test.go
index 1c9814a41..8f4fb006b 100644
--- a/config/config_test.go
+++ b/config/config_test.go
@@ -158,6 +158,14 @@ creation_rules:
     unencrypted_regex: "^dec:"
     `)
 
+var sampleConfigWithMACOnlyEncrypted = []byte(`
+creation_rules:
+  - path_regex: barbar*
+    kms: "1"
+    pgp: "2"
+    mac_only_encrypted: true
+    `)
+
 var sampleConfigWithInvalidParameters = []byte(`
 creation_rules:
   - path_regex: foobar*
@@ -416,6 +424,12 @@ func TestLoadConfigFileWithEncryptedRegex(t *testing.T) {
 	assert.Equal(t, "^enc:", conf.EncryptedRegex)
 }
 
+func TestLoadConfigFileWithMACOnlyEncrypted(t *testing.T) {
+	conf, err := parseCreationRuleForFile(parseConfigFile(sampleConfigWithMACOnlyEncrypted, t), "/conf/path", "barbar", nil)
+	assert.Equal(t, nil, err)
+	assert.Equal(t, true, conf.MACOnlyEncrypted)
+}
+
 func TestLoadConfigFileWithInvalidParameters(t *testing.T) {
 	_, err := parseCreationRuleForFile(parseConfigFile(sampleConfigWithInvalidParameters, t), "/conf/path", "foobar", nil)
 	assert.NotNil(t, err)
diff --git a/sops_test.go b/sops_test.go
index 549de80bf..56a1c3da4 100644
--- a/sops_test.go
+++ b/sops_test.go
@@ -242,6 +242,90 @@ func TestUnencryptedRegex(t *testing.T) {
 	}
 }
 
+func TestMACOnlyEncrypted(t *testing.T) {
+	branches := TreeBranches{
+		TreeBranch{
+			TreeItem{
+				Key:   "foo_encrypted",
+				Value: "bar",
+			},
+			TreeItem{
+				Key: "bar",
+				Value: TreeBranch{
+					TreeItem{
+						Key:   "foo",
+						Value: "bar",
+					},
+				},
+			},
+		},
+	}
+	tree := Tree{Branches: branches, Metadata: Metadata{EncryptedSuffix: "_encrypted", MACOnlyEncrypted: true}}
+	onlyEncrypted := TreeBranches{
+		TreeBranch{
+			TreeItem{
+				Key:   "foo_encrypted",
+				Value: "bar",
+			},
+		},
+	}
+	treeOnlyEncrypted := Tree{Branches: onlyEncrypted, Metadata: Metadata{EncryptedSuffix: "_encrypted", MACOnlyEncrypted: true}}
+	cipher := reverseCipher{}
+	mac, err := tree.Encrypt(bytes.Repeat([]byte("f"), 32), cipher)
+	if err != nil {
+		t.Errorf("Encrypting the tree failed: %s", err)
+	}
+	macOnlyEncrypted, err := treeOnlyEncrypted.Encrypt(bytes.Repeat([]byte("f"), 32), cipher)
+	if err != nil {
+		t.Errorf("Encrypting the treeOnlyEncrypted failed: %s", err)
+	}
+	if mac != macOnlyEncrypted {
+		t.Errorf("MACs don't match:\ngot \t\t%+v,\nexpected \t\t%+v", mac, macOnlyEncrypted)
+	}
+}
+
+func TestMACOnlyEncryptedNoConfusion(t *testing.T) {
+	branches := TreeBranches{
+		TreeBranch{
+			TreeItem{
+				Key:   "foo_encrypted",
+				Value: "bar",
+			},
+			TreeItem{
+				Key: "bar",
+				Value: TreeBranch{
+					TreeItem{
+						Key:   "foo",
+						Value: "bar",
+					},
+				},
+			},
+		},
+	}
+	tree := Tree{Branches: branches, Metadata: Metadata{EncryptedSuffix: "_encrypted", MACOnlyEncrypted: true}}
+	onlyEncrypted := TreeBranches{
+		TreeBranch{
+			TreeItem{
+				Key:   "foo_encrypted",
+				Value: "bar",
+			},
+		},
+	}
+	treeOnlyEncrypted := Tree{Branches: onlyEncrypted, Metadata: Metadata{EncryptedSuffix: "_encrypted"}}
+	cipher := reverseCipher{}
+	mac, err := tree.Encrypt(bytes.Repeat([]byte("f"), 32), cipher)
+	if err != nil {
+		t.Errorf("Encrypting the tree failed: %s", err)
+	}
+	macOnlyEncrypted, err := treeOnlyEncrypted.Encrypt(bytes.Repeat([]byte("f"), 32), cipher)
+	if err != nil {
+		t.Errorf("Encrypting the treeOnlyEncrypted failed: %s", err)
+	}
+	if mac == macOnlyEncrypted {
+		t.Errorf("MACs match but they should not")
+	}
+}
+
 type MockCipher struct{}
 
 func (m MockCipher) Encrypt(value interface{}, key []byte, path string) (string, error) {