Skip to content

feat: warn when system keyring is unavailable#3357

Open
acaloiaro wants to merge 1 commit intogopasspw:masterfrom
acaloiaro:acaloiaro/age/warn-when-keyring-unavailable
Open

feat: warn when system keyring is unavailable#3357
acaloiaro wants to merge 1 commit intogopasspw:masterfrom
acaloiaro:acaloiaro/age/warn-when-keyring-unavailable

Conversation

@acaloiaro
Copy link
Copy Markdown

@acaloiaro acaloiaro commented Mar 14, 2026

Summary

  • Emit a one-time warning when age.usekeychain is enabled but no OS keyring provider is available, so users understand why passphrase caching won't persist across sessions
  • Warn when writing a passphrase to the OS keyring fails at runtime
  • The cacher.Set and askPass.Passphrase signatures now accept a context.Context to support the warning output
  • Add tests for askPass initialization, osKeyring get/set paths, and the user-facing warning

@acaloiaro acaloiaro force-pushed the acaloiaro/age/warn-when-keyring-unavailable branch 7 times, most recently from 11a28f0 to 380d679 Compare March 14, 2026 18:13
Signed-off-by: Adriano Caloiaro <code@adriano.fyi>
@acaloiaro acaloiaro marked this pull request as ready for review March 14, 2026 18:14
Copy link
Copy Markdown
Member

@dominikschulz dominikschulz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Please fix the lint issue and then we're good.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.Set and askPass.Passphrase to accept context.Context, and update call sites.
  • Emit a one-time warning when age.usekeychain is 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.

Comment on lines 45 to 49
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 {
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment on lines 49 to +51
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)
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants