Skip to content

[OID4VCI-HAIP] Pass oid4vci-1_0-issuer-fail-invalid-client-attestation-pop-signature (blocked)#47605

Closed
tdiesler wants to merge 3 commits intokeycloak:mainfrom
tdiesler:ghi47584
Closed

[OID4VCI-HAIP] Pass oid4vci-1_0-issuer-fail-invalid-client-attestation-pop-signature (blocked)#47605
tdiesler wants to merge 3 commits intokeycloak:mainfrom
tdiesler:ghi47584

Conversation

@tdiesler
Copy link
Copy Markdown
Contributor

@tdiesler tdiesler commented Mar 30, 2026 •
edited
Loading

@tdiesler tdiesler requested review from a team as code owners March 30, 2026 15:57
@tdiesler tdiesler force-pushed the ghi47584 branch 2 times, most recently from b240f0f to 9ae9f67 Compare March 31, 2026 03:47
@tdiesler tdiesler mentioned this pull request Mar 31, 2026
Closed
@tdiesler tdiesler force-pushed the ghi47584 branch 3 times, most recently from 75c7842 to bca0904 Compare April 1, 2026 14:29
IngridPuppet
IngridPuppet reviewed Apr 1, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@IngridPuppet IngridPuppet left a comment

Choose a reason for hiding this comment

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

Hello @tdiesler - I reviewed the last commit of the PR and a few comments. Not sure if it is the only relevant commit of the PR.

Comment thread ...a/org/keycloak/authentication/authenticators/client/AttestationBasedClientAuthenticator.java Outdated
Comment thread core/src/main/java/org/keycloak/jose/jws/crypto/ECDSAProvider.java
Comment on lines +69 to +74
try {
Signature verifier = getSignature(input.getHeader().getAlgorithm());
verifier.initVerify(publicKey);
verifier.update(input.getEncodedSignatureInput().getBytes(StandardCharsets.UTF_8));
byte[] derSignature = transcodeSignatureToDER(input.getSignature());
return verifier.verify(derSignature);
Copy link
Copy Markdown
Contributor

@IngridPuppet IngridPuppet Apr 1, 2026

Choose a reason for hiding this comment

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

This code is very critical. We never know what slight error may be overlooked in the implementation of transcodeSignatureToDER below. I think it is preferable you reuse the ECDSASignatureVerifierContext class or at least the methods it reuses.

keycloak/core/src/main/java/org/keycloak/crypto/ECDSASignatureVerifierContext.java

Lines 27 to 32 in d7238a7

public boolean verify(byte[] data, byte[] signature) throws VerificationException {
try {
int expectedSize = ECDSAAlgorithm.getSignatureLength(getAlgorithm());
byte[] derSignature = ECDSAAlgorithm.concatenatedRSToASN1DER(signature, expectedSize);
return super.verify(data, derSignature);
} catch (Exception e) {

Comment thread ...a/org/keycloak/authentication/authenticators/client/AttestationBasedClientAuthenticator.java
Copy link
Copy Markdown
Contributor

@IngridPuppet IngridPuppet Apr 1, 2026

Choose a reason for hiding this comment

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

I also fail to notice tests for the added logic. Is it done in another PR? I think it is really important to have some, especially with the introduction of ECDSAProvider.

@tdiesler tdiesler changed the title [OID4VCI-HAIP] Pass oid4vci-1_0-issuer-fail-invalid-client-attestation-pop-signature [OID4VCI-HAIP] Pass oid4vci-1_0-issuer-fail-invalid-client-attestation-pop-signature (blocked) Apr 8, 2026
@tdiesler tdiesler force-pushed the ghi47584 branch 4 times, most recently from 75bcb79 to 51bbe31 Compare April 10, 2026 06:10
keycloak-github-bot[bot]
keycloak-github-bot Bot reviewed Apr 10, 2026
View reviewed changes
Copy link
Copy Markdown

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

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

@keycloak-github-bot
Copy link
Copy Markdown

keycloak-github-bot Bot commented Apr 10, 2026

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.cluster.UserFederationInvalidationClusterTest#crudWithFailover

Keycloak CI - Clustering IT

java.lang.AssertionError
	at org.junit.Assert.fail(Assert.java:87)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.junit.Assert.assertFalse(Assert.java:65)
	at org.junit.Assert.assertFalse(Assert.java:75)
...

Report flaky test

@tdiesler tdiesler closed this Apr 16, 2026
@tdiesler tdiesler deleted the ghi47584 branch April 16, 2026 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@IngridPuppet IngridPuppet IngridPuppet left review comments

@keycloak-github-bot keycloak-github-bot[bot] keycloak-github-bot[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

flaky-test 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-invalid-client-attestation-pop-signature

2 participants

@tdiesler @IngridPuppet