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.

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

25940 support credentials issuance through oid4vci #27931

Conversation

wistefan
Copy link
Contributor

Implements the OID4VCI endpoints and the Pre-Authorized Code Grant to allow issuance of verifiable credentials

closes #25940

Copy link

@keycloak-github-bot keycloak-github-bot bot left a comment

Choose a reason for hiding this comment

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

Unreported flaky test detected, please review

@keycloak-github-bot
Copy link

Unreported flaky test detected

If 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.x509.X509BrowserCRLTest#loginFailedWithIntermediateRevocationListFromHttp

Keycloak CI - FIPS IT (strict)

java.lang.RuntimeException: Could not create statement
	at org.jboss.arquillian.junit.Arquillian.methodBlock(Arquillian.java:307)
	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
...

Report flaky test

org.keycloak.testsuite.x509.X509BrowserCRLTest#loginFailedWithInvalidSignatureCRL

Keycloak CI - FIPS IT (strict)

java.lang.RuntimeException: Could not create statement
	at org.jboss.arquillian.junit.Arquillian.methodBlock(Arquillian.java:307)
	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
...

Report flaky test

org.keycloak.testsuite.x509.X509BrowserCRLTest#loginWithMultipleRevocationLists

Keycloak CI - FIPS IT (strict)

java.lang.RuntimeException: Could not create statement
	at org.jboss.arquillian.junit.Arquillian.methodBlock(Arquillian.java:307)
	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
...

Report flaky test

Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com>
Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com>
Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com>
Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com>
Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com>
Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com>
Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com>
Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com>
Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com>
Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com>
Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com>
Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com>
@wistefan wistefan force-pushed the 25940-support-credentials-issuance-through-oid4vci branch from 456ebc0 to 7beca5e Compare April 8, 2024 12:07
Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com>
@wistefan
Copy link
Contributor Author

wistefan commented Apr 8, 2024

Hi,
I have run OID4VCIssuerEndpointTest on “25940-support-credentials-issuance-through-oid4vci” branch. I found out something wrong, so let me share that here.
OID4VCIssuerEndpointTest , as it is committed, goes well actually. But I have tried to dig deeper and added some assert statement(*) in requestOffer method for checking inside vc. Then it looked something going wrong.
Here is what happens: Some claims, which is supposed to be in vc object, is missing like below.

{
"nbf" : 1712172643,
"jti" : "urn:uuid:b9ca7966-5841-4087-b9ee-526e19bad68b",
"iss" : "did:web:test.org",
"vc" : {
"type" : [ "VerifiableCredential" ],
"issuer" : "did:web:test.org",
"issuanceDate" : 1712172643192,
"credentialSubject" : { }
}

Maybe it is because mapper is not called properly, where it should be.
OID4VCIssuerEndpoint with getCredential method tries to fetch OID4VCMapper from session with “session.getAllProviders(OID4VCMapper.class)”. But actually it gets empty object and as a result no mapper gets called there.
(*) FYI, added assert statements are something like below at the very last of the requestOffer method in OID4VCIssuerEndpointTest.

        JsonWebToken jsonWebToken = TokenVerifier.create((String) credentialResponse.getCredential(), JsonWebToken.class).getToken();
        assertEquals("did:web:test.org", jsonWebToken.getIssuer());

        VerifiableCredential credential = new ObjectMapper().convertValue(jsonWebToken.getOtherClaims().get("vc"), VerifiableCredential.class);
        assertEquals(List.of("VerifiableCredential"), credential.getType());
        assertEquals(URI.create("did:web:test.org"), credential.getIssuer());
        assertEquals("john@email.cz", credential.getCredentialSubject().getClaims().get("email"));

Hi, thank you, I will check whats happening.

@bucchi
I fixed it in 7beca5e and added your assertions.

@ahus1
Copy link
Contributor

ahus1 commented Apr 8, 2024

@wistefan - thank, now I understand you situation better. If you run into problems running the test locally, I'm happy to lend a hand for that. Sorry for my comment here which was more harsh than it should have been.

To use the GitHub actions to run the tests, you can start a PR as "draft" and then only those people you mention will get notifications. The notifications to all code owners are then sent out only once you mark it "ready for review".

If you start it as "read to review", and then later switch it to "draft", maintainers will still get notifications, but they can unsubscribe until you switch it to "read for review again".

Best,
Alexander

@wistefan
Copy link
Contributor Author

wistefan commented Apr 9, 2024

@wistefan Thank you for your work. Some CI tests failed. Before reviewing the PR, could you revise codes to pass these tests?

@tnorimat I fixed the tests in 3372d3c. The issue happend, because enabling the OID4VCI feature for the test worked on the default test environment(with undertow) but on quarkus(the auth server used in the CI) requires a restart.

@tnorimat
Copy link
Contributor

@wistefan Hello, I am back from my business trip in Europe. I will review the PR in this week.

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 This is great stuff!

The PR is a bit bigger and I did not reviewed extensively all the aspects of the protocol. Relying on the review of @tnorimat, @francis-pouatcha and @babisRoutis for that. Thanks to all of you for the review!

But overally, seems to me as great stuff. I've added some comments inline (none of them is probably blocker of this PR and some are more a questions, but if you can address them, it can be nice).

Besides that, I hope to merge once @tnorimat reviews and approves this PR.

Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com>
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 Hello,
I partially reviewed the PR and add some comments Could you check them?

Also, I need more time to complete the PR because the size of codes of the PR is big as @mposolda said. I will do my best for it in the week.

@@ -0,0 +1,19 @@
#
# Copyright 2016 Red Hat, Inc. and/or its affiliates
Copy link
Contributor

Choose a reason for hiding this comment

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

2016 might be 2024

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in bbdb87f

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you.

*
* @param session - keycloak session to be used
* @param authenticatedClientSession - client session to be persisted
* @param expirationTime - expiration time of the code, the code should be short-lifed
Copy link
Contributor

Choose a reason for hiding this comment

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

short-lifed might be short-lived

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in bbdb87f

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you.

import org.keycloak.events.EventType;
import org.keycloak.models.AuthenticatedClientSessionModel;
import org.keycloak.models.KeycloakSession;
import org.keycloak.protocol.oid4vc.OID4VCClientRegistrationProvider;
Copy link
Contributor

Choose a reason for hiding this comment

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

OID4VCClientRegistrationProvider might not be used in the 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.

Fixed in bbdb87f

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you.

session.getContext().setClient(result.getClientSession().getClient());


// set the client as retrieved from the pre-authorized session
Copy link
Contributor

Choose a reason for hiding this comment

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

L79-80 mighit be the same as L75-76. Is this duplication needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in bbdb87f

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you.

Response.Status.BAD_REQUEST);
}
AuthenticatedClientSessionModel clientSession = result.getClientSession();
DefaultClientSessionContext sessionContext = DefaultClientSessionContext.fromClientSessionAndScopeParameter(clientSession,
Copy link
Contributor

Choose a reason for hiding this comment

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

ClientSessionContext might be better instead of DefaultClientSessionContext

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in bbdb87f

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you.

import static org.junit.Assert.assertEquals;

@EnableFeature(value = Profile.Feature.OID4VC_VCI, skipRestart = true)
public class PreAuthorizedGrantTest extends AbstractTestRealmKeycloakTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that tests do not access Credential Endpoint to get a verifiable credential in return for an access token. I think we need to test this point.

Would you intend not to do such the tests in the PR, but do suche tests in the follwoing PR after the PR is mereged?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see the actual credential retrieval rather as part of the OID4VCIssuerEndpointTest(where I do test exactly what 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.

Thank you for the clarification.

Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com>
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 I completed the review of the PR. I added some comments. Could you check them?

*
* @author <a href="https://github.com/wistefan">Stefan Wiedemann</a>
*/
public class OID4VCClientRegistrationProvider extends AbstractClientRegistrationProvider {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you tell me why OID4VCI specific client OID4VCClient is needed? Compared with a normal client OIDCClientRepresentation, OID4VCClient' adds the following field:

/** * Comma-separated list of supported credentials types */ private List<SupportedCredentialConfiguration> supportedVCTypes;
If so, adding it onto OIDCClientRepresentation is also an option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, I´m not sure if I fully understand. Beside the supportedVCTypes, it also adds the did of the client. Its used to provide the ability to create credentials focused for clients supporting OID4VC standards, not neccessarily OIDC. Mixing both might be confusing, especially if one only supports one of them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the clarification.


/**
* Implementation of the {@link ClientRegistrationProviderFactory} to integrate the OID4VC protocols with
* Keycloaks client-registration.
Copy link
Contributor

Choose a reason for hiding this comment

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

Keycloaks might be Keycloak's

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 940c4af

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you.


@Override
public LoginProtocol create(KeycloakSession session) {
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

OIDCLoginProtocolFactory returns an instance of OIDCLoginProtocol.
Is it OK to return null here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We currently support to login with oid4vp in keycloak, thus no such protocl is implemented. As far as I understand this means returning null here is ok.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the clarification.

* @author <a href="https://github.com/wistefan">Stefan Wiedemann</a>
*/
@JsonInclude(JsonInclude.Include.NON_NULL)
public class CredentialOfferURI {
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://openid.net/specs/openid-4-verifiable-credential-issuance-1_0.html#section-4.1.3 ,
The format of credential_offer_uri is String.
Could you tell me why the PR defines credential_offer_uri as the class (issuer, nonce). ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to build it that way, since the retrieval endpoint for that credential_offer_uri is not defined in the spec, just the way how it should be shown to a wallet(from my understanding). We use it to generate the string in the frontend for the qr from it. However, we could add a plain/text endpoint, too, if you think its valuable

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the clarification.

*
* @author <a href="https://github.com/wistefan">Stefan Wiedemann</a>
*/
public enum ErrorType {
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://openid.net/specs/openid-4-verifiable-credential-issuance-1_0.html#section-7.3.1.2 , error types are as follows:

invalid_credential_request
unsupported_credential_type
unsupported_credential_format
invalid_proof
invalid_encryption_parameters

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably something changed to an older draft again, I'll updated 940c4af

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you.

@@ -58,6 +58,8 @@
import org.keycloak.models.utils.KeycloakModelUtils;
import org.keycloak.protocol.oidc.OIDCLoginProtocol;
import org.keycloak.protocol.oidc.OIDCLoginProtocolService;
import org.keycloak.protocol.oidc.grants.PreAuthorizedCodeGrantType;
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that it is not used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 42f5903

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you.

@@ -790,6 +793,27 @@ public AccessTokenResponse doClientCredentialsGrantAccessTokenRequest(String cli
}
}

public AccessTokenResponse doPreauthorizedTokenRequest(String preAuthorizedCode) throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why it is not used in PreAuthorizedGrantTest ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I wanted to make the usage of the api and its parameters more explicit in the test. E.g. for testing the PreAuthorizedToken Feature, I build the requests in the test, for using the feature in other tests(e.g. the OID4VCI tests), I use the helper method here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the clarification.

@@ -53,7 +52,7 @@
import static org.junit.Assert.fail;


public class JwtSigningServiceTest extends SigningServiceTest {
public class JwtSigningServiceTest extends OID4VCTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

L76 and L78 are opposite.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Amazing, how are you able to spot such thing!?:) Fixed in 42f5903

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you.

import org.keycloak.common.util.CertificateUtils;
import org.keycloak.common.util.KeyUtils;
import org.keycloak.common.util.MultivaluedHashMap;
import org.keycloak.common.util.PemUtils;
import org.keycloak.crypto.KeyUse;
import org.keycloak.crypto.KeyWrapper;
import org.keycloak.models.KeycloakSession;
import org.keycloak.models.utils.KeycloakModelUtils;
import org.keycloak.protocol.oid4vc.OID4VCClientRegistrationProviderFactory;
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that it is not used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 42f5903

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you.

import org.keycloak.testsuite.AbstractTestRealmKeycloakTest;
import org.keycloak.testsuite.arquillian.annotation.EnableFeature;
import org.keycloak.testsuite.util.RoleBuilder;
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that it is not used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 42f5903

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you.

Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com>
Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com>
Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com>
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.

LGTM.

@tnorimat
Copy link
Contributor

@wistefan Thank you very much for your work.
@mposolda Could you re-check the PR and merge it if it is OK?

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 @babisRoutis Thanks everyone for the update and review of this PR!

@mposolda mposolda merged commit b08c644 into keycloak:main Apr 22, 2024
65 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Credentials Issuance through the OID4VCI Protocol
8 participants