1 flaky tests on run #8179 ↗︎

0

527

48

0

1

Details:

  cypress/e2e/authentication_test.spec.ts • 1 flaky test • chrome

View Output Video

Test		Artifacts
Authentication test > should add a condition		Output Screenshots

_{This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.}

@tnorimat The mix-up attack being mitigated by this implementation is basically an additional barrier to Keycloak clients, right?

Most of the time, I see libraries integrating with Keycloak (including our adapters) validating the iss claim in both ID and our JWT Access tokens.

It looks more like a OAuth2-specific exploit, right? Is FAPI now forcing clients to use this parameter?

@pedroigor There is some interesting scenario described here https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics-19#section-4.4.1 .

@pedroigor @tnorimat I think that this is nice addition, but it will be likely still good to mention this in the "Upgrading guide" IMO. The OAuth2 mentions that client should ignore unknown parameters https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.2 , however I still have some fear that people will start reporting bugs due their client adapters screaming about "Unknown parameter: iss" ;-) So will be good to have this documented at least. But apart from that, I am not seeing any reason why the parameter should not be included by default and I hope we don't need any additional configuration for that.

@pedroigor Can you think of any issue with including iss in the authorization response by default?

@pedroigor @mposolda FAPI 2.0 requires this feature (#7 of FAPI 2.0 spec). The conformance test of FAPI 2.0 security profile checks this point, so we need this feature to pass the conformance test.

IMO, it is good to add iss parameter as default, and I agree with Marek's proposal for the documentation.
Of course, we add a settings of a client to determine whether iss parameter is added or not, but the number of settings increases.
It is difficult to realize this feature by Client policies because the current client policies cannot add iss parameter to a normal and error authorization response.

@pedroigor @mposolda Some tests failed because an application using keycloak oidc adapter cannot strip iss parameter when it create redirect_uri parameter of a token request. I stripped iss parameter by adapter codes of OAuthRequestAuthenticator and ServerRequest. However, it seems that they did not work. If possible, could you give me some advices?

@pedroigor @mposolda I do not know the reason why but erros happend by keycloak-adapter-core were resolved. I will investigate remaining errors in Base IT (5).

@tnorimat @mposolda I don't see any issue. I was just curious if that affects OpenID Connect clients (not pure OAuth2 clients).

The fact you have the issuer there (and we are using JWT), makes this attack not really possible if your OIDC client (e.g.: quarkus-oidc, Elytron OIDC, or even our legacy adapters) is checking the iss from the ID Token.

Adding this change to the release notes makes a lot of sense ...

@pedroigor I think attack may be still possible even for OIDC clients.

If I understand correctly the scenario in https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics-19#section-4.4.1, the idea is that client receives AuthorizationResponse from "Honest AS", but then it sends the token request to "Attacker AS" (including code and state etc), which means that attacker will be able to use the code to obtain the tokens from "Honest AS". Per my understanding, the iss parameter in AuthorizationResponse is a hint for client, which AS to use. So since client see iss for "Honest AS", it won't try to send TokenRequest to "attacker AS".

@tnorimat The PR looks good to me, Thanks for all the updates!

I think that test failures are not related to this PR, so should be fine.

I have last comment related to docs: Is it possible to move the changes to file changes-23_0_0.adoc instead of changes-22_0_0.adoc? As Keycloak 22 is already released and we unfortunately did not manage to have this in time for 22. Maybe you will need to create the file changes-23_0_0.adoc and add it to changes.adoc.

@andymunro Could you please review the documentation part of this PR? See my previous comment for the migration, so you can ignore this in your review.

@mposolda I added the file changes-23_0_0.adoc .

@tnorimat Thanks! We should be good for docs now, so the only missing piece might be to fix the UI test failure. Maybe rebase helps (if you did not already?). Another possibility is to do what @jonkoops suggested in the comment above?

@jonkoops @tnorimat Also I don't know if it is relevant, but when I browse through the files https://github.com/keycloak/keycloak/pull/20621/files, then going bottom to the file OpenIDConnectCompatibilityModes.tsx and I see some message like:

 Check failure on line 73 in js/apps/admin-ui/src/clients/advanced/OpenIdConnectCompatibilityModes.tsx
GitHub Actions / Admin UI

Insert `,`

Could it be related to UI test failure?

@mposolda @jonkoops I fixed the tsx file as Marek suggested.

@jonkoops @andymunro @tnorimat Thanks for the reviews and for all the work on this!

@mposolda Thank you. I will try to work on #21181 . Also, I will re-check whether the current keycloak can pass FAPI 2.0 Impl conformance test.