Repository navigation
[OID4VCI] Review support for proofs, attestations and key binding - #52637
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Logging severity, mdoc configuration guidance, attestation wording, diagram text, and coverage of the new error mapping need correction.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Expands OID4VCI proof, attestation, key-binding, and experimental mdoc documentation while improving diagnostic logging.
Changes:
- Adds proof and key-binding examples and configuration guidance.
- Documents experimental mdoc support and community examples.
- Adds debug/trace logging for validation failures.
File summaries
| File | Description |
|---|---|
IdentityProvidersResource.java |
Logs identity-provider creation failures. |
IdentityProviderResource.java |
Logs identity-provider update failures. |
OID4VCIssuerEndpoint.java |
Records and traces proof-validation errors. |
vc-issuer-configuration.adoc |
Updates mdoc and binding documentation. |
proofs.adoc |
Expands proof, attestation, and key-binding guidance. |
additional-examples.adoc |
Lists community example projects. |
proof_part1-no-proof.drawio.svg |
Adds the no-proof sequence diagram. |
Review details
- Files reviewed: 6/11 changed files
- Comments generated: 5
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Moderate documentation and logging issues remain unresolved.
Review details
Suppressed comments (6)
docs/documentation/server_admin/topics/oid4vci/proofs.adoc:36
- “bearer/reply attacks” is a typo: “reply” changes the security term and could confuse readers; this should say “bearer/replay attacks.”
. Thanks to the key binding, verifier is able to verify Key-Binding JWT signature against `cnf.jwk` from issuer signed JWT. This allows to prevent bearer/reply attacks, which is main benefit against use case 1. For more details, see the <<_oid4vci_proofs_key_binding, Key Binding section>>
docs/documentation/server_admin/topics/oid4vci/proofs.adoc:87
- When binding is OFF, proofs are not merely optional:
OID4VCIssuerEndpoint.enforceProofContractForCredentialrejects a supplied proof when the credential has no proof metadata (services/src/main/java/org/keycloak/protocol/oid4vc/issuance/OID4VCIssuerEndpoint.java:1454-1470). Please state that proofful requests are not accepted, so wallet implementers do not assume they can send proofs without enabling binding.
* **Cryptographic binding required** - When OFF, proofs are not required in credential request and key binding will not be used for verifiable credentials. This is <<_oid4vci_proofs_use_cases, use case 1 from examples above>>.
docs/documentation/server_admin/topics/oid4vci/proofs.adoc:35
- A Key-Binding JWT is specific to the SD-JWT presentation flow, while the later JWT VC guidance describes binding via
cnfand a verifier challenge. Qualify this step to SD-JWT VC presentations; as written it incorrectly applies that presentation format to every credential format.
. At presentation, Wallet appends a Key-Binding JWT signed with private proof key.
docs/documentation/server_admin/topics/oid4vci/proofs.adoc:49
- The new use-case text points to a
Trusted Key IDP, but that name is not a configured component and does not match the documented/current mechanism (trust-material identity providersselected viaoid4vci.attester_trust_idps). This can send readers to the wrong configuration; use the trust-material identity-provider terminology here.
. Issuer validates the attestation JWT against the Attester's public key (registered in Trusted Key IDP); for a `jwt` proof, it also validates the enclosing proof JWT. The public key (or certificate) of the Attester must be configured in {project_name} to establish trust.
docs/documentation/server_admin/topics/oid4vci/vc-issuer-configuration.adoc:417
- Now that this table advertises mdoc, the binding-method row still says that
jwkis the only allowed value. The mdoc builder supportscose_keyonly and the default mdoc scope uses that value, so users following this table will configure an invalid scope; document the format-specific values here.
| Cryptographic Binding Required
| Off
| When enabled, the credential must be cryptographically bound to the holder's wallet. More details about this option are in the <<_oid4vci_proofs_client_scope_config,proofs section>>.
services/src/main/java/org/keycloak/services/resources/admin/IdentityProvidersResource.java:301
- This branch is guarded by
isDebugEnabled()but emits at ERROR, so each invalid create request is promoted to the error log whenever debug logging is enabled. The matching update path useslogger.debug; use the same level here to avoid noisy and misleading error logs.
logger.error(message, e);
- Files reviewed: 6/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
mposolda
force-pushed
the
46272-oid4vci-proofs
branch
from
September 11, 2026 06:47
4a78dad to
041e6bb
Compare
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
One or more issues must be addressed before approval.
Review details
Suppressed comments (4)
docs/documentation/server_admin/topics/oid4vci/proofs.adoc:49
- This new step refers to a “Trusted Key IDP”, but that is not the configuration used by the documented flow: attester keys/certificates now come from selected trust-material identity providers linked through OID4VCI Attester Trust Identity Providers (lines 104-116), and the former realm-level trusted-key settings are explicitly obsolete. Use the current trust-material identity-provider terminology here so readers are not sent to a nonexistent/removed configuration.
. Issuer validates the attestation JWT against the Attester's public key (registered in Trusted Key IDP); for a `jwt` proof, it also validates the enclosing proof JWT. The public key (or certificate) of the Attester must be configured in {project_name} to establish trust.
docs/documentation/server_admin/topics/oid4vci/proofs.adoc:87
- When this option is OFF, proofs are not merely optional:
enforceProofContractForCredentialrejects a supplied proof wheneverproof_types_supportedis absent (services/src/main/java/org/keycloak/protocol/oid4vc/issuance/OID4VCIssuerEndpoint.java:1454-1470). Please state that proofs are neither required nor accepted in this mode; otherwise wallets may reasonably send an optional proof and receiveinvalid_proof.
* **Cryptographic binding required** - When OFF, proofs are not required in credential request and key binding will not be used for verifiable credentials. This is <<_oid4vci_proofs_use_cases, use case 1 from examples above>>.
docs/documentation/server_admin/topics/oid4vci/proofs.adoc:331
- This new link directs readers to the attestation structure, but that section still documents the JWT
typasopenid4vci-attestation+jwt. The validator expectskey-attestation+jwt(with onlykeyattestation+jwtaccepted for legacy compatibility), so following the documented attestation flow produces an invalid proof; please update the linked structure.
The example is described in the <<_oid4vci_proofs_use_cases, use case 3 from examples above>>.
docs/documentation/server_admin/topics/oid4vci/vc-issuer-configuration.adoc:421
- The default binding method is not always
jwk: client-scope defaults selectcose_keyformso_mdoc, and the mdoc builder accepts onlycose_key(services/src/main/java/org/keycloak/protocol/oid4vc/OID4VCLoginProtocolFactory.java:255,274-276; services/src/main/java/org/keycloak/protocol/oid4vc/issuance/credentialbuilder/MdocCredentialBuilder.java:42-45). As written, mdoc users may configure the documented default and have it rejected; document the format-specific defaults instead.
| Comma-separated cryptographic binding methods. Default value is `jwk`, which is only supported method for the formats like SD-JWT VC or JWT VC. Different methods might be possible for other formats (like mdoc).
- Files reviewed: 6/10 changed files
- Comments generated: 0 new
- Review effort level: Lite
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.forms.BruteForceTest#testCacheExpiryForTemporaryLockoutKeycloak CI - Forms IT (chrome) |
closes keycloak#46272 closes keycloak#46273 Signed-off-by: mposolda <mposolda@gmail.com>
mposolda
force-pushed
the
46272-oid4vci-proofs
branch
from
September 11, 2026 09:12
041e6bb to
58b4193
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The documented mdoc binding default is incorrect, and the new diagram contains malformed visible punctuation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 6/10 changed files
- Comments generated: 1
- Review effort level: Balanced
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Marek Posolda <mposolda@gmail.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The new documentation 10000 contains misleading JWT terminology and a visibly malformed diagram label.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
docs/documentation/server_admin/topics/oid4vci/proofs.adoc:33
jwkis a JOSE header parameter, not a JWT claim; calling it a claim can lead wallet implementers to place it in the payload, whichJwtProofValidatorwill reject. Please use “header parameter” and fix the singular/plural wording.
- Files reviewed: 6/10 changed files
- Comments generated: 0 new
- Review effort level: Balanced
graziang
approved these changes
Sep 11, 2026
Contributor
Author
|
@graziang Thank you for the review! |
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.
closes #46272
closes #46273
Did review of the proofs, attestations and key-binding. No major issues found (reported issue #52517 , which was discussed on the last meeting does not look like a blocker for the OID4VCI preview feature)
Added some more debug/trace loggings, which I found useful when testing setup related to proofs, their configuration etc. Might be useful for the customers as well (EG. for the case when they want to configure the certificate/key for trusted-key IDP and they upload the key in the incorrect format)
Documentation polishing - mostly for the sections related to proofs, attestations and key-binding. Most important parts: