Skip to content

[OID4VCI] Enforce HAIP x5c rules for SD-JWT issuance - #48528

Merged
mposolda merged 7 commits into
keycloak:mainfrom
adorsys:oid4vci-conformace-3
Jul 27, 2026
Merged

[OID4VCI] Enforce HAIP x5c rules for SD-JWT issuance#48528
mposolda merged 7 commits into
keycloak:mainfrom
adorsys:oid4vci-conformace-3

Conversation

@forkimenjeckayang

@forkimenjeckayang forkimenjeckayang commented Apr 28, 2026

Copy link
Copy Markdown
Contributor

closes #51176

This PR enforces HAIP-6.1.1 x5c handling for SD-JWT credential issuance.

Changes:

  • Always add an HAIP-compliant x5c header for SD-JWT credentials using the selected signing key certificate chain.
  • Reject SD-JWT issuance when the signing certificate itself is self-signed.
  • Remove trailing self-signed root / trust-anchor certificates from the emitted x5c header.
  • Detect self-signed certificates by verifying the certificate with its own public key instead of relying on subject == issuer.
  • Remove the vc.issuer_key_resolution_strategy / haip_x5c configuration option, since HAIP-style x5c handling is now the default.
  • Make the HAIP SD-JWT signing key setup in tests idempotent to avoid duplicate key providers.
  • Update existing SD-JWT signer tests for the default HAIP x5c behavior.

Conformance Issue Addressed

  • Test environment / plan: oid4vci-1_0-issuer-test-plan
  • Variant highlights: client_auth_type=private_key_jwt, sender_constrain=dpop, credential_format=sd_jwt_vc, vci_grant_type=authorization_code, vci_credential_encryption=encrypted, fapi_profile=vci_haip
  • Failing test: oid4vci-1_0-issuer-happy-flow
  • Failure: HAIP-6.1.1 ValidateSdJwtCredentialX5cCertificateChain
  • Error: Trust anchor certificate must not be included in x5c chain
    This PR resolves that conformance failure by ensuring the SD-JWT x5c chain contains only end-entity/intermediate certificates and excludes the root trust anchor, as required by HAIP.
image

Related to : #46265
See: adorsys#245 (comment)

@forkimenjeckayang
forkimenjeckayang requested review from a team as code owners April 28, 2026 07:38
@forkimenjeckayang forkimenjeckayang changed the title [OI4VCI] enforce HAIP x5c trust-anchor rules and migrate signing/builder tests to new testsuite [OID4VCI] enforce HAIP x5c trust-anchor rules and migrate signing/builder tests to new testsuite Apr 28, 2026
@forkimenjeckayang
forkimenjeckayang force-pushed the oid4vci-conformace-3 branch 2 times, most recently from 137ae93 to 303491b Compare April 29, 2026 07:38
@forkimenjeckayang
forkimenjeckayang force-pushed the oid4vci-conformace-3 branch 2 times, most recently from 51281ff to 2d10913 Compare May 5, 2026 14:32
@keycloak-github-bot

Copy link
Copy Markdown

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.federation.ldap.LDAPGroupMapperTest#test01_ldapOnlyGroupMappings

Keycloak CI - Base IT (5)

org.opentest4j.AssertionFailedError: expected: <4> but was: <1>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:150)
...

Report flaky test

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

Copy link
Copy Markdown

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

@VinodAnandan

Copy link
Copy Markdown
Contributor

@forkimenjeckayang thank you for your PR. Unfortunately, there are some new conflicts that need to be resolved. Could you please take a look when you get a chance?

@VinodAnandan

Copy link
Copy Markdown
Contributor

@mposolda, @graziang, @pskopek, @jimmychakkalakal: Could you please help review this community contribution PR when you have a chance? It has been awaiting feedback for quite some time, and any early comments, suggestions, or thoughts would be greatly appreciated.

@dominikschlosser

dominikschlosser commented May 26, 2026

Copy link
Copy Markdown
Contributor

As HAIP is an addition on top of OID4VC, i am not sure if it should be this tightly coupled. This will also get relevant when we add OID4VP (presentation), where HAIP adds constraints but also different paths than Non-HAIP VP (for example plain OID4VP allows to use the .well-known/jwt-vc-issuer endpoint to get the cert for credential validation, while HAIP mandates to only use the x5c header).

Maybe it should be more flexible like the new TrustMaterialIdentityProvider allows for verification strategy.

@forkimenjeckayang
forkimenjeckayang requested review from a team as code owners June 1, 2026 09:17
@keycloak-github-bot

Copy link
Copy Markdown

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.forms.MultipleTabsLoginTest#multipleTabsLoginAndPassiveCheck

Keycloak CI - Forms IT (chrome)

org.opentest4j.MultipleFailuresError: 
Multiple Failures (2 failures)
	org.openqa.selenium.TimeoutException: timeout: Timed out receiving message from renderer: 10.000
  (Session info: chrome=149.0.7827.53)
Build info: version: '4.28.1', revision: '73f5ad48a2'
...

Report flaky test

org.keycloak.testsuite.forms.MultipleTabsLoginTest#multipleTabsParallelLoginTestWithAuthSessionExpiredInTheMiddle

Keycloak CI - Forms IT (chrome)

org.openqa.selenium.TimeoutException: 
timeout: Timed out receiving message from renderer: 10.000
  (Session info: chrome=149.0.7827.53)
Build info: version: '4.28.1', revision: '73f5ad48a2'
System info: os.name: 'Linux', os.arch: 'amd64', os.version: '6.17.0-1018-azure', java.version: '25.0.3'
...

Report flaky test

org.keycloak.testsuite.forms.MultipleTabsLoginTest#loginActionWithoutExecutionInRequiredActions

Keycloak CI - Forms IT (chrome)

org.openqa.selenium.TimeoutException: 
timeout: Timed out receiving message from renderer: 10.000
  (Session info: chrome=149.0.7827.53)
Build info: version: '4.28.1', revision: '73f5ad48a2'
System info: os.name: 'Linux', os.arch: 'amd64', os.version: '6.17.0-1018-azure', java.version: '25.0.3'
...

Report flaky test

org.keycloak.testsuite.forms.MultipleTabsLoginTest#multipleTabsParallelLoginTestWithAuthSessionExpiredAndRefreshInTab1

Keycloak CI - Forms IT (chrome)

org.openqa.selenium.TimeoutException: 
timeout: Timed out receiving message from renderer: 10.000
  (Session info: chrome=149.0.7827.53)
Build info: version: '4.28.1', revision: '73f5ad48a2'
System info: os.name: 'Linux', os.arch: 'amd64', os.version: '6.17.0-1018-azure', java.version: '25.0.3'
...

Report flaky test

org.keycloak.testsuite.forms.MultipleTabsLoginTest#testLoginPageRefresh

Keycloak CI - Forms IT (chrome)

org.openqa.selenium.TimeoutException: 
timeout: Timed out receiving message from renderer: 10.000
  (Session info: chrome=149.0.7827.53)
Build info: version: '4.28.1', revision: '73f5ad48a2'
System info: os.name: 'Linux', os.arch: 'amd64', os.version: '6.17.0-1018-azure', java.version: '25.0.3'
...

Report flaky test

org.keycloak.testsuite.forms.MultipleTabsLoginTest#loginWithDifferentClients

Keycloak CI - Forms IT (chrome)

org.openqa.selenium.TimeoutException: 
timeout: Timed out receiving message from renderer: 10.000
  (Session info: chrome=149.0.7827.53)
Build info: version: '4.28.1', revision: '73f5ad48a2'
System info: os.name: 'Linux', os.arch: 'amd64', os.version: '6.17.0-1018-azure', java.version: '25.0.3'
...

Report flaky test

org.keycloak.testsuite.forms.MultipleTabsLoginTest#multipleTabsParallelLoginTestWithAuthSessionExpiredAndRegisterClick

Keycloak CI - Forms IT (chrome)

org.openqa.selenium.TimeoutException: 
timeout: Timed out receiving message from renderer: 10.000
  (Session info: chrome=149.0.7827.53)
Build info: version: '4.28.1', revision: '73f5ad48a2'
System info: os.name: 'Linux', os.arch: 'amd64', os.version: '6.17.0-1018-azure', java.version: '25.0.3'
...

Report flaky test

org.keycloak.testsuite.forms.MultipleTabsLoginTest#multipleTabsParallelLoginTest

Keycloak CI - Forms IT (chrome)

org.openqa.selenium.TimeoutException: 
timeout: Timed out receiving message from renderer: 10.000
  (Session info: chrome=149.0.7827.53)
Build info: version: '4.28.1', revision: '73f5ad48a2'
System info: os.name: 'Linux', os.arch: 'amd64', os.version: '6.17.0-1018-azure', java.version: '25.0.3'
...

Report flaky test

org.keycloak.testsuite.forms.MultipleTabsLoginTest#expiredAuthenticationAction_expiredCodeCurrentExecution

Keycloak CI - Forms IT (chrome)

org.openqa.selenium.TimeoutException: 
timeout: Timed out receiving message from renderer: 10.000
  (Session info: chrome=149.0.7827.53)
Build info: version: '4.28.1', revision: '73f5ad48a2'
System info: os.name: 'Linux', os.arch: 'amd64', os.version: '6.17.0-1018-azure', java.version: '25.0.3'
...

Report flaky test

org.keycloak.testsuite.forms.MultipleTabsLoginTest#testInjectRedirectUriInClientDataAfterAuthSessionExpiration

Keycloak CI - Forms IT (chrome)

org.openqa.selenium.TimeoutException: 
timeout: Timed out receiving message from renderer: 10.000
  (Session info: chrome=149.0.7827.53)
Build info: version: '4.28.1', revision: '73f5ad48a2'
System info: os.name: 'Linux', os.arch: 'amd64', os.version: '6.17.0-1018-azure', java.version: '25.0.3'
...

Report flaky test

… to new testsuite

Signed-off-by: forkimenjeckayang <forkimenjeckayang@gmail.com>
Copilot AI review requested due to automatic review settings June 26, 2026 11:37

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR tightens SD-JWT issuance to meet HAIP-6.1.1 requirements around x5c handling (reject self-signed signing certs and avoid including trust anchors), and migrates/massages OID4VC credential signing/builder tests into the new testsuite structure.

Changes:

  • Add an optional issuer key resolution strategy (haip_x5c) into credential scope/config parsing and propagate it into SD-JWT signing behavior.
  • Update SdJwtCredentialSigner to emit x5c only for the HAIP strategy and enforce HAIP-6.1.1 constraints (no self-signed signing cert, no trust anchor in x5c).
  • Migrate/rework signing and credential builder tests to the new testsuite, adding a shared local base utility and adjusting test key/certificate setup.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tests/base/src/test/java/org/keycloak/tests/oid4vc/OID4VCIssuerTestBase.java Adds HAIP-oriented SD-JWT signing key provider setup in the new testsuite base.
tests/base/src/test/java/org/keycloak/tests/oid4vc/issuance/signing/SdJwtCredentialSignerTest.java Updates SD-JWT signer tests for HAIP x5c behavior and new base class.
tests/base/src/test/java/org/keycloak/tests/oid4vc/issuance/signing/OID4VCTest.java Introduces a shared local test utility base for migrated signing/builder tests.
tests/base/src/test/java/org/keycloak/tests/oid4vc/issuance/signing/JwtCredentialSignerTest.java Migrates JWT signer test to the new testsuite base and simplifies setup.
tests/base/src/test/java/org/keycloak/tests/oid4vc/issuance/credentialbuilder/SdJwtCredentialBuilderTest.java Minor formatting/structure adjustments during test migration.
tests/base/src/test/java/org/keycloak/tests/oid4vc/issuance/credentialbuilder/JwtCredentialBuilderTest.java Removes redundant comments during migration; keeps test behavior.
tests/base/src/test/java/org/keycloak/tests/oid4vc/issuance/credentialbuilder/CredentialBuilderTest.java Switches builder tests to a local RSA key and the new shared base.
services/src/main/java/org/keycloak/protocol/oid4vc/model/CredentialScopeRepresentation.java Exposes issuer key resolution strategy via credential scope attributes.
services/src/main/java/org/keycloak/protocol/oid4vc/model/CredentialBuildConfig.java Adds issuer key resolution strategy to parsed build config + equality/hash.
services/src/main/java/org/keycloak/protocol/oid4vc/issuance/signing/SdJwtCredentialSigner.java Enforces HAIP-specific x5c rules (self-signed rejection + trust-anchor exclusion).
server-spi-private/src/main/java/org/keycloak/models/oid4vci/CredentialScopeModel.java Adds new credential scope attribute constant + accessors for the strategy.

@forkimenjeckayang

Copy link
Copy Markdown
Contributor Author

I had to run this particular test plan again against Keycloak main with conformance suit Version: 5.2.0 and I ran into same error

Screenshot from 2026-06-26 12-49-51 Screenshot from 2026-06-26 12-50-28

But wiht the chnages on this branch, that was resolved

Screenshot from 2026-06-26 13-20-51

Signed-off-by: forkimenjeckayang <forkimenjeckayang@gmail.com>
@dominikschlosser

Copy link
Copy Markdown
Contributor

Keycloak now includes conformance tests, which already pass the oid4vci-1_0-issuer-happy-flow module with all variants (including HAIP). It sets up signing keys (in VciTestSigningKey) in a correct way for this to work: Root-Cert that is self-signed and leaf-cert that is signed by the root cert.
The changes in this PR would make this fail (which CI doesnt show, because conformance tests are only executed in nightly build...), because issuer_key_resolution_strategy is not set there. So no x5c-header would be included at all.

Also issuer_key_resolution_strategy seems like a bit too generic a name to me. Its basically a boolean... HAIP on/off.

@forkimenjeckayang

Copy link
Copy Markdown
Contributor Author

Keycloak now includes conformance tests, which already pass the oid4vci-1_0-issuer-happy-flow module with all variants (including HAIP). It sets up signing keys (in VciTestSigningKey) in a correct way for this to work: Root-Cert that is self-signed and leaf-cert that is signed by the root cert. The changes in this PR would make this fail (which CI doesnt show, because conformance tests are only executed in nightly build...), because issuer_key_resolution_strategy is not set there. So no x5c-header would be included at all.

Also issuer_key_resolution_strategy seems like a bit too generic a name to me. Its basically a boolean... HAIP on/off.

I agree the current gating on issuer_key_resolution_strategy is too strict for the conformance setup. If a signer already has a usable certificate chain, especially the conformance setup with a self-signed root and a leaf signed by that root, we should be able to include x5c without requiring an extra credential-scope attribute.

I’ll adjust the signer so x5c is added automatically when the selected signing key exposes a certificate chain with a non-self-signed leaf certificate. For keys without a certificate chain, or with only a self-signed signing certificate, plain SD-JWT issuance should continue without adding x5c unless HAIP behavior is explicitly required.

That should make the conformance HAIP variant work while avoiding failures for regular SD-JWT setups that only have the default self-signed generated key.

I also agree that issuer_key_resolution_strategy is too generic / heavy for what is effectively HAIP x5c on/off. I’ll either remove the need for it in this flow or rename/scope it more clearly if we still need an explicit switch.

Good ?

@tdiesler

Copy link
Copy Markdown
Contributor

IIRC, we fixed an x5c issue for the HAIP tests such that the chain was included in the proofs but the self signed root.

@mposolda mposolda left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@forkimenjeckayang @tdiesler @dominikschlosser I would like to check if we really need another config option or if we can just stick with always using HAIP way (with proper x5c chain)?

For security reasons, it can be always good to avoid using support of self-signed certificates for signing unless we really need to do so.

@forkimenjeckayang Is it any real use-case for support self-signed certificates? If not, you can perhaps just update your conformance setup as non-HAIP setup can also use the same way as HAIP (with proper certificate chain).

If we really need the new config option, it can be good to make it HAIP compliant by default and have the less secure option just if explicitly configured. This should also fix the issue pointed by @dominikschlosser per my understanding as there will not be a need to update anything in the conformance setup for HAIP in that case.

On the other hand: If we can avoid introducing new config option, it would be nice to include your changes in SdJwtCredentialSigner class. I found your code more readable (IMO the isSelfSigned is better name than isTrustAnchor, which IMO is a bit confusing) and explicitly throwing CredentialSignerException in case of issues is also good IMO.

@mposolda mposolda self-assigned this Jun 30, 2026
@forkimenjeckayang

Copy link
Copy Markdown
Contributor Author

@forkimenjeckayang @tdiesler @dominikschlosser I would like to check if we really need another config option or if we can just stick with always using HAIP way (with proper x5c chain)?

For security reasons, it can be always good to avoid using support of self-signed certificates for signing unless we really need to do so.

@forkimenjeckayang Is it any real use-case for support self-signed certificates? If not, you can perhaps just update your conformance setup as non-HAIP setup can also use the same way as HAIP (with proper certificate chain).

If we really need the new config option, it can be good to make it HAIP compliant by default and have the less secure option just if explicitly configured. This should also fix the issue pointed by @dominikschlosser per my understanding as there will not be a need to update anything in the conformance setup for HAIP in that case.

On the other hand: If we can avoid introducing new config option, it would be nice to include your changes in SdJwtCredentialSigner class. I found your code more readable (IMO the isSelfSigned is better name than isTrustAnchor, which IMO is a bit confusing) and explicitly throwing CredentialSignerException in case of issues is also good IMO.

I checked the conformance setup and agree that the new issuer_key_resolution_strategy gate is the problematic part. VciTestSigningKey already creates the expected certificate chain: a leaf signing certificate signed by a self-signed root certificate. So the signer should be able to emit x5c without requiring an additional credential-scope attribute.

Unless we identify a concrete use case for continuing to issue SD-JWT credentials with self-signed signing certificates, I’ll remove the new issuer_key_resolution_strategy option and make the SD-JWT signer use the HAIP/x5c path by default.

The intended behavior would be:

  • if the selected signing key has a proper certificate chain, include x5c;
  • remove the trailing self-signed root/trust anchor from the x5c header;
  • reject a self-signed signing certificate with CredentialSignerException;
  • keep the clearer isSelfSigned helper and explicit HAIP error handling.

If we later decide that self-signed signing certificates must remain supported for some dev/backward-compatibility scenario, I agree that should be an explicit less-secure opt-out rather than the default.

Signed-off-by: forkimenjeckayang <forkimenjeckayang@gmail.com>
Copilot AI review requested due to automatic review settings July 6, 2026 09:03
@forkimenjeckayang forkimenjeckayang changed the title [OID4VCI] enforce HAIP x5c trust-anchor rules and migrate signing/builder tests to new testsuite [OID4VCI] Enforce HAIP x5c rules for SD-JWT issuance Jul 6, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Signed-off-by: forkimenjeckayang <forkimenjeckayang@gmail.com>
@mposolda

Copy link
Copy Markdown
Contributor

@forkimenjeckayang Thanks for the updates! FYI. Created documentation issue as possible follow-up as AFAIK this change may affect documentation as well? #51177

@mposolda
mposolda merged commit 901b343 into keycloak:main Jul 27, 2026
91 checks passed
@forkimenjeckayang

Copy link
Copy Markdown
Contributor Author

@forkimenjeckayang Thanks for the updates! FYI. Created documentation issue as possible follow-up as AFAIK this change may affect documentation as well? #51177

Will take a look at this. Thansk for creating the issue @mposolda

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.

[OID4VCI] Enforce HAIP x5c rules for SD-JWT issuance

7 participants