[OID4VCI-HAIP] Pass oid4vci-1_0-issuer-happy-flow by tdiesler · Pull Request #47405 · keycloak/keycloak

tdiesler · 2026-03-24T15:03:36Z

depends on

tdiesler · 2026-03-24T15:07:03Z

The list of dependencies is quite long. Next merge candidates are #47296 then #47150

IngridPuppet

Hello @tdiesler - I only reviewed commit "[OID4VCI-HAIP] Pass oid4vci-1_0-issuer-happy-flow". I'm not sure if it is only commit relevant for this PR. As per my review, this PR enables ABCA for the HAIP test to pass. I left some comments, please could you check?

IngridPuppet · 2026-04-01T14:20:05Z

+
+            if (kid == null || kid.equals(currentKid)) {
+                String kty = key.get("kty").asText();


In case kid is null, only the first JWK in the list will be considered. I'd rather either all are considered as potential verifier keys or null kids are not supported at all. WDYT?

Doing this for now ...

if (Strings.isEmpty(kid)) throw new IllegalStateException("Invalid attester kid: " + kid); ABCAConfig attesterKeys = JsonSerialization.valueFromString(abcaConfigValue, ABCAConfig.class); for (JWK key : attesterKeys.keys) { if (kid.equals(key.getKeyId())) { ... return new JWKParser(key).toPublicKey(); } }

i.e. we require the JWS header to provide a valid kid - this is lookup semantics rather than iterate-retry-on-failure - works for conformance tests.

This is fine for me. Thx.

IngridPuppet · 2026-04-01T14:20:37Z

+
+                String n = key.get("n").asText();
+                String e = key.get("e").asText();
+
+                BigInteger modulus = new BigInteger(1, Base64Url.decode(n));
+                BigInteger exponent = new BigInteger(1, Base64Url.decode(e));
+
+                RSAPublicKeySpec spec = new RSAPublicKeySpec(modulus, exponent);
+                return KeyFactory.getInstance("RSA").generatePublic(spec);


Consider reusing this class for this related logic.

keycloak/core/src/main/java/org/keycloak/sdjwt/JwkParsingUtils.java

Lines 33 to 48 in d7238a7

public class JwkParsingUtils {

public static SignatureVerifierContext convertJwkNodeToVerifierContext(JsonNode jwkNode) {

JWK jwk;

try {

jwk = SdJwtUtils.mapper.convertValue(jwkNode, JWK.class);

} catch (Exception e) {

throw new IllegalArgumentException("Malformed JWK");

}

return convertJwkToVerifierContext(jwk);

}

public static SignatureVerifierContext convertJwkToVerifierContext(JWK jwk) {

// Wrap JWK

We are doing this ...

// The signature of the Client Attestation JWT verifies with the public key of a known and trusted Attester // PublicKey publicKey = findAttesterPublicKey(context, jws.getHeader().getKeyId()); // Verification and Processing // TokenVerifier.create(headerValue, ClientAttestationJwt.class) .publicKey(publicKey) .withChecks(subCheck, issCheck, cnfCheck, TokenVerifier.IS_ACTIVE) .verify().getToken();

Is that ok?

Thank you for the context. No strong opinion, that's fine for me.

IngridPuppet · 2026-04-01T14:21:49Z

+        HttpHeaders headers = httpRequest.getHttpHeaders();
+        String headerValue = headers.getHeaderString(OAUTH_CLIENT_ATTESTATION_HEADER);
+        if (headerValue == null)
+            throw new IllegalStateException("Required header " + OAUTH_CLIENT_ATTESTATION_HEADER + " for is missing");


This looks more like a BadRequestException. Same in the rest of the method.

I changed to IllegalArgumentException where appropriate. It doesn't matter much for these private methods - every Exception is caught and converted

try { ClientAttestationJwt attesterJwt = validateClientAttestationJwt(context); validateClientAttestationPoPJwt(context, attesterJwt); context.success(); } catch (Exception ex) { ServicesLogger.LOGGER.errorValidatingAssertion(ex); Response challengeResponse = ClientAuthUtil.errorResponse(Response.Status.BAD_REQUEST.getStatusCode(), OAuthErrorException.INVALID_CLIENT_ATTESTATION, ex.getMessage()); context.failure(AuthenticationFlowError.INVALID_CLIENT_ATTESTATION, challengeResponse); }

Thank you for the clarification.

IngridPuppet · 2026-04-01T14:24:08Z

+        String clientIdParam = formParams.getFirst(CLIENT_ID);
+        if (clientIdParam != null && !clientIdParam.equals(jwt.getSubject()))
+            throw new IllegalStateException("The client attestation subject does not match the client_id parameter");


No error if clientIdParam == null. Does not seem intended.

I believe this is ok. The AttestationBasedClientAuthenticator is called twice, first on the PAR endpoint with a client_id param, then on the token endpoint without it.

We do ...

// Get the client model from the JWT subject ClientAttestationJwt attesterJwt = jws.readJsonContent(ClientAttestationJwt.class); ClientModel clientModel = Optional.ofNullable(attesterJwt.getSubject()) .map(realmModel::getClientByClientId) .orElseThrow(() -> new TokenVerificationException(attesterJwt, "The sub (subject) claim MUST identify a known client_id")); String clientIdParam = formParams.getFirst(CLIENT_ID); if (clientIdParam != null && !clientIdParam.equals(t.getSubject())) throw new TokenVerificationException(t, "The sub claim (subject) MUST match the client_id parameter");

Actually, I'm not sure whether it is correct that this is done twice. The spec says

(6) The Client Instance sends both the Client Attestation JWT and the Client Attestation PoP JWT to the authorization server, e.g. within a token request. The authorization server validates the Client Attestation and thus authenticates the Client Instance

Thank you for clarification that clientIdParam is not required at the token endpoint. It's worth confirming whether the client needs to be authenticated twice, but I see nothing wrong with it.

Thank you for clarification that clientIdParam is not required at the token endpoint. It's worth confirming whether the client needs to be authenticated twice, but I see nothing wrong with it.

We need to look into this nevertheless. With the TokenRequest (i.e. when there might be no client_id) the server should still be able to remember that it previously received an AuthorizationRequest

Doing this is fishy ...

ClientModel clientModel = Optional.ofNullable(attesterJwt.getSubject())

It should use the provided client_id from the authorization request, but I couldn't find an obvious way to get to it.

tdiesler · 2026-04-08T10:34:31Z

I suggest we focus on the other two PRs that this one depends on. Then, I'll bring the ABCA implementation (which is now spread over multiple PRs) forward to this one and then we can address all comments related to that (non-trivial) change. Indeed, I expect and welcome quite a bit of feedback on the ABCA implementation.

VinodAnandan · 2026-04-10T12:34:15Z

@tdiesler @mposolda Just checking in on this one! If I understand correctly, the blockers have been cleared. Could you please confirm if it's ready for review?

tdiesler · 2026-04-10T13:17:33Z

@tdiesler @mposolda Just checking in on this one! If I understand correctly, the blockers have been cleared. Could you please confirm if it's ready for review?

Now the actual work starts. I'll bring everything abca related forward to this PR - then we have a complete view on what is there already (and what isn't ;-)

Signed-off-by: Thomas Diesler <[email protected]>

tdiesler requested review from a team as code owners March 24, 2026 15:03

keycloak-github-bot Bot added team/ui team/cloud-native team/core-clients team/core-iam labels Mar 24, 2026

tdiesler force-pushed the ghi47316 branch 15 times, most recently from 4ca0091 to 12ed169 Compare March 26, 2026 06:09

tdiesler marked this pull request as draft March 26, 2026 06:14

tdiesler force-pushed the ghi47316 branch 3 times, most recently from 309b796 to 38889bd Compare March 26, 2026 09:08

tdiesler force-pushed the ghi47316 branch 5 times, most recently from 5446980 to 5615979 Compare April 1, 2026 14:25

IngridPuppet reviewed Apr 1, 2026

View reviewed changes

tdiesler force-pushed the ghi47316 branch from 5615979 to 4a869fc Compare April 1, 2026 17:06

tdiesler changed the title ~~[OID4VCI-HAIP] Pass oid4vci-1_0-issuer-happy-flow~~ [OID4VCI-HAIP] Pass oid4vci-1_0-issuer-happy-flow (blocked) Apr 8, 2026

tdiesler force-pushed the ghi47316 branch 7 times, most recently from 3bc0bba to 337dfd7 Compare April 10, 2026 06:00

tdiesler force-pushed the ghi47316 branch from 337dfd7 to 73413e6 Compare April 10, 2026 13:34

tdiesler marked this pull request as draft April 10, 2026 13:34

tdiesler changed the title ~~[OID4VCI-HAIP] Pass oid4vci-1_0-issuer-happy-flow (blocked)~~ [OID4VCI-HAIP] Pass oid4vci-1_0-issuer-happy-flow Apr 10, 2026

keycloak-github-bot Bot added the flaky-test label Apr 10, 2026

keycloak-github-bot Bot mentioned this pull request Apr 10, 2026

Flaky test: org.keycloak.testsuite.forms.AuthenticatorSubflowsTest2#testSubflow2 #47477

Closed

tdiesler force-pushed the ghi47316 branch 2 times, most recently from 3b459be to 63f0df4 Compare April 10, 2026 14:38

tdiesler added 2 commits April 10, 2026 18:23

Add support for OAuth 2.0 Attestation-based client authentication

c76cc67

Signed-off-by: Thomas Diesler <[email protected]>

[OID4VCI-HAIP] Pass oid4vci-1_0-issuer-happy-flow

ce31940

Signed-off-by: Thomas Diesler <[email protected]>

tdiesler force-pushed the ghi47316 branch from 63f0df4 to ce31940 Compare April 10, 2026 16:25

tdiesler closed this Apr 11, 2026

tdiesler deleted the ghi47316 branch April 11, 2026 06:05


		if (kid == null \|\| kid.equals(currentKid)) {
		String kty = key.get("kty").asText();

	public class JwkParsingUtils {

	public static SignatureVerifierContext convertJwkNodeToVerifierContext(JsonNode jwkNode) {
	JWK jwk;

	try {
	jwk = SdJwtUtils.mapper.convertValue(jwkNode, JWK.class);
	} catch (Exception e) {
	throw new IllegalArgumentException("Malformed JWK");
	}

	return convertJwkToVerifierContext(jwk);
	}

	public static SignatureVerifierContext convertJwkToVerifierContext(JWK jwk) {
	// Wrap JWK

Conversation

tdiesler commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tdiesler commented Mar 24, 2026

Uh oh!

IngridPuppet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tdiesler Apr 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tdiesler Apr 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tdiesler Apr 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tdiesler commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VinodAnandan commented Apr 10, 2026

Uh oh!

tdiesler commented Apr 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tdiesler commented Mar 24, 2026 •

edited

Loading

tdiesler Apr 11, 2026 •

edited

Loading

tdiesler Apr 11, 2026 •

edited

Loading

tdiesler Apr 13, 2026 •

edited

Loading

tdiesler commented Apr 8, 2026 •

edited

Loading