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

JWT::Encode refactorings, alg and exp related bugfixes #293

Merged
merged 1 commit into from
Jan 21, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions lib/jwt.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# frozen_string_literal: true

require 'base64'
require 'jwt/base64'
require 'jwt/json'
require 'jwt/decode'
require 'jwt/default_options'
require 'jwt/encode'
Expand All @@ -17,12 +18,13 @@ module JWT
module_function

def encode(payload, key, algorithm = 'HS256', header_fields = {})
encoder = Encode.new payload, key, algorithm, header_fields
encoder.segments
Encode.new(payload: payload,
key: key,
algorithm: algorithm,
headers: header_fields).segments
end

def decode(jwt, key = nil, verify = true, options = {}, &keyfinder)
decoder = Decode.new(jwt, key, verify, DEFAULT_OPTIONS.merge(options), &keyfinder)
decoder.decode_segments
Decode.new(jwt, key, verify, DEFAULT_OPTIONS.merge(options), &keyfinder).decode_segments
end
end
19 changes: 19 additions & 0 deletions lib/jwt/base64.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# frozen_string_literal: true

require 'base64'

module JWT
# Base64 helpers
class Base64
class << self
def url_encode(str)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you really want to add two more methods to the public API of this gem that handle Base64 encoding? This kind of thing should probably be kept private.

::Base64.encode64(str).tr('+/', '-_').gsub(/[\n=]/, '')
end

def url_decode(str)
str += '=' * (4 - str.length.modulo(4))
::Base64.decode64(str.tr('-_', '+/'))
end
end
end
end
9 changes: 2 additions & 7 deletions lib/jwt/decode.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,6 @@
module JWT
# Decoding logic for JWT
class Decode
def self.base64url_decode(str)
str += '=' * (4 - str.length.modulo(4))
Base64.decode64(str.tr('-_', '+/'))
end

def initialize(jwt, key, verify, options, &keyfinder)
raise(JWT::DecodeError, 'Nil JSON web token') unless jwt
@jwt = jwt
Expand Down Expand Up @@ -80,7 +75,7 @@ def segment_length
end

def decode_crypto
@signature = Decode.base64url_decode(@segments[2])
@signature = JWT::Base64.url_decode(@segments[2])
end

def header
Expand All @@ -96,7 +91,7 @@ def signing_input
end

def parse_and_decode(segment)
JSON.parse(Decode.base64url_decode(segment))
JSON.parse(JWT::Base64.url_decode(segment))
rescue JSON::ParserError
raise JWT::DecodeError, 'Invalid segment encoding'
end
Expand Down
79 changes: 52 additions & 27 deletions lib/jwt/encode.rb
Original file line number Diff line number Diff line change
@@ -1,51 +1,76 @@
# frozen_string_literal: true

require 'json'

# JWT::Encode module
module JWT
# Encoding logic for JWT
class Encode
attr_reader :payload, :key, :algorithm, :header_fields, :segments
ALG_NONE = 'none'.freeze
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't nil be a better representation of none?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is something related to creating a JWT without a signature. none is the algorithm the JWA spec defines as "Unsecured JWS"

ALG_KEY = 'alg'.freeze
EXP_KEY = 'exp'.freeze
EXP_KEYS = [EXP_KEY, EXP_KEY.to_sym].freeze
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be better off refactoring to use a HashWithIndifferentAccess

Copy link
Member Author

Choose a reason for hiding this comment

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

Did not spend that much time focusing on this payload validation, think that it belongs somewhere else than the Encode class. I think #287 is a more elegant way to handle this.


def self.base64url_encode(str)
Base64.encode64(str).tr('+/', '-_').gsub(/[\n=]/, '')
def initialize(options)
@payload = options[:payload]
@key = options[:key]
@algorithm = options[:algorithm]
@headers = options[:headers]
end

def initialize(payload, key, algorithm, header_fields)
@payload = payload
@key = key
@algorithm = algorithm
@header_fields = header_fields
@segments = encode_segments
def segments
@segments ||= combine(encoded_header_and_payload, encoded_signature)
end

private

def validate_payload!
return unless @payload && @payload.is_a?(Hash)

validate_exp!
end

def validate_exp!
return if EXP_KEYS.all? { |key| [email protected]?(key) || @payload[key].is_a?(Integer) }

raise InvalidPayload, 'exp claim must be an integer'
end

def encoded_header
header = { 'alg' => @algorithm }.merge(@header_fields)
Encode.base64url_encode(JSON.generate(header))
@encoded_header ||= encode_header
end

def encoded_payload
raise InvalidPayload, 'exp claim must be an integer' if @payload && @payload.is_a?(Hash) && @payload.key?('exp') && !@payload['exp'].is_a?(Integer)
Encode.base64url_encode(JSON.generate(@payload))
@encoded_payload ||= encode_payload
end

def encoded_signature
@encoded_signature ||= encode_signature
end

def encoded_header_and_payload
@encoded_header_and_payload ||= combine(encoded_header, encoded_payload)
end

def encode_header
encode(@headers.merge(ALG_KEY => @algorithm))
end

def encode_payload
validate_payload!
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

def encoded_signature(signing_input)
if @algorithm == 'none'
''
else
signature = JWT::Signature.sign(@algorithm, signing_input, @key)
Encode.base64url_encode(signature)
end
def encode(data)
JWT::Base64.url_encode(JWT::JSON.generate(data))
end

def encode_segments
header = encoded_header
payload = encoded_payload
signature = encoded_signature([header, payload].join('.'))
[header, payload, signature].join('.')
def combine(*parts)
parts.join('.')
end
end
end
18 changes: 18 additions & 0 deletions lib/jwt/json.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# frozen_string_literal: true

require 'json'

module JWT
# JSON wrapper
class JSON
class << self
def generate(data)
::JSON.generate(data)
end

def parse(data)
::JSON.parse(data)
end
end
end
end
8 changes: 4 additions & 4 deletions lib/jwt/jwk/rsa.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,16 +27,16 @@ def kid
def export
{
kty: KTY,
n: Base64.urlsafe_encode64(public_key.n.to_s(BINARY), padding: false),
e: Base64.urlsafe_encode64(public_key.e.to_s(BINARY), padding: false),
n: ::Base64.urlsafe_encode64(public_key.n.to_s(BINARY), padding: false),
e: ::Base64.urlsafe_encode64(public_key.e.to_s(BINARY), padding: false),
kid: kid
}
end

def self.import(jwk_data)
imported_key = OpenSSL::PKey::RSA.new
imported_key.set_key(OpenSSL::BN.new(Base64.urlsafe_decode64(jwk_data[:n]), BINARY),
OpenSSL::BN.new(Base64.urlsafe_decode64(jwk_data[:e]), BINARY),
imported_key.set_key(OpenSSL::BN.new(::Base64.urlsafe_decode64(jwk_data[:n]), BINARY),
OpenSSL::BN.new(::Base64.urlsafe_decode64(jwk_data[:e]), BINARY),
nil)
self.new(imported_key)
end
Expand Down
44 changes: 32 additions & 12 deletions spec/jwt_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -57,21 +57,33 @@
expect(header['alg']).to eq alg
expect(jwt_payload).to eq payload
end
end

it 'should display a better error message if payload exp is_a?(Time)' do
payload['exp'] = Time.now
context 'payload validation' do
subject { JWT.encode(payload, nil, 'none') }
let(:payload) { { 'exp' => exp } }

expect do
JWT.encode payload, nil, alg
end.to raise_error JWT::InvalidPayload
context 'when exp is given as a non Integer' do
let(:exp) { Time.now.to_i.to_s }
it 'raises an JWT::InvalidPayload error' do
expect { subject }.to raise_error(JWT::InvalidPayload, 'exp claim must be an integer')
end
end

it 'should display a better error message if payload exp is not an Integer' do
payload['exp'] = Time.now.to_i.to_s
context 'when exp is given as an Integer' do
let(:exp) { 1234 }

expect do
JWT.encode payload, nil, alg
end.to raise_error JWT::InvalidPayload
it 'encodes the payload' do
expect(subject).to be_a(String)
end
end

context 'when the key for exp is a symbol' do
let(:payload) { { :exp => 'NotAInteger' } }

it 'raises an JWT::InvalidPayload error' do
expect { subject }.to raise_error(JWT::InvalidPayload, 'exp claim must be an integer')
end
end
end

Expand Down Expand Up @@ -228,7 +240,7 @@
translated_alg = alg.gsub('PS', 'sha')
valid_signature = data[:rsa_public].verify_pss(
translated_alg,
JWT::Decode.base64url_decode(signature),
JWT::Base64.url_decode(signature),
[header, body].join('.'),
salt_length: :auto,
mgf1_hash: translated_alg
Expand Down Expand Up @@ -339,7 +351,7 @@
context 'Base64' do
it 'urlsafe replace + / with - _' do
allow(Base64).to receive(:encode64) { 'string+with/non+url-safe/characters_' }
expect(JWT::Encode.base64url_encode('foo')).to eq('string-with_non-url-safe_characters_')
expect(JWT::Base64.url_encode('foo')).to eq('string-with_non-url-safe_characters_')
end
end

Expand All @@ -364,4 +376,12 @@
JWT.encode 'Hello World', 'secret'
end.not_to raise_error
end

context 'when the alg value is given as a header parameter' do

it 'does not override the actual algorithm used' do
headers = JSON.parse(::JWT::Base64.url_decode(JWT.encode('Hello World', 'secret', 'HS256', { alg: 'HS123'}).split('.').first))
expect(headers['alg']).to eq('HS256')
end
end
end