Implement DPoP for all grantTypes #29967

Captain-P-Goldfish · 2024-05-29T10:31:32Z

Fix claiming userInfoToken with DPoP

tokenType must be DPoP as defined in RFC9449 chapter 7.1

closes #30181
closes #30179

relatesTo #22311

keycloak-github-bot

Unreported flaky test detected, please review

keycloak-github-bot · 2024-05-29T19:59:20Z

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.broker.KcOidcBrokerTest#testPostBrokerLoginFlowWithOTP_bruteForceEnabled

Keycloak CI - Java Distribution IT (windows-latest - temurin - 19)

java.lang.AssertionError: expected:<Invalid authenticator code.> but was:<null>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:120)
	at org.junit.Assert.assertEquals(Assert.java:146)
...

Report flaky test

mposolda · 2024-05-30T14:38:49Z

@tnorimat Could you please help me review this one if you have some free cycles?

keycloak-github-bot · 2024-05-30T14:51:21Z

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.broker.KcOidcBrokerTest#testPostBrokerLoginFlowWithOTP_bruteForceEnabled

Keycloak CI - Java Distribution IT (windows-latest - temurin - 19)

java.lang.AssertionError: expected:<Invalid authenticator code.> but was:<null>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:120)
	at org.junit.Assert.assertEquals(Assert.java:146)
...

Report flaky test

keycloak-github-bot

Unreported flaky test detected, please review

tnorimat · 2024-05-30T20:44:20Z

@mposolda Hello, Yes I will review the PR.

tnorimat

@Captain-P-Goldfish Hello, I add one minor review comment. Could you check it?

tnorimat · 2024-05-31T02:06:37Z

...integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/OAuthClient.java

        try (CloseableHttpClient client = HttpClientBuilder.create().build()) {
            HttpGet get = new HttpGet(getUserInfoUrl());
-            get.setHeader("Authorization", "Bearer " + accessToken);
+            get.setHeader("Authorization", accessTokenResponse.getTokenType() + " " + accessTokenResponse.getAccessToken());
            if (dpopProof != null) {
                get.addHeader("DPoP", dpopProof);


"DPoP" might be TokenUtil.TOKEN_TYPE_DPOP.

I am not sure where you are getting at?
Do you want the code like this?

get.setHeader("Authorization", TokenUtil.TOKEN_TYPE_DPOP + " " + accessTokenResponse.getAccessToken());

I don't think that this is a good idea. There is at least one case in which token_type Bearer is returned:

DPoP not required

DPoP not sent in AuthorizationRequest

You also added such a test and this test would fail then.

L1183 get.addHeader("DPoP", dpopProof);
to
get.addHeader(TokenUtil.TOKEN_TYPE_DPOP, dpopProof);
But, you need not to modify it.

Captain-P-Goldfish · 2024-05-31T05:21:18Z

It would also be good to restructure the implementation here.
Currently DPoP is not supported for other grantTypes than AuthorizationCode. We should think of a way to add it dynamically to all grantTypes even customized grantTypes. But this would be a different ticket :-)

EDIT:
I created a task for this
#30179

mposolda · 2024-06-05T19:03:30Z

It would also be good to restructure the implementation here. Currently DPoP is not supported for other grantTypes than AuthorizationCode. We should think of a way to add it dynamically to all grantTypes even customized grantTypes. But this would be a different ticket :-)

@Captain-P-Goldfish Could you please create separate GH issue for this topic you mentioned and add the comment here with the link to that issue? TBH I am not 100% sure from the top of my head if DPoP is supposed to be used in other grant types besides authorizationCode. But it might be good to investigate this anyway...

mposolda · 2024-06-05T19:11:08Z

@Captain-P-Goldfish Also it will be good to create dedicated GH issue for this issue itself with some more description. As every GH pull request (and commit) should have reference to the GH issue.

Captain-P-Goldfish · 2024-06-05T19:44:51Z

done.
I edited the comments above and linked the two issues:
Task: #30179
Bug #30181

Captain-P-Goldfish · 2024-06-18T07:18:18Z

unit tests are fixed except for the flaky tests from the main-branch :-)

mposolda · 2024-06-18T13:54:42Z

@Captain-P-Goldfish Thanks! Failing DockerClientTest seems like a regression of this PR?

Captain-P-Goldfish · 2024-06-19T06:09:26Z

that should fix it. I am having serious trouble getting the DockerClientTest running locally due to ClassNotFound exceptions and an outdated testcontainers version that does not get along with my docker-desktop version... Is there somewhere a documentation that explains how to get this test running reliably?

The DockerClientLogin implementation was trying to get the ProtocolMappers which I extended by the DPoP ProtocolMapper and expected that the mappers were having only a single entry. I extended the stream by a filter that makes sure that only the expected mapper is returned.

Captain-P-Goldfish · 2024-06-21T17:44:04Z

@mposolda are you going to review this or is someone else doing it?
If you would like to have any changes on this please let me know.

mposolda · 2024-07-16T10:00:31Z

@tnorimat Does this PR look ok to you?

tnorimat

@Captain-P-Goldfish I added some minor review comments. Could you check them?

services/src/main/java/org/keycloak/protocol/ProtocolMapperUtils.java

services/src/main/java/org/keycloak/protocol/oidc/grants/RefreshTokenGrantType.java

services/src/main/java/org/keycloak/protocol/oidc/grants/AuthorizationCodeGrantType.java

services/src/main/java/org/keycloak/protocol/oidc/endpoints/TokenEndpoint.java

services/src/main/java/org/keycloak/protocol/oidc/TokenManager.java

services/src/main/java/org/keycloak/services/managers/AppAuthManager.java

- tokenType must be DPoP as defined in RFC9449 chapter 7.1 relatesTo keycloak#22311 Signed-off-by: Pascal Knüppel <captain.p.goldfish@gmx.de>

fixes keycloak#30179 fixes keycloak#30181 Signed-off-by: Pascal Knüppel <pascal.knueppel@governikus.de>

Signed-off-by: Pascal Knüppel <pascal.knueppel@governikus.de>

Captain-P-Goldfish · 2024-07-18T06:46:43Z

@tnorimat why do you want the documentation to be removed? I added it for those who read the code after us. People not knowing what DPoP is can find the related RFC and reasons why the code was added in this way faster if the comments are present. What is your reason that you want them removed?

tnorimat · 2024-07-18T07:13:20Z

@Captain-P-Goldfish Hello, IMO, removing comments are Keycloak's convention. When I sent the PRs implementing several RFCs with comments, I was requested to remove the comments.
@mposolda WDYT? If you are OK, keeping comments seems to be preferable.

mposolda

@Captain-P-Goldfish Thanks! I've added small comment related to the refresh token constructor.

@Captain-P-Goldfish @tnorimat Regarding comments: I think comments in the code and also javadoc can be useful, but always depends on the context. It is not good if comments are mis-used and there is too much of them, but IMO comments are good if it makes the code more readable and adds the useful info why the particular code is used etc. So I think it is case by case.

AFAIK Keycloak does not have any strong convention on how much comments to use. Added the inline notes with some suggestions where comments can be useful IMO. For the other places mentioned by @tnorimat , I agree we can possibly remove comments.

What do you think?

mposolda · 2024-07-26T14:59:07Z

core/src/main/java/org/keycloak/representations/RefreshToken.java

     */
-    public RefreshToken(AccessToken token) {
+    public RefreshToken(AccessToken token, Confirmation confirmation) {


Is it possible if you keep also the constructor with single refresh token argument just for the backwards compatibility? So possibly maybe just something like this:

public RefreshToken(AccessToken token) { this(token, null); }

services/src/main/java/org/keycloak/protocol/oidc/TokenManager.java

services/src/main/java/org/keycloak/protocol/oidc/endpoints/TokenEndpoint.java

Signed-off-by: Pascal Knüppel <pascal.knueppel@governikus.de>

Captain-P-Goldfish · 2024-07-26T18:56:20Z

I have removed the comments that were not commented by @mposolda and re-added the constructor.

tnorimat

LGTM

tnorimat · 2024-07-29T00:27:53Z

@mposolda I approved the PR. Could you check it?

mposolda

@Captain-P-Goldfish @tnorimat Thanks a lot for the reviews and applying all the comments from the review!

Captain-P-Goldfish requested review from a team as code owners May 29, 2024 10:31

keycloak-github-bot bot added team/cloud-native team/core-clients team/core-iam labels May 29, 2024

Captain-P-Goldfish force-pushed the dpop branch from 18b3ea4 to bff2c73 Compare May 29, 2024 10:40

mposolda self-assigned this May 29, 2024

keycloak-github-bot bot added the flaky-test label May 29, 2024

keycloak-github-bot bot reviewed May 29, 2024

View reviewed changes

mposolda assigned tnorimat May 30, 2024

keycloak-github-bot bot reviewed May 30, 2024

View reviewed changes

tnorimat suggested changes May 31, 2024

View reviewed changes

Captain-P-Goldfish force-pushed the dpop branch from df8a930 to e73cec8 Compare May 31, 2024 05:18

Captain-P-Goldfish force-pushed the dpop branch from e73cec8 to 566ea9f Compare May 31, 2024 07:06

keycloak-github-bot bot mentioned this pull request May 31, 2024

Flaky test: org.keycloak.testsuite.oidc.AuthenticationMethodReferenceTest#testAmrPastMaxAge #26117

Closed

keycloak-github-bot bot mentioned this pull request Jun 17, 2024

Flaky test: org.keycloak.testsuite.broker.KcOidcBrokerTest#testPostBrokerLoginFlowWithOTP_bruteForceEnabled #30188

Closed

Captain-P-Goldfish force-pushed the dpop branch from 25dd3f5 to dc9ce52 Compare June 18, 2024 06:32

keycloak-github-bot bot mentioned this pull request Jun 18, 2024

Flaky test: org.keycloak.testsuite.oauth.TokenIntrospectionTest#testUnsupportedToken #30111

Closed

Captain-P-Goldfish force-pushed the dpop branch from dc9ce52 to a6be324 Compare June 19, 2024 06:02

This was referenced Jul 8, 2024

Add getter-methods to OAuth2GrantType.Context #31077

Merged

Blog post on DPoP keycloak/kc-sig-fapi#453

Open

Several changes based on OpenID4VCI keycloak/kc-sig-fapi#593

Open

mposolda requested a review from tnorimat July 16, 2024 10:00

tnorimat suggested changes Jul 18, 2024

View reviewed changes

Captain-P-Goldfish and others added 3 commits July 18, 2024 08:44

Fix claiming userInfoToken with DPoP

f2333f8

- tokenType must be DPoP as defined in RFC9449 chapter 7.1 relatesTo keycloak#22311 Signed-off-by: Pascal Knüppel <captain.p.goldfish@gmx.de>

Implement DPoP for all grant-types

1a7845c

fixes keycloak#30179 fixes keycloak#30181 Signed-off-by: Pascal Knüppel <pascal.knueppel@governikus.de>

Remove unused imports

01e267d

Signed-off-by: Pascal Knüppel <pascal.knueppel@governikus.de>

Captain-P-Goldfish force-pushed the dpop branch from ac10947 to 01e267d Compare July 18, 2024 06:44

VinodAnandan mentioned this pull request Jul 19, 2024

enable dpop token on client credential grant type #31357

Closed

mposolda reviewed Jul 26, 2024

View reviewed changes

Remove unwanted comments and re-add RefreshToken constructor

13e856c

Signed-off-by: Pascal Knüppel <pascal.knueppel@governikus.de>

tnorimat approved these changes Jul 27, 2024

View reviewed changes

mposolda approved these changes Jul 29, 2024

View reviewed changes

mposolda merged commit 9478418 into keycloak:main Jul 29, 2024
74 checks passed

Captain-P-Goldfish deleted the dpop branch August 1, 2024 12:00

Feelas mentioned this pull request Oct 10, 2024

[Regression] 26.0.0 return empty "access: []" JWT for Docker-v2 Auth provider, resulting in "access denied" #33776

Closed

2 tasks

Implement DPoP for all grantTypes #29967

Implement DPoP for all grantTypes #29967

Uh oh!

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Unreported flaky test detected

org.keycloak.testsuite.broker.KcOidcBrokerTest#testPostBrokerLoginFlowWithOTP_bruteForceEnabled

Uh oh!

Uh oh!

Unreported flaky test detected

org.keycloak.testsuite.broker.KcOidcBrokerTest#testPostBrokerLoginFlowWithOTP_bruteForceEnabled

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!