Repository navigation
Resolve ABCA attester key from x5c JOSE header - #52883
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The implementation satisfies the requested behavior; the identified unused helper is non-blocking cleanup.
Pull request overview
Adds trusted x5c certificate-chain resolution for ABCA attester keys while retaining JWKS fallback.
Changes:
- Validates
x5cchains through configured trust-material providers. - Adds trusted and untrusted certificate-chain integration tests.
File summaries
| File | Description |
|---|---|
AttestationBasedClientAuthenticator.java |
Resolves attester keys from x5c before falling back to kid. |
OIDCAttestationBasedClientAuthenticationTest.java |
Tests trusted and untrusted x5c authentication. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@Awambeng @Ogenbertrand @IngridPuppet @forkimenjeckayang Do you have opportunity to review the PR? Also tagging, so that you are aware of this PR as not sure if it is in the conflict (or relation) with some of your work? |
There was a problem hiding this comment.
Thanks for the solid fix, @adityaanikam! However, I left a few minor comments. Could you please check them when you get a chance?
1991b7a to
e754e99
Compare
|
Thanks for the review, all addressed in the updated commit (rebased on main, still one commit). Formatting fixed (that block was tab indented by mistake) and the redundant import removed. The call site now matches the suggestion. The error message for the null case now says no X.509 trust anchors are configured. I removed the unused assertX5cAttestationOutcome helper Copilot flagged, since runX5cAttestationTest already covers that flow. On reusing OID4VCProofTestUtils: it only had createRsaKeyPair and createEcKeyPair, nothing that builds a CA plus end entity chain, so there was nothing to reuse directly. I moved the certificate generation there as createCaCertificate and createEndEntityCertificate, so this test no longer carries its own BouncyCastle code and other tests can use them. Happy to inline it back or rename if you'd prefer a different shape. |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
|
Hi @adityaanikam, could you please review the Copilot comments and address the ones that make sense? |
e754e99 to
463ea6e
Compare
463ea6e to
7afd02e
Compare
There was a problem hiding this comment.
Thanks for considering my previous comments, @adityaanikam. I left a few minor ones for you to check.
Could you also review the Copilot comments, resolve them or leave feedback if they aren't valid, and mark each conversation as resolved once addressed?
7afd02e to
2ddfff4
Compare
2ddfff4 to
3c328f5
Compare
|
Thanks @Awambeng, all addressed in the updated commit (still one commit). Test with no kid: the attester key now has its kid cleared, so the x5c tests send a JWS with no kid in the header and cover the case from #52766. Fallback to kid: good catch. validateX509Chain returns null when no X.509 trust anchors are configured (JWKS-only) and throws when a configured chain fails validation, so a client sending both kid and x5c would have regressed from working via kid before this change. findAttesterKey now falls back to the kid lookup on null. If there is no kid either, the "No X.509 trust anchors are configured" message is kept. I added testClientAttestationWithKidAndX5cFallsBackToKidWithoutX509TrustAnchors for this and moved the shared token request part of the x5c tests into a helper so the tests do not duplicate it. Full chain in x5c: I left this one as is. The leaf is signed directly by the configured trust anchor and real chains often omit the anchor, so adding it would not be closer to a real request. Covering an intermediate would need extra certificate scaffolding that seems out of proportion here. Copilot comments: findAttesterKey now creates one TrustMaterialResolver, signCertificate is documented as RSA-only, the rejection assertion checks the invalid_client_attestation error and that no access token is returned, and certificate serial numbers reuse one SecureRandom. |
There was a problem hiding this comment.
@adityaanikam @Awambeng Thanks for the PR and review! Approving based on the @Awambeng review. I hope to merge if tests are OK (there was a failure in MSSQL, which looks unrelated to this change)
|
This overlaps with #54522 (ETSI LoTE trust), which needs this change for client attestations. I'll drop my version from #54522 and build on this PR instead. One gap I found while testing: when a client trusts a provider with X.509 trust anchors, an attestation without |
AttestationBasedClientAuthenticator.findAttesterKey required a kid and looked it up via JWKS, rejecting any Client Attestation JWT that carries its signing certificate chain in the x5c header instead, with "Invalid attester kid: null". x5c was never read. Prefer x5c when present: validate the chain against the X.509 trust anchors of the configured attester_trust_idps using TrustMaterialResolver.validateX509Chain, the mechanism keycloak#51042 added for key attestations, and use the leaf certificate's public key. Fall back to the existing kid/JWKS path when x5c is absent. Closes keycloak#52766 Signed-off-by: adityaanikam <adityanikam9502@gmail.com>
3c328f5 to
b04d334
Compare
There was a problem hiding this comment.
@adityaanikam @Awambeng @dominikschlosser Thanks for the PR and reviews!
@dominikschlosser Created #54683 as a follow-up to your comment
What
AttestationBasedClientAuthenticator.findAttesterKeyonly resolved the attester's signing key through thekidJOSE header via JWKS. A Client Attestation JWT that carries its signing certificate chain in thex5cheader (nokid) was rejected before any verification, withInvalid attester kid: null;x5cwas never read.Why
draft-ietf-oauth-attestation-based-client-auth-10 Section 9.8 lists
x5cas a valid attester-key resolution path, and the WE BUILD interoperability profile's reference wallet sendsx5cwith nokid. #51042 already added X.509 chain validation (TrustMaterialResolver.validateX509Chain) for key attestations at the credential endpoint; this reuses the same mechanism for client attestations.How
In
findAttesterKey, when the JOSE header carries a non-emptyx5c, validate it against the X.509 trust anchors of the configuredattester_trust_idpsand use the leaf certificate's public key. Otherwise, fall back to the existingkid/JWKS resolution unchanged.Testing
Extended
OIDCAttestationBasedClientAuthenticationTestwith two new tests.testClientAttestationHappyFlowWithX5csends an attestation JWT signed with a leaf cert chaining to adefault-trustidentity provider configured in X.509 mode, with nokid, and the token request succeeds.testClientAttestationX5cUntrustedChainIsRejectedsends the same, but the leaf cert chains to a CA that is not configured as trusted, and the token request is rejected.Closes #52766