feat(a2a-auth-callout): credentials (api_key, mtls, oidc) - #371
Merged
Conversation
yordis
commented
Jun 21, 2026
yordis
commented
Member
- adds the three credential verifiers the dispatcher slice will route against (HMAC API key, mTLS chain verification, OIDC discovery + JWKS), each with the typed-value-object boundary the rest of the crate already follows
- landing them as one slice keeps each verifier paired with the test surface it ships with (rcgen-generated certs, RSA-key JWKS, wiremock-served discovery) instead of leaving partially-wired modules across PRs
Adds the three credential verifiers the dispatcher routes against: HMAC-SHA256 API key, mTLS chain verification via x509-parser + rustls-webpki, and OIDC discovery + JWT decode via reqwest + jwks. Each verifier has the typed value-object pattern around its credential material, so the dispatcher slice can wire them up without further refactoring. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
PR SummaryHigh Risk Overview API key: HMAC-SHA256 digest registry and mTLS: OIDC: Crate deps expand for HTTP/TLS/X.509 ( Reviewed by Cursor Bugbot for commit 33666d8. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
Warning Review limit reached
More reviews will be available in 23 minutes and 53 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ 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 refill rate. 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, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAdds a new ChangesAuth Callout Credentials Module
Sequence Diagram(s)sequenceDiagram
rect rgba(70, 130, 180, 0.5)
Note over Client,UserJwtClaims: OIDC Path
Client->>JwksOidcVerifier: verify(BearerToken, AudienceAccount)
JwksOidcVerifier->>JwksSource: fetch_jwks()
JwksSource->>reqwest: GET jwks_uri (if Remote)
reqwest-->>JwksSource: JwkSet
JwksOidcVerifier->>jsonwebtoken: decode header → kid
JwksOidcVerifier->>jsonwebtoken: validate JWT (issuer + audiences)
JwksOidcVerifier->>JwksOidcVerifier: sub → ExternalSubject → caller_id
JwksOidcVerifier-->>Client: Ok(UserJwtClaims)
end
rect rgba(60, 179, 113, 0.5)
Note over Client,UserJwtClaims: mTLS Path
Client->>X509MtlsVerifier: verify(ClientCertPem, AudienceAccount)
X509MtlsVerifier->>x509_parser: parse leaf PEM + trust anchors
X509MtlsVerifier->>X509MtlsVerifier: check validity at now_utc
X509MtlsVerifier->>X509MtlsVerifier: match issuer to anchor, verify signature
X509MtlsVerifier->>X509MtlsVerifier: extract ExternalSubject (DN or DER fallback)
X509MtlsVerifier-->>Client: Ok(UserJwtClaims)
end
rect rgba(210, 105, 30, 0.5)
Note over Client,UserJwtClaims: API Key Path (deprecated)
Client->>HmacApiKeyVerifier: verify(api_key: &str)
HmacApiKeyVerifier->>ApiKeyDigest: HMAC-SHA256(key, secret)
HmacApiKeyVerifier->>ApiKeyRegistry: lookup(digest)
ApiKeyRegistry-->>HmacApiKeyVerifier: ApiKeyEntry
HmacApiKeyVerifier-->>Client: Ok(UserJwtClaims)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
Code Coverage SummaryDetailsDiff against mainResults for commit: 33666d8 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
rsworkspace/crates/a2a-auth-callout/src/credentials/mod.rs (1)
9-14: 💤 Low valueConsider adding common derives for
CredentialSource.The enum lacks
#[derive(Debug, Clone, Copy, PartialEq, Eq)]which are typically expected for simple discriminant enums. These enable logging, pattern matching in tests, and equality comparisons.Suggested addition
+#[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum CredentialSource { Oidc, MTls, #[deprecated(note = "transitional only; remove after OIDC migration")] ApiKey, }🤖 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 `@rsworkspace/crates/a2a-auth-callout/src/credentials/mod.rs` around lines 9 - 14, The `CredentialSource` enum is missing standard derives that provide common functionality for simple discriminant enums. Add the derive attribute `#[derive(Debug, Clone, Copy, PartialEq, Eq)]` before the `pub enum CredentialSource` declaration to enable logging support, cloning and copying instances, and equality comparisons needed for testing and pattern matching.rsworkspace/crates/a2a-auth-callout/src/credentials/mtls.rs (3)
166-169: 💤 Low valueRedundant
Ok(...?)pattern.Since
verify_syncalready returnsResult<UserJwtClaims, AuthCalloutError>, theOk(...?)is a no-op.♻️ Simplify
async fn verify(&self, cert: &ClientCertPem, account: &AudienceAccount) -> Result<UserJwtClaims, AuthCalloutError> { let now = OffsetDateTime::now_utc(); - Ok(self.verify_sync(cert, account, now)?) + self.verify_sync(cert, account, now) }🤖 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 `@rsworkspace/crates/a2a-auth-callout/src/credentials/mtls.rs` around lines 166 - 169, The verify async function contains a redundant Ok(...?) pattern when returning the result from verify_sync. Since verify_sync already returns Result<UserJwtClaims, AuthCalloutError>, the wrapping with Ok() is unnecessary. Remove the Ok(...?) wrapper and directly return the result from the verify_sync call using the ? operator, which will properly propagate any errors while returning the success value as-is.
48-59: ⚡ Quick winError context is discarded via string formatting.
Per coding guidelines: "Never discard error context by converting a typed error into a string; wrap the source error as a field or variant instead."
This pattern appears throughout the file (lines 51, 63, 75, 94, 110, 122, 128, 151, 155):
.map_err(|e| AuthCalloutError::CredentialVerification(format!("...: {e}")))Consider adding structured variants to
AuthCalloutErrorthat wrap source errors:// In error module pub enum MtlsError { PemParse(x509_parser::pem::PemError), CertificateParse(x509_parser::error::X509Error), SignatureVerification(x509_parser::error::X509Error), // ... }This preserves the error chain for debugging and allows callers to match on specific failure modes.
🤖 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 `@rsworkspace/crates/a2a-auth-callout/src/credentials/mtls.rs` around lines 48 - 59, The function `leaf_der` and other error handling throughout the file are discarding error context by converting typed errors into strings using format macros. Instead of using `AuthCalloutError::CredentialVerification(format!("..."))` pattern, add structured error variants to the `AuthCalloutError` enum that wrap the source errors as fields (such as variants for `PemParse`, `CertificateParse`, `SignatureVerification`, etc.). Then replace all instances of `.map_err()` calls that use the string formatting pattern with these new structured variants, ensuring the source error types are preserved in the variant fields for proper error chain preservation.Source: Coding guidelines
13-37: 💤 Low valueConsider validating PEM structure at construction or renaming to indicate boundary type.
Per coding guidelines, domain value objects should guarantee correctness at construction. These wrappers currently accept any string and defer validation to
verify_sync. Two options:
- Validate PEM structure in
new()and returnResult<Self, ...>- Rename to
ClientCertPemInput/TrustAnchorPemInputto signal these are boundary types awaiting conversionThe current approach is functional but blurs the boundary/domain distinction.
🤖 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 `@rsworkspace/crates/a2a-auth-callout/src/credentials/mtls.rs` around lines 13 - 37, The ClientCertPem and TrustAnchorPem structs currently accept any string without validation, blurring the boundary between input and domain types. Choose one approach: either validate PEM structure in the new() methods of both ClientCertPem and TrustAnchorPem to return Result<Self, Error> and guarantee correctness at construction, or rename both structs to ClientCertPemInput and TrustAnchorPemInput to clearly signal they are boundary types awaiting validation. Apply the chosen approach consistently across both structs to properly reflect domain value object semantics.Source: Coding guidelines
rsworkspace/crates/a2a-auth-callout/src/credentials/oidc.rs (2)
147-147: 🏗️ Heavy liftJWKS is fetched on every verification; consider caching.
fetch_jwks()makes an HTTP request to the JWKS endpoint for every token verification when usingJwksSource::Remote. JWKS typically changes infrequently (during key rotation). High-traffic scenarios will generate excessive requests and may trigger rate limiting from OIDC providers.Consider adding JWKS caching with a TTL (e.g., 5-60 minutes) or background refresh.
🤖 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 `@rsworkspace/crates/a2a-auth-callout/src/credentials/oidc.rs` at line 147, The `fetch_jwks()` method is being called on every token verification without caching, causing excessive HTTP requests to the JWKS endpoint. Implement a caching mechanism with a time-to-live (TTL) expiry in the OIDC credentials structure to store the fetched JWKS. Before calling `fetch_jwks()`, check if a valid cached JWKS exists within the TTL window (suggested 5-60 minutes). Only perform the HTTP request if the cache is absent or expired. This will significantly reduce unnecessary requests while allowing JWKS to be refreshed after key rotation.
158-163: Explicitly specify allowed algorithms instead of using the JWT header'salgvalue.
Validation::new(header.alg)configures the validator based on the algorithm claimed in the JWT header. While the jsonwebtoken library does validate that the algorithm matches the key type (an RSA key with HS256 would fail verification), relying on the header's algorithm is not best practice. Explicitly specify the algorithms your system expects (e.g., RS256, RS384, RS512) to follow defense-in-depth principles and avoid potential confusion if library behavior changes.Suggested fix
- let mut validation = Validation::new(header.alg); + let mut validation = Validation::new(jsonwebtoken::Algorithm::RS256); + // Optionally allow RS384, RS512 if needed: + // validation.algorithms = vec![ + // jsonwebtoken::Algorithm::RS256, + // jsonwebtoken::Algorithm::RS384, + // jsonwebtoken::Algorithm::RS512, + // ]; validation.set_issuer(&[self.issuer.as_str()]); validation.set_audience(&auds);🤖 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 `@rsworkspace/crates/a2a-auth-callout/src/credentials/oidc.rs` around lines 158 - 163, The Validation::new(header.alg) call in the OIDC token verification code is configuring the JWT validator based on the algorithm claimed in the token header, which is not a security best practice. Instead of using the header's alg value, explicitly specify the allowed algorithms that your system expects (such as RS256, RS384, or RS512) by creating the Validation instance with one of these explicitly defined algorithms rather than relying on the header value. This follows defense-in-depth principles and ensures predictable behavior regardless of any library changes.
🤖 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 `@rsworkspace/crates/a2a-auth-callout/src/credentials/api_key.rs`:
- Around line 28-32: The `From<ApiKeyError> for AuthCalloutError` implementation
converts the error to a string using `to_string()`, which loses the original
typed error information. To preserve error context, add a dedicated
`ApiKey(ApiKeyError)` variant to the `AuthCalloutError` enum, then update the
`From` implementation to return `Self::ApiKey(e)` instead of converting to
string via the `CredentialVerification` variant.
- Around line 114-132: The verify method has two error handling inconsistencies
that lose type information. In the registry lookup call, replace the ok_or_else
pattern that constructs a generic CredentialVerification error with a direct
typed error variant like ApiKeyError::Unknown. Similarly, in the
derive_caller_id call, remove the map_err wrapper that converts the error to a
formatted string and instead use the ? operator to propagate the typed error
directly, allowing the From impl to handle the conversion. This ensures
consistent, typed error handling throughout the method.
In `@rsworkspace/crates/a2a-auth-callout/src/credentials/mtls.rs`:
- Around line 104-119: The certificate verification loop iterates through
trusted CAs and verifies the leaf certificate's signature but does not validate
that each CA certificate is currently within its validity period. Add a validity
check for each CA certificate in the verification loop (before or after matching
the issuer and verifying the signature) to ensure that expired or not-yet-valid
CA certificates are rejected as trust anchors. This validation should be
performed on each CA in the iteration where leaf.verify_signature is called to
prevent accepting invalid CAs.
In `@rsworkspace/crates/a2a-auth-callout/src/credentials/oidc.rs`:
- Around line 84-108: The reqwest::Client built in the OIDC credential
verification code lacks a timeout configuration, which could cause indefinite
hangs if the OIDC discovery or JWKS endpoint becomes unresponsive. Add a timeout
configuration to the Client::builder() chain by calling .timeout() with an
appropriate duration before the .build() call, ensuring that HTTP requests to
the OIDC discovery endpoint and JWKS URI will fail gracefully rather than block
indefinitely.
- Around line 36-45: The `OidcClientId::new` method currently accepts any string
including empty strings, which is inconsistent with `OidcIssuerUrl::parse`
validation. Add validation to the `OidcClientId::new` method to reject empty or
whitespace-only strings and return a Result type (or panic) to enforce that only
valid non-empty client IDs can be created, matching the validation pattern used
in `OidcIssuerUrl::parse`.
---
Nitpick comments:
In `@rsworkspace/crates/a2a-auth-callout/src/credentials/mod.rs`:
- Around line 9-14: The `CredentialSource` enum is missing standard derives that
provide common functionality for simple discriminant enums. Add the derive
attribute `#[derive(Debug, Clone, Copy, PartialEq, Eq)]` before the `pub enum
CredentialSource` declaration to enable logging support, cloning and copying
instances, and equality comparisons needed for testing and pattern matching.
In `@rsworkspace/crates/a2a-auth-callout/src/credentials/mtls.rs`:
- Around line 166-169: The verify async function contains a redundant Ok(...?)
pattern when returning the result from verify_sync. Since verify_sync already
returns Result<UserJwtClaims, AuthCalloutError>, the wrapping with Ok() is
unnecessary. Remove the Ok(...?) wrapper and directly return the result from the
verify_sync call using the ? operator, which will properly propagate any errors
while returning the success value as-is.
- Around line 48-59: The function `leaf_der` and other error handling throughout
the file are discarding error context by converting typed errors into strings
using format macros. Instead of using
`AuthCalloutError::CredentialVerification(format!("..."))` pattern, add
structured error variants to the `AuthCalloutError` enum that wrap the source
errors as fields (such as variants for `PemParse`, `CertificateParse`,
`SignatureVerification`, etc.). Then replace all instances of `.map_err()` calls
that use the string formatting pattern with these new structured variants,
ensuring the source error types are preserved in the variant fields for proper
error chain preservation.
- Around line 13-37: The ClientCertPem and TrustAnchorPem structs currently
accept any string without validation, blurring the boundary between input and
domain types. Choose one approach: either validate PEM structure in the new()
methods of both ClientCertPem and TrustAnchorPem to return Result<Self, Error>
and guarantee correctness at construction, or rename both structs to
ClientCertPemInput and TrustAnchorPemInput to clearly signal they are boundary
types awaiting validation. Apply the chosen approach consistently across both
structs to properly reflect domain value object semantics.
In `@rsworkspace/crates/a2a-auth-callout/src/credentials/oidc.rs`:
- Line 147: The `fetch_jwks()` method is being called on every token
verification without caching, causing excessive HTTP requests to the JWKS
endpoint. Implement a caching mechanism with a time-to-live (TTL) expiry in the
OIDC credentials structure to store the fetched JWKS. Before calling
`fetch_jwks()`, check if a valid cached JWKS exists within the TTL window
(suggested 5-60 minutes). Only perform the HTTP request if the cache is absent
or expired. This will significantly reduce unnecessary requests while allowing
JWKS to be refreshed after key rotation.
- Around line 158-163: The Validation::new(header.alg) call in the OIDC token
verification code is configuring the JWT validator based on the algorithm
claimed in the token header, which is not a security best practice. Instead of
using the header's alg value, explicitly specify the allowed algorithms that
your system expects (such as RS256, RS384, or RS512) by creating the Validation
instance with one of these explicitly defined algorithms rather than relying on
the header value. This follows defense-in-depth principles and ensures
predictable behavior regardless of any library changes.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 167cade7-e0cd-4b91-ad22-033f9e1f6ab6
⛔ Files ignored due to path filters (1)
rsworkspace/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
rsworkspace/crates/a2a-auth-callout/Cargo.tomlrsworkspace/crates/a2a-auth-callout/src/credentials/api_key.rsrsworkspace/crates/a2a-auth-callout/src/credentials/mod.rsrsworkspace/crates/a2a-auth-callout/src/credentials/mtls.rsrsworkspace/crates/a2a-auth-callout/src/credentials/oidc.rsrsworkspace/crates/a2a-auth-callout/src/lib.rs
- mTLS: refuse CA-bit certs as client end-entity (presenting an intermediate / root CA would otherwise pass verification and mint claims for the CA subject), and check trust-anchor validity at verification time rather than only the leaf. - OIDC: require the discovery doc's iss to match the configured OidcIssuerUrl and constrain jwks_uri to the same origin so a tampered/MITM'd discovery response can't redirect us to attacker- controlled JWKS. Also bound the reqwest client with a 10s timeout and 5s connect deadline so a misbehaving IdP can't hang the verifier indefinitely. OidcClientId rejects empty/whitespace input to match the other typed value objects. - API key: ApiKeyVerifier now takes the connection's resolved AudienceAccount and refuses keys whose registry audience doesn't match — sibling verifiers (mTLS/OIDC) already drive `aud` off the connection, so this brings api_key in line and stops a valid key from minting for an account the client never requested. ApiKeyError gains typed CallerIdDerivation(JwtError) + AudienceMismatch variants so the source chain replaces format!() stringification. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…rigin default ports - mTLS verification used to bail on the first matching trust anchor whose validity check or signature verification failed. During CA rotation that rejects valid clients whenever an older anchor is still listed alongside the current one. Continue the scan instead so a still-valid sibling anchor can vouch for the leaf; only fail when no anchor accepts it. - OIDC same_origin compared raw port tokens, so 'https://host' vs 'https://host:443' resolved as different origins and discovery would reject a legitimate jwks_uri the IdP just spelled with the default port present. Normalize the default ports against the scheme. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 2588ca8. Configure here.
…rects in JWKS fetch - mTLS used to require the leaf's direct issuer to be in the trust anchor bundle, which fails the common leaf→intermediate→root setup when the bundle only contains the root. Parse every CERTIFICATE block in the client PEM as a chain, then walk leaf → intermediates → trust anchor at each hop checking validity + signature. The leaf end-entity check stays before the walk so a CA bit on the leaf still rejects up front. - OIDC same-origin validation gated `jwks_uri` at discovery, but fetch_jwks then used a default reqwest client that follows redirects. A same-origin URL could 302 to an attacker host and load malicious keys while tokens still claim the configured iss. Disable redirects on the client so the IdP must serve JWKS from the same origin directly. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
This was referenced Jun 21, 2026
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.