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) +}