From 4035bab95245393e1c4fb03d8b3eecdcd9617173 Mon Sep 17 00:00:00 2001 From: Samuel Gunter Date: Mon, 15 Dec 2025 19:11:38 -0600 Subject: [PATCH] fix(secrets): allow deletion of tracked secrets which no longer exist When deleting a secret, it's possible the underlying driver no longer holds that secret, in which case an ErrNoSuchSecret error is thrown. In previous versions, this would force the secrets manager to keep a reference to that non-existing secret. Fixes containers/podman#27666 Signed-off-by: Samuel Gunter --- common/pkg/secrets/secrets.go | 3 +- common/pkg/secrets/secrets_test.go | 124 +++++++++++++++++++++++++++++ 2 files changed, 126 insertions(+), 1 deletion(-) diff --git a/common/pkg/secrets/secrets.go b/common/pkg/secrets/secrets.go index 1edf766168..f58ef9ebab 100644 --- a/common/pkg/secrets/secrets.go +++ b/common/pkg/secrets/secrets.go @@ -276,7 +276,8 @@ func (s *SecretsManager) Delete(nameOrID string) (string, error) { } err = driver.Delete(secretID) - if err != nil { + // If the driver does not have that secret, it's considered to be deleted + if err != nil && !errors.Is(err, define.ErrNoSuchSecret) { return "", fmt.Errorf("deleting secret %s: %w", nameOrID, err) } diff --git a/common/pkg/secrets/secrets_test.go b/common/pkg/secrets/secrets_test.go index 2f720da1dc..353ce78a69 100644 --- a/common/pkg/secrets/secrets_test.go +++ b/common/pkg/secrets/secrets_test.go @@ -2,6 +2,10 @@ package secrets import ( "bytes" + "context" + "io" + "os" + "os/exec" "testing" "github.com/stretchr/testify/require" @@ -300,3 +304,123 @@ func TestSecretList(t *testing.T) { require.NoError(t, err) require.Len(t, allSecrets, 2) } + +// Deleting a secret tracked by the manager, which doesn't exist in the driver, +// should succeed without error +func TestDeletePartiallyDeletedSecret(t *testing.T) { + manager, opts := setup(t) + + storeOpts := StoreOptions{ + DriverOpts: opts, + } + + // Create two secrets + _, err := manager.Store("mysecret", []byte("mydata"), drivertype, storeOpts) + require.NoError(t, err) + mysecret2ID, err := manager.Store("mysecret2", []byte("mydata2"), drivertype, storeOpts) + require.NoError(t, err) + + allSecrets, err := manager.List() + require.NoError(t, err) + require.Len(t, allSecrets, 2) + + // Calling delete on the manager should update the count + _, err = manager.Delete("mysecret") + require.NoError(t, err) + + allSecrets, err = manager.List() + require.NoError(t, err) + require.Len(t, allSecrets, 1) + + // Calling delete directly on the driver shouldn't update the manager + fileDriver, err := getDriver("file", opts) + require.NoError(t, err) + + // Simulating someone manually editing the file, + // or another driver returning NoSuchSecret for any other reason + err = fileDriver.Delete(mysecret2ID) + require.NoError(t, err) + + allSecrets, err = manager.List() + require.NoError(t, err) + require.Len(t, allSecrets, 1) + + // If the driver does not have that secret, it's considered to be deleted + _, err = manager.Delete("mysecret2") + require.NoError(t, err) + + allSecrets, err = manager.List() + require.NoError(t, err) + require.Len(t, allSecrets, 0) +} + +// Deleting a secret tracked by the manager, which doesn't exist in the driver, +// should succeed without error +func TestDeletePartiallyDeletedSecretPassDriver(t *testing.T) { + if _, err := exec.LookPath("gpg"); err != nil { + t.Skip("gpg executable not found, skipping pass driver tests") + } + + // Setup similar to passdriver_test.go tests + gpgRoot := t.TempDir() + gpgHomeDir := t.TempDir() + + gpgTestID := "testing@passdriver" + + opts := map[string]string{ + "root": gpgRoot, + "key": gpgTestID, + "gpghomedir": gpgHomeDir, + } + + cmd := exec.CommandContext(context.TODO(), "gpg", "--homedir", gpgHomeDir, "--batch", "--passphrase", "--quick-generate-key", gpgTestID) + cmd.Env = os.Environ() + cmd.Stdin = nil + cmd.Stdout = nil + cmd.Stderr = io.Discard + err := cmd.Run() + require.NoError(t, err) + + manager, _ := setup(t) + + storeOpts := StoreOptions{ + DriverOpts: opts, + } + + // Create two secrets + _, err = manager.Store("mysecret", []byte("mydata"), "pass", storeOpts) + require.NoError(t, err) + _, err = manager.Store("mysecret2", []byte("mydata2"), "pass", storeOpts) + require.NoError(t, err) + + allSecrets, err := manager.List() + require.NoError(t, err) + require.Len(t, allSecrets, 2) + + // Calling delete on the manager should update the count + _, err = manager.Delete("mysecret") + require.NoError(t, err) + + allSecrets, err = manager.List() + require.NoError(t, err) + require.Len(t, allSecrets, 1) + + // Simulating the gpg storage being corrupted/gone + err = os.RemoveAll(gpgRoot) + require.NoError(t, err) + err = os.RemoveAll(gpgHomeDir) + require.NoError(t, err) + + // Manager shouldn't know about the gpg storage being bad + allSecrets, err = manager.List() + require.NoError(t, err) + require.Len(t, allSecrets, 1) + + // If the driver does not have that secret, it's considered to be deleted + _, err = manager.Delete("mysecret2") + require.NoError(t, err) + + allSecrets, err = manager.List() + require.NoError(t, err) + require.Len(t, allSecrets, 0) +}