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

Openid connect jwt #488

Merged
merged 9 commits into from Jan 30, 2018

Conversation

Projects
None yet
6 participants
@wiliamsouza
Copy link
Member

commented Oct 3, 2017

This pull request adds support to receive JWT token in request in the following form:

curl -vv --header "Content-Type: application/json" \
         --header "Accept: application/json; indent=4" \
         --header "Authorization: Bearer eyJhbGciOiAiUlMyNTYifQ..." \
         http://127.0.0.1:8000/v1/users/
...
> GET /v1/users/ HTTP/1.1
> Host: 127.0.0.1:8000
> User-Agent: curl/7.53.1
> Content-Type: application/json
> Accept: application/json; indent=4
> Authorization: Bearer eyJhbGciOiAiUlMyNTYifQ....
> 
* HTTP 1.0, assume close after body
< HTTP/1.0 200 OK
< Date: Sun, 01 Oct 2017 19:53:52 GMT
< Server: WSGIServer/0.2 CPython/3.6.2
< Content-Type: application/json
< Vary: Accept
< Allow: GET, POST, HEAD, OPTIONS
< X-Frame-Options: SAMEORIGIN
< Content-Length: 159
< 
[
    {
        "url": "http://127.0.0.1:8000/v1/users/1/",
        "username": "wiliam",
        "email": "wiliam@olist.com",
        "is_staff": true
    }
* Closing connection 0
]

Don't find tests for tokens.py model. Any clue?

@wiliamsouza

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2017

@thedrow it would be great to have these changes in the next release.

@thedrow

This comment has been minimized.

Copy link
Collaborator

commented Oct 3, 2017

If there are no existing tests could you cover only what you added? If you have the time to cover all the module that would be great.

@skion

This comment has been minimized.

Copy link
Member

commented Oct 5, 2017

I wonder if it is better to unlink ID Tokens from (more general) JWT tokens here...

Some providers generate ID Tokens fit for one-time validation with an expiry that cover just the login process. And such providers may want to issue JWT-structured access tokens instead.

I'm guessing the validation semantics would be largely the same, but the naming might be confusing in this situation?

@wiliamsouza

This comment has been minimized.

Copy link
Member Author

commented Oct 5, 2017

The suggestion is rename JWTToken to IDToken?

@skion

This comment has been minimized.

Copy link
Member

commented Oct 5, 2017

No, the other way around...

@duaneking

This comment has been minimized.

Copy link
Member

commented Nov 13, 2017

Can we please get a update on the progress of this?

I can see this as helping the transition to supporting OIDC and it looks like a lot of work for adding non-openidconenct JWT support per #50 may be shared by this.

@thedrow

This comment has been minimized.

Copy link
Collaborator

commented Nov 14, 2017

@wiliamsouza ping?

@duaneking

This comment has been minimized.

Copy link
Member

commented Nov 15, 2017

To be clear: I think this will help to support RFC7519

@wiliamsouza

This comment has been minimized.

Copy link
Member Author

commented Nov 20, 2017

@thedrow Missing time to write the tests (If someone want help feel free).

I will get back to openid connect next month as a professional job priority.

@duaneking This is part of a effort to add oidc support to django-oauth-toolkit [1] I'm not sure where the code necessary to create JWT should live so for now it's all on django-oauth-toolkit side.

[1] jazzband/django-oauth-toolkit@master...wiliamsouza:openid-connect

Peter-Slump and others added some commits Dec 2, 2017

Merge pull request #1 from Peter-Slump/openid-connect-jwt
Added unittest for JWTToken model
@wiliamsouza

This comment has been minimized.

Copy link
Member Author

commented Dec 15, 2017

@thedrow tests done.
@skion I Change my mind ID Token is a JWT with a fields defined in oidc spec and when referring to OpenID connect this seems to be a better name.

Kudos to @Peter-Slump for the tests!

@thedrow

This comment has been minimized.

Copy link
Collaborator

commented Dec 16, 2017

I see that you added a method that raises a NotImplementedError. Is this PR a breaking change?

@@ -344,6 +344,31 @@ def get_id_token(self, token, token_handler, request):
# the request.scope should be used by the get_id_token() method to determine which claims to include in the resulting id_token
raise NotImplementedError('Subclasses must implement this method.')

def validate_id_token(self, token, scopes, request):

This comment has been minimized.

Copy link
@skion

skion Dec 17, 2017

Member

I still wouldn't tie the naming here to ID Tokens specifically, as I wouldn't expect all JWT-based access tokens to be ID Tokens. Some servers might hand out special purpose JWT access tokens, next to their ID Tokens.

In that light, could this be named validate_jwt_bearer_token instead and have a companion get_jwt_bearer_token to create it?

Then people could alias those back to get_id_token if they want, but also have the option to split them up.

This comment has been minimized.

Copy link
@wiliamsouza

wiliamsouza Dec 17, 2017

Author Member

This effort is to add OpenID connect support and for this use case the name is correct. Add JWT token support to OAuth is a different thing and can be handled in near future and at this point all methods that handle ID token and JWT token can converge but think it's a future refactoring not part of this PR.

This comment has been minimized.

Copy link
@skion

skion Dec 17, 2017

Member

Does OIDC specify somewhere how to use ID Tokens as JWT Bearer tokens?

To me it seems that you have in fact implemented generic (OAuth, not OIDC) JWT Bearer token support (as also the generic class name JWTToken would suggest), except for the token validator functions that have ID Token specific names.

Generalising this later would likely lead to an inconvenient upgrade path, since then JWTToken is already expected to call the OIDC-specific validators.

Unless I'm missing the point of the PR, I see no reason to keep this OIDC-specific detail in. Or is there?

This comment has been minimized.

Copy link
@wiliamsouza

wiliamsouza Dec 19, 2017

Author Member

request.expires_in = expires_in

return self.request_validator.get_id_token(None, None, request)

This comment has been minimized.

Copy link
@skion

skion Dec 17, 2017

Member

Similar to my other comment: What if someone wants to hand out JWT access tokens that are different from the ID Tokens they generate?

This comment has been minimized.

Copy link
@wiliamsouza

wiliamsouza Dec 19, 2017

Author Member

Will rename this call to get_jwt_bearer_token and let explicit that for OIDC this should call get_id_token

def validate_request(self, request):
token = None
if 'Authorization' in request.headers:
token = request.headers.get('Authorization')[7:]

This comment has been minimized.

Copy link
@skion

skion Dec 17, 2017

Member

Might be nice to align this with #491 already?

This comment has been minimized.

Copy link
@wiliamsouza

wiliamsouza Dec 17, 2017

Author Member

The last to be merged resolve the conflict?

This comment has been minimized.

Copy link
@MattBlack85

MattBlack85 Feb 6, 2018

Contributor

rebased and solved the conflict


def estimate_type(self, request):
token = request.headers.get('Authorization', '')[7:]
if token.count('.') in (2, 4):

This comment has been minimized.

Copy link
@skion

skion Dec 17, 2017

Member

Curious: When would there be 4 segment separators?

Also, there's no rule saying a normal access token can't contain dots of course. Wondering if you should include a check for ey or .ey here perhaps.

This comment has been minimized.

Copy link
@wiliamsouza

wiliamsouza Dec 17, 2017

Author Member

I really don't remember... Will try to generate every accepted ID tokens by the specification to see a way to improve this tests

This comment has been minimized.

Copy link
@skion

skion Dec 19, 2017

Member

Ah, JWE's! 👍

About the non-JWT character set: Yes, that's how oauthlib generates tokens, but another server might still include periods in them. And someone might use oauthlib as a client against a 3rd party server...

That said, I'm not sure what the best way is either, just that checking for .'s feels a bit shallow.

This comment has been minimized.

Copy link
@wiliamsouza

wiliamsouza Dec 19, 2017

Author Member

When used as client oauthlib validate tokens before send it to 3rd party server?

It's guaranteed ey is added to every JWT token?

Can we let this check and see if it is a problem?

This comment has been minimized.

Copy link
@skion

skion Dec 19, 2017

Member

When used as client oauthlib validate tokens before send it to 3rd party server?

Exactly

It's guaranteed ey is added to every JWT token?

I tried this yesterday and could not thing of a reason why a JWT would not start with {", but haven't thoroughly checked it either.

Can we let this check and see if it is a problem?

Just beware that you'll go wrong with the first example token in the RFC 😨

This comment has been minimized.

Copy link
@wiliamsouza

wiliamsouza Dec 20, 2017

Author Member

@skion I'm can't see other approach to solve this than validate the token at it early stage. What you think?

Some thing like:

>>> from jwcrypto import jwt
>>>jwt.JWT(jwt='token')
...
ValueError: Token format unrecognized

>>> jwt.JWT(jwt='t.oke.n')
...
InvalidJWSObject: Invalid JWS Object [Invalid format] {ValueError('Invalid base64 string',)}

>>> jwt.JWT(jwt='t.o.k.e.n')
...
InvalidJWEData: Invalid format {ValueError('Invalid base64 string',)}

>>> jwt.JWT(jwt='eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiYWRtaW4iOnRydWV9.TJVA95OrM7E2cBab30RMHrHDcEfxjoYZgeFONFh7HgQ')
<jwcrypto.jwt.JWT at 0x7f059df786d8>

This comment has been minimized.

Copy link
@skion

skion Dec 22, 2017

Member

Yeah, that crossed my mind as well, but the tricky bit with it would be that you'd need to exclude all exceptions that might be raised for valid but expired or otherwise bad tokens.

And I also liked your PR in the way that the JWT logic itself was contained to the request validator, leaving the developer free to use whatever JWT library they might prefer.

Perhaps the following is a reasonable trade off that doesn't pull in a JWT lib as a dependency?

if token.startswith('ey') and token.count('.') in (2, 4):

This comment has been minimized.

Copy link
@wiliamsouza

wiliamsouza Dec 22, 2017

Author Member

@skion Done! Thanks.

@wiliamsouza

This comment has been minimized.

Copy link
Member Author

commented Dec 17, 2017

@thedrow This method is only required for OpenID connect this mean when you use response_type=id_token implementations that use only OAuth will not broke.

Add get_jwt_bearer_token and validate_jwt_bearer_token oauthlib.oauth…
…2.RequestValidator and change oauthlib.oauth2.tokens JWTToken to use it
@wiliamsouza

This comment has been minimized.

Copy link
Member Author

commented Dec 19, 2017

@thedrow @skion PR updated

@wiliamsouza

This comment has been minimized.

Copy link
Member Author

commented Jan 8, 2018

@skion Need your ok here.

@duaneking

This comment has been minimized.

Copy link
Member

commented Jan 8, 2018

Thanks for the work on this @wiliamsouza !

In your mind how hard would it be to fully support RFC7519 (Per https://tools.ietf.org/html/rfc7519) after this change? I noticed your curl didnt have it, but thats a hard requirment for OIDC and that seems to be your goal here.

@skion

This comment has been minimized.

Copy link
Member

commented Jan 8, 2018

@wiliamsouza About @thedrow's question, I would think that existing code that happens to use pre_configured.Server() might indeed hit the NotImplemented in case a user sends a token that looks like a JWT? Or is this not the case.

Also, is by intention to add the JWT support only to the pre_configured.Server(), and not the other servers that support a resource endpoint?

@wiliamsouza

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2018

@duaneking What is the way to instruct to OAuth to use JWT? Add settings like OAUTHLIB_USE_JWT_TOKEN=True? If there is a issue for JWT support we can move that discussion to that isssue.

@wiliamsouza

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2018

@skion It will hit NotImplemented if token looks like JWT:
https://github.com/idan/oauthlib/pull/488/files#diff-f1e3c9d49e6a9992e075c962b46b6f38R344
The chance a token start with ey and have two to four point is very difficult.

My suggestion is to create new issues to add support in pre_configured.WebApplicationServer and pre_configured.MobileApplicationServer in a separated pull request.

@skion

This comment has been minimized.

Copy link
Member

commented Jan 10, 2018

The chance a token start with ey and have two to four point is very difficult.

Someone could intentionally send a JWT though, in which case the NotImplemented would always be hit and (likely) cause a 5xx rather a 401/3 response. Not ideal I guess, even if we mark this as a breaking change.

Would there be a way to make the PR backward compatible? Perhaps if get_jwt_bearer_token in the request validator returns None by default instead of raising an exception?

@wiliamsouza

This comment has been minimized.

Copy link
Member Author

commented Jan 10, 2018

@skion seems to work but the used method is validate_jwt_bearer_token it respond 401 Unauthorized:

curl -vv --header "Content-Type: application/json" --header "Accept: applicaton/json; indent=4" --header "Authorization: Bearer eyJhbGciOiAiUlMyNTYifQ.eyJpc3MiOiAiaHR0cHM6Ly9pZC5vbGlzdC5jb20iLCAic3ViIjogIjEiLCAiYXVkIjogIkt0bjBTaDRoTzJnQThQS0MyYXFzYXVZNFpDeE55SWRGMXdOZkZmSjMiLCAiZXhwIjogMTUxNTY0MjgzNSwgImlhdCI6IDE1MTU2MDY4MzUsICJhdXRoX3RpbWUiOiAxNTE1NDk3MzM3LCAibm9uY2UiOiAibi0wUzZfV3pBMk1qIiwgImF0X2hhc2giOiAiT0ROaE9HVmhNVGRtTW1RNU5qWTFaQT09In0.u0_tMWAxxKX3Ax9xy0ZwRW08vDwd2AIaIwFEo913d90x5DW59kStYTUu-Y2ri2Fq6qtrPovWugRC2Dyy7VZVgEdCbpSlPkbsg41lTdO2jG9HvoGI7j7eGEYwOtnrIdbVxn583Zdeol9IiUDvtCVG52MhyzfYFzA2CQ-XC6s-AEzzp9fwtYoToHNSXKzpqGTJwmLj3kf9bX9H4jgZ3sTF4m6Kh0NLiQo1GreKuavjfUwvGxXi5XHWNv1lSoFpdfFlN9LMUOnGvYzBwniusTf7PQh-S06j8UntRsfUyLDQtJY7c1an2-JP3YwnD0_K3UCDEzq9DCr-y37qWtLTwghvqw" --request GET http://127.0.0.1:8000/v1/users/
Note: Unnecessary use of -X or --request, GET is already inferred.
*   Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to 127.0.0.1 (127.0.0.1) port 8000 (#0)
> GET /v1/users/ HTTP/1.1
> Host: 127.0.0.1:8000
> User-Agent: curl/7.53.1
> Content-Type: application/json
> Accept: application/json; indent=4
> Authorization: Bearer eyJhbGciOiAiUlMyNTYifQ.eyJpc3MiOiAiaHR0cHM6Ly9pZC5vbGlzdC5jb20iLCAic3ViIjogIjEiLCAiYXVkIjogIkt0bjBTaDRoTzJnQThQS0MyYXFzYXVZNFpDeE55SWRGMXdOZkZmSjMiLCAiZXhwIjogMTUxNTY0MjgzNSwgImlhdCI6IDE1MTU2MDY4MzUsICJhdXRoX3RpbWUiOiAxNTE1NDk3MzM3LCAibm9uY2UiOiAibi0wUzZfV3pBMk1qIiwgImF0X2hhc2giOiAiT0ROaE9HVmhNVGRtTW1RNU5qWTFaQT09In0.u0_tMWAxxKX3Ax9xy0ZwRW08vDwd2AIaIwFEo913d90x5DW59kStYTUu-Y2ri2Fq6qtrPovWugRC2Dyy7VZVgEdCbpSlPkbsg41lTdO2jG9HvoGI7j7eGEYwOtnrIdbVxn583Zdeol9IiUDvtCVG52MhyzfYFzA2CQ-XC6s-AEzzp9fwtYoToHNSXKzpqGTJwmLj3kf9bX9H4jgZ3sTF4m6Kh0NLiQo1GreKuavjfUwvGxXi5XHWNv1lSoFpdfFlN9LMUOnGvYzBwniusTf7PQh-S06j8UntRsfUyLDQtJY7c1an2-JP3YwnD0_K3UCDEzq9DCr-y37qWtLTwghvqw
> 
* HTTP 1.0, assume close after body
< HTTP/1.0 401 Unauthorized
< Date: Wed, 10 Jan 2018 17:55:08 GMT
< Server: WSGIServer/0.2 CPython/3.6.2
< Content-Type: application/json
< WWW-Authenticate: Bearer realm="api"
< Vary: Accept
< Allow: GET, POST, HEAD, OPTIONS
< X-Frame-Options: SAMEORIGIN
< Content-Length: 65
< 
{
    "detail": "Authentication credentials were not provided."
* Closing connection 0

What is the best use default to return None or NotImplemented and document to use return None in validate_jwt_bearer_token if not using OpenID Connect support?

@duaneking

This comment has been minimized.

Copy link
Member

commented Jan 12, 2018

@wiliamsouza Right now the lib just seems to accept random data as the token. Looking at the code its clear to me there was some assumptions made that will need to be refactored to support other token formats. Your work is helping with that, but because you are doing it, it also means we/you have an opportunity to set the architectural tone for support.

My thinking is that the entire idea of a token format or style is something most to the system simply shouldn't care about; it should just be factored out generically so the system can be as agnostic about the token formats it consumes/validates as possible while still actually doing validation and enforcing a default for generation. In the best case, it should support multiple formats at the same time for consumption/validation, depending on whats enabled, so you can do a seamless update/upgrade of token formats and not lose currently logged in users on a redeploy that enables a new token field or something.

In my mind JWT via #50 adds the most benefit over the current "simple random string token" format created out of random strings, so simply any effort to enable it is a positive.

@wiliamsouza

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 2018

@thedrow Need your ok too.
@skion ping.

@wiliamsouza wiliamsouza referenced this pull request Jan 18, 2018

Open

[Work in progress] Openid Connect Core support #545

1 of 2 tasks complete
@wiliamsouza

This comment has been minimized.

Copy link
Member Author

commented Jan 29, 2018

@thedrow, @skion Ping! Don't let this PR die, please! With the OAuthLib new release we have the change o approval of this PR jazzband/django-oauth-toolkit#545 that adds support to django-oauth-toolkit

@thedrow

This comment has been minimized.

Copy link
Collaborator

commented Jan 30, 2018

As far as I'm concerned this PR LGTM.
But I think you should have a look at @duaneking's comments and see if there's something you'd rather change.

@skion

This comment has been minimized.

Copy link
Member

commented Jan 30, 2018

Same for me!

FWIW: I tested this branch in my environment with good result.

@wiliamsouza

This comment has been minimized.

Copy link
Member Author

commented Jan 30, 2018

@skion Thanks!
@thedrow Can you merge this! Please :) What we need to release 3.0.0 as a backward incompatible?
@duaneking Can you provide a PR after this get merged about your refactor ideas?

@thedrow

This comment has been minimized.

Copy link
Collaborator

commented Jan 30, 2018

@wiliamsouza Just the release notes. I'd appreciate if you'd release 3.0.0 yourself.

@thedrow thedrow merged commit 2fe1cdb into oauthlib:master Jan 30, 2018

@duaneking

This comment has been minimized.

Copy link
Member

commented Feb 4, 2018

@wiliamsouza I just really don't want args to be switched around; if we have backward compatibility needs then args should stay in the same positional index and new ones should have sane but config overridden enabled defaults.

@wiliamsouza wiliamsouza referenced this pull request Feb 6, 2018

Closed

Release 3.0.0 #513

@skion skion referenced this pull request Mar 9, 2018

Merged

Release 2.0.7 #519

@skion skion added this to the 3.0.0 milestone Mar 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.