8000 [OID4VCI-HAIP] Pass oid4vci-1_0-issuer-fail-invalid-client-attestation-pop-signature (blocked) by tdiesler · Pull Request #47605 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

Repository navigation

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

Closed
tdiesler wants to merge 3 commits into
keycloak: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 into
keycloak:mainfrom
tdiesler:ghi47584

Conversation

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

@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
@IngridPuppet IngridPuppet 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.

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 8000 ...a/org/keycloak/authentication/authenticators/client/AttestationBasedClientAuthenticator.java Resolved
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);
@IngridPuppet IngridPuppet Apr 1, 2026
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.

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
@IngridPuppet IngridPuppet Apr 1, 2026
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.

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
@keycloak-github-bot keycloak-github-bot Bot left a comment
Copy link
Copy Markdown

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
keycloak-github-bot Bot commented Apr 10, 2026
Copy link
Copy Markdown

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 added 3 commits April 16, 2026 10:44
@tdiesler
Add support for OAuth 2.0 Attestation-based client authentication
0ffbdf8 
Signed-off-by: Thomas Diesler <tdiesler@proton.me>
@tdiesler
-- address review comments from @IngridPuppet
8e94e04 
Signed-off-by: Thomas Diesler <tdiesler@proton.me>
@tdiesler
[OID4VCI-HAIP] Pass oid4vci-1_0-issuer-fail-invalid-client-attestatio…
af23bb5 
…n-pop-signature

Signed-off-by: Thomas Diesler <tdiesler@proton.me>
@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
0