Repository navigation
Conversation
GATEWAY_API_KEY_HASH_SECRET now accepts a comma-separated keyring (newest first). Provider-key ciphertexts are written as llmgw:v2 with an HKDF-derived key id so decryption picks the right keyring entry; legacy llmgw:v1 rows trial-decrypt against every entry. Fingerprint HMACs and prompt-cache hashing keep using the current (first) secret only; backfill re-encryption and multi-secret HMAC lookup are deliberately deferred until a rotation is actually needed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
There was a problem hiding this comment.
🟢 Ready to approve
The keyring + v2 ciphertext changes are internally consistent, preserve existing behavior for single-secret deployments, and are backed by targeted unit and integration test updates.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Lays groundwork for future rotation of GATEWAY_API_KEY_HASH_SECRET by introducing a “keyring” (comma-separated secrets, newest-first) and moving provider-key ciphertext writes to a versioned llmgw:v2:<kid>:... format that embeds a stable key identifier for precise decryption during rotation.
Changes:
- Added
getApiKeyHashSecrets()keyring support andgetSecretKeyId()for deriving a short key identifier from a secret. - Updated provider-key crypto to write
llmgw:v2ciphertexts (stamped withkid) and to decrypt both legacyv1(trial-decrypt across keyring) andv2(direct keyring selection bykid). - Expanded tests and updated docs / env examples to reflect keyring and rotation semantics.
File summaries
| File | Description |
|---|---|
| packages/shared/src/api-key-hash.ts | Introduces keyring parsing (getApiKeyHashSecrets) and secret key id derivation (getSecretKeyId), while keeping getApiKeyHashSecret() as “current secret”. |
| packages/shared/src/api-key-hash.spec.ts | Adds unit tests covering keyring parsing behavior and key-id stability. |
| packages/actions/src/provider-key/crypto.ts | Implements llmgw:v2 encryption (with kid) and supports decrypting both v1 and v2 formats using the keyring. |
| packages/actions/src/provider-key/crypto.spec.ts | Expands crypto tests for v2 stamping, rotation scenarios, legacy v1 decrypt, and dropped-secret error messaging. |
| packages/actions/src/provider-key/read.spec.ts | Updates the “unknown version prefix” test case to use llmgw:v3 now that v2 is valid. |
| apps/api/src/routes/v1-master-custom-catalog.spec.ts | Updates expectations to match the new llmgw:v2 ciphertext prefix. |
| apps/api/src/routes/keys-provider.spec.ts | Updates expectations to match the new llmgw:v2 ciphertext prefix. |
| apps/api/src/routes/keys-provider.e2e.ts | Updates e2e assertions to expect llmgw:v2 ciphertext prefix. |
| apps/api/src/routes/admin-provider-credentials.spec.ts | Updates expectations to match the new llmgw:v2 ciphertext prefix. |
| docs/internal/2026-05-21-byok.md | Documents keyring semantics and explicitly calls out what is not yet rotation-safe. |
| .env.unified.example | Updates env var comments to describe keyring usage during rotation (decrypt-only for older entries). |
| .env.example | Updates env var comments to describe keyring semantics and decrypt-only behavior for older entries. |
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 0
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
steebchen
merged commit Aug 2, 2026
d37720f
into
claude/provider-credentials-ui-lgai9x
9 checks passed
steebchen
added a commit
that referenced
this pull request
Aug 2, 2026
Stacked on #3234 (follows #3370, which merged into the same base). ## Problem The Provider Credentials page could not tell an operator that a credential had burned through its spend cap and was no longer eligible for selection: - A key the billing worker auto-disabled rendered as a plain `inactive` badge — indistinguishable from one an admin switched off by hand. - Every row was numbered by its position in the list, so a shut-off key read as `#1` in the rotation while the gateway was silently serving from the one below it. - There was no warning before a cap tripped, only after. - Org BYOK provider keys showed no spend information at all. ## Changes Spend-limit state is derived in one place (`ee/admin/src/lib/provider-key-spend.ts`) and shared by the managed-credentials page and the per-organization BYOK table: - **`limit reached` badge** no longer requires the status flip to have landed. Enforcement lags by one worker batch, and the key keeps serving traffic in that window, so an over-cap key that is still active renders as `limit reached · disabling` rather than a reassuring `active`. - **Usage bar** with an amber tint from 80% of the cap, so a key approaching its limit is visible before it trips. - **Rotation positions count only selectable keys.** Provider-key selection filters on `status = 'active'`, so out-of-rotation rows now show `—` with an explanatory tooltip and a de-emphasised background instead of a misleading rank. ## Verification - `pnpm format`, `pnpm build` — clean. - 62 tests pass across `provider-key-spend.spec.ts` (13 new, covering the state machine including the lag window and a zero/malformed cap), `provider-key-stats.spec.ts`, and `admin-provider-credentials.spec.ts`. - Driven in a real browser against the dev stack with all four states seeded (at-cap/inactive, 85% warning, over-cap/still-active, uncapped). Confirmed the badges, tooltips, usage bars and rotation numbering render as intended, and that the spend dialog reconciles: window total `$14.5125` matched the database, split across `FinTech Global $10.75` and `Test Organization $3.7625`. Both bucket granularities (hourly for 24h, daily for 7d) verified. Dev data restored afterwards. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: AYOUB BENDARSI <137005356+RATCHAW@users.noreply.github.com>
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
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
Groundwork for rotating
GATEWAY_API_KEY_HASH_SECRET, laid now while all provider-key ciphertexts are still a single generation. Full rotation machinery (ciphertext backfill, multi-secret HMAC lookup / lazy re-stamping) is deliberately deferred until a rotation is actually needed.Stacked on #3234 (targets its branch, not
main), since the provider-key crypto module only exists there. It will retarget tomainautomatically when #3234 merges.What changed
GATEWAY_API_KEY_HASH_SECRETnow accepts a comma-separated list, newest first (getApiKeyHashSecrets()inpackages/shared). The first entry is the current secret: all fingerprint HMACs, prompt-cache-key hashing, and new encryptions use it exclusively, so existing single-secret deployments behave identically.llmgw:v2:<kid>:<iv>:<ct>:<tag>, wherekidis an HKDF-derived 8-hex-char id of the encrypting secret (getSecretKeyId(), one-way, reveals nothing about the secret). Decryption resolves the keyring entry by key id and reports precisely when a needed secret was dropped from the keyring, instead of a bare GCM failure.llmgw:v1ciphertexts (no key id) are trial-decrypted against every keyring entry, so even pre-v2 rows survive a rotation as long as the old secret stays in the keyring..env.example/.env.unified.example, and a rotation-groundwork section indocs/internal/2026-05-21-byok.mddocumenting the procedure and what is explicitly NOT yet rotation-safe (master_key.tokenHash,scim_token.tokenHash, historicallog.usedApiKeyHash).Verification
packages/shared/src/api-key-hash.spec.ts) and expanded crypto spec: rotation round-trips for v2 and legacy v1, key-id stamping, dropped-secret errors, AAD scope enforcement under previous secrets — 72 tests green.pnpm formatclean, fullpnpm build17/17 green.🤖 Generated with Claude Code