fix(breg): name static-JWKS key rotation in refusals and document re-pinning - #984
fix(breg): name static-JWKS key rotation in refusals and document re-pinning#984jeremi wants to merge 1 commit into
Conversation
…pinning A statically pinned jwksSource turns provider-side key rotation into a silent outage: every token fails with the same value-free authentication.refused, and nothing on the engine surface distinguishes a rotated signing key from any other invalid credential. The refusal the caller sees stays unchanged; RegistryAuthenticator now records on the registry_breg::auth target that the refused token cites a signing key outside the configured JWKS and points at re-pinning, so an operator can tell rotation from an invalid token in one log line. The operations guide gains the matching runbook: fetch the provider's current JWKS over TLS, replace the documentRef secret, restart, verify a fresh login while removed keys stay refused. The focused test drives the whole story with the MockIdP's second key: pinned acceptance, rotation refusal with the named diagnostic, re-pin from the rotated document, and stale-key refusal after re-pinning. Closes #982. Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e508ed8fe0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // names an operational cause instead of a bad | ||
| // credential: a key outside the configured JWKS is how | ||
| // provider-side rotation presents. | ||
| if matches!(error, OidcError::UnknownKid) { |
There was a problem hiding this comment.
Any unauthenticated client can send a syntactically valid JWT with an allowed alg/typ and an arbitrary kid; TokenVerifier::verify_access_token then returns UnknownKid before verifying the signature, and enforce_kid_allowed returns the same variant for deliberately denied keys. This branch consequently emits a warning for every such request—even with discovery-backed JWKS—and falsely recommends re-pinning, allowing warning-log flooding and potentially sending operators toward an unnecessary trust-anchor change. Only diagnose static rotation when the source and cause are actually distinguishable, and rate-limit attacker-controlled misses.
Useful? React with 👍 / 👎.
| use registry_platform_authcommon::{parse_bearer_token, validate_compact_access_token}; | ||
| use registry_platform_oidc::{ | ||
| is_access_token_typ_pair, Audience, JwksFetcher, TokenVerifier, TokenVerifierConfig, | ||
| is_access_token_typ_pair, Audience, JwksFetcher, OidcError, TokenVerifier, TokenVerifierConfig, |
There was a problem hiding this comment.
Commit 3b8485a25e5e9e39de874cb58c4cd46092da5064 has no Signed-off-by trailer, so it violates the repository's mandatory DCO rule; recreate this review commit with git commit -s before merging.
AGENTS.md reference: AGENTS.md:L370-L372
Useful? React with 👍 / 👎.
|
|
||
| ## Unreleased | ||
|
|
||
| - A Base Registry Engine deployment with a statically pinned `jwksSource` |
There was a problem hiding this comment.
This commit modifies both crates/registry-breg/ and docs/site/, despite the repository rule requiring each change to remain within one owning area. Split the documentation update from the runtime/test change so each review unit has a single owner.
AGENTS.md reference: AGENTS.md:L378-L380
Useful? React with 👍 / 👎.
| 2. Replace the file the `documentRef` names with those bytes, keeping the secret-file rules | ||
| below (owner, mode, no trailing newline). |
There was a problem hiding this comment.
When secretProviders.environment is enabled, the supported documentRef can be secret:env/<NAME>, so there is no file to replace and these recovery steps cannot be completed as written. Add an environment-backed branch that tells the operator how to replace the injected value and restart, rather than assuming every static pin uses the file provider.
AGENTS.md reference: docs/site/AGENTS.md:L116-L127
Useful? React with 👍 / 👎.
| at the re-pin procedure. The operations guide gained a tested re-pin | ||
| runbook: fetch the provider's current JWKS over TLS, replace the | ||
| `documentRef` secret, restart, and verify a fresh login while tokens from |
There was a problem hiding this comment.
The cited test replaces an in-memory JwksFetcher and calls authenticate; it does not fetch JWKS over TLS, replace a documentRef secret, restart breg, or perform a provider login. Calling the entire operational runbook “tested” therefore overstates the available evidence; either add coverage for those steps or weaken this claim.
AGENTS.md reference: docs/site/AGENTS.md:L40-L43
Useful? React with 👍 / 👎.
1 participant
Stacks on #983 (shared files and import lines); retarget to
mainonce that merges — GitHub does it automatically when the base branch is deleted.What
A statically pinned
jwksSourceturns provider-side key rotation into a silent outage: every token fails with the same value-freeauthentication.refused, and nothing distinguished a rotated signing key from any other invalid credential (the story behind #982, seen with both ZITADEL and ThunderID pins; ThunderID regenerates keys on state loss).The caller-facing refusal is deliberately unchanged.
RegistryAuthenticator::authenticatenow records atracing::warnon theregistry_breg::authtarget when the platform verifier answersUnknownKid: the refused token cites a signing key outside the configured JWKS, and for a static pin that means the provider rotated — re-pin and restart. One log line separates rotation from an invalid token without leaking values.The operations guide (
operate/breg) gains the tested re-pin runbook next to thejwksSource: kind: staticconfig: fetch the provider's current JWKS from itsjwks_uriover TLS, confirm unannounced keys with the provider operator, replace thedocumentRefsecret under the secret-file rules, restartbreg, then verify a fresh login succeeds while tokens from removed keys stay refused. The runbook states the trust consideration: the new document becomes the anchor for bearer verification, so re-pinning is a deliberate act.Verification
cargo test --locked -p registry-breg --features runtime --test http_auth— 26 passed. Newstatic_jwks_rotation_is_diagnosed_and_repinning_restores_authenticationdrives the whole story with the MockIdP's rotated key (kid -1→kid -2): pinned acceptance; rotation refusal asserting the captured log names the missing signing key; re-pin from the rotated document; stale-key refusal after re-pinning.cargo clippy --locked -p registry-breg --all-targets --features runtime,tooling -- -D warningsclean;cargo fmt --checkclean.Closes #982.