fix(cloud): allow issued_token key in bootstrap audit metadata#663
Open
jacsdev wants to merge 3 commits into
Open
fix(cloud): allow issued_token key in bootstrap audit metadata#663jacsdev wants to merge 3 commits into
jacsdev wants to merge 3 commits into
Conversation
`engram cloud bootstrap admin --issue-token` always failed with "sensitive auth audit metadata is not allowed: issued_token". The bootstrap completion audit records a boolean `issued_token` flag, but sensitiveAuthAuditKey rejected every key containing "token". Because the token and its audit are persisted atomically, the token was never minted, and since bootstrap refuses to run twice this left a token-less admin with no supported recovery path (all token-issuing routes require an existing managed-admin token). Whitelist `issued_token` next to the existing `token_prefix` exception: it is a non-secret boolean flag, not credential material. Add a regression test asserting the key is accepted. Fixes Gentleman-Programming#597 Fixes Gentleman-Programming#603
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe sensitive audit metadata filter now exempts ChangesAudit metadata validation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/cloud/cloudstore/identity.go`:
- Around line 958-964: The issued_token whitelist in
internal/cloud/cloudstore/identity.go:958-964 must only bypass
sensitive-metadata filtering when its value is boolean, both at the top level
and recursively; update the relevant filtering logic around the visible key
switch while preserving token_prefix behavior. Add rejection tests in
internal/cloud/cloudstore/identity_storage_test.go:542-549 covering
string/token-like issued_token values and nested non-boolean values.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 60a511db-15c4-444d-8acc-444dc9df2a73
📒 Files selected for processing (2)
internal/cloud/cloudstore/identity.gointernal/cloud/cloudstore/identity_storage_test.go
Address review feedback: the issued_token whitelist bypassed the sensitive-metadata filter by key name alone, so a non-boolean value could smuggle secret material under a trusted key. Exempt issued_token only when its value is actually a bool, at the top level and recursively; otherwise the key falls through to the normal sensitive-fragment heuristic. Adds rejection tests for string and nested non-boolean issued_token values.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/cloud/cloudstore/identity_storage_test.go`:
- Around line 547-558: Extend the rejectSensitiveAuthAuditMetadata tests to
cover issued_token set to false, including the nested metadata case if following
the existing true assertion pattern. Assert no error is returned, preserving the
exemption for both boolean values while retaining rejection of non-boolean
issued_token values.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ed409729-5169-428a-8d4e-aeaae0308c53
📒 Files selected for processing (2)
internal/cloud/cloudstore/identity.gointernal/cloud/cloudstore/identity_storage_test.go
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.
Problem
engram cloud bootstrap admin --issue-tokenalways fails with:The admin principal is created and committed, but token issuance rolls back (the token and its audit are written atomically), so a fresh deployment is left with a token-less admin and no supported recovery path — every token-issuing route requires an existing managed-admin token. Reported in #597 and #603.
Root cause
cloudBootstrapCompletionMetadata(cmd/engram/cloud_bootstrap.go) records a booleanissued_tokenflag.sensitiveAuthAuditKey(internal/cloud/cloudstore/identity.go) classifies any key containing the substringtokenas sensitive, except the whitelistedtoken_prefix.issued_tokencontainstoken, so it is rejected and the atomic token+audit write fails.The same completion metadata is also written when bootstrapping with
--grant-project(even without--issue-token), so that path fails at the completion audit as well.Fix
Whitelist
issued_tokenalongside the existingtoken_prefixexception insensitiveAuthAuditKey. It is a non-secret boolean flag, not credential material — the secret-bearing keys (raw_token,token_hash,authorization,password, …) remain rejected, including nested values.Test
Regression assertions added to
TestCloudstoreIdentityPureHelpers:sensitiveAuthAuditKey("issued_token")must befalserejectSensitiveAuthAuditMetadata({"issued_token": true, ...})must returnnilFixes #597
Fixes #603
Summary by CodeRabbit
issued_tokenas safe only when its value is a boolean.issued_tokenwith boolean values, while still rejectingissued_tokenwhen the value is not a boolean, including within nested metadata.issued_tokenvalidation at both top-level and nested metadata to prevent future validation regressions.