Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Issue Verifiable Credentials in the SD-JWT-VC format #25942 #27207

Merged
merged 7 commits into from
Mar 11, 2024
Merged

Issue Verifiable Credentials in the SD-JWT-VC format #25942 #27207

merged 7 commits into from
Mar 11, 2024

Conversation

wistefan
Copy link
Contributor

Adds support for the SD-JWT Format

closes #25942

@wistefan wistefan marked this pull request as ready for review February 21, 2024 14:20
@wistefan wistefan requested review from a team as code owners February 21, 2024 14:20
@keycloak-github-bot keycloak-github-bot bot mentioned this pull request Feb 21, 2024
Closed
@mposolda mposolda self-assigned this Feb 21, 2024
mposolda
mposolda reviewed Feb 22, 2024
View reviewed changes
Copy link
Contributor

@mposolda mposolda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wistefan Thanks! Added few minor inline comments.

@tnorimat @francis-pouatcha do you please have opportunity to review as well?

services/src/main/java/org/keycloak/protocol/oid4vc/issuance/signing/SdJwtSigningService.java

DisclosureSpec.Builder disclosureSpecBuilder = DisclosureSpec.builder();
CredentialSubject credentialSubject = verifiableCredential.getCredentialSubject();
JsonNode claimSet = objectMapper.valueToTree(credentialSubject);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Do we need to use JsonNode directly here or is it possible to use some Java class dedicated for this like we're doing in other places?

If JsonNode is really needed, is it at least possible to introduce constants for fields like iss, nbf, vct, _sd_alg and jti ? I suppose things like iss, nbf, jti can be probably added directly to JsonWebToken class as they are quite generic. For others, probably into some JWT-VC specific class?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The JsonNode needs to be used, because the claims could be anything. However, I will introduce constants for the well-known attributes you mentioned

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wistefan Will be good. Thanks!

services/src/main/java/org/keycloak/protocol/oid4vc/issuance/signing/SigningProperties.java Show resolved Hide resolved
@tnorimat
Copy link
Contributor

@mposolda Yes, I will review the PR.

tnorimat
tnorimat reviewed Feb 22, 2024
View reviewed changes
...se/src/test/java/org/keycloak/testsuite/oid4vc/issuance/signing/SdJwtSigningServiceTest.java
@@ -0,0 +1,312 @@
package org.keycloak.testsuite.oid4vc.issuance.signing;

This comment was marked as resolved.

Sign in to view

tnorimat
tnorimat reviewed Feb 24, 2024
View reviewed changes
Copy link
Contributor

@tnorimat tnorimat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wistefan It seems that this PR does not include JWT VC Issuer Metadata. I think you drop the support because it is optinal. Is that right?

tnorimat
tnorimat reviewed Feb 24, 2024
View reviewed changes
services/src/main/java/org/keycloak/protocol/oid4vc/issuance/signing/SdJwtSigningService.java Outdated
throw new SigningServiceException("SD-JWT only supports single type credentials.");
}
rootNode.put(VERIFIABLE_CREDENTIAL_TYPE_CLAIM, verifiableCredential.getType().get(0));
rootNode.put(SELECTIVE_DISCLOSURE_ALGORITHM_CLAIM, hashAlgorithm);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be

rootNode.put("_sd_alg", hashAlgorithm);

or

rootNode.put(IssuerSignedJWT.CLAIM_NAME_SD_HASH_ALGORITHM, hashAlgorithm);

tnorimat
tnorimat suggested changes Feb 24, 2024
View reviewed changes
Copy link
Contributor

@tnorimat tnorimat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added review comments. Could you check them?

...se/src/test/java/org/keycloak/testsuite/oid4vc/issuance/signing/SdJwtSigningServiceTest.java

String jwt = new StringJoiner(".")
// header
.add(splittedToken[0])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to https://drafts.oauth.net/oauth-sd-jwt-vc/draft-ietf-oauth-sd-jwt-vc.html#name-jose-header, "typ" must be "vc+sd-jwt". How about checking it somewhere?

@francis-pouatcha
Copy link
Contributor

I will be proceeding with a review this weekend.

@wistefan wistefan mentioned this pull request Mar 4, 2024
Merged
@wistefan
Copy link
Contributor Author

wistefan commented Mar 4, 2024

@wistefan It seems that this PR does not include JWT VC Issuer Metadata. I think you drop the support because it is optinal. Is that right?

@tnorimat Not sure if we are talking about the same things, but I want to include the well-known endpoints in the PR for #25940. Thus, no endpoints are included in the signing service PRs

@tnorimat
Copy link
Contributor

tnorimat commented Mar 4, 2024

@wistefan

@tnorimat Not sure if we are talking about the same things, but I want to include the well-known endpoints in the PR for #25940. Thus, no endpoints are included in the signing service PRs

I see. Thank you for your clarification.

@wistefan wistefan force-pushed the 25942-issue-verifiable-credentials-in-the-sd-jwt-vc-format branch from c91eb2c to 4386ed2 Compare March 6, 2024 15:01
This was referenced Mar 6, 2024
Open
Open
@wistefan wistefan force-pushed the 25942-issue-verifiable-credentials-in-the-sd-jwt-vc-format branch 2 times, most recently from 984cbba to db22fcd Compare March 7, 2024 07:14
wistefan and others added 7 commits March 7, 2024 08:31
@wistefan
implement sd-jwt signing
7e90da3 
Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com>
@wistefan
cleanup
37c098c 
Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com>
@wistefan
add claim constants and add header
e6ee25f 
Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com>
@wistefan @francis-pouatcha
Update services/src/main/java/org/keycloak/protocol/oid4vc/issuance/s…
5dfe61e 
…igning/SdJwtSigningService.java

Co-authored-by: Francis Pouatcha <francis.pouatcha@adorsys.com>
Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com>
@wistefan @francis-pouatcha
Update services/src/main/java/org/keycloak/protocol/oid4vc/issuance/s…
5123e64 
…igning/SdJwtSigningService.java

Co-authored-by: Francis Pouatcha <francis.pouatcha@adorsys.com>
Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com>
@wistefan @francis-pouatcha
Update services/src/main/java/org/keycloak/protocol/oid4vc/issuance/s…
eec38cf 
…igning/SdJwtSigningService.java

Co-authored-by: Francis Pouatcha <francis.pouatcha@adorsys.com>
Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com>
@wistefan
fix tests
c5ca762 
Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com>
@wistefan wistefan force-pushed the 25942-issue-verifiable-credentials-in-the-sd-jwt-vc-format branch from db22fcd to c5ca762 Compare March 7, 2024 07:32
mposolda
mposolda approved these changes Mar 11, 2024
View reviewed changes
Copy link
Contributor

@mposolda mposolda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wistefan @tnorimat @francis-pouatcha Thanks for the updates and review of this PR!

@mposolda mposolda merged commit 6fc69b6 into keycloak:main Mar 11, 2024
65 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mposolda mposolda mposolda approved these changes

@francis-pouatcha francis-pouatcha francis-pouatcha approved these changes

@tnorimat tnorimat tnorimat approved these changes

Assignees

@mposolda mposolda

@tnorimat tnorimat

Labels
flaky-test team/cloud-native team/core-clients team/core-iam
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Issue Verifiable Credentials in the SD-JWT-VC format
4 participants
@wistefan @tnorimat @francis-pouatcha @mposolda