From 2d5a29725958dd557427ed3a8586459012801534 Mon Sep 17 00:00:00 2001 From: Nicola Sella Date: Mon, 29 Jun 2026 18:20:57 +0200 Subject: [PATCH] passdriver: fix errors "no such secret" errors When gpg --decrypt fails for any reason other than the file being absent, Lookup() was returning ErrNoSuchSecret. GPG's stderr was also discarded, making the real cause invisible. Four issues found: - Lookup: use fileutils.Exists to separate "file absent" (ErrNoSuchSecret) from "decrypt failed" - gpg: always capture stderr and fold it into the returned error so all callers get diagnostic output on failure - Store: check file existence via fileutils.Exists instead of Lookup, so a secret that exists but is temporarily unreadable is not silently overwritten - List: skip directories and non-.gpg files. The previous slice would panic on or return as garbage IDs Fixes: https://github.com/podman-container-tools/podman/issues/28938 Signed-off-by: Nicola Sella --- common/pkg/secrets/passdriver/passdriver.go | 38 +++++++++---- .../pkg/secrets/passdriver/passdriver_test.go | 55 +++++++++++++++++++ 2 files changed, 83 insertions(+), 10 deletions(-) diff --git a/common/pkg/secrets/passdriver/passdriver.go b/common/pkg/secrets/passdriver/passdriver.go index 92f3a2711a..68209d27a2 100644 --- a/common/pkg/secrets/passdriver/passdriver.go +++ b/common/pkg/secrets/passdriver/passdriver.go @@ -102,9 +102,12 @@ func (d *Driver) List() (secrets []string, err error) { return nil, fmt.Errorf("failed to read secret directory: %w", err) } for _, f := range files { - fileName := f.Name() - withoutSuffix := fileName[:len(fileName)-len(".gpg")] - secrets = append(secrets, withoutSuffix) + if f.IsDir() { + continue + } + if name, ok := strings.CutSuffix(f.Name(), ".gpg"); ok { + secrets = append(secrets, name) + } } sort.Strings(secrets) return secrets, nil @@ -117,8 +120,16 @@ func (d *Driver) Lookup(id string) ([]byte, error) { if err != nil { return nil, err } + // Check file existence before invoking GPG. GPG exits non-zero both when + // the file is missing and when decryption fails (e.g. expired agent cache) + if err := fileutils.Exists(key); err != nil { + if errors.Is(err, os.ErrNotExist) { + return nil, fmt.Errorf("%s: %w", id, define.ErrNoSuchSecret) + } + return nil, fmt.Errorf("accessing secret %s: %w", id, err) + } if err := d.gpg(context.TODO(), nil, out, "--decrypt", key); err != nil { - return nil, fmt.Errorf("%s: %w", id, define.ErrNoSuchSecret) + return nil, fmt.Errorf("decrypting secret %s: %w", id, err) } if out.Len() == 0 { return nil, fmt.Errorf("%s: %w", id, define.ErrNoSuchSecret) @@ -128,14 +139,14 @@ func (d *Driver) Lookup(id string) ([]byte, error) { // Store saves the bytes associated with an ID. An error is returned if the ID already exists. func (d *Driver) Store(id string, data []byte) error { - if _, err := d.Lookup(id); err == nil { - return fmt.Errorf("%s: %w", id, define.ErrSecretIDExists) - } - in := bytes.NewReader(data) key, err := d.getPath(id) if err != nil { return err } + if err := fileutils.Exists(key); err == nil { + return fmt.Errorf("%s: %w", id, define.ErrSecretIDExists) + } + in := bytes.NewReader(data) return d.gpg(context.TODO(), in, nil, "--encrypt", "-r", d.KeyID, "-o", key) } @@ -162,8 +173,15 @@ func (d *Driver) gpg(ctx context.Context, in io.Reader, out io.Writer, args ...s cmd.Env = os.Environ() cmd.Stdin = in cmd.Stdout = out - cmd.Stderr = io.Discard - return cmd.Run() + var errBuf bytes.Buffer + cmd.Stderr = &errBuf + if err := cmd.Run(); err != nil { + if stderr := strings.TrimSpace(errBuf.String()); stderr != "" { + return fmt.Errorf("%w: %s", err, stderr) + } + return err + } + return nil } func (d *Driver) getPath(id string) (string, error) { diff --git a/common/pkg/secrets/passdriver/passdriver_test.go b/common/pkg/secrets/passdriver/passdriver_test.go index b3664de5a1..aad72bec2a 100644 --- a/common/pkg/secrets/passdriver/passdriver_test.go +++ b/common/pkg/secrets/passdriver/passdriver_test.go @@ -3,6 +3,8 @@ package passdriver import ( "context" "fmt" + "os" + "path/filepath" "testing" "github.com/stretchr/testify/require" @@ -170,3 +172,56 @@ func TestDelete(t *testing.T) { }) } } + +// TestLookupDecryptFailureNotMasked reproduces the core bug from issue 28938: +// a GPG decrypt failure must not be reported as ErrNoSuchSecret. +func TestLookupDecryptFailureNotMasked(t *testing.T) { + driver := setupDriver(t) + + err := driver.Store("corrupted", []byte("secret-data")) + require.NoError(t, err) + + // Corrupt the .gpg file so GPG cannot decrypt it. + key, err := driver.getPath("corrupted") + require.NoError(t, err) + err = os.WriteFile(key, []byte("not-valid-gpg-data"), 0o644) + require.NoError(t, err) + + // Lookup must fail with a decrypt error, not ErrNoSuchSecret. + _, err = driver.Lookup("corrupted") + require.Error(t, err) + require.NotErrorIs(t, err, define.ErrNoSuchSecret, + "Lookup must not report 'no such secret' when the .gpg file exists but decryption fails") +} + +// TestListFiltersNonGpgEntries verifies that List skips directories and +// non-.gpg files instead of panicking or returning garbage IDs. +func TestListFiltersNonGpgEntries(t *testing.T) { + driver := setupDriver(t) + require.NoError(t, driver.Store("valid", []byte("data"))) + + // Add a non-.gpg file and a subdirectory to the store root. + err := os.WriteFile(filepath.Join(driver.Root, "readme.txt"), []byte("hi"), 0o644) + require.NoError(t, err) + err = os.Mkdir(filepath.Join(driver.Root, "subdir"), 0o755) + require.NoError(t, err) + + list, err := driver.List() + require.NoError(t, err) + require.Equal(t, []string{"valid"}, list, + "List should only return IDs from .gpg files, skipping directories and other files") +} + +// TestStoreExistenceCheckWithoutGpg verifies that Store detects duplicate IDs +// via os.Stat rather than a GPG decrypt, and returns ErrSecretIDExists. +func TestStoreExistenceCheckWithoutGpg(t *testing.T) { + driver := setupDriver(t) + + err := driver.Store("exists", []byte("data")) + require.NoError(t, err) + + // Storing the same ID again must fail with ErrSecretIDExists. + err = driver.Store("exists", []byte("other")) + require.Error(t, err) + require.ErrorIs(t, err, define.ErrSecretIDExists) +}