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

Improve support for Unsecured JWT #352

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hesalx
Copy link

@hesalx hesalx commented Apr 28, 2020

This allows for symmetric support of Unsecured JWT. It now works as any other algorithm for both encoding and decoding.

Fixes #323.

https://tools.ietf.org/html/rfc7519#section-6 allows for Unsecured JWT using "alg" value of "none". It does not forbid further processing of Unsecured JWT and thus claim verification still applies.

https://tools.ietf.org/html/rfc7518#section-3.6 requires the signature to be an empty string.

https://tools.ietf.org/html/rfc7518#section-3.6 and https://tools.ietf.org/html/rfc7518#section-8.5 forbid accepting Unsecured JWT unless explicitly allowed (this has already been addressed).

While not explicitly addressed by any RFC, based on common sense this commit explicitly forbids the use of any signing key for Unsecured JWT. As Unsecured JWT requires an empty signature and cannot possibly make use of a key, it should be considered an error to use a key together with algorithm value of "none". For an Unsecured JWT the signing key should be set to either false or nil.

@sourcelevel-bot
Copy link

Hello, @hesalx! This is your first Pull Request that will be reviewed by SourceLevel, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information.

lib/jwt/algos/none.rb Outdated Show resolved Hide resolved
lib/jwt/algos/none.rb Outdated Show resolved Hide resolved
lib/jwt/algos/none.rb Outdated Show resolved Hide resolved
lib/jwt/algos/none.rb Outdated Show resolved Hide resolved
lib/jwt/algos/none.rb Outdated Show resolved Hide resolved
lib/jwt/algos/none.rb Outdated Show resolved Hide resolved
lib/jwt/algos/none.rb Outdated Show resolved Hide resolved
lib/jwt/algos/none.rb Outdated Show resolved Hide resolved
lib/jwt/algos/none.rb Outdated Show resolved Hide resolved
@@ -13,7 +13,7 @@ def initialize(jwt, key, verify, options, &keyfinder)
@jwt = jwt
@key = key
@options = options
@segments = jwt.split('.')
@segments = jwt.split('.', -1)
Copy link
Author

Choose a reason for hiding this comment

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

#split by default ignores empty strings. This change is needed to correctly identify an empty string signature in a JWT structured according to https://tools.ietf.org/html/rfc7519#section-7.2 and https://tools.ietf.org/html/rfc7516#section-9

@@ -66,7 +66,6 @@ def verify_claims

def validate_segment_count!
return if segment_length == 3
return if !@verify && segment_length == 2 # If no verifying required, the signature is not needed
Copy link
Author

@hesalx hesalx Apr 28, 2020

Choose a reason for hiding this comment

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

It seems that in no point any RFC allows for a 2-segment JWT.

Very likely this line was there only due to how #split works by default.

All relevant RFCs explicitly define JWT to have two periods (with the possibility of the last segment, the signature, to be an empty string).
See https://tools.ietf.org/html/rfc7515#section-3.1 and https://tools.ietf.org/html/rfc7516#section-9

The "at least one period ('.')" in https://tools.ietf.org/html/rfc7519#section-7.2 does not actually signify that there exists any valid JWT format with "exactly one period".

Copy link
Member

Choose a reason for hiding this comment

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

This would introduce a breaking change.

Experiences from past breaking changes showed that this kind of change creates a lot of chaos out there.

I recommend creating a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

On second thoughts. The whole PR will be a breaking change then. So no separate PR needed. Scheduled for 3.0.0 milestone.

Allows for symmetric support of Unsecured JWT. It now works as any other algorithm for both encoding and decoding.

Fixes jwt#323.
Comment on lines -361 to -366
context 'a token with two segments but does not require verifying' do
it 'raises something else than "Not enough or too many segments"' do
expect { JWT.decode('ThisIsNotAValidJWTToken.second', nil, false) }.to raise_error(JWT::DecodeError, 'Invalid segment encoding')
end
end

Copy link
Author

Choose a reason for hiding this comment

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

See the above comments regarding segments count.

There existed circumstances where 2-segment token would be processed without an exception ("alg": "none" along with verify=false) while such a format is not defined by any RFC and should be invalid.

If this is considered a breaking change we may handle it separately from this PR.

@glasses618

This comment has been minimized.

@excpt excpt self-requested a review July 6, 2020 23:57
@excpt excpt added this to the Version 3.0.0 milestone Jul 7, 2020
@anakinj
Copy link
Member

anakinj commented Jun 17, 2024

@hesalx Are you still active in the Ruby world? I'm thinking we could include the suggested improvements to the none algorithm in a 3.0 version of the gem.

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

Successfully merging this pull request may close these issues.

When using none alg (no signature), should the claims verification be ignored?
4 participants