[OID4VCI] Enforce HAIP x5c rules for SD-JWT issuance - #48528
Conversation
137ae93 to
303491b
Compare
51281ff to
2d10913
Compare
Unreported flaky test detectedIf 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 |
|
@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? |
ea91ffc to
d7af0ef
Compare
|
@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. |
|
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 Maybe it should be more flexible like the new |
Unreported flaky test detectedIf 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#multipleTabsLoginAndPassiveCheckKeycloak CI - Forms IT (chrome) org.keycloak.testsuite.forms.MultipleTabsLoginTest#multipleTabsParallelLoginTestWithAuthSessionExpiredInTheMiddleKeycloak CI - Forms IT (chrome) org.keycloak.testsuite.forms.MultipleTabsLoginTest#loginActionWithoutExecutionInRequiredActionsKeycloak CI - Forms IT (chrome) org.keycloak.testsuite.forms.MultipleTabsLoginTest#multipleTabsParallelLoginTestWithAuthSessionExpiredAndRefreshInTab1Keycloak CI - Forms IT (chrome) org.keycloak.testsuite.forms.MultipleTabsLoginTest#testLoginPageRefreshKeycloak CI - Forms IT (chrome) org.keycloak.testsuite.forms.MultipleTabsLoginTest#loginWithDifferentClientsKeycloak CI - Forms IT (chrome) org.keycloak.testsuite.forms.MultipleTabsLoginTest#multipleTabsParallelLoginTestWithAuthSessionExpiredAndRegisterClickKeycloak CI - Forms IT (chrome) org.keycloak.testsuite.forms.MultipleTabsLoginTest#multipleTabsParallelLoginTestKeycloak CI - Forms IT (chrome) org.keycloak.testsuite.forms.MultipleTabsLoginTest#expiredAuthenticationAction_expiredCodeCurrentExecutionKeycloak CI - Forms IT (chrome) org.keycloak.testsuite.forms.MultipleTabsLoginTest#testInjectRedirectUriInClientDataAfterAuthSessionExpirationKeycloak CI - Forms IT (chrome) |
… to new testsuite Signed-off-by: forkimenjeckayang <forkimenjeckayang@gmail.com>
d7e11d6 to
3281c60
Compare
There was a problem hiding this comment.
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
SdJwtCredentialSignerto emitx5conly for the HAIP strategy and enforce HAIP-6.1.1 constraints (no self-signed signing cert, no trust anchor inx5c). - 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. |
…d4vc into oid4vci-conformace-3
Signed-off-by: forkimenjeckayang <forkimenjeckayang@gmail.com>
|
Keycloak now includes conformance tests, which already pass the Also |
I agree the current gating on I’ll adjust the signer so 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 Good ? |
|
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
left a comment
There was a problem hiding this comment.
@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 Unless we identify a concrete use case for continuing to issue SD-JWT credentials with self-signed signing certificates, I’ll remove the new The intended behavior would be:
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>
Signed-off-by: forkimenjeckayang <forkimenjeckayang@gmail.com>
|
@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 |
closes #51176
This PR enforces HAIP-6.1.1 x5c handling for SD-JWT credential issuance.
Changes:
x5cheader for SD-JWT credentials using the selected signing key certificate chain.x5cheader.subject == issuer.vc.issuer_key_resolution_strategy/haip_x5cconfiguration option, since HAIP-style x5c handling is now the default.Conformance Issue Addressed
oid4vci-1_0-issuer-test-planclient_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_haipoid4vci-1_0-issuer-happy-flowHAIP-6.1.1 ValidateSdJwtCredentialX5cCertificateChainTrust anchor certificate must not be included in x5c chainThis PR resolves that conformance failure by ensuring the SD-JWT
x5cchain contains only end-entity/intermediate certificates and excludes the root trust anchor, as required by HAIP.Related to : #46265
See: adorsys#245 (comment)