Phase G: multi-backend secrets (keychain + AWS Secrets Manager)#10
Merged
Conversation
- Add two secret-ref backends on the existing secrets.Backend seam (no
seam changes — the package already named these schemes):
- keychain:// (OS credential store) via go-keyring — macOS Keychain,
Windows Credential Manager, or Linux Secret Service.
keychain://<account> uses service "siphon"; the
keychain://<service>/<account> form addresses any stored credential.
- awssm:// (AWS Secrets Manager) via the AWS SDK, reusing the same
credential chain as S3 storage. awssm://<id>#<json-key> selects one
field of a JSON secret (the common Secrets Manager shape). The SDK
call sits behind a tiny interface so it is faked in tests.
- Register backends in order env → keychain → awssm → passthrough;
passthrough stays last so a literal value with no scheme resolves
as-is. awssm is off by default (building it loads AWS config); enable
via the new secrets.awssm config flag (+ optional awssm_region).
- Errors map to the errs taxonomy: a missing key / unknown field is a
CodeUser error, transport is CodeSystem (like env).
- Test keychain via the keyring mock, awssm via a faked client (whole
secret, JSON #key, not-found, missing field), and the full backend
chain dispatch. Document in docs/OPS.md; README + CHANGELOG note the
Phase G ops pillar is now complete.
|
Warning Review limit reached
More reviews will be available in 49 minutes and 46 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe PR adds keychain and AWS Secrets Manager secret-ref backends, wires them into resolver construction with config gating, and updates tests and docs to reflect the supported schemes and literal fallback behavior. ChangesMulti-backend secrets
Sequence Diagram(s)sequenceDiagram
participant buildDeps
participant buildResolver
participant secrets.Env
participant secrets.Keychain
participant secrets.NewAWSSM
participant secrets.Passthrough
buildDeps->>buildResolver: build resolver from cfg
buildResolver->>secrets.Env: add env backend
buildResolver->>secrets.Keychain: add keychain backend
alt cfg.secrets.awssm
buildResolver->>secrets.NewAWSSM: create AWSSM backend
secrets.NewAWSSM-->>buildResolver: AWSSM backend
end
buildResolver->>secrets.Passthrough: append literal fallback
buildResolver-->>buildDeps: *secrets.Resolver
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
internal/secrets/keychain_test.go (1)
33-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the unresolved-secret sentinel here too.
This only checks
CodeUser, so a regression that stops returningerrs.ErrSecretUnresolvedwould still pass even though callers can match that sentinel.Suggested fix
var e *errs.Error if !errors.As(err, &e) || e.Code != errs.CodeUser { t.Fatalf("missing key err = %v, want CodeUser", err) } + if !errors.Is(err, errs.ErrSecretUnresolved) { + t.Fatalf("missing key err = %v, want ErrSecretUnresolved", err) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/secrets/keychain_test.go` around lines 33 - 39, The Keychain test only verifies that Resolve returns a user error, so it can miss regressions where the unresolved-secret sentinel is no longer returned. Update TestKeychain_NotFoundIsUserError to also assert that the error matches errs.ErrSecretUnresolved in addition to checking errs.CodeUser. Use the existing Resolve method and the err returned from keychain://missing to confirm both the code and sentinel remain intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/secrets/awssm.go`:
- Around line 66-68: Reject empty `#` selectors in `ParseAWSSMRef`/the
`strings.IndexByte(rest, '#')` parsing path instead of falling back to the
whole-secret branch when `jsonKey` is empty. Update the selector validation so
`awssm://...#` is treated as an invalid reference and returns an error, and add
a regression test covering this exact case to prevent the whole-secret path from
being taken again.
- Around line 73-75: The secret resolution path is still contextless, and
awssm.go is calling Secrets Manager with a background context. Update
Backend.Resolve and Resolver.Resolve to accept a context.Context, then thread
that context through the call chain into the GetSecretValue call in awssm.go.
Also update the callers, including the command flow in gate.go, to pass the
existing command context so secret lookups can be canceled or time out properly.
In `@internal/secrets/keychain.go`:
- Around line 40-43: Reject empty service references in the keychain ref parser
before any keyring access. Update the parsing/validation in the keychain
handling flow around the service/account split so that keychain:///acct (where
service is empty) is treated as an invalid ref and returns a bad-ref error
instead of reaching keyring.Get with an empty service. Use the existing keychain
reference parsing logic and the account validation branch to add an explicit
service-empty check.
In `@README.md`:
- Line 45: The README wording overstates the safety of secret refs by implying
plaintext passwords never live in config, but the resolver still allows literal
passthrough for plaintext values. Update the copy around the named
profiles/secret refs description to limit the “safe to commit” claim to actual
secret references only, and avoid saying all passwords are resolved at runtime
or never present in config; adjust the affected text in the README section that
describes env, keychain, and awssm handling.
---
Nitpick comments:
In `@internal/secrets/keychain_test.go`:
- Around line 33-39: The Keychain test only verifies that Resolve returns a user
error, so it can miss regressions where the unresolved-secret sentinel is no
longer returned. Update TestKeychain_NotFoundIsUserError to also assert that the
error matches errs.ErrSecretUnresolved in addition to checking errs.CodeUser.
Use the existing Resolve method and the err returned from keychain://missing to
confirm both the code and sentinel remain intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3b44cde-da77-48eb-906e-12a008b34885
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
CHANGELOG.mdREADME.mddocs/OPS.mdgo.modinternal/cli/root.gointernal/config/config.gointernal/secrets/awssm.gointernal/secrets/awssm_test.gointernal/secrets/keychain.gointernal/secrets/keychain_test.gointernal/secrets/resolver_test.go
- awssm: reject an empty "#" selector (awssm://id#) as malformed instead
of silently falling back to the whole-secret path. Bound the
GetSecretValue call with a 15s timeout so an unreachable Secrets
Manager can't hang resolution — the Backend.Resolve seam is contextless
(a Phase-A interface; threading context through it touches every
backend + profile resolution + every verb), so the timeout is applied
at the one call that does network I/O; full context-threading is left
as a separate refactor.
- keychain: reject keychain:///acct (empty service) as a bad ref instead
of calling keyring.Get("", "acct"), which would be backend-dependent.
- README: temper the secret-ref wording — passthrough means a literal
password DOES live in config, so don't claim plaintext "never" lives
there or that the file is unconditionally "safe to commit".
- Add regression tests for both new guards (empty # selector, empty
keychain service).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The final Phase G cycle: two new secret-ref backends so connection passwords can come from a credential store instead of config. Both implement the existing
secrets.Backendseam (which has named these schemes since Phase A), so this is purely additive — no seam or resolver changes.keychain://<account>(servicesiphon) orkeychain://<service>/<account>(any stored credential), viago-keyring: macOS Keychain, Windows Credential Manager, Linux Secret Service. No config, no network.awssm://<secret-id>, orawssm://<secret-id>#<json-key>to pull one field from a JSON secret (the common SM shape). Reuses the same AWS credential chain as the S3 storage backend. Off by default (building it loads AWS config); enable withsecrets.awssm: true(+ optionalawssm_region).Resolution order is
env → keychain → awssm → passthrough; passthrough stays last, so a literal password with no scheme (even one containing a colon) still resolves as-is.Testing
make test— 20 packages pass;make lint— 0 issues;go vet -tags=integration ./...clean.go-keyring's in-memory mock (short + long form, not-found → CodeUser, bad ref); awssm via a fakedGetSecretValueclient (whole secret, JSON#keyextraction, not-found, missing field, non-JSON-with-key); plus a full-chain dispatch test (each scheme routes to its backend, literal falls through to passthrough). No real-cloud integration test — the faked client and keyring mock cover the logic.Notes
github.com/zalando/go-keyringandaws-sdk-go-v2/service/secretsmanager(the SDK core was already a dep from the storage cycle).envbackend: missing/unknown =CodeUser, transport =CodeSystem.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation