diff --git a/services/src/main/java/org/keycloak/protocol/oid4vc/issuance/OID4VCIssuerEndpoint.java b/services/src/main/java/org/keycloak/protocol/oid4vc/issuance/OID4VCIssuerEndpoint.java index d8724ac58b6..ed6a83c9442 100644 --- a/services/src/main/java/org/keycloak/protocol/oid4vc/issuance/OID4VCIssuerEndpoint.java +++ b/services/src/main/java/org/keycloak/protocol/oid4vc/issuance/OID4VCIssuerEndpoint.java @@ -603,7 +603,7 @@ public class OID4VCIssuerEndpoint { CredentialOfferStorage offerStorage = session.getProvider(CredentialOfferStorage.class); CredentialOfferState offerState = offerStorage.findOfferStateByNonce(session, nonce); if (offerState == null) { - var errorMessage = "No credential offer state for nonce: " + nonce; + var errorMessage = "Credential offer not found or already consumed"; eventBuilder.detail(Details.REASON, errorMessage).error(Errors.INVALID_REQUEST); throw new BadRequestException(getErrorResponse(INVALID_CREDENTIAL_OFFER_REQUEST, errorMessage)); } @@ -620,14 +620,17 @@ public class OID4VCIssuerEndpoint { throw new BadRequestException(getErrorResponse(INVALID_CREDENTIAL_OFFER_REQUEST, errorMessage)); } - boolean isFirstConsumption = offerStorage.markAsConsumedIfNotConsumed(session, nonce); - if (!isFirstConsumption) { - var errorMessage = "Credential offer has already been consumed"; - LOGGER.debugf("Credential offer with nonce %s has already been consumed", nonce); + // Remove the nonce entry atomically for replay protection + // This prevents the same offer URL from being accessed multiple times + // while keeping pre-authorized code and credential identifier entries available + Map removed = session.singleUseObjects().remove(nonce); + if (removed == null) { + var errorMessage = "Credential offer not found or already consumed"; + LOGGER.debugf("Credential offer with nonce %s not found or already consumed", nonce); eventBuilder.detail(Details.REASON, errorMessage).error(Errors.INVALID_REQUEST); throw new BadRequestException(getErrorResponse(INVALID_CREDENTIAL_OFFER_REQUEST, errorMessage)); } - LOGGER.debugf("Marked credential offer with nonce %s as consumed", nonce); + LOGGER.debugf("Removed credential offer nonce %s for replay protection", nonce); // Add event details if (offerState.getClientId() != null) { @@ -794,6 +797,8 @@ public class OID4VCIssuerEndpoint { CredentialScopeModel requestedCredential; OID4VCAuthorizationDetailResponse authDetails; + CredentialOfferState offerState = null; + CredentialOfferStorage offerStorage = null; // When the CredentialRequest contains a credential identifier the caller must have gone through the // CredentialOffer process or otherwise have set up a valid CredentialOfferState @@ -802,8 +807,8 @@ public class OID4VCIssuerEndpoint { // First check if the credential identifier exists // This allows proper error reporting for unknown identifiers - CredentialOfferStorage offerStorage = session.getProvider(CredentialOfferStorage.class); - CredentialOfferState offerState = offerStorage.findOfferStateByCredentialId(session, credentialIdentifier); + offerStorage = session.getProvider(CredentialOfferStorage.class); + offerState = offerStorage.findOfferStateByCredentialId(session, credentialIdentifier); if (offerState == null) { var errorMessage = "No credential offer state for credential id: " + credentialIdentifier; eventBuilder.detail(Details.REASON, errorMessage).error(Errors.INVALID_REQUEST); @@ -939,6 +944,13 @@ public class OID4VCIssuerEndpoint { .detail(Details.VERIFIABLE_CREDENTIALS_ISSUED, String.valueOf(responseVO.getCredentials().size())); eventBuilder.success(); + // Clean up offer state after successful credential issuance + // This prevents memory leaks while ensuring the state remains available during the request + if (offerState != null) { + offerStorage.removeOfferState(session, offerState); + LOGGER.debugf("Removed credential offer state after successful issuance for credential identifier: %s", credentialIdentifier); + } + return response; } diff --git a/services/src/main/java/org/keycloak/protocol/oid4vc/issuance/credentialoffer/CredentialOfferStorage.java b/services/src/main/java/org/keycloak/protocol/oid4vc/issuance/credentialoffer/CredentialOfferStorage.java index 2cc3d7fe7da..142997456c7 100644 --- a/services/src/main/java/org/keycloak/protocol/oid4vc/issuance/credentialoffer/CredentialOfferStorage.java +++ b/services/src/main/java/org/keycloak/protocol/oid4vc/issuance/credentialoffer/CredentialOfferStorage.java @@ -42,10 +42,6 @@ public interface CredentialOfferStorage extends Provider { private String nonce; private int expiration; private OID4VCAuthorizationDetailResponse authorizationDetails; - /** - * Flag indicating whether this credential offer has been consumed (accessed via the credential offer URL). - */ - private boolean consumed; public CredentialOfferState(CredentialsOffer credOffer, String clientId, String userId, int expiration) { this.credentialsOffer = credOffer; @@ -53,7 +49,6 @@ public interface CredentialOfferStorage extends Provider { this.userId = userId; this.expiration = expiration; this.nonce = Base64Url.encode(RandomSecret.createRandomSecret(64)); - this.consumed = false; } // For json serialization @@ -120,14 +115,6 @@ public interface CredentialOfferStorage extends Provider { void setExpiration(int expiration) { this.expiration = expiration; } - - public boolean isConsumed() { - return consumed; - } - - public void setConsumed(boolean consumed) { - this.consumed = consumed; - } } void putOfferState(KeycloakSession session, CredentialOfferState entry); @@ -140,19 +127,6 @@ public interface CredentialOfferStorage extends Provider { void replaceOfferState(KeycloakSession session, CredentialOfferState entry); - /** - * Atomically marks a credential offer as consumed if it is not already consumed. - * This method provides thread-safe replay protection by ensuring only one thread - * can successfully mark an offer as consumed. - * - * @param session the Keycloak session - * @param nonce the nonce identifying the credential offer - * @return true if the offer was successfully marked as consumed (was not consumed before), - * false if the offer was already consumed (replay attempt) or does not exist. - * The caller should verify the offer exists before calling this method. - */ - boolean markAsConsumedIfNotConsumed(KeycloakSession session, String nonce); - void removeOfferState(KeycloakSession session, CredentialOfferState entry); @Override diff --git a/services/src/main/java/org/keycloak/protocol/oid4vc/issuance/credentialoffer/DefaultCredentialOfferStorage.java b/services/src/main/java/org/keycloak/protocol/oid4vc/issuance/credentialoffer/DefaultCredentialOfferStorage.java index 4ee4a26cc6f..837290270d7 100644 --- a/services/src/main/java/org/keycloak/protocol/oid4vc/issuance/credentialoffer/DefaultCredentialOfferStorage.java +++ b/services/src/main/java/org/keycloak/protocol/oid4vc/issuance/credentialoffer/DefaultCredentialOfferStorage.java @@ -122,25 +122,6 @@ class DefaultCredentialOfferStorage implements CredentialOfferStorage { }); } - @Override - public boolean markAsConsumedIfNotConsumed(KeycloakSession session, String nonce) { - CredentialOfferState currentState = findOfferStateByNonce(session, nonce); - if (currentState == null || currentState.isConsumed()) { - return false; - } - - currentState.setConsumed(true); - String consumedStateJson = JsonSerialization.valueAsString(currentState); - boolean replaced = session.singleUseObjects().replace(nonce, Map.of(ENTRY_KEY, consumedStateJson)); - - if (replaced) { - replaceOfferState(session, currentState); - return true; - } - - return false; - } - @Override public void removeOfferState(KeycloakSession session, CredentialOfferState entry) { session.singleUseObjects().remove(entry.getNonce()); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oid4vc/issuance/signing/OID4VCCredentialOfferCorsTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oid4vc/issuance/signing/OID4VCCredentialOfferCorsTest.java index 39e3c4187a5..fac1b24f10d 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oid4vc/issuance/signing/OID4VCCredentialOfferCorsTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oid4vc/issuance/signing/OID4VCCredentialOfferCorsTest.java @@ -382,8 +382,11 @@ public class OID4VCCredentialOfferCorsTest extends OID4VCIssuerEndpointTest { .user((String) null) .session((String) null) // Storage prunes expired single-use entries before lookup; lookup failure yields INVALID_REQUEST + // The error message indicates the offer was not found (pruned due to expiration) or already consumed .error(Errors.INVALID_REQUEST) - .detail(Details.REASON, Matchers.containsString("No credential offer state")) + .detail(Details.REASON, Matchers.anyOf( + Matchers.containsString("not found"), + Matchers.containsString("already consumed"))) .assertEvent(); } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oid4vc/issuance/signing/OID4VCJWTIssuerEndpointTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oid4vc/issuance/signing/OID4VCJWTIssuerEndpointTest.java index ce78d50c246..73d87b314d3 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oid4vc/issuance/signing/OID4VCJWTIssuerEndpointTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oid4vc/issuance/signing/OID4VCJWTIssuerEndpointTest.java @@ -1123,8 +1123,8 @@ public class OID4VCJWTIssuerEndpointTest extends OID4VCIssuerEndpointTest { assertEquals("Error type should be INVALID_CREDENTIAL_OFFER_REQUEST", ErrorType.INVALID_CREDENTIAL_OFFER_REQUEST.name(), response.getError()); - assertTrue("Error description should mention that offer has been consumed", - response.getErrorDescription().contains("already been consumed")); + assertTrue("Error description should mention that offer is not found or already consumed", + response.getErrorDescription().contains("not found") || response.getErrorDescription().contains("already consumed")); } /** @@ -1206,7 +1206,7 @@ public class OID4VCJWTIssuerEndpointTest extends OID4VCIssuerEndpointTest { } /** - * Test that consuming the credential offer (setting consumed=true) does not invalidate + * Test that removing the nonce entry (for replay protection) does not invalidate * the Pre-Authorized Code. This verifies that the replay protection mechanism doesn't * interfere with the normal token request flow using the pre-authorized code. */ @@ -1229,7 +1229,7 @@ public class OID4VCJWTIssuerEndpointTest extends OID4VCIssuerEndpointTest { String nonce = credentialOfferURI.getNonce(); assertNotNull("Nonce should not be null", nonce); - // 2. Fetch the Offer JSON (this triggers the consumed flag) + // 2. Fetch the Offer JSON (this removes the nonce entry for replay protection) CredentialsOffer credentialsOffer = oauth.oid4vc() .credentialOfferRequest(nonce) .bearerToken(token) @@ -1256,7 +1256,7 @@ public class OID4VCJWTIssuerEndpointTest extends OID4VCIssuerEndpointTest { AccessTokenResponse accessTokenResponse = oauth.oid4vc() .preAuthorizedCodeGrantRequest(preAuthorizedCode) .send(); - assertEquals("Token request should succeed even after offer is consumed", + assertEquals("Token request should succeed even after nonce is removed for replay protection", HttpStatus.SC_OK, accessTokenResponse.getStatusCode()); assertNotNull("Access token should be present", accessTokenResponse.getAccessToken());