Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial implementation of a DPoP proofing mechanism #277

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

acoburn
Copy link
Contributor

@acoburn acoburn commented Jul 8, 2020

Resolves #165

This adds the initial structure for handling proof of possession semantics at the application layer.

This PR does not contain any tests, as I would like some initial feedback on this implementation. The basic design assumes that the AbstractDpopTokenValidator class would be extended and the following methods implemented:

  • ::getDpopHeaderValue -- this returns the raw HTTP DPoP header value
  • ::getRequestUri -- this returns the HTTP request URI
  • ::getRequestMethod -- this returns the HTTP request method

Then, given a @JsonWebToken object, the ::verify method checks that the thumbprint of any access token-bound key:

"cnf": {
    "jkt": "<thumbprint>"
}

matches the public key in the request-specific DPoP token (i.e. that the DPoP token is scoped to the request URI and method). This method will throw a ParseException in any of the following cases:

  • No key binding in the access token
  • No DPoP proofing token available
  • Any parse or validation failures of the DPoP token itself

If this seems like a reasonable approach, I will add accompanying tests.

Note: this does not implement jti constraints (which are optional according to DPoP and difficult to support across a cluster of stateless resource servers)

Also note: this makes use of the algorithm defined in authContextInfo.getKeyEncryptionAlgorithm().getAlgorithm(), though it isn't strictly necessary to align those (an identity provider may sign with RSA and a browser may use ECDSA for DPoP proofing -- ephemeral ECDSA keys for use with DPoP are considerably faster to generate in a browser)

DPoP Specification Reference: https://tools.ietf.org/html/draft-fett-oauth-dpop-04

Resolves smallrye#165

This adds the initial structure for handling proof of possession
semantics at the application layer.
@sberyozkin
Copy link
Contributor

@acoburn Thanks, let me add a few initial comments

final JwtConsumer parser = new JwtConsumerBuilder()
.setRequireJwtId()
.setExpectedType(true, DPOP_JWT_TYPE)
.setJwsAlgorithmConstraints(new AlgorithmConstraints(PERMIT,
Copy link
Contributor

@sberyozkin sberyozkin Jul 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This algorithm is used for decrypting the claims or inner signed tokens when dealing with the access token. I believe you meant getSignatureAlgorithm.
But you have a good point about ES256/etc - I propose that we have a new property added, smallrye.jwt.verify.dpop.signature-algorithm (and we doc it is for the DPoP proof JWT verification), its default value is the same as that of smallrye.jwt.verify.signature-algorithm which is RS256 but the users can choose ES256.
What do you think ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I did mean getSignatureAlgorithm. I like your suggestion for adding a smallrye.jwt.verify.dpop.signature-algorithm config property

@sberyozkin
Copy link
Contributor

sberyozkin commented Jul 9, 2020

Note: this does not implement jti constraints

If the IDP providers will start supporting the DPoP proof verification too then they'd deal with jti.
Perhaps we can also add a protected validateJti method which would return a no op Validatorand then in Quarkus/etc it would be possible to extend and return some custom jti validator...
We can also ship basic validator OOB which will keep a size-constraint Set of jti and keep optimizing it going forward..

Overall, a great start, thanks for spending the time on it. Let me also ping Keycloak colleagues :-)

@sberyozkin
Copy link
Contributor

Hey @acoburn How are you, by the way I've been thinking of having an implementation/jwt-dpop module with your PR eventually making it to that new module. I might give it a go after we release 3.0.0 final, cheers

@acoburn
Copy link
Contributor Author

acoburn commented Feb 8, 2021

@sberyozkin that sounds great. I know this PR has gone largely dormant, but I would be happy to help move it forward

Base automatically changed from master to main March 15, 2021 22:14
@sberyozkin sberyozkin added this to the 4.5.0 milestone Nov 24, 2023
@sberyozkin
Copy link
Contributor

Keycloak now supports DPoP as an experimental feature so it is a good time to complete this PR, I'll try to have a look

@VinodAnandan
Copy link

Hey @acoburn @sberyozkin ,

I hope you are well. I was wondering if there have been any updates regarding the PR? Additionally, are there any significant hurdles or blockers that need to be addressed? Your insights would be greatly appreciated.

I believe this is the correct link to the RFC : https://datatracker.ietf.org/doc/html/rfc9449
Reference implementation in JavaScript/node - https://github.com/search?q=repo%3Apanva%2Fnode-openid-client+dpop&type=commits

@sberyozkin sberyozkin removed this from the 4.5.0 milestone Mar 20, 2024
@sberyozkin
Copy link
Contributor

@VinodAnandan Hi, preparation for testing it is in #772, but I won't have time to fix it in the very short term in smallrye-jwt. I'll play with it at the quarkus-oidc level and test against Keycloak and once it is done, this PR will be complete, though of course if Aaron @acoburn decides to finalize it sooner, it would be awesome

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add DPoP proofing mechanism
3 participants