From 91ce00de568dba64699721d05cd375eb84e4ab0e Mon Sep 17 00:00:00 2001 From: Rohit Behera <126186063+r0h1tb@users.noreply.github.com> Date: Sun, 2 Aug 2026 01:26:25 +0530 Subject: [PATCH] Omit blank key attestation values from OID4VCI metadata Closes #51347 Both key attestation setters silently discarded their argument: Optional.ofNullable(keyStorage) .map(list -> String.join(",")) .orElse(null); That is String.join(CharSequence, CharSequence...) with zero elements, so it returns "" and ignores list entirely. Configuring any resistance level wrote an empty attribute. This is the source of the reported metadata: the testsuite's key-attestation-credential scope is built with List.of(MODERATE), the value was dropped on write, and the getter then rendered the blank attribute as: "key_attestations_required": {"key_storage":[""],"user_authentication":[""]} The same defect is present in CredentialScopeRepresentation, so both classes are fixed to String.join(",", list). OID4VCI 12.2.4 requires key_storage and user_authentication to be non-empty arrays when present, and permits an empty key_attestations_required object when neither is constrained. The getters now filter blank entries and collapse to null when nothing remains, which is the contract their existing comment already describes. CredentialScopeRepresentation gains the same treatment so the two classes agree on what a blank attribute means. KeyAttestationsRequired is annotated @JsonInclude(NON_NULL), so null members are omitted and the bare "key_attestations_required": {} falls out without further change. Values are also trimmed, so " a , b " no longer yields entries with surrounding whitespace. Testing moves to the base testsuite per review. The unit test and its java.lang.reflect.Proxy stub are removed in favour of two methods on OID4VCIssuerWellKnownProviderTest, which exercise the real metadata endpoint: one asserting a configured resistance level reaches the metadata, and one walking the cases from the report. Asserting literal expected values matters here -- the existing coverage derives its expectation from the same getter under test, so it holds regardless of what that getter returns. Signed-off-by: Rohit Behera <126186063+r0h1tb@users.noreply.github.com> --- .../models/oid4vci/CredentialScopeModel.java | 16 ++- .../model/CredentialScopeRepresentation.java | 16 ++- .../OID4VCIssuerWellKnownProviderTest.java | 116 ++++++++++++++++++ 3 files changed, 140 insertions(+), 8 deletions(-) diff --git a/server-spi-private/src/main/java/org/keycloak/models/oid4vci/CredentialScopeModel.java b/server-spi-private/src/main/java/org/keycloak/models/oid4vci/CredentialScopeModel.java index ab45f954e8c5..be80001ae4bd 100644 --- a/server-spi-private/src/main/java/org/keycloak/models/oid4vci/CredentialScopeModel.java +++ b/server-spi-private/src/main/java/org/keycloak/models/oid4vci/CredentialScopeModel.java @@ -401,7 +401,11 @@ public void setKeyAttestationRequired(boolean keyAttestationRequired) { public List getRequiredKeyAttestationKeyStorage() { return Optional.ofNullable(clientScope.getAttribute(VC_KEY_ATTESTATION_REQUIRED_KEY_STORAGE)) - .map(s -> Arrays.asList(s.split(","))) + .map(s -> Arrays.stream(s.split(",")) + .map(String::trim) + .filter(value -> !value.isEmpty()) + .toList()) + .filter(values -> !values.isEmpty()) // it is important to return null here instead of an empty list: // If both key_storage and user_authentication parameters are absent, the // key_attestations_required parameter may be empty, indicating a key attestation is needed @@ -411,12 +415,16 @@ public List getRequiredKeyAttestationKeyStorage() { public void setRequiredKeyAttestationKeyStorage(List keyStorage) { clientScope.setAttribute(VC_KEY_ATTESTATION_REQUIRED_KEY_STORAGE, Optional.ofNullable(keyStorage) - .map(list -> String.join(",")).orElse(null)); + .map(list -> String.join(",", list)).orElse(null)); } public List getRequiredKeyAttestationUserAuthentication() { return Optional.ofNullable(clientScope.getAttribute(VC_KEY_ATTESTATION_REQUIRED_USER_AUTH)) - .map(s -> Arrays.asList(s.split(","))) + .map(s -> Arrays.stream(s.split(",")) + .map(String::trim) + .filter(value -> !value.isEmpty()) + .toList()) + .filter(values -> !values.isEmpty()) // it is important to return null here instead of an empty list: // If both key_storage and user_authentication parameters are absent, the // key_attestations_required parameter may be empty, indicating a key attestation is needed @@ -426,7 +434,7 @@ public List getRequiredKeyAttestationUserAuthentication() { public void setRequiredKeyAttestationUserAuthentication(List userAuthentication) { clientScope.setAttribute(VC_KEY_ATTESTATION_REQUIRED_USER_AUTH, Optional.ofNullable(userAuthentication) - .map(list -> String.join(",")).orElse(null)); + .map(list -> String.join(",", list)).orElse(null)); } @Override diff --git a/services/src/main/java/org/keycloak/protocol/oid4vc/model/CredentialScopeRepresentation.java b/services/src/main/java/org/keycloak/protocol/oid4vc/model/CredentialScopeRepresentation.java index 5c559f6e62ed..5da3c412e4bf 100644 --- a/services/src/main/java/org/keycloak/protocol/oid4vc/model/CredentialScopeRepresentation.java +++ b/services/src/main/java/org/keycloak/protocol/oid4vc/model/CredentialScopeRepresentation.java @@ -278,7 +278,11 @@ public CredentialScopeRepresentation setKeyAttestationRequired(boolean keyAttest public List getRequiredKeyAttestationKeyStorage() { return Optional.ofNullable(getAttribute(VC_KEY_ATTESTATION_REQUIRED_KEY_STORAGE)) - .map(s -> Arrays.asList(s.split(","))) + .map(s -> Arrays.stream(s.split(",")) + .map(String::trim) + .filter(value -> !value.isEmpty()) + .toList()) + .filter(values -> !values.isEmpty()) // it is important to return null here instead of an empty list: // If both key_storage and user_authentication parameters are absent, the // key_attestations_required parameter may be empty, indicating a key attestation is needed @@ -288,12 +292,16 @@ public List getRequiredKeyAttestationKeyStorage() { public CredentialScopeRepresentation setRequiredKeyAttestationKeyStorage(List keyStorage) { return setAttribute(VC_KEY_ATTESTATION_REQUIRED_KEY_STORAGE, Optional.ofNullable(keyStorage) - .map(list -> String.join(",")).orElse(null)); + .map(list -> String.join(",", list)).orElse(null)); } public List getRequiredKeyAttestationUserAuthentication() { return Optional.ofNullable(getAttribute(VC_KEY_ATTESTATION_REQUIRED_USER_AUTH)) - .map(s -> Arrays.asList(s.split(","))) + .map(s -> Arrays.stream(s.split(",")) + .map(String::trim) + .filter(value -> !value.isEmpty()) + .toList()) + .filter(values -> !values.isEmpty()) // it is important to return null here instead of an empty list: // If both key_storage and user_authentication parameters are absent, the // key_attestations_required parameter may be empty, indicating a key attestation is needed @@ -303,7 +311,7 @@ public List getRequiredKeyAttestationUserAuthentication() { public CredentialScopeRepresentation setRequiredKeyAttestationUserAuthentication(List userAuthentication) { return setAttribute(VC_KEY_ATTESTATION_REQUIRED_USER_AUTH, Optional.ofNullable(userAuthentication) - .map(list -> String.join(",")).orElse(null)); + .map(list -> String.join(",", list)).orElse(null)); } public T getCredentialPolicyValue(CredentialClientPolicy policy) { diff --git a/tests/base/src/test/java/org/keycloak/tests/oid4vc/OID4VCIssuerWellKnownProviderTest.java b/tests/base/src/test/java/org/keycloak/tests/oid4vc/OID4VCIssuerWellKnownProviderTest.java index 89ea881373d4..0ff5605f270b 100644 --- a/tests/base/src/test/java/org/keycloak/tests/oid4vc/OID4VCIssuerWellKnownProviderTest.java +++ b/tests/base/src/test/java/org/keycloak/tests/oid4vc/OID4VCIssuerWellKnownProviderTest.java @@ -30,6 +30,7 @@ import java.util.function.Function; import org.keycloak.VCFormat; +import org.keycloak.admin.client.resource.ClientScopeResource; import org.keycloak.admin.client.resource.ComponentsResource; import org.keycloak.common.util.Time; import org.keycloak.crypto.Algorithm; @@ -62,6 +63,7 @@ import org.keycloak.protocol.oid4vc.model.ProofTypesSupported; import org.keycloak.protocol.oid4vc.model.SupportedCredentialConfiguration; import org.keycloak.protocol.oid4vc.model.SupportedProofTypeData; +import org.keycloak.representations.idm.ClientScopeRepresentation; import org.keycloak.representations.idm.ProtocolMapperRepresentation; import org.keycloak.testframework.annotations.KeycloakIntegrationTest; import org.keycloak.testframework.annotations.TestSetup; @@ -80,6 +82,8 @@ import org.hamcrest.Matchers; import org.junit.jupiter.api.Test; +import static org.keycloak.OID4VCConstants.KeyAttestationResistanceLevels.HIGH; +import static org.keycloak.OID4VCConstants.KeyAttestationResistanceLevels.MODERATE; import static org.keycloak.OID4VCConstants.SIGNED_METADATA_JWT_TYPE; import static org.keycloak.VCFormat.JWT_VC; import static org.keycloak.VCFormat.SD_JWT_VC; @@ -569,6 +573,118 @@ public void testBatchCredentialIssuanceValidation() { testBatchSizeValidation("invalid", false, null); } + /** + * Configured key attestation resistance levels must reach the metadata. + * + *

The {@code key-attestation-credential} scope is set up with {@link org.keycloak.OID4VCConstants.KeyAttestationResistanceLevels#MODERATE} + * for both members, so the metadata has to advertise it. Asserting the literal value matters here: + * deriving the expectation from {@code CredentialScopeModel} would pass even when the configured + * values never made it into the client scope attribute in the first place. + */ + @Test + public void testKeyAttestationsRequiredAdvertisesConfiguredResistanceLevels() { + + KeyAttestationsRequired keyAttestationsRequired = getKeyAttestationsRequired( + keyAttestationCredentialScope.getCredentialConfigurationId(), ProofType.JWT); + + assertNotNull(keyAttestationsRequired, "key_attestations_required should be advertised"); + MatcherAssert.assertThat("Configured key_storage level should reach the metadata", + keyAttestationsRequired.getKeyStorage(), Matchers.contains(MODERATE)); + MatcherAssert.assertThat("Configured user_authentication level should reach the metadata", + keyAttestationsRequired.getUserAuthentication(), Matchers.contains(MODERATE)); + } + + /** + * Key attestation required, but neither resistance level configured. + * + *

Per OID4VCI 12.2.4 {@code key_storage} and {@code user_authentication} are non-empty arrays + * when present, and {@code key_attestations_required} may be empty when neither is constrained. + * The attribute is stored blank rather than absent in this case, which previously produced + * {@code {"key_storage":[""],"user_authentication":[""]}}. + */ + @Test + public void testKeyAttestationsRequiredOmitsUnconfiguredResistanceLevels() throws IOException { + + ClientScopeResource scopeResource = testRealm.admin().clientScopes() + .get(keyAttestationCredentialScope.getId()); + ClientScopeRepresentation original = scopeResource.toRepresentation(); + + try { + // neither constrained -> "key_attestations_required": {} + KeyAttestationsRequired neither = updateResistanceLevels(scopeResource, "", ""); + assertNotNull(neither, + "key_attestations_required should still be advertised when attestation is required"); + assertNull(neither.getKeyStorage(), + "key_storage must be omitted rather than advertised as an array of blanks"); + assertNull(neither.getUserAuthentication(), + "user_authentication must be omitted rather than advertised as an array of blanks"); + assertEquals("{}", JsonSerialization.valueAsString(neither), + "key_attestations_required should serialize to an empty object"); + + // only key_storage constrained + KeyAttestationsRequired keyStorageOnly = updateResistanceLevels(scopeResource, HIGH, ""); + MatcherAssert.assertThat(keyStorageOnly.getKeyStorage(), Matchers.contains(HIGH)); + assertNull(keyStorageOnly.getUserAuthentication(), + "user_authentication must be omitted when it is not constrained"); + + // only user_authentication constrained + KeyAttestationsRequired userAuthOnly = updateResistanceLevels(scopeResource, "", HIGH); + assertNull(userAuthOnly.getKeyStorage(), + "key_storage must be omitted when it is not constrained"); + MatcherAssert.assertThat(userAuthOnly.getUserAuthentication(), Matchers.contains(HIGH)); + + // separator-only and padded values collapse the same way + KeyAttestationsRequired separatorOnly = updateResistanceLevels(scopeResource, ",", " , "); + assertEquals("{}", JsonSerialization.valueAsString(separatorOnly), + "Separator-only values must not produce blank entries"); + + KeyAttestationsRequired padded = updateResistanceLevels(scopeResource, " " + HIGH + " ", ""); + MatcherAssert.assertThat("Surrounding whitespace should be trimmed", + padded.getKeyStorage(), Matchers.contains(HIGH)); + } finally { + scopeResource.update(original); + } + } + + /** + * Rewrites both resistance-level attributes on a credential scope and returns the + * {@code key_attestations_required} the metadata endpoint advertises afterwards. + */ + private KeyAttestationsRequired updateResistanceLevels(ClientScopeResource scopeResource, + String keyStorage, + String userAuthentication) { + ClientScopeRepresentation update = scopeResource.toRepresentation(); + update.getAttributes().put(CredentialScopeModel.VC_KEY_ATTESTATION_REQUIRED_KEY_STORAGE, keyStorage); + update.getAttributes().put(CredentialScopeModel.VC_KEY_ATTESTATION_REQUIRED_USER_AUTH, userAuthentication); + scopeResource.update(update); + + return getKeyAttestationsRequired( + keyAttestationCredentialScope.getCredentialConfigurationId(), ProofType.JWT); + } + + /** + * Reads {@code key_attestations_required} for one credential configuration straight off the + * metadata endpoint. + */ + private KeyAttestationsRequired getKeyAttestationsRequired(String credentialConfigurationId, String proofType) { + + CredentialIssuer credentialIssuer = oauth.oid4vc() + .doIssuerMetadataRequest() + .getMetadata(); + + SupportedCredentialConfiguration supportedConfig = credentialIssuer.getCredentialsSupported() + .get(credentialConfigurationId); + assertNotNull(supportedConfig, "Configuration '" + credentialConfigurationId + "' must be present"); + + ProofTypesSupported proofTypesSupported = supportedConfig.getProofTypesSupported(); + assertNotNull(proofTypesSupported, "proof_types_supported must be present"); + + SupportedProofTypeData proofTypeData = proofTypesSupported.getSupportedProofTypes().get(proofType); + assertNotNull(proofTypeData, proofType + " proof type must be present"); + + return proofTypeData.getKeyAttestationsRequired(); + } + @Test public void testOldOidcDiscoveryCompliantWellKnownUrlWithDeprecationHeaders() {