Make acceptable AAGUID ckeck in WebAuthn stricter by rmartinc · Pull Request #48404 · keycloak/keycloak

rmartinc · 2026-04-23T11:28:29Z

PR to make the AAGUID check in WebAuthn strict. Previously we were accepting some corner cases that allow to inject the AAGUID from an untrusted source. The PR also limits the AAGUID to not include the none attestation (which is also untrusted). Normally the none format anonymizes the AAGUID to the zero AAGUID, so it's totally useless. I added a note as notable change, but it's between notable and breaking, a proper configuration is OK, but maybe some weird configurations can fail (like allowing none with the zero AAGUID as accepted, it's useless, you are accepting any authenticator with no attestation). I can move the note to a breaking change if you think it's more a breaking change than a notable one. The note is for 26.6.2, I'll move it if not ready for the new version. Tests added but in the old TS, we need to disable the trust-store for keycloak and that is going to be complicated in the new TS. So I prefer to not do it for this PR.

keycloak-github-bot · 2026-04-23T12:27:14Z

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.forms.MultipleTabsLoginTest#testWithAuthSessionExpiredInTheMiddle_badRedirectUri

Keycloak CI - Forms IT (chrome)

org.opentest4j.AssertionFailedError: Expected ErrorPage but was localhost (https://localhost:8543/auth/realms/test/login-actions/authenticate?execution=0bec3531-f144-465f-97c8-6d8b0e3584a5&client_id=test-app&tab_id=LZ6NbdjwwOM&client_data=eyJydSI6Imh0dHBzOi8vbG9jYWxob3N0Ojg1NDMvYXV0aC9yZWFsbXMvbWFzdGVyL2FwcC9hdXRoIiwicnQiOiJjb2RlIn0) ==> expected: <true> but was: <false>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertTrue.failNotTrue(AssertTrue.java:63)
	at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:36)
...

Report flaky test

org.keycloak.testsuite.authz.PolicyEvaluationTest#

Keycloak CI - Base IT (3)

org.opentest4j.AssertionFailedError: expected: <PERMIT> but was: <null>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:182)
...

org.opentest4j.AssertionFailedError: expected: <PERMIT> but was: <null>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:182)
...

org.opentest4j.AssertionFailedError: expected: <PERMIT> but was: <null>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:182)
...

Report flaky test

keycloak-github-bot

Unreported flaky test detected, please review

rmartinc · 2026-04-23T17:30:31Z

@tnorimat I have added you as a reviewer. Your review would be awesome if you have time. If not, don't worry.

tnorimat · 2026-04-23T21:01:20Z

@rmartinc Hello, yes I will review the PR soon.

tnorimat

@rmartinc Hello, I added minor comments. Could you check them?
Also, it seems that you implemented the tests by arquillian integration test. Is that OK?
It seems that it might be better to implement the tests by new JUnit test.

rmartinc · 2026-04-24T07:11:09Z

Thanks @tnorimat! Added the two suggestions. Regarding the tests, yes, it's better to use the new TS. But some of the new tests need to disable the keycloak trustmanger (chrome virtual webauthn authneticator uses a self-signed certificate). In the old TS we do it using a method in the testing API, but that is complicated in the new TS. We have to add that to the new TS before, so I decided to not move this class to the new TS for this PR.

tnorimat · 2026-04-24T07:21:02Z

@rmartinc Thank you for the clarification why using old arquillian integration tests. Yes please keep it.

tnorimat

LGTM

mabartos

@rmartinc Thanks for the PR! But I think it's the responsibility of the admin to properly set it up the policies. AFAIK, there's no negotiation of the attestation conveyance on the authenticator level, but tied to the policies themselves.

Would it be possible to add some validation even on the admin console to deny this case? Because otherwise the admin is able to set the denied case for policies, but the user will face the error. I think it's better to have some more boundaries here. If we provide just console validation, the existing realms should not be affected at all - or potentially consider to validate it on the server level when accepting these policies.

WDYT?

rmartinc · 2026-04-24T12:11:31Z

@mabartos I modified the admin console to not allow none if acceptable AAGUIds are defined. Does this work for you?

rmartinc · 2026-04-24T13:44:22Z

Maybe this conflicts with #46960, so better wait the other to be merged and then move this one forward. Let's see if there is a conflict finally.

mabartos

@rmartinc Please try to rebase it as the issue #46960 is already closed.

I see the build failure after the rebase.

mabartos

@rmartinc LGTM, just a few nitpicks.

Closes keycloak#48388 Signed-off-by: rmartinc <rmartinc@redhat.com>

…one in admin console Closes keycloak#48388 Signed-off-by: rmartinc <rmartinc@redhat.com>

Closes keycloak#48388 Signed-off-by: rmartinc <rmartinc@redhat.com>

mabartos

Once CI passes, I'm ok with this. @rmartinc Thanks!

keycloak-github-bot · 2026-04-27T10:44:59Z

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.model.user.UserModelTest#testAddDirtyRemoveFederationUsersInTheSameGroupConcurrent

Keycloak CI - Store Model Tests

org.opentest4j.MultipleFailuresError: 
Multiple Failures (2 failures)
	java.lang.NullPointerException: <no message>
	org.infinispan.commons.IllegalLifecycleStateException: Cache container has been stopped and cannot be reused. Recreate the cache container.
	Suppressed: java.lang.NullPointerException
...

Report flaky test

keycloak-github-bot

Unreported flaky test detected, please review

rmartinc requested review from a team as code owners April 23, 2026 11:28

keycloak-github-bot Bot added team/ui team/cloud-native team/core-clients team/core-iam flaky-test labels Apr 23, 2026

keycloak-github-bot Bot reviewed Apr 23, 2026

View reviewed changes

mabartos self-requested a review April 23, 2026 13:24

rmartinc requested a review from tnorimat April 23, 2026 17:29

tnorimat reviewed Apr 23, 2026

View reviewed changes

Comment thread services/src/main/java/org/keycloak/authentication/requiredactions/WebAuthnRegister.java Outdated

Comment thread ...se/src/test/java/org/keycloak/testsuite/webauthn/registration/WebAuthnOtherSettingsTest.java Outdated

rmartinc force-pushed the issue-48388 branch from c1911c4 to 386b3f7 Compare April 24, 2026 07:07

tnorimat approved these changes Apr 24, 2026

View reviewed changes

mabartos reviewed Apr 24, 2026

View reviewed changes

rmartinc force-pushed the issue-48388 branch from 386b3f7 to 30daa79 Compare April 24, 2026 12:04

mabartos reviewed Apr 24, 2026

View reviewed changes

Comment thread js/apps/admin-ui/src/authentication/policies/WebauthnPolicy.tsx

keycloak-github-bot Bot mentioned this pull request Apr 24, 2026

Flaky test: org.keycloak.testsuite.authz.PolicyEvaluationTest# #48446

Closed

rmartinc force-pushed the issue-48388 branch from 30daa79 to 9d45a04 Compare April 24, 2026 13:38

keycloak-github-bot Bot mentioned this pull request Apr 24, 2026

Flaky test: org.keycloak.testsuite.model.session.UserSessionProviderOfflineModelTest#testOfflineSessionLazyLoading #47979

Closed

mabartos reviewed Apr 27, 2026

View reviewed changes

Comment thread js/apps/admin-ui/src/authentication/policies/WebauthnPolicy.tsx

mabartos reviewed Apr 27, 2026

View reviewed changes

Comment thread js/apps/admin-ui/maven-resources/theme/keycloak.v2/admin/messages/messages_en.properties Outdated

Comment thread docs/documentation/upgrading/topics/changes/changes-26_6_2.adoc Outdated

rmartinc added 3 commits April 27, 2026 10:53

Make acceptable AAGUID ckeck in WebAuthn stricter

98fafa2

Closes keycloak#48388 Signed-off-by: rmartinc <rmartinc@redhat.com>

Check Acceptable AAGUIDs sets a attestation preference different to N…

dc2a7fe

…one in admin console Closes keycloak#48388 Signed-off-by: rmartinc <rmartinc@redhat.com>

Changes for rebase and review.

7b47efd

Closes keycloak#48388 Signed-off-by: rmartinc <rmartinc@redhat.com>

rmartinc force-pushed the issue-48388 branch from 9d45a04 to 7b47efd Compare April 27, 2026 09:26

mabartos approved these changes Apr 27, 2026

View reviewed changes

This was referenced Apr 27, 2026

Flaky test: org.keycloak.testsuite.model.singleUseObject.SingleUseObjectModelTest#testCluster #48441

Closed

Flaky test: org.keycloak.testsuite.model.user.UserModelTest#testAddRemoveUserConcurrent #48442

Closed

keycloak-github-bot Bot reviewed Apr 27, 2026

View reviewed changes

pedroigor approved these changes Apr 27, 2026

View reviewed changes

pedroigor merged commit e03bc86 into keycloak:main Apr 27, 2026
86 checks passed

This was referenced Apr 27, 2026

Make acceptable AAGUID ckeck in WebAuthn stricter (26.6) #48513

Merged

Make acceptable AAGUID ckeck in WebAuthn stricter (26.4) #48517

Merged

Conversation

rmartinc commented Apr 23, 2026

Uh oh!

keycloak-github-bot Bot commented Apr 23, 2026

Unreported flaky test detected

org.keycloak.testsuite.forms.MultipleTabsLoginTest#testWithAuthSessionExpiredInTheMiddle_badRedirectUri

org.keycloak.testsuite.authz.PolicyEvaluationTest#

Uh oh!

keycloak-github-bot Bot left a comment

Choose a reason for hiding this comment

Uh oh!

rmartinc commented Apr 23, 2026

Uh oh!

tnorimat commented Apr 23, 2026

Uh oh!

tnorimat left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rmartinc commented Apr 24, 2026

Uh oh!

tnorimat commented Apr 24, 2026

Uh oh!

tnorimat left a comment

Choose a reason for hiding this comment

Uh oh!

mabartos left a comment

Choose a reason for hiding this comment

Uh oh!

rmartinc commented Apr 24, 2026

Uh oh!

Uh oh!

rmartinc commented Apr 24, 2026

Uh oh!

mabartos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mabartos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mabartos left a comment

Choose a reason for hiding this comment

Uh oh!

keycloak-github-bot Bot commented Apr 27, 2026

Unreported flaky test detected

org.keycloak.testsuite.model.user.UserModelTest#testAddDirtyRemoveFederationUsersInTheSameGroupConcurrent

Uh oh!

keycloak-github-bot Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants