8000 [OID4VCI-HAIP] Pass oid4vci-1_0-issuer-fail-replay-nonce by tdiesler · Pull Request #47479 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

[OID4VCI-HAIP] Pass oid4vci-1_0-issuer-fail-replay-nonce - #47479

Merged
mposolda merged 2 commits into
keycloak:mainfrom
tdiesler:ghi47469
Apr 9, 2026
Merged

[OID4VCI-HAIP] Pass oid4vci-1_0-issuer-fail-replay-nonce#47479
mposolda merged 2 commits into
keycloak:mainfrom
tdiesler:ghi47469

Conversation

@tdiesler
@tdiesler tdiesler commented Mar 26, 2026 •
edited
Loading
Copy link
Copy Markdown
Contributor

closes #47469

@tdiesler
tdiesler requested review from a team as code owners March 26, 2026 13:26
@tdiesler
tdiesler commented Mar 26, 2026 •
edited
Loading
Copy link
Copy Markdown
Contributor Author

@Awambeng could you perhaps take a look at this nonce replay protection. Also, I wonder whether the nonce's expiry is checked somewhere, is it?

You don't have to look at zillions of files from dependent PRs, just this snippet in AttestationProofValidator ...

            // Nonce replay protection
            //
            String nonce = attestationBody.getNonce();
            if (nonce != null) {
                SingleUseObjectProvider singleUseCache = keycloakSession.singleUseObjects();
                String hashString = Hex.encodeHexString(HashUtils.hash("SHA1", nonce.getBytes()));
                if (!singleUseCache.putIfAbsent(hashString, Time.currentTime() + 3600 * 24 * 3)) {
                    throw new VCIssuerException(INVALID_NONCE, "Nonce in proof has already been used");
                }
            }

@tdiesler
tdiesler force-pushed the ghi47469 branch 3 times, most recently from 76e8025 to 8b854b5 Compare March 27, 2026 06:21
@mposolda mposolda self-assigned this Mar 27, 2026
@Awambeng
Awambeng commented Mar 27, 2026 •
edited
Loading
Copy link
Copy Markdown
Contributor

@Awambeng could you perhaps take a look at this nonce replay protection. Also, I wonder whether the nonce's expiry is checked somewhere, is it?

Hi @tdiesler,
I've looked into the both the replay protection and the expiry handling and yes nonce's expiry is checked. The AttestationProofValidator delegates validation to the utility layer, where CNonceHandler.verifyCNonce is used. In the JwtCNonceHandler implementation, the exp claim of the nonce (as it is a signed JWT) is explicitly validated.

You don't have to look at zillions of files from dependent PRs, just this snippet in AttestationProofValidator ...

            // Nonce replay protection
            //
            String nonce = attestationBody.getNonce();
            if (nonce != null) {
                SingleUseObjectProvider singleUseCache = keycloakSession.singleUseObjects();
                String hashString = Hex.encodeHexString(HashUtils.hash("SHA1", nonce.getBytes()));
                if (!singleUseCache.putIfAbsent(hashString, Time.currentTime() + 3600 * 24 * 3)) {
                    throw new VCIssuerException(INVALID_NONCE, "Nonce in proof has already been used");
                }
            }

The replay protection logic here is solid. Using singleUseObjects() with a hashed nonce ensures replay prevention works correctly, since the nonce is stored and checked atomically via putIfAbsent.

One small observation: the TTL Time.currentTime() + 3600 * 24 * 3 (3 days) seems quite long for a nonce lifecycle. In most cases, nonces are expected to be short-lived (often seconds to a couple of minutes), so it might be worth revisiting this value (maybe aligning with c_nonce lifetime) or making it configurable depending on the intended security requirements.

Finally, I noticed that while we've implemented this stateful replay protection for attestation proofs, the standard
JwtProofValidator still relies on stateless JWT verification (checking the expiry claim). For complete consistency across the protocol, we might want to consider if similar stateful protection should be brought to JwtProofValidator as well.

@tdiesler
tdiesler force-pushed the ghi47469 branch 3 times, most recently from beda280 to a415075 Compare April 7, 2026 10:30
@tdiesler
tdiesler commented Apr 7, 2026 •
edited
Loading
Copy link
Copy Markdown
Contributor Author

One small observation: the TTL Time.currentTime() + 3600 * 24 * 3 (3 days) seems quite long

This is not the nonce TTL. It is the expiry of the cache entry (i.e. after three days the entry is deleted). How about we set it to 10 x C_NONCE_LIFETIME_IN_SECONDS?

tdiesler added 2 commits April 7, 2026 12:32
@tdiesler
[OID4VCI-HAIP] Pass oid4vci-1_0-issuer-fail-replay-nonce
7aafae9 
Signed-off-by: Thomas Diesler <tdiesler@proton.me>
@tdiesler
-- use 10 x C_NONCE_LIFETIME_IN_SECONDS
e3e7173 
Signed-off-by: Thomas Diesler <tdiesler@proton.me>
@Awambeng
Awambeng commented Apr 7, 2026
Copy link
Copy Markdown
Contributor

This is not the nonce TTL. It is the expiry of the cache entry (i.e. after three days the entry is deleted). How about we set it to 10 x C_NONCE_LIFETIME_IN_SECONDS?

Ahh got it, thanks for clarifying.
Yeah, that makes sense. I think we can go with that instead of the 3-day window, which keeps the cache entry around longer than needed.

Awambeng
Awambeng approved these changes Apr 9, 2026
View reviewed changes
@Awambeng Awambeng 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.

LGTM

mposolda
mposolda approved these changes Apr 9, 2026
View reviewed changes
@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.

@tdiesler @Awambeng Thanks for the PR and review!

@mposolda
mposolda merged commit 74a6109 into keycloak:main Apr 9, 2026
84 checks passed
@tdiesler
tdiesler deleted the ghi47469 branch April 9, 2026 17:19
@stianst stianst mentioned this pull request Jun 1, 2026
Closed
sashyo pushed a commit to tide-foundation/keycloak-IGA that referenced this pull request Jul 20, 2026
@tdiesler @sashyo
[OID4VCI-HAIP] Pass oid4vci-1_0-issuer-fail-replay-nonce (keycloak#47479
aea0809 
)

closes keycloak#47469


Signed-off-by: Thomas Diesler <tdiesler@proton.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mposolda mposolda mposolda approved these changes
+1 more reviewer
@Awambeng Awambeng Awambeng approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@mposolda mposolda

Labels

team/cloud-native team/core-clients team/core-iam

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[OID4VCI-HAIP] Pass oid4vci-1_0-issuer-fail-replay-nonce

3 participants

@tdiesler @Awambeng @mposolda
0