Implement multiple credential issuance #48
Draft
Conversation
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
IngridPuppet
requested changes
May 14, 2025
There was a problem hiding this comment.
Hello @Ogenbertrand - I checked your implementation and quickly realized it is essentially disconnected from the specification. Please take more time to immerse yourself into the new requirements at the credential endpoint and update your implementation accordingly.
services/src/main/java/org/keycloak/protocol/oid4vc/issuance/OID4VCIssuerEndpoint.java
Outdated
Show resolved
Hide resolved
services/src/main/java/org/keycloak/protocol/oid4vc/issuance/OID4VCIssuerEndpoint.java
Outdated
Show resolved
Hide resolved
services/src/main/java/org/keycloak/protocol/oid4vc/issuance/OID4VCIssuerEndpoint.java
Outdated
Show resolved
Hide resolved
services/src/main/java/org/keycloak/protocol/oid4vc/issuance/OID4VCIssuerEndpoint.java
Outdated
Show resolved
Hide resolved
services/src/main/java/org/keycloak/protocol/oid4vc/model/BatchCredentialRequest.java
Outdated
Show resolved
Hide resolved
services/src/main/java/org/keycloak/protocol/oid4vc/issuance/OID4VCIssuerEndpoint.java
Outdated
Show resolved
Hide resolved
|
Thanka for the comments @IngridPuppet, I'll review them and get back to you. |
|
Hello @IngridPuppet can u check this again please? |
Ogenbertrand
changed the title
IngridPuppet
requested changes
May 28, 2025
Comment on lines
36
to
+54
| /** | ||
| * Returns the credential format supported by the builder. | ||
| * Returns the format supported by this builder. | ||
| */ | ||
| String getSupportedFormat(); | ||
|
|
||
| /** | ||
| * Builds a verifiable credential of a specific format from the basis of | ||
| * an internal representation of the credential. | ||
| * | ||
| * <p> | ||
| * The credential is built incompletely, intended that it would be signed externally. | ||
| * </p> | ||
| * | ||
| * @param verifiableCredential an internal representation of the credential | ||
| * @param credentialBuildConfig additional configurations for building the credential | ||
| * @return the built verifiable credential of the specific format, ready to be signed | ||
| * Builds a credential body from a VerifiableCredential and configuration. | ||
| */ | ||
| CredentialBody buildCredentialBody(VerifiableCredential verifiableCredential, CredentialBuildConfig credentialBuildConfig) | ||
| throws CredentialBuilderException; | ||
|
|
||
| /** | ||
| * Builds a credential body with an optional proof for key binding, used for multiple credential issuance. | ||
| */ | ||
| CredentialBody buildCredentialBody( | ||
| VerifiableCredential verifiableCredential, | ||
| CredentialBuildConfig credentialBuildConfig | ||
| ) throws CredentialBuilderException; | ||
| default CredentialBody buildCredentialBody(VerifiableCredential verifiableCredential, CredentialBuildConfig credentialBuildConfig, Proof proof) | ||
| throws CredentialBuilderException { | ||
| // Default implementation for backward compatibility: ignore proof and call the original method | ||
| return buildCredentialBody(verifiableCredential, credentialBuildConfig); | ||
| } |
There was a problem hiding this comment.
What is the goal of this conceptual change?
Comment on lines
51
to
65
| public JwtCredentialBody buildCredentialBody( | ||
| VerifiableCredential verifiableCredential, | ||
| CredentialBuildConfig credentialBuildConfig | ||
| ) throws CredentialBuilderException { | ||
| return buildCredentialBody(verifiableCredential, credentialBuildConfig, null); | ||
| } | ||
|
|
||
| @Override | ||
| public JwtCredentialBody buildCredentialBody( | ||
| VerifiableCredential verifiableCredential, | ||
| CredentialBuildConfig credentialBuildConfig, | ||
| Proof proof | ||
| ) throws CredentialBuilderException { | ||
| // Populate the issuer field of the VC | ||
| verifiableCredential.setIssuer(URI.create(credentialIssuer)); |
There was a problem hiding this comment.
You update the CredentialBuilder interface, inject a proof here, but do not anything with. You even add a comment:
// Key binding is handled by ProofValidator, which adds the JWK to the credential bodyCould you communicate the rationale for these updates?
Comment on lines
20
to
45
| import org.keycloak.jose.jwk.JWK; | ||
| import org.keycloak.protocol.oid4vc.issuance.VCIssuanceContext; | ||
| import org.keycloak.protocol.oid4vc.issuance.VCIssuerException; | ||
| import org.keycloak.protocol.oid4vc.model.Proof; | ||
| import org.keycloak.provider.Provider; | ||
|
|
||
| /** | ||
| * Interface for validating proofs provided in credential requests. | ||
| * | ||
| * @author <a href="https://github.com/wistefan">Stefan Wiedemann</a> | ||
| */ | ||
| public interface ProofValidator extends Provider { | ||
|
|
||
| @Override | ||
| default void close() { | ||
| } | ||
|
|
||
| /** | ||
| * Validates a client-provided key binding proof. | ||
| * | ||
| * @param vcIssuanceContext the issuance context with credential request and config | ||
| * @return the JWK to bind to the credential | ||
| * Validates a proof in the issuance context and returns the JWK for key binding. | ||
| */ | ||
| JWK validateProof(VCIssuanceContext vcIssuanceContext) throws VCIssuerException; | ||
|
|
||
| /** | ||
| * Validates a specific proof and returns the JWK for key binding, used for multiple credential issuance. | ||
| */ | ||
| default JWK validateProof(VCIssuanceContext vcIssuanceContext, Proof proof) throws VCIssuerException { | ||
| // Default implementation for backward compatibility: use the original method | ||
| return validateProof(vcIssuanceContext); | ||
| } | ||
| } |
There was a problem hiding this comment.
A vcIssuanceContext already embeds a proof. What is the need for these changes?
Comment on lines
31
to
+39
| * @author <a href="https://github.com/wistefan">Stefan Wiedemann</a> | ||
| */ | ||
| @JsonInclude(JsonInclude.Include.NON_NULL) | ||
| public class CredentialRequest { | ||
|
|
||
| @JsonProperty("credential_specs") | ||
| private List<CredentialSpec> credentialSpecs; | ||
|
|
||
| // Backward compatibility fields for single credential requests |
There was a problem hiding this comment.
A Credential Request has a structure defined by the spec, carrying a vision of mutiple credential issuance that we must comply with. Please consider reading this section of the spec and update this structure accordingly: https://openid.net/specs/openid-4-verifiable-credential-issuance-1_0-15.html#name-credential-request
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.
I implemented the multiple credential insuance in kc, that's updating credential endpoint to understand the request of multiple credential instances. please review and leave your thoughts.