From 2b13d2763833b1f00b41271b06c1236d34ff89b3 Mon Sep 17 00:00:00 2001 From: Steven Richards Date: Mon, 19 Oct 2026 06:50:36 -0400 Subject: [PATCH] Keep edits made with `sops edit --show-master-keys` With --show-master-keys the editor gets the whole tree including the sops metadata, and runEditorUntilOk replaced the tree after reloading it with `opts.Tree = &t`. That only changed the local copy of the options; the caller then encrypted and wrote back its original tree, so the edit was silently discarded (exit 0, new MAC and timestamp, old content). Update the caller's tree in place instead, and only replace the branches in the non --show-master-keys path. Signed-off-by: Steven Richards --- cmd/sops/edit.go | 8 +-- cmd/sops/edit_test.go | 50 +++++++++++++++++++ cmd/sops/helpers_test.go | 102 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 157 insertions(+), 3 deletions(-) create mode 100644 cmd/sops/edit_test.go create mode 100644 cmd/sops/helpers_test.go diff --git a/cmd/sops/edit.go b/cmd/sops/edit.go index bfc3a37b94..74e954995e 100644 --- a/cmd/sops/edit.go +++ b/cmd/sops/edit.go @@ -268,8 +268,10 @@ func runEditorUntilOk(opts runEditorUntilOkOpts) error { continue } // Replace the whole tree, because otherwise newBranches would - // contain the SOPS metadata - opts.Tree = &t + // contain the SOPS metadata. The caller's tree has to be updated + // in place, since the caller encrypts and emits that tree. + t.FilePath = opts.Tree.FilePath + *opts.Tree = t } else { if userErr, _ := validateFileForEncryption(opts.OutputStore, newBranches); userErr != nil { log.WithField( @@ -279,8 +281,8 @@ func runEditorUntilOk(opts runEditorUntilOkOpts) error { waitForKeyPress() continue } + opts.Tree.Branches = newBranches } - opts.Tree.Branches = newBranches needVersionUpdated, err := version.AIsNewerThanB(version.Version, opts.Tree.Metadata.Version) if err != nil { return common.NewExitError(fmt.Sprintf("Failed to compare document version %q with program version %q: %v", opts.Tree.Metadata.Version, version.Version, err), codes.FailedToCompareVersions) diff --git a/cmd/sops/edit_test.go b/cmd/sops/edit_test.go new file mode 100644 index 0000000000..1b5dc8c9d4 --- /dev/null +++ b/cmd/sops/edit_test.go @@ -0,0 +1,50 @@ +package main + +import ( + "os" + "path/filepath" + "runtime" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/getsops/sops/v3" + "github.com/getsops/sops/v3/aes" +) + +func TestEditKeepsChanges(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("the test editor is a shell script") + } + for _, showMasterKeys := range []bool{false, true} { + name := "plain" + if showMasterKeys { + name = "show-master-keys" + } + t.Run(name, func(t *testing.T) { + isolateAgeEnvironment(t) + key := newTestAgeKey(t) + path := encryptTestFile(t, "secret: old\n", []sops.KeyGroup{{key}}) + + editor := filepath.Join(t.TempDir(), "editor.sh") + script := "#!/bin/sh\nsed 's/secret: old/secret: new/' \"$1\" > \"$1.tmp\" && mv \"$1.tmp\" \"$1\"\n" + require.NoError(t, os.WriteFile(editor, []byte(script), 0700)) + t.Setenv("SOPS_EDITOR", editor) + + store := testYAMLStore() + output, err := edit(editOpts{ + Cipher: aes.NewCipher(), + InputStore: store, + OutputStore: store, + InputPath: path, + KeyServices: testKeyServices(), + ShowMasterKeys: showMasterKeys, + }) + require.NoError(t, err) + require.NoError(t, os.WriteFile(path, output, 0600)) + + assert.Equal(t, "secret: new\n", decryptTestFile(t, path)) + }) + } +} diff --git a/cmd/sops/helpers_test.go b/cmd/sops/helpers_test.go new file mode 100644 index 0000000000..ceb9cc77c6 --- /dev/null +++ b/cmd/sops/helpers_test.go @@ -0,0 +1,102 @@ +package main + +import ( + "os" + "path/filepath" + "strings" + "testing" + + fage "filippo.io/age" + "github.com/stretchr/testify/require" + + "github.com/getsops/sops/v3" + "github.com/getsops/sops/v3/aes" + "github.com/getsops/sops/v3/age" + "github.com/getsops/sops/v3/cmd/sops/common" + "github.com/getsops/sops/v3/cmd/sops/formats" + "github.com/getsops/sops/v3/config" + "github.com/getsops/sops/v3/keyservice" +) + +// isolateAgeEnvironment makes sure that the age key source only sees the +// identities passed through SOPS_AGE_KEY by newTestAgeKey, and not keys of +// the user running the tests (like ~/.ssh/id_ed25519). +func isolateAgeEnvironment(t *testing.T) { + t.Helper() + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("XDG_CONFIG_HOME", filepath.Join(home, ".config")) + for _, name := range []string{ + age.SopsAgeKeyEnv, + age.SopsAgeKeyFileEnv, + age.SopsAgeKeyCmdEnv, + age.SopsAgeSshPrivateKeyFileEnv, + age.SopsAgeSshPrivateKeyCmdEnv, + } { + // t.Setenv registers the cleanup that restores the original value. + t.Setenv(name, "") + require.NoError(t, os.Unsetenv(name)) + } +} + +// newTestAgeKey generates a throwaway age identity, makes it available for +// decryption through SOPS_AGE_KEY, and returns the matching master key. +func newTestAgeKey(t *testing.T) *age.MasterKey { + t.Helper() + identity, err := fage.GenerateX25519Identity() + require.NoError(t, err) + identities := []string{identity.String()} + if existing, ok := os.LookupEnv(age.SopsAgeKeyEnv); ok && existing != "" { + identities = append([]string{existing}, identities...) + } + t.Setenv(age.SopsAgeKeyEnv, strings.Join(identities, "\n")) + key, err := age.MasterKeyFromRecipient(identity.Recipient().String()) + require.NoError(t, err) + return key +} + +func testYAMLStore() common.Store { + return common.StoreForFormat(formats.Yaml, config.NewStoresConfig()) +} + +func testKeyServices() []keyservice.KeyServiceClient { + return []keyservice.KeyServiceClient{keyservice.NewLocalClient()} +} + +// encryptTestFile writes plaintext YAML to a temporary file, encrypts it in +// place with the given key groups, and returns the file's path. +func encryptTestFile(t *testing.T, plaintext string, groups []sops.KeyGroup) string { + t.Helper() + path := filepath.Join(t.TempDir(), "secret.yaml") + require.NoError(t, os.WriteFile(path, []byte(plaintext), 0600)) + store := testYAMLStore() + encrypted, err := encrypt(encryptOpts{ + Cipher: aes.NewCipher(), + InputStore: store, + OutputStore: store, + InputPath: path, + KeyServices: testKeyServices(), + encryptConfig: encryptConfig{ + KeyGroups: groups, + UnencryptedSuffix: sops.DefaultUnencryptedSuffix, + }, + }) + require.NoError(t, err) + require.NoError(t, os.WriteFile(path, encrypted, 0600)) + return path +} + +// decryptTestFile decrypts the file at path and returns the plaintext YAML. +func decryptTestFile(t *testing.T, path string) string { + t.Helper() + store := testYAMLStore() + plaintext, err := decrypt(decryptOpts{ + Cipher: aes.NewCipher(), + InputStore: store, + OutputStore: store, + InputPath: path, + KeyServices: testKeyServices(), + }) + require.NoError(t, err) + return string(plaintext) +}