Parse DPoP header in LogoutEndpoint to fix DPoP-bound refresh token logout#50528
Parse DPoP header in LogoutEndpoint to fix DPoP-bound refresh token logout#50528sergioperezcheco wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes RP-initiated (legacy) logout with DPoP-bound refresh tokens by ensuring LogoutEndpoint.logoutToken() parses and validates the DPoP request header and stores the proof in the session before TokenManager.verifyRefreshToken() enforces DPoP binding.
Changes:
- Add
DPoPUtil.handleDPoPHeader(...)toLogoutEndpoint.logoutToken()prior to refresh-token verification. - Import
DPoPUtilintoLogoutEndpoint.
| import org.keycloak.services.util.LocaleUtil; | ||
| import org.keycloak.services.util.DPoPUtil; | ||
| import org.keycloak.services.util.MtlsHoKTokenUtil; |
| // Parse and validate the DPoP proof from the request header (if present), | ||
| // so that DPoP-bound refresh tokens can be verified in verifyRefreshToken. | ||
| DPoPUtil.handleDPoPHeader(session, event, cors, OIDCAdvancedConfigWrapper.fromClientModel(client)); | ||
|
|
b9dd71d to
50883f5
Compare
50883f5 to
767e9b7
Compare
|
|
||
| // Parse and validate the DPoP proof from the request header (if present), | ||
| // so that DPoP-bound refresh tokens can be verified in verifyRefreshToken. | ||
| DPoPUtil.handleDPoPHeader(session, event, cors, OIDCAdvancedConfigWrapper.fromClientModel(client)); |
|
|
||
| // Parse and validate the DPoP proof from the request header (if present), | ||
| // so that DPoP-bound refresh tokens can be verified in verifyRefreshToken. | ||
| DPoPUtil.handleDPoPHeader(session, event, cors, OIDCAdvancedConfigWrapper.fromClientModel(client)); |
|
|
||
| // Parse and validate the DPoP proof from the request header (if present), | ||
| // so that DPoP-bound refresh tokens can be verified in verifyRefreshToken. | ||
| DPoPUtil.handleDPoPHeader(session, event, cors, OIDCAdvancedConfigWrapper.fromClientModel(client)); |
44f1832 to
12bb848
Compare
Passing client config forced DPoP proof on all logouts for DPoP-enabled clients, even when the refresh token is not DPoP-bound. Pass null and let verifyRefreshToken() enforce binding via the token's cnf claim, mirroring ParEndpoint. Addresses Copilot review feedback on keycloak#50528.
|
Addressed the main review concern in commit a7374b3: Regarding the integration-test request: the DPoP logout path is exercised by the existing DPoPTest refresh-then-revoke flows, but a dedicated legacy-logout-with-DPoP-bound-refresh-token case would be valuable. I can add one if maintainers confirm the desired test layout (it touches both DPoPTest and LogoutTest fixtures). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
services/src/main/java/org/keycloak/protocol/oidc/endpoints/LogoutEndpoint.java:515
- Please add a regression test that obtains a DPoP-bound refresh token and calls the refresh-token logout endpoint with a fresh proof, asserting logout succeeds; also cover a missing or mismatched proof. The existing DPoP integration suite exercises refresh-token proof validation but not this newly added logout path, so the reported regression could return unnoticed.
DPoPUtil.handleDPoPHeader(session, event, cors, null);
… logout LogoutEndpoint.logoutToken() calls verifyRefreshToken() which checks for a DPoP proof in the session, but LogoutEndpoint never parses the DPoP HTTP header unlike TokenEndpoint and ParEndpoint. This causes logout with DPoP-bound refresh tokens to always fail with 'DPoP proof is missing'. Add DPoPUtil.handleDPoPHeader() call before verifyRefreshToken() in logoutToken(), matching the pattern used by TokenEndpoint. Closes keycloak#50306 Signed-off-by: sergioperezcheco <checo520@outlook.com>
Copilot review noted the import broke alphabetical ordering. Signed-off-by: sergioperezcheco <checo520@outlook.com>
Passing client config forced DPoP proof on all logouts for DPoP-enabled clients, even when the refresh token is not DPoP-bound. Pass null and let verifyRefreshToken() enforce binding via the token's cnf claim, mirroring ParEndpoint. Addresses Copilot review feedback on keycloak#50528. Signed-off-by: sergioperezcheco <checo520@outlook.com>
a7374b3 to
aea27cc
Compare
|
Gentle ping for review — this parses the DPoP header in |
hey, so I found that once you bind a refresh token to DPoP, there's no way to log out with it — the logout endpoint always returns 400 with 'DPoP proof is missing'.
Turns out only TokenEndpoint and ParEndpoint bother to parse the DPoP HTTP header (via DPoPUtil.handleDPoPHeader). LogoutEndpoint calls verifyRefreshToken which checks the session for a DPoP proof, but nobody ever put one there because the header was never parsed in the first place.
The fix is just adding the same DPoPUtil.handleDPoPHeader() call before verifyRefreshToken() in logoutToken(), same pattern as what TokenEndpoint does at line 168. That way if a DPoP proof is present in the request, it gets validated and stored in the session where verifyRefreshToken can find it.
This is a regression from #36475 which made the DPoP check token-based (isDPoPToken) rather than client-setting-based — the check is now shared by the logout path which never had DPoP header parsing.
Closes #50306