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
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 21 additions & 0 deletions lib/jwt/algos/none.rb
@@ -0,0 +1,21 @@
module JWT
module Algos
module None
# Unsecured JWT
module_function

SUPPORTED = %w[none].freeze

def sign(to_sign)
raise EncodeError, 'Signing key not supported for Unsecured JWT' if to_sign.key
''
end

def verify(to_verify)
raise VerificationError, 'Signing key not supported for Unsecured JWT' if to_verify.public_key
raise VerificationError, 'Signature should be empty for Unsecured JWT' unless to_verify.signature == ''
true
end
end
end
end
3 changes: 1 addition & 2 deletions lib/jwt/decode.rb
Expand Up @@ -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

@verify = verify
@signature = ''
@keyfinder = keyfinder
Expand Down Expand Up @@ -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.


raise(JWT::DecodeError, 'Not enough or too many segments')
end
Expand Down
2 changes: 0 additions & 2 deletions lib/jwt/encode.rb
Expand Up @@ -52,8 +52,6 @@ def encode_payload
end

def encode_signature
return '' if @algorithm == ALG_NONE

JWT::Base64.url_encode(JWT::Signature.sign(@algorithm, encoded_header_and_payload, @key))
end

Expand Down
2 changes: 2 additions & 0 deletions lib/jwt/signature.rb
Expand Up @@ -7,6 +7,7 @@
require 'jwt/algos/ecdsa'
require 'jwt/algos/rsa'
require 'jwt/algos/ps'
require 'jwt/algos/none'
require 'jwt/algos/unsupported'
begin
require 'rbnacl'
Expand All @@ -25,6 +26,7 @@ module Signature
Algos::Rsa,
Algos::Eddsa,
Algos::Ps,
Algos::None,
Algos::Unsupported
].freeze
ToSign = Struct.new(:algorithm, :msg, :key)
Expand Down
32 changes: 26 additions & 6 deletions spec/jwt_spec.rb
Expand Up @@ -44,19 +44,45 @@

context 'alg: NONE' do
let(:alg) { 'none' }
let(:sig) { 'kWOVtIOpWcG7JnyJG0qOkTDbOy636XrrQhMm_8JrRQ8' }

it 'should generate a valid token' do
token = JWT.encode payload, nil, alg

expect(token).to eq data['NONE']
end

it 'with key should raise JWT::EncodeError' do
expect do
JWT.encode payload, data[:secret], alg
end.to raise_error JWT::EncodeError, 'Signing key not supported for Unsecured JWT'
end

it 'should decode a valid token' do
jwt_payload, header = JWT.decode data['NONE'], nil, false

expect(header['alg']).to eq alg
expect(jwt_payload).to eq payload
end

it 'should decode and verify a valid token' do
jwt_payload, header = JWT.decode data['NONE'], nil, true, algorithm: alg

expect(header['alg']).to eq alg
expect(jwt_payload).to eq payload
end

it 'with signature should raise JWT::VerificationError' do
expect do
JWT.decode data['NONE'] + sig, nil, true, algorithm: alg
end.to raise_error JWT::VerificationError, 'Signature should be empty for Unsecured JWT'
end

it 'with key should raise JWT::VerificationError' do
expect do
JWT.decode data['NONE'], data[:secret], true, algorithm: alg
end.to raise_error JWT::VerificationError, 'Signing key not supported for Unsecured JWT'
end
end

context 'payload validation' do
Expand Down Expand Up @@ -358,12 +384,6 @@
end
end

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

Comment on lines -361 to -366
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.

context 'Base64' do
it 'urlsafe replace + / with - _' do
allow(Base64).to receive(:encode64) { 'string+with/non+url-safe/characters_' }
Expand Down