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

Submitting alg=none should not discuss class #5

Open
rwinch opened this issue Jun 21, 2018 · 4 comments
Open

Submitting alg=none should not discuss class #5

rwinch opened this issue Jun 21, 2018 · 4 comments
Labels
bug Something isn't working
Milestone

Comments

@rwinch
Copy link
Collaborator

rwinch commented Jun 21, 2018

Summary

Submitting an algorithm of none produces an error stating to "extend class to handle". The error message reveals too much developer information and is not well worded for a user. We should use an error message that states that an alg none is not supported. We should not discuss anything about extending a class.

> GET / HTTP/1.1
> Host: localhost:8081
> User-Agent: curl/7.47.0
> Accept: */*
> Authorization: Bearer ew0KICAiYWxnIjogIm5vbmUiLA0KICAidHlwIjogIkpXVCINCn0.ew0KICAic3ViIjogIjEyMzQ1Njc4OTAiLA0KICAibmFtZSI6ICJKb2huIERvZSIsDQogICJpYXQiOiAxNTE2MjM5MDIyDQp9.
> 
< HTTP/1.1 401 
< WWW-Authenticate: Bearer error="invalid_request", error_description="An error occurred while attempting to decode the Jwt: Unsecured (plain) JWTs are rejected, extend class to handle", error_uri="https://tools.ietf.org/html/rfc6750#section-3.1"
< X-Content-Type-Options: nosniff
< X-XSS-Protection: 1; mode=block
< Cache-Control: no-cache, no-store, max-age=0, must-revalidate
< Pragma: no-cache
< Expires: 0
< X-Frame-Options: DENY
< Content-Length: 0
< Date: Thu, 21 Jun 2018 17:27:21 GMT
<
@rwinch rwinch added the bug Something isn't working label Jun 21, 2018
@jgrandja
Copy link
Collaborator

For the given scenario where alg=none, the error code should be invalid_token instead of invalid_request.

As per spec:

invalid_token
The access token provided is expired, revoked, malformed, or
invalid for other reasons
. The resource SHOULD respond with
the HTTP 401 (Unauthorized) status code. The client MAY
request a new access token and retry the protected resource
request.

JwtAuthenticationProvider assumes invalid_request for all cases where JwtDecoder.decode() fails. This is not the case in all instances. The resulting JwtException should be evaluated to determine the appropriate error_code and error_description to set.

@jzheaux
Copy link
Owner

jzheaux commented Jun 25, 2018

Currently, Nimbus offers no effective way to evaluate the nature of the exception short of parsing the error message itself, which would likely prove brittle over time.

To that end, I've logged an issue with Nimbus to offer a couple of ideas on how their exceptions could be more distinguishable: https://bitbucket.org/connect2id/nimbus-jose-jwt/issues/264/badjwtexception-contains-no

In the meantime, @rwinch suggested the idea of extending the process(PlainJWT) method and throw our own exception. This will address this specific leak completely, though hopefully Nimbus can be enhanced to allow us to address other exception customization in the future.

@jzheaux
Copy link
Owner

jzheaux commented Jun 26, 2018

@jgrandja, good points - #3 is related to your point, too, I believe

@jgrandja
Copy link
Collaborator

@jzheaux Yes #3 is related

jzheaux added a commit that referenced this issue Jun 29, 2018
When Nimbus fails to parse either a JWK response or a JWT response,
the error message contains information that either should or cannot be
included in a Bearer Token response.

For example, if the response from a JWK endpoint is invalid JSON, then
Nimbus will send the entire response from the authentication server in
the resulting exception message.

This commit captures these exceptions and removes the parsing detail,
replacing it with more generic information about the nature of the
error.

Issue: gh-5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants