feat: warn when system keyring is unavailable#3357
feat: warn when system keyring is unavailable#3357acaloiaro wants to merge 1 commit intogopasspw:masterfrom
Conversation
11a28f0 to
380d679
Compare
Signed-off-by: Adriano Caloiaro <code@adriano.fyi>
dominikschulz
left a comment
There was a problem hiding this comment.
LGTM. Please fix the lint issue and then we're good.
There was a problem hiding this comment.
Pull request overview
Adds user-facing warnings around age passphrase keychain caching when the OS keyring is unavailable or when runtime keyring writes fail, and threads context.Context through the caching and askpass call paths to support those warnings.
Changes:
- Extend
cacher.SetandaskPass.Passphraseto acceptcontext.Context, and update call sites. - Emit a one-time warning when
age.usekeychainis enabled but no OS keyring provider is available. - Warn when writing a passphrase to the OS keyring fails, and add targeted tests for keyring/get/set + warning behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/cache/inmem.go | Updates in-memory cache Set signature to accept context.Context (unused) for interface compatibility. |
| internal/cache/inmem_test.go | Updates tests to pass t.Context() into Set. |
| internal/backend/crypto/age/keyring.go | Passes ctx into askPass.Passphrase within password callback wiring. |
| internal/backend/crypto/age/identities.go | Passes ctx into askPass.Passphrase in identity load/save flows. |
| internal/backend/crypto/age/decrypt.go | Passes ctx into askPass.Passphrase in decrypt fallback flow. |
| internal/backend/crypto/age/askpass.go | Adds one-time “keyring unavailable” warning, adds runtime keyring-write warning, and threads ctx into cache Set. |
| internal/backend/crypto/age/askpass_test.go | Adds tests covering askPass initialization, OS keyring get/set behavior, and user-facing warning output. |
| return sec, true | ||
| } | ||
|
|
||
| func (o *osKeyring) Set(name, value string) { | ||
| func (o *osKeyring) Set(ctx context.Context, name, value string) { | ||
| if err := keyring.Set("gopass", name, value); err != nil { |
There was a problem hiding this comment.
osKeyring.Get currently uses o.knownKeys[name] = true (just above this return), but the parameter is named key. This will not compile and also prevents tracking retrieved keys. Use the key variable (or rename the parameter consistently) when updating knownKeys.
| if err := keyring.Set("gopass", name, value); err != nil { | ||
| debug.Log("failed to set %s: %w", name, err) | ||
| out.Warningf(ctx, "Failed to cache passphrase in OS keyring: %s", err) |
There was a problem hiding this comment.
debug.Log ultimately formats via fmt.Fprintf, where the %w verb isn’t supported (it will render as %!w(...)). Use %v/%s for logging errors here (and similarly in the OS keyring get/set log messages).
Summary
age.usekeychainis enabled but no OS keyring provider is available, so users understand why passphrase caching won't persist across sessionscacher.SetandaskPass.Passphrasesignatures now accept acontext.Contextto support the warning outputaskPassinitialization,osKeyringget/set paths, and the user-facing warning