From b92c2a70840ee4e9b5a402aa01abeac9173df405 Mon Sep 17 00:00:00 2001 From: Samuel Gunter Date: Wed, 10 Dec 2025 04:14:03 -0600 Subject: [PATCH] fix(secrets): replace using correct driver, safe replace for new secrets When replacing an existing secret, the delete operation should ran on the old driver, which is not necessarily the new one. When replacing a nonexisting secret (Replace=true), ignore the replace flag, treat it as an entirely new secret. (previous behavior: would attempt to delete a secret whose name or id matches an empty string query, which either failed due to ambiguity or successfully deleted an existing secret if exactly 1 secret was present) Solves issues 1 and 2 of containers/podman#27130 Signed-off-by: Samuel Gunter --- common/pkg/secrets/secrets.go | 33 ++++++----- common/pkg/secrets/secrets_test.go | 91 ++++++++++++++++++++++++++++++ 2 files changed, 111 insertions(+), 13 deletions(-) diff --git a/common/pkg/secrets/secrets.go b/common/pkg/secrets/secrets.go index 1edf766168..3d3a47d407 100644 --- a/common/pkg/secrets/secrets.go +++ b/common/pkg/secrets/secrets.go @@ -199,6 +199,26 @@ func (s *SecretsManager) Store(name string, data []byte, driverType string, opti if options.IgnoreIfExists { return secr.ID, nil } + + if options.Replace { + // We must call Delete using whatever driver the secret was created with, + // which is not necessarily driver where the new value will go + prevDriver, err := getDriver(secr.Driver, secr.DriverOptions) + if err != nil { + return "", err + } + err = prevDriver.Delete(secr.ID) + if err != nil { + if !errors.Is(err, define.ErrNoSuchSecret) { + return "", fmt.Errorf("deleting driver secret %s: %w", secr.ID, err) + } + } else { + if err := s.delete(secr.ID); err != nil && !errors.Is(err, define.ErrNoSuchSecret) { + return "", fmt.Errorf("deleting secret %s: %w", secr.ID, err) + } + } + } + secr.UpdatedAt = time.Now() } else { secr = new(Secret) @@ -227,19 +247,6 @@ func (s *SecretsManager) Store(name string, data []byte, driverType string, opti return "", err } - if options.Replace { - err := driver.Delete(secr.ID) - if err != nil { - if !errors.Is(err, define.ErrNoSuchSecret) { - return "", fmt.Errorf("deleting driver secret %s: %w", secr.ID, err) - } - } else { - if err := s.delete(secr.ID); err != nil && !errors.Is(err, define.ErrNoSuchSecret) { - return "", fmt.Errorf("deleting secret %s: %w", secr.ID, err) - } - } - } - secr.ID, err = s.newID() if err != nil { return "", err diff --git a/common/pkg/secrets/secrets_test.go b/common/pkg/secrets/secrets_test.go index 2f720da1dc..81030b759a 100644 --- a/common/pkg/secrets/secrets_test.go +++ b/common/pkg/secrets/secrets_test.go @@ -2,9 +2,13 @@ package secrets import ( "bytes" + "fmt" "testing" "github.com/stretchr/testify/require" + + "go.podman.io/common/pkg/secrets/filedriver" + "go.podman.io/common/pkg/secrets/shelldriver" ) var drivertype = "file" @@ -300,3 +304,90 @@ func TestSecretList(t *testing.T) { require.NoError(t, err) require.Len(t, allSecrets, 2) } + +// Creating a new secret with Replace=true should not remove other existing secrets. +func TestReplaceNewSecretDoesNotDeleteOthers(t *testing.T) { + manager, _ := setup(t) + + // file driver storage + pathFile := t.TempDir() + fileOpts := StoreOptions{DriverOpts: map[string]string{"path": pathFile}} + + // shell driver storage (different directory) + shellBase := t.TempDir() + shellOptsMap := map[string]string{ + "store": fmt.Sprintf("cat - > %s/${SECRET_ID}", shellBase), + "lookup": fmt.Sprintf("cat %s/${SECRET_ID}", shellBase), + "delete": "true", // emulate a non-zero exit code + "list": "true", + } + shellOpts := StoreOptions{DriverOpts: shellOptsMap, Replace: true} + + // Store first secret using file driver + id1, err := manager.Store("test1", []byte("data1"), "file", fileOpts) + require.NoError(t, err) + require.NotEmpty(t, id1) + + // Store a new secret using shell driver and Replace=true (should not affect test1) + id2, err := manager.Store("test2", []byte("data2"), "shell", shellOpts) + require.NoError(t, err) + require.NotEmpty(t, id2) + + // Both secrets should exist in the manager + all, err := manager.List() + require.NoError(t, err) + require.Len(t, all, 2) + + // Verify file driver data still contains id1 + fd, err := filedriver.NewDriver(pathFile) + require.NoError(t, err) + d1, err := fd.Lookup(id1) + require.NoError(t, err) + require.Equal(t, []byte("data1"), d1) + + // Verify shell storage contains id2 + sd, err := shelldriver.NewDriver(shellOptsMap) + require.NoError(t, err) + d2, err := sd.Lookup(id2) + require.NoError(t, err) + require.Equal(t, []byte("data2"), d2) +} + +func TestReplaceUsesOriginalDriverToDelete(t *testing.T) { + manager, _ := setup(t) + + // Two separate filedriver storage roots + path1 := t.TempDir() + path2 := t.TempDir() + + // Store secret initially in path1 + storeOpts1 := StoreOptions{DriverOpts: map[string]string{"path": path1}} + id1, err := manager.Store("mysecret", []byte("orig"), "file", storeOpts1) + require.NoError(t, err) + require.NotEmpty(t, id1) + + // Ensure the secret entry exists in path1 data file + fd1, err := filedriver.NewDriver(path1) + require.NoError(t, err) + dOrig, err := fd1.Lookup(id1) + require.NoError(t, err) + require.Equal(t, []byte("orig"), dOrig) + + // Replace the secret using a different driver storage path (path2) + storeOpts2 := StoreOptions{DriverOpts: map[string]string{"path": path2}, Replace: true} + id2, err := manager.Store("mysecret", []byte("new"), "file", storeOpts2) + require.NoError(t, err) + require.NotEmpty(t, id2) + require.NotEqual(t, id1, id2) + + // Old id should no longer exist in path1 (deleted via previous driver) + _, err = fd1.Lookup(id1) + require.Error(t, err, "old secret id should no longer exist in path1") + + // New id should exist in path2 + fd2, err := filedriver.NewDriver(path2) + require.NoError(t, err) + dNew, err := fd2.Lookup(id2) + require.NoError(t, err) + require.Equal(t, []byte("new"), dNew) +}