From 5484073535313e5b1b9afd38e9047c86897bd5dd Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Mon, 18 May 2026 17:43:25 -0400 Subject: [PATCH 1/2] =?UTF-8?q?fix:=20post-merge=20audit=20remediation=20?= =?UTF-8?q?=E2=80=94=20=C2=A71.12=20positional=20leak=20+=20=C2=A71.8=20ke?= =?UTF-8?q?ychain=20fail-loud=20[INT-443]?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Post-merge Codex audit of INT-442 (#93) found defects the pre-merge daemon APPROVE missed (the codex-pass-batching loop-3 final pass was skipped): Blockers: - init / set-credential used cobra.NoArgs, which quotes args[0] in its error — a fat-fingered `nrq init NRAK-...` would echo the literal API key to stderr and logs. New root.NoPositionalArgs validator rejects with a static message that never contains the value. - keychainRead folded every `security` failure (timeout / locked / denied) into not-found, so a real legacy Keychain secret could be silently skipped, a divergence missed, and the original left behind. It is now tri-state and discover() fails loud on a non-not-found error so the one-time migration never runs on a partial view. Majors: - config clear --all now also scrubs the legacy macOS Keychain accounts (keychain.ScrubLegacyKeychain), closing the same restore-on-next-Open() gap the plaintext-file scrub closed. - the equal-multi-source idempotent path now records a change and signals once when legacy originals are scrubbed (deleting the user's plaintext file / Keychain item is itself a one-time migration side effect). - api.New() restored as a deprecated shim returning an actionable ErrNewRemoved (api/ stays pure) instead of a bare compile break. Minor/Nit: doc wording no longer overstates the file-backend posture ("never on disk" -> "never in plaintext, never in config.yml"); the init --overwrite example is no longer self-contradictory. Regression tests: positional-arg no-echo (both ingress commands), the scrub-only signal, and the updated discover() signature. Closes #94 --- CLAUDE.md | 12 ++-- README.md | 4 +- api/client.go | 21 +++++++ internal/cmd/configcmd/config.go | 12 +++- internal/cmd/initcmd/init.go | 5 +- internal/cmd/root/root.go | 19 ++++++ internal/keychain/migrate.go | 97 +++++++++++++++++++++++++++---- internal/keychain/migrate_test.go | 30 +++++++++- internal/noleak/noleak_test.go | 23 ++++++++ 9 files changed, 201 insertions(+), 22 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 07f07a6..647f5e8 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -4,7 +4,7 @@ This file provides guidance for AI agents working with the newrelic-cli codebase ## Project Overview -newrelic-cli is a command-line interface for New Relic written in Go. It uses the Cobra framework for commands and provides a public `api/` package that can be imported as a Go library. The CLI supports multiple output formats (table, JSON, plain). The API key is stored only in the OS keyring via the shared `cli-common/credstore` (macOS Keychain / Windows Credential Manager / Linux Secret Service); non-secret `account_id`/`region` live in `~/.config/newrelic-cli/config.yml`. See "Credentials" below. +newrelic-cli is a command-line interface for New Relic written in Go. It uses the Cobra framework for commands and provides a public `api/` package that can be imported as a Go library. The CLI supports multiple output formats (table, JSON, plain). The API key is stored in the OS keyring via the shared `cli-common/credstore` (macOS Keychain / Windows Credential Manager / Linux Secret Service), or an encrypted file with the explicit file-backend opt-in — never in plaintext and never in `config.yml`; non-secret `account_id`/`region` live in `~/.config/newrelic-cli/config.yml`. See "Credentials" below. ## Quick Commands @@ -308,10 +308,12 @@ The View struct handles all output formatting. To add a new format: ## Credentials (Secret-Handling Standard §2.5) -The API key is stored **only** in the OS keyring via `cli-common/credstore` -under ref `newrelic-cli/default`, key `api_key` (one logical credential, one -key — §1.3). It is **never** on disk and **never** read from the environment -at runtime. `account_id`/`region` are non-secret and live in +The API key is stored in the OS keyring via `cli-common/credstore` under ref +`newrelic-cli/default`, key `api_key` (one logical credential, one key — +§1.3), or — with the explicit `keyring.backend: file` opt-in — an encrypted +file. It is **never** stored in plaintext, **never** in `config.yml`, and +**never** read from the environment at runtime. `account_id`/`region` are +non-secret and live in `~/.config/newrelic-cli/config.yml` alongside `credential_ref` and the optional `keyring.backend: file` opt-in. diff --git a/README.md b/README.md index 277b1e3..c240fee 100644 --- a/README.md +++ b/README.md @@ -15,7 +15,7 @@ A command-line interface for interacting with New Relic APIs. - **Synthetic Monitors**: List and inspect synthetic monitoring configurations - **Users**: List and view user details - **Multiple Output Formats**: Table, JSON, and plain (scriptable) output -- **Secure Credential Storage**: API key stored in the OS keyring (macOS Keychain / Windows Credential Manager / Linux Secret Service), never on disk +- **Secure Credential Storage**: API key stored in the OS keyring (macOS Keychain / Windows Credential Manager / Linux Secret Service), or an encrypted file with the explicit file-backend opt-in — never in plaintext and never in `config.yml` ## Installation @@ -117,7 +117,7 @@ go install github.com/open-cli-collective/newrelic-cli/cmd/nrq@latest ## Quick Start ```bash -# 1. First-time setup (API key stored in the OS keyring, never on disk) +# 1. First-time setup (API key stored in the OS keyring — never in plaintext, never in config.yml) nrq init # 2. Verify configuration diff --git a/api/client.go b/api/client.go index 8ac24f7..57e05a2 100644 --- a/api/client.go +++ b/api/client.go @@ -3,12 +3,33 @@ package api import ( "bytes" "encoding/json" + "errors" "fmt" "io" "net/http" "time" ) +// ErrNewRemoved is returned by the deprecated New shim. It exists so an +// external importer of this pre-1.0 package gets an actionable runtime +// error instead of a bare compile break, and so the credential-resolution +// move is self-documenting at the old call site. +var ErrNewRemoved = errors.New( + "api.New() has been removed: the API key now resolves from the OS keyring " + + "via `nrq init` / `nrq set-credential`, not from this package. " + + "Construct the client with api.NewWithConfig(api.ClientConfig{...}) " + + "using values you resolve yourself") + +// New is deprecated and non-functional. +// +// Deprecated: New previously resolved credentials from the environment and +// config file. Per Secret-Handling Standard §2.5/§1.11 the api/ package no +// longer reads the keyring, environment, or config, nor runs the §1.8 +// migration (that is a command-layer side effect — see NewWithConfig). This +// shim only returns ErrNewRemoved so existing importers fail clearly. Use +// NewWithConfig. +func New() (*Client, error) { return nil, ErrNewRemoved } + // Region represents a New Relic region type Region string diff --git a/internal/cmd/configcmd/config.go b/internal/cmd/configcmd/config.go index 95dda0e..acb0884 100644 --- a/internal/cmd/configcmd/config.go +++ b/internal/cmd/configcmd/config.go @@ -10,6 +10,7 @@ import ( "fmt" "io" "os" + "runtime" "strings" "github.com/spf13/cobra" @@ -67,7 +68,7 @@ Examples: op read "op://Vault/New Relic/api key" | nrq set-credential --key api_key --stdin nrq set-credential --key api_key --from-env NEWRELIC_API_KEY nrq set-credential --ref newrelic-cli/default --key api_key --stdin`, - Args: cobra.NoArgs, + Args: root.NoPositionalArgs, // never echo a fat-fingered secret (§1.12) RunE: func(cmd *cobra.Command, _ []string) error { return runSetCredential(o) }, @@ -477,6 +478,9 @@ func runClear(o *clearOptions) error { if _, statErr := os.Stat(config.LegacyCredentialsPath()); statErr == nil { v.Println("would remove: " + config.LegacyCredentialsPath() + " (legacy plaintext)") } + if runtime.GOOS == "darwin" { + v.Println("would remove: legacy macOS Keychain accounts for service \"newrelic-cli\" (if present)") + } } return nil } @@ -513,6 +517,12 @@ func runClear(o *clearOptions) error { v.Success("Removed %s (legacy plaintext)", lp) } } + // Also scrub the legacy macOS Keychain accounts: otherwise a + // pre-migration `clear --all` on macOS is silently undone by the + // next Open() re-migrating the surviving Keychain item. + if err := keychain.ScrubLegacyKeychain(); err != nil { + return fmt.Errorf("remove legacy macOS Keychain accounts: %w", err) + } } return nil } diff --git a/internal/cmd/initcmd/init.go b/internal/cmd/initcmd/init.go index 9d1a8d0..9f0062d 100644 --- a/internal/cmd/initcmd/init.go +++ b/internal/cmd/initcmd/init.go @@ -55,8 +55,9 @@ and region are non-secret and written to config.yml.`, nrq init --api-key-from-env NEWRELIC_API_KEY --account-id 12345 # Resolve a one-time migration conflict by forcing the legacy value - nrq init --overwrite --api-key-stdin`, - Args: cobra.NoArgs, + # (no ingress flag — stdin/env would replace the forced legacy value) + nrq init --overwrite`, + Args: root.NoPositionalArgs, // never echo a fat-fingered API key (§1.12) RunE: func(cmd *cobra.Command, _ []string) error { return runInit(o) }, diff --git a/internal/cmd/root/root.go b/internal/cmd/root/root.go index b93e3a0..a73ac84 100644 --- a/internal/cmd/root/root.go +++ b/internal/cmd/root/root.go @@ -1,6 +1,7 @@ package root import ( + "errors" "io" "os" @@ -150,3 +151,21 @@ func RegisterAll(cmd *cobra.Command, opts *Options, fns ...RegisterFunc) { fn(cmd, opts) } } + +// NoPositionalArgs is a cobra Args validator for the secret-ingress commands +// (init, set-credential). cobra.NoArgs formats its error as +// `unknown command %q for %q`, quoting args[0] — so `nrq init NRAK-xxx` +// would echo the fat-fingered API key to stderr and any logs (§1.12). This +// rejects positional args with a STATIC message that never contains the +// argument value. +func NoPositionalArgs(_ *cobra.Command, args []string) error { + if len(args) > 0 { + return errNoPositionalArgs + } + return nil +} + +var errNoPositionalArgs = errors.New( + "this command takes no positional arguments; the API key must be provided " + + "via --api-key-stdin, --api-key-from-env, or an interactive prompt — " + + "never as a command-line argument (§1.5.1)") diff --git a/internal/keychain/migrate.go b/internal/keychain/migrate.go index 26926b1..224ad72 100644 --- a/internal/keychain/migrate.go +++ b/internal/keychain/migrate.go @@ -82,7 +82,13 @@ type discovered struct { // `--overwrite` path) forces a legacy api_key over an existing keyring entry; // it cannot resolve a legacy-vs-legacy disagreement — the user must pick. func migrateLegacyOverwrite(s *Store, cfg *config.Config, overwrite bool) error { - d := discover() + d, err := discover() + if err != nil { + // A legacy source could not be read definitively (locked/denied/ + // timed-out Keychain). Fail loud (§1.8): never migrate on a partial + // view that could miss a divergence or strand the original. + return err + } if len(d.secrets) == 0 && len(d.nonSecrets) == 0 { return nil // nothing legacy anywhere — the steady state } @@ -156,6 +162,11 @@ func migrateLegacyOverwrite(s *Store, cfg *config.Config, overwrite bool) error } else { if plan.movedSecret { credstore.EmitMigrationStderr(secretField, s.ref) + } else if plan.scrubbedOnly { + fmt.Fprintf(os.Stderr, + "removed legacy %s credential source(s) for %s "+ + "(the keyring already held it); this is a one-time operation\n", + secretField, s.ref) } for _, f := range plan.movedNonSecret { // Distinct human line: non-secret moves go to config.yml, @@ -181,6 +192,12 @@ type migrationPlan struct { // replacedExisting: --overwrite forced a legacy value over a DIFFERENT // pre-existing keyring value (destructive — warn the user). replacedExisting bool + // scrubbedOnly: the keyring already held the (equal) secret so nothing + // was written, but legacy originals existed and are being deleted. That + // removal is itself a one-time migration side effect and must be + // signaled once (§1.8 — a silent scrub of the user's plaintext file or + // legacy Keychain item is still a migration the user should learn of). + scrubbedOnly bool } // planMigration is the pure §1.8 resolver. It performs NO I/O (`current` @@ -209,8 +226,21 @@ func planMigration(service, profile, ref string, cfg *config.Config, d discovere case hasTarget && !overwrite && secretDisagrees(distinct, target): return migrationPlan{}, secretConflictErr(service, profile, ref, d.secrets, hasTarget) case hasTarget && !secretDisagrees(distinct, target): - // Already migrated (values match): no write, no signal. The - // leftover originals are still cleaned up by the deleters. + // Already migrated (values match): no write. But if legacy + // originals still exist they are about to be scrubbed by the + // deleters — that IS a one-time migration side effect, so + // record it and signal once (§1.8). + if len(d.deleters) > 0 { + p.scrubbedOnly = true + locs := make([]string, 0, len(d.secrets)) + for _, c := range d.secrets { + locs = append(locs, c.location) + } + p.changes = append(p.changes, credstore.MigrationJSONEntry( + secretField, strings.Join(locs, ", "), + fmt.Sprintf("keyring:%s/%s/%s (already present; legacy copies removed)", + service, profile, KeyAPIKey))) + } default: p.writeSecret = true p.secretValue = d.secrets[0].value @@ -308,12 +338,16 @@ func secretConflictErr(service, profile, ref string, group []secretCandidate, ha // file path is the released layout — $XDG_CONFIG_HOME/newrelic-cli/credentials // else ~/.config/newrelic-cli/credentials — on Linux AND Windows alike (the // legacy code has no %APPDATA% branch, so no speculative Windows path). -func discover() discovered { +func discover() (discovered, error) { var d discovered if runtime.GOOS == "darwin" && os.Getenv(legacyKeychainScanDisabledEnv) == "" { var kcAccounts []string - if v, ok := keychainRead(legacyKeychainService, secretField); ok { + v, ok, err := keychainRead(legacyKeychainService, secretField) + if err != nil { + return discovered{}, err + } + if ok { d.secrets = append(d.secrets, secretCandidate{ location: fmt.Sprintf("keychain:%s/%s", legacyKeychainService, secretField), value: v, @@ -321,7 +355,11 @@ func discover() discovered { kcAccounts = append(kcAccounts, secretField) } for _, field := range nonSecretFields { - if v, ok := keychainRead(legacyKeychainService, field); ok { + v, ok, err := keychainRead(legacyKeychainService, field) + if err != nil { + return discovered{}, err + } + if ok { d.nonSecrets = append(d.nonSecrets, nonSecretCandidate{ field: field, value: v, @@ -390,7 +428,28 @@ func discover() discovered { }) } } - return d + return d, nil +} + +// ScrubLegacyKeychain best-effort deletes nrq's legacy macOS Keychain +// accounts (service "newrelic-cli": api_key/account_id/region). It is a +// no-op on non-darwin and when the test seam disables the scan. `config +// clear --all` calls it so a pre-migration wipe is not silently undone by +// the next Open() re-migrating a surviving legacy Keychain item — the same +// restore-on-next-Open() gap the plaintext-file scrub closes. keychainDelete +// already maps errSecItemNotFound to nil, so an absent account is success; +// every account is attempted and the full failure set is returned. +func ScrubLegacyKeychain() error { + if runtime.GOOS != "darwin" || os.Getenv(legacyKeychainScanDisabledEnv) != "" { + return nil + } + var errs []error + for _, a := range append([]string{secretField}, nonSecretFields...) { + if err := keychainDelete(legacyKeychainService, a); err != nil { + errs = append(errs, err) + } + } + return errors.Join(errs...) } // legacyCredentialsPath delegates to config.LegacyCredentialsPath so the @@ -423,19 +482,35 @@ func readLegacyFile(path string) map[string]string { // with a clear error rather than block the CLI's first run indefinitely. const securityTimeout = 5 * time.Second -func keychainRead(service, account string) (string, bool) { +// keychainRead is tri-state: (value, true, nil) when present; ("", false, +// nil) ONLY when `security` definitively reports the item absent +// (errSecItemNotFound) or it is present-but-empty; ("", false, err) for any +// other failure — timeout, locked Keychain, access denial, SIP/MDM. A +// non-not-found failure must NOT be folded into "absent": that would let +// §1.8 silently skip a real legacy secret, miss a divergence with the file +// or keyring, and leave the original behind. +func keychainRead(service, account string) (string, bool, error) { ctx, cancel := context.WithTimeout(context.Background(), securityTimeout) defer cancel() out, err := exec.CommandContext(ctx, "security", "find-generic-password", "-s", service, "-a", account, "-w").Output() //nolint:gosec // darwin-only migration probe of nrq's own legacy items if err != nil { - return "", false + var ee *exec.ExitError + if errors.As(err, &ee) && ee.ExitCode() == securityErrItemNotFound { + return "", false, nil // definitively absent — not an error + } + if ctx.Err() != nil { + return "", false, fmt.Errorf( + "read legacy keychain item %s/%s timed out after %s (locked Keychain or unlock prompt?): %w", + service, account, securityTimeout, ctx.Err()) + } + return "", false, fmt.Errorf("read legacy keychain item %s/%s: %w", service, account, err) } v := strings.TrimRight(string(out), "\r\n") if v == "" { - return "", false + return "", false, nil // present-but-empty: nothing to migrate } - return v, true + return v, true, nil } // securityErrItemNotFound is `security`'s exit status when the item is absent diff --git a/internal/keychain/migrate_test.go b/internal/keychain/migrate_test.go index e765d18..fb08a2c 100644 --- a/internal/keychain/migrate_test.go +++ b/internal/keychain/migrate_test.go @@ -54,6 +54,33 @@ func TestPlanMigration_EqualMultiSource_Idempotent(t *testing.T) { assert.Empty(t, p.changes) } +// When the keyring already holds the (equal) secret but legacy originals +// still exist and will be scrubbed, that scrub IS a one-time migration side +// effect: planMigration must flag scrubbedOnly and record a change so the +// signal fires once (§1.8). No deleters → genuinely nothing to signal. #M2. +func TestPlanMigration_EqualButLegacyPresent_SignalsScrub(t *testing.T) { + d := discovered{ + secrets: []secretCandidate{{location: "file:/p#api_key", value: "NRAK-X"}}, + deleters: []labeledDeleter{{label: "file /p", del: func() error { return nil }}}, + } + p, err := planMigration(svc, prof, ref, &config.Config{}, d, target("NRAK-X"), false) + require.NoError(t, err) + assert.False(t, p.writeSecret, "value already present — no write") + assert.False(t, p.movedSecret) + assert.True(t, p.scrubbedOnly, "scrubbing legacy originals is a one-time side effect") + require.Len(t, p.changes, 1) + assert.Equal(t, "api_key", p.changes[0].Field) + assert.Contains(t, p.changes[0].To, "legacy copies removed") + + // No deleters present → truly nothing happened → no signal. + p2, err := planMigration(svc, prof, ref, &config.Config{}, + discovered{secrets: []secretCandidate{{location: "file:/p#api_key", value: "NRAK-X"}}}, + target("NRAK-X"), false) + require.NoError(t, err) + assert.False(t, p2.scrubbedOnly) + assert.Empty(t, p2.changes) +} + func TestPlanMigration_SecretDivergence_KeychainVsFile_FailsNamingAll(t *testing.T) { d := discovered{secrets: []secretCandidate{ {location: "keychain:newrelic-cli/api_key", value: "NRAK-A"}, @@ -206,7 +233,8 @@ func TestDiscover_LabelsLegacyFileSource(t *testing.T) { require.NoError(t, os.MkdirAll(legacyDir, 0o700)) require.NoError(t, os.WriteFile(filepath.Join(legacyDir, "credentials"), []byte("api_key=NRAK-x\n"), 0o600)) - d := discover() + d, err := discover() + require.NoError(t, err) require.NotEmpty(t, d.deleters) var found bool for _, ld := range d.deleters { diff --git a/internal/noleak/noleak_test.go b/internal/noleak/noleak_test.go index 8006f6f..d834908 100644 --- a/internal/noleak/noleak_test.go +++ b/internal/noleak/noleak_test.go @@ -391,6 +391,29 @@ func TestConfigClear_DryRunAndAll(t *testing.T) { assert.NoError(t, err, "clear is idempotent") } +// §1.12: a fat-fingered API key passed as a POSITIONAL argument must never +// be echoed back. cobra.NoArgs quotes args[0] in its error; the custom +// root.NoPositionalArgs validator must reject with a static message that +// contains neither the value nor any quoted arg. Both secret-ingress +// commands (init, set-credential). #B1. +func TestNoLeak_PositionalArgRejected_NoEcho(t *testing.T) { + cases := [][]string{ + {"init", sentinel}, + {"set-credential", "--key", "api_key", sentinel}, + } + for _, args := range cases { + t.Run(args[0], func(t *testing.T) { + testutil.Setup(t) + out, errOut, err := run(t, args...) + require.Error(t, err, "a positional arg must be rejected") + assert.NotContains(t, out, sentinel, "stdout must not echo the arg") + assert.NotContains(t, errOut, sentinel, "stderr must not echo the arg") + assert.NotContains(t, err.Error(), sentinel, "error must not contain the value") + assert.Contains(t, err.Error(), "no positional arguments") + }) + } +} + // §1.7/§1.8: `clear --all` must also scrub the legacy plaintext credentials // file. Otherwise a workstation decommission that never ran the one-time // migration leaves the secret on disk and the very next Open() re-migrates From af4b8fc0fc26ba3575850ff312cdda2b14767440 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Mon, 18 May 2026 17:49:33 -0400 Subject: [PATCH 2/2] =?UTF-8?q?fix:=20address=20Codex=20re-review=20?= =?UTF-8?q?=E2=80=94=20help-text=20wording,=20generic=20no-args=20message,?= =?UTF-8?q?=20security=20exec=20seam=20[INT-443]?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - root/init Long help no longer says "never on disk" (file backend is on disk, encrypted) — now "never plaintext, never in config.yml" - root.NoPositionalArgs message is backend-generic (shared by set-credential, which uses --stdin/--from-env, not --api-key-*) - extract a securityRun seam classifying exit-code/timeout so keychainRead tri-state and ScrubLegacyKeychain all-accounts behavior are unit-tested hermetically (the highest-risk §1.8 fail-loud paths) Codex re-review: blockers=0 majors=0 (endorsed the equal-source scrub signal as the correct §1.8 fix). These close the 2 minors + 1 nit. --- internal/cmd/initcmd/init.go | 8 ++-- internal/cmd/root/root.go | 8 ++-- internal/keychain/migrate.go | 79 ++++++++++++++++++++----------- internal/keychain/migrate_test.go | 69 +++++++++++++++++++++++++++ 4 files changed, 128 insertions(+), 36 deletions(-) diff --git a/internal/cmd/initcmd/init.go b/internal/cmd/initcmd/init.go index 9f0062d..b801087 100644 --- a/internal/cmd/initcmd/init.go +++ b/internal/cmd/initcmd/init.go @@ -43,10 +43,10 @@ func Register(rootCmd *cobra.Command, opts *root.Options) { cmd := &cobra.Command{ Use: "init", Short: "First-time setup (stores the API key in the OS keyring)", - Long: `Configure nrq. The API key is stored in the OS keyring (never on -disk) and is ingested ONLY via stdin, a named env var, or an interactive -no-echo prompt — never as a flag/positional literal (§1.5.1). account_id -and region are non-secret and written to config.yml.`, + Long: `Configure nrq. The API key is stored in the OS keyring (never in +plaintext, never in config.yml) and is ingested ONLY via stdin, a named env +var, or an interactive no-echo prompt — never as a flag/positional literal +(§1.5.1). account_id and region are non-secret and written to config.yml.`, Example: ` # Interactive (no-echo API key prompt) nrq init diff --git a/internal/cmd/root/root.go b/internal/cmd/root/root.go index a73ac84..a72136a 100644 --- a/internal/cmd/root/root.go +++ b/internal/cmd/root/root.go @@ -87,7 +87,7 @@ const rootLong = `nrq is a command-line interface for New Relic. It provides commands for managing applications, dashboards, alerts, users, and other New Relic resources. -First-time setup (stores the API key in the OS keyring, never on disk): +First-time setup (API key in the OS keyring — never plaintext, never in config.yml): nrq init Non-interactive credential ingress: @@ -166,6 +166,6 @@ func NoPositionalArgs(_ *cobra.Command, args []string) error { } var errNoPositionalArgs = errors.New( - "this command takes no positional arguments; the API key must be provided " + - "via --api-key-stdin, --api-key-from-env, or an interactive prompt — " + - "never as a command-line argument (§1.5.1)") + "this command takes no positional arguments; a secret must be provided " + + "via stdin, a named environment variable, or an interactive prompt — " + + "never as a command-line argument (see --help; §1.5.1)") diff --git a/internal/keychain/migrate.go b/internal/keychain/migrate.go index 224ad72..ac8c188 100644 --- a/internal/keychain/migrate.go +++ b/internal/keychain/migrate.go @@ -482,6 +482,34 @@ func readLegacyFile(path string) map[string]string { // with a clear error rather than block the CLI's first run indefinitely. const securityTimeout = 5 * time.Second +// securityErrItemNotFound is `security`'s exit status when the item is absent +// (errSecItemNotFound). Only this is idempotent success — a denial/locked +// failure must surface so we don't leave a legacy secret behind. +const securityErrItemNotFound = 44 + +// securityRun executes `security` with args under securityTimeout and +// classifies the outcome: exitCode 0 on success or the clean non-zero exit +// code; timedOut=true if the deadline fired; err only for a non-exit failure +// (binary missing, etc.). It is a package var so tests can drive the +// not-found / denied / timeout classification in keychainRead and +// ScrubLegacyKeychain without shelling out (the highest-risk §1.8 paths). +var securityRun = func(args ...string) (out []byte, exitCode int, timedOut bool, err error) { + ctx, cancel := context.WithTimeout(context.Background(), securityTimeout) + defer cancel() + o, e := exec.CommandContext(ctx, "security", args...).Output() //nolint:gosec // darwin-only migration on nrq's own legacy items + if ctx.Err() != nil { + return nil, -1, true, nil + } + if e != nil { + var ee *exec.ExitError + if errors.As(e, &ee) { + return nil, ee.ExitCode(), false, nil + } + return nil, -1, false, e + } + return o, 0, false, nil +} + // keychainRead is tri-state: (value, true, nil) when present; ("", false, // nil) ONLY when `security` definitively reports the item absent // (errSecItemNotFound) or it is present-but-empty; ("", false, err) for any @@ -490,22 +518,23 @@ const securityTimeout = 5 * time.Second // §1.8 silently skip a real legacy secret, miss a divergence with the file // or keyring, and leave the original behind. func keychainRead(service, account string) (string, bool, error) { - ctx, cancel := context.WithTimeout(context.Background(), securityTimeout) - defer cancel() - out, err := exec.CommandContext(ctx, "security", "find-generic-password", - "-s", service, "-a", account, "-w").Output() //nolint:gosec // darwin-only migration probe of nrq's own legacy items + out, code, timedOut, err := securityRun("find-generic-password", "-s", service, "-a", account, "-w") + if timedOut { + return "", false, fmt.Errorf( + "read legacy keychain item %s/%s timed out after %s (locked Keychain or unlock prompt?)", + service, account, securityTimeout) + } if err != nil { - var ee *exec.ExitError - if errors.As(err, &ee) && ee.ExitCode() == securityErrItemNotFound { - return "", false, nil // definitively absent — not an error - } - if ctx.Err() != nil { - return "", false, fmt.Errorf( - "read legacy keychain item %s/%s timed out after %s (locked Keychain or unlock prompt?): %w", - service, account, securityTimeout, ctx.Err()) - } return "", false, fmt.Errorf("read legacy keychain item %s/%s: %w", service, account, err) } + if code == securityErrItemNotFound { + return "", false, nil // definitively absent — not an error + } + if code != 0 { + return "", false, fmt.Errorf( + "read legacy keychain item %s/%s: security exited %d (access denied or locked Keychain?)", + service, account, code) + } v := strings.TrimRight(string(out), "\r\n") if v == "" { return "", false, nil // present-but-empty: nothing to migrate @@ -513,22 +542,16 @@ func keychainRead(service, account string) (string, bool, error) { return v, true, nil } -// securityErrItemNotFound is `security`'s exit status when the item is absent -// (errSecItemNotFound). Only this is idempotent success — a denial/locked -// failure must surface so we don't leave a legacy secret behind. -const securityErrItemNotFound = 44 - func keychainDelete(service, account string) error { - ctx, cancel := context.WithTimeout(context.Background(), securityTimeout) - defer cancel() - err := exec.CommandContext(ctx, "security", "delete-generic-password", - "-s", service, "-a", account).Run() //nolint:gosec // darwin-only migration cleanup of nrq's own legacy items - if err == nil { - return nil + _, code, timedOut, err := securityRun("delete-generic-password", "-s", service, "-a", account) + if timedOut { + return fmt.Errorf("remove legacy keychain item %s/%s timed out after %s", service, account, securityTimeout) + } + if err != nil { + return fmt.Errorf("remove legacy keychain item %s/%s: %w", service, account, err) } - var ee *exec.ExitError - if errors.As(err, &ee) && ee.ExitCode() == securityErrItemNotFound { - return nil // already absent — fine for idempotent cleanup + if code == 0 || code == securityErrItemNotFound { + return nil // removed, or already absent — idempotent cleanup } - return fmt.Errorf("remove legacy keychain item %s/%s: %w", service, account, err) + return fmt.Errorf("remove legacy keychain item %s/%s: security exited %d", service, account, code) } diff --git a/internal/keychain/migrate_test.go b/internal/keychain/migrate_test.go index fb08a2c..4ab4efd 100644 --- a/internal/keychain/migrate_test.go +++ b/internal/keychain/migrate_test.go @@ -4,6 +4,7 @@ import ( "errors" "os" "path/filepath" + "runtime" "strings" "testing" @@ -313,6 +314,74 @@ func TestRepairContract_EmptyEntry_OverwriteRepairs(t *testing.T) { assert.Equal(t, "NRAK-repaired-value-x", got, "repair must leave a usable key") } +// keychainRead must be tri-state: a definitive not-found is absent (no +// error), but a denial / non-zero / timeout MUST fail loud so §1.8 never +// migrates on a partial view that could miss a divergence. #B1.2 / audit-M2. +func TestKeychainRead_TriState(t *testing.T) { + orig := securityRun + defer func() { securityRun = orig }() + + securityRun = func(...string) ([]byte, int, bool, error) { return []byte("NRAK-present\n"), 0, false, nil } + v, ok, err := keychainRead("svc", "acct") + require.NoError(t, err) + assert.True(t, ok) + assert.Equal(t, "NRAK-present", v) + + securityRun = func(...string) ([]byte, int, bool, error) { return nil, securityErrItemNotFound, false, nil } + _, ok, err = keychainRead("svc", "acct") + require.NoError(t, err, "errSecItemNotFound is definitive absence, not an error") + assert.False(t, ok) + + securityRun = func(...string) ([]byte, int, bool, error) { return nil, 51, false, nil } + _, _, err = keychainRead("svc", "acct") + require.Error(t, err, "a non-not-found exit (denied/locked) must fail loud") + + securityRun = func(...string) ([]byte, int, bool, error) { return nil, -1, true, nil } + _, _, err = keychainRead("svc", "acct") + require.Error(t, err, "a timeout must fail loud") + assert.Contains(t, err.Error(), "timed out") + + securityRun = func(...string) ([]byte, int, bool, error) { return []byte("\n"), 0, false, nil } + _, ok, err = keychainRead("svc", "acct") + require.NoError(t, err) + assert.False(t, ok, "present-but-empty is nothing to migrate") +} + +// ScrubLegacyKeychain must attempt EVERY legacy account (not stop at the +// first), and a not-found is idempotent success. audit-M1. +func TestScrubLegacyKeychain_AttemptsAllAccounts(t *testing.T) { + if runtime.GOOS != "darwin" { + t.Skip("ScrubLegacyKeychain is darwin-only") + } + orig := securityRun + defer func() { securityRun = orig }() + t.Setenv(legacyKeychainScanDisabledEnv, "") // enable the darwin path + + var deleted []string + securityRun = func(args ...string) ([]byte, int, bool, error) { + deleted = append(deleted, args[len(args)-1]) // trailing -a + return nil, securityErrItemNotFound, false, nil + } + require.NoError(t, ScrubLegacyKeychain(), "all-absent is idempotent success") + assert.ElementsMatch(t, append([]string{secretField}, nonSecretFields...), deleted) + + // One account hard-fails: the others are still attempted and the error + // is surfaced (best-effort across all, joined). + deleted = nil + securityRun = func(args ...string) ([]byte, int, bool, error) { + acct := args[len(args)-1] + deleted = append(deleted, acct) + if acct == secretField { + return nil, 51, false, nil // access denied + } + return nil, securityErrItemNotFound, false, nil + } + err := ScrubLegacyKeychain() + require.Error(t, err) + assert.ElementsMatch(t, append([]string{secretField}, nonSecretFields...), deleted, + "a failure on one account must not strand the others") +} + func TestConflictErr_NoValueLeak(t *testing.T) { err := secretConflictErr(svc, prof, ref, []secretCandidate{ {location: "file:/p#api_key", value: "SUPER-SECRET"},