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.

Sign up for GitHub

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

Jump to bottom

Support application/jwt in userinfo endpoint #521

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Support application/jwt in userinfo endpoint #521

wants to merge 2 commits into from

Conversation

sergei-maertens
Copy link

Closes #517

This is an initial draft to spark discussion about implementation details.

Changes:

  • Refactored the JWS verification into a generic helper - the goal is that the middleware methods are backwards compatible
  • Added (partial) handling for application/jwt processing in the userinfo endpoint

Topics to discuss:

  • test failures due to missing Content-Type header -> can we use a utility like vcr.py to record real requests instead of using unittest.mock / request mocking approaches? My experience is that those tools are very fragile
  • I have a keycloak setup that can be added to the docker-compose which is configured for application/jwt - this is a powerful test setup with vcr.py from the above bullet
  • How do we handle keys that are not loaded from a JWKS endpoint? I think a new setting would be relevant, but is that even a case you want to support and/or is used in real deployments? Looking at the code again, I'm not sure that HMAC 256 is even configurable?
  • I've used type annotations because they help me write better code - some maintainers have strong opinions about these, just let me know your position about this and I'll adapt.
  • Is this the right direction? It works like a breeze for us in our downstream library, but I'm open to feedback of course
  • How do you feel about block-listing algorithm none?
  • I looked into re-using parse_www_authenticate_header for the content type header processing, but it kinda borks on a value like application/json; charset=utf-8, so instead I use the (private) utility from the requests library which can be controversial. I don't trust myself enough to correctly and safely parse HTTP headers 😬

@sergei-maertens
♻️ [mozilla#517] Extract JWS/JWK verification and decoding into gener…
a8cd537 
…ic utility

This allows us to re-use the verification functionality for
both the access token processing and userinfo response
data processing without duplicating the low-level
implementation.

Notable changes:

* the 'none' algorithm is blocked as this is a common
  exploit vector
* added some more documentation about the parameters
* added type hints to document expected parameter types
* did some slight refactoring/restructuring to make the
  code type-safe
@sergei-maertens
✨ [mozilla#517] Implement content-type negotation in userinfo endpoint
6bfe752 
TODO: address broken tests
TODO: add new tests
@sergei-maertens sergei-maertens mentioned this pull request Feb 14, 2024
Open
@ikarius
Copy link

ikarius commented Aug 6, 2024 •
edited
Loading

Seems legit for the handling of the application/jwt content-type.
We stumbled on this issue where JWT content was in binary/byte array form, and the userinfo response was impossible to decode (because not in standard JSON).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Authentication backend get_userinfo incorrectly assumes the endpoint responds with application/json
2 participants
@sergei-maertens @ikarius