nov / json-jwt

JSON Web Token and its family (JSON Web Signature, JSON Web Encryption and JSON Web Key) in Ruby
MIT License
299 stars 81 forks source link

verify throwing an error in JSON::JWT.decode #32

Closed onli closed 7 years ago

onli commented 8 years ago

In https://github.com/onli/sinatra-browserid/blob/88ae06ebf8248a277759f9917eb65249cf6711c2/lib/sinatra/browserid.rb#L35 I'm calling decode on string which is generated in https://github.com/callahad/authbackend/blob/e14f16899fc7a3dadf84651b897eac2eccc1d74d/server.rb#L381, by JWT.encode. If not skipping verification, this is leading to an error:

NoMethodError - undefined method `verify' for #<String:0x00000003e86478>:
    /home/onli/Dropbox-decrypted/ursprung.git/vendor/bundle/ruby/2.2.0/gems/json-jwt-1.5.2/lib/json/jws.rb:94:in `valid?'
    /home/onli/Dropbox-decrypted/ursprung.git/vendor/bundle/ruby/2.2.0/gems/json-jwt-1.5.2/lib/json/jws.rb:25:in `verify!'
    /home/onli/Dropbox-decrypted/ursprung.git/vendor/bundle/ruby/2.2.0/gems/json-jwt-1.5.2/lib/json/jws.rb:162:in `decode_compact_serialized'
    /home/onli/Dropbox-decrypted/ursprung.git/vendor/bundle/ruby/2.2.0/gems/json-jwt-1.5.2/lib/json/jwt.rb:77:in `decode_compact_serialized'
    /home/onli/Dropbox-decrypted/ursprung.git/vendor/bundle/ruby/2.2.0/gems/json-jwt-1.5.2/lib/json/jose.rb:39:in `decode'

I'm assuming I have to wrap the jwt-token-string in something. But this is not covered by the docu in https://github.com/nov/json-jwt/wiki/JWS#verifying, where that function is called with a normal string. Thus I assume this is a bug in docu or implementation.

milankubin commented 8 years ago

Thank you for putting this doc here, I was having problems in Rails with this, and couldn't spin my head around. I would suggest to check the algorithm you are using, and should be HMAC for strings! The document you provided actually states it, and ended up fixing it for me:

`Supported key representations are

String (for shared key) OpenSSL::PKey::RSA OpenSSL::PKey::EC JSON::JWK JSON::JWK::Set skip_verification # NOTE: magic word for skipping signature verification For HMAC keys, String, JSON::JWK and JSON::JWK::Set instances are available.

For RSA/ECDSA keys, OpenSSL::PKey::RSA, OpenSSL::PKey::EC, JSON::JWK and JSON::JWK::Set > instances are available.`

onli commented 8 years ago

I'm not sure I follow. Could you point out more directly what the solution would be? Instead of JSON::JWT.decode params[:id_token], public_key for decoding the encoded string, I would use a different call? Or would the server have to change something?

nov commented 8 years ago

I assume your public_key on JSON::JWT.decode params[:id_token], public_key call is a String, and your JWT header includes alg=RS256.

In that case, you need to put a OpenSSL::PKey::RSA or JSON::JWK as the public_key.

like JSON::JWT.decode params[:id_token], OpenSSL::PKey::RSA.new(public_key)

onli commented 8 years ago

Hi @nov, thanks for your answer!

My JWT header indeed includes alg=RS256 I'm actually not sure whether the header is properly set. But public_key is not a string, it is already a JSON::JWK. params[:id_token] is a String, but this is the whole idea of the decode function, isn't it? This is the relevant code on my side:

public_key_jwks = URI.parse(URI.escape(settings.browserid_url + '/jwks.json')).read
public_key = JSON::JWK.new(public_key_jwks)
id_token = JSON::JWT.decode params[:id_token], public_key

The jwks.json on the server is (linked above) defined like this:

json ({
  keys: [{
    kty: 'RSA',
    alg: 'RS256',
    use: 'sig',
    kid: settings.kid,
    n: Base64.urlsafe_encode64([settings.pubkey.params['n'].to_s(16)].pack('H*')).gsub(/=*$/, ''),
    e: Base64.urlsafe_encode64([settings.pubkey.params['e'].to_s(16)].pack('H*')).gsub(/=*$/, '')
  }]
})

I think it is something in the internals of JSON::JWT.decode that makes the verify try to work on a string.

I did not highlight that properly above, but the JWT.encode that encodes params[:id_token] for transport is coming from the 'jwt' gem, not 'json/jwt'. That's the call:

headers = {}
headers[:kid] = settings.kid if settings.kid
JWT.encode payload, settings.privkey, 'RS256', headers

An incompatibility?

nov commented 8 years ago

Ah, JWKS (JSON::JWK::Set, an array of JWKs) is different from JWK (JSON::JWK). Try this.

jwks_json = {
  keys: [{
    kty: 'RSA',
    alg: 'RS256',
    use: 'sig',
    n: 'a',
    e: 'b'
  }]
}
jwks = JSON::JWK::Set.new jwks_json
JSON::JWT.decode params[:id_token], jwks
onli commented 8 years ago

Thanks for the answer, and sorry for the delay.

That is sadly leading to a different error message:

 /home/onli/Dropbox-decrypted/ursprung.git/vendor/bundle/ruby/2.2.0/gems/json-jwt-1.6.2/lib/json/jose.rb in with_jwk_support
        end.try(:to_key) or raise JWK::Set::KidNotFound
/home/onli/Dropbox-decrypted/ursprung.git/vendor/bundle/ruby/2.2.0/gems/json-jwt-1.6.2/lib/json/jws.rb in valid?
      public_key_or_secret = with_jwk_support public_key_or_secret
/home/onli/Dropbox-decrypted/ursprung.git/vendor/bundle/ruby/2.2.0/gems/json-jwt-1.6.2/lib/json/jws.rb in verify!
        public_key_or_secret && valid?(public_key_or_secret) or
/home/onli/Dropbox-decrypted/ursprung.git/vendor/bundle/ruby/2.2.0/gems/json-jwt-1.6.2/lib/json/jws.rb in decode_compact_serialized
        jws.verify! public_key_or_secret unless public_key_or_secret == :skip_verification
/home/onli/Dropbox-decrypted/ursprung.git/vendor/bundle/ruby/2.2.0/gems/json-jwt-1.6.2/lib/json/jwt.rb in decode_compact_serialized
          JWS.decode_compact_serialized jwt_string, key_or_secret
/home/onli/Dropbox-decrypted/ursprung.git/vendor/bundle/ruby/2.2.0/gems/json-jwt-1.6.2/lib/json/jose.rb in decode
          decode_compact_serialized input, key_or_secret
/home/onli/Dropbox-decrypted/sinatra-browserid.git/lib/sinatra/browserid.rb in block in registered
                  id_token = JSON::JWT.decode params[:id_token], public_key

I debugged this as far as I could. In with_jwk_support in jose.rb, https://github.com/nov/json-jwt/blob/master/lib/json/jose.rb#L28, this is how key looks like:

{"k"=>"{\"keys\":[{\"kty\":\"RSA\",\"alg\":\"RS256\",\"use\":\"sig\",\"kid\":\"5b6eab5fafe3a0c681c4dab6847242264889b885\",\"n\":\"rcgjgnPWAO7ANqwqkmfLSGzUp_BA_bx2uaF1QnYA8-gfRRLVi5l6RNbAddK_NzqfFOqXPGF1-dbLrtwC_3L8iS6nP71w-FcOmaho0S62puPy_zlaXtraznN8emLDvOJn55AMcoXeTeWofdQ45Rt9H44-9OAZeVcvozP2jkD8VTH5hlxcp0WBXIFbapIRGG3yd_Bnfcd60oCsz2Y3egr6tg1SmhiO1vTT1fCYMIwrYNoGmNk0mMF0HMPRGr6C998RuoX-joRnnq2eacxSr6h-zf9hkg9-hR5eJGfspSJGiH1LA_WpKhkxk4NOLFL7biRXRojUXjlg2vDwte5Ud9Ozlw\",\"e\":\"AQAB\"}]}", "kty"=>:oct, "kid"=>"6Y3ITVdZL5w5zFYr96kH4vjoY3Lcxr_SuNiVvDznmDw"
}

Please notice the kid below k, and the other kid above. The kid at the top level is coming from the jwk constructor in jwk.rb:

self[:kid] ||= thumbprint rescue nil #ignore

thumbprint is what ends up in key. I read up the code of that, it seems to be the base64 encoded sha256 hash of parts of the jwk, e kty and n. But my backend is defining the kid as such:

set :kid, Digest::SHA1.hexdigest(settings.pubkey.to_s)

As far as I know the spec, how kid defined is unspecified. But to change the backend to work with one specific jwk library seems wrong. May I suggest changing that code to work with the inner kid? Assuming I'm not on the wrong track here.

nov commented 8 years ago
self[:kid] ||= thumbprint rescue nil #ignore

shouldn't overwrite your kid. see below for more details.

require 'json/jwt'

jwks_json_with_kid = {
  keys: [{
    kty: 'RSA',
    alg: 'RS256',
    use: 'sig',
    n: 'a',
    e: 'b',
    kid: 'your-kid'
  }]
}

jwks_json_without_kid = {
  keys: [{
    kty: 'RSA',
    alg: 'RS256',
    use: 'sig',
    n: 'a',
    e: 'b'
  }]
}

jwks_with_kid = JSON::JWK::Set.new jwks_json_with_kid
jwks_without_kid = JSON::JWK::Set.new jwks_json_without_kid

puts jwks_with_kid.first[:kid] # => 'your-kid'
puts jwks_without_kid.first[:kid] # => 'UnjciMDiBlgL2poMrWAdSjH8JQ_lINur2kt9zvWOxuE'
nov commented 8 years ago

ah, and your input is handles as a shared secret, since you gave a string, not a hash.

onli commented 8 years ago

You are right, the test case works:

require 'json/jwt'
require 'open-uri'

jwks_json_with_kid = {
  keys: [{
    kty: 'RSA',
    alg: 'RS256',
    use: 'sig',
    n: 'a',
    e: 'b',
    kid: 'your-kid'
  }]
}

jwks_json_without_kid = {
  keys: [{
    kty: 'RSA',
    alg: 'RS256',
    use: 'sig',
    n: 'a',
    e: 'b'
  }]
}

jwks_json_backend = JSON.parse(URI.parse(URI.escape('https://laoidc.herokuapp.com/jwks.json')).read)

jwks_with_kid = JSON::JWK::Set.new jwks_json_with_kid
jwks_without_kid = JSON::JWK::Set.new jwks_json_without_kid
jwks_backend = JSON::JWK::Set.new jwks_json_backend

puts jwks_with_kid.first[:kid] # => 'your-kid'
puts jwks_without_kid.first[:kid] # => 'UnjciMDiBlgL2poMrWAdSjH8JQ_lINur2kt9zvWOxuE'
puts jwks_backend.first[:kid] # => '5b6eab5fafe3a0c681c4dab6847242264889b885'

The error is in how it is called?

Edit: But is this really testing what I described above? I saw that kid is set correctly, but also that another kid is set one level above.

nov commented 8 years ago

Your code is wrong when creating this key, for some reason. It is probably generated by calling JSON::JWK.new "non-parsed-json-strong" or such.

{"k"=>"{\"keys\":[{\"kty\":\"RSA\",\"alg\":\"RS256\",\"use\":\"sig\",\"kid\":\"5b6eab5fafe3a0c681c4dab6847242264889b885\",\"n\":\"rcgjgnPWAO7ANqwqkmfLSGzUp_BA_bx2uaF1QnYA8-gfRRLVi5l6RNbAddK_NzqfFOqXPGF1-dbLrtwC_3L8iS6nP71w-FcOmaho0S62puPy_zlaXtraznN8emLDvOJn55AMcoXeTeWofdQ45Rt9H44-9OAZeVcvozP2jkD8VTH5hlxcp0WBXIFbapIRGG3yd_Bnfcd60oCsz2Y3egr6tg1SmhiO1vTT1fCYMIwrYNoGmNk0mMF0HMPRGr6C998RuoX-joRnnq2eacxSr6h-zf9hkg9-hR5eJGfspSJGiH1LA_WpKhkxk4NOLFL7biRXRojUXjlg2vDwte5Ud9Ozlw\",\"e\":\"AQAB\"}]}", "kty"=>:oct, "kid"=>"6Y3ITVdZL5w5zFYr96kH4vjoY3Lcxr_SuNiVvDznmDw"
}
onli commented 8 years ago

The jwks is created by hand in https://github.com/callahad/authbackend/blob/e14f16899fc7a3dadf84651b897eac2eccc1d74d/server.rb#L384. The jwt encoding is handled by another gem, in https://github.com/callahad/authbackend/blob/e14f16899fc7a3dadf84651b897eac2eccc1d74d/server.rb#L381, but that part seems compatible. You think it is wrong to not define a kid at the top level of the jwks? Are we misreading the spec?

To give some context: This is not a blocker for me. I have a working version using the same gem as used in the backend. But it would be great to have the jwt decoding compatible with multiple libs (also in other languages than ruby, the same problem might arise there), I preferred your way of handling the jwk/jwks, and I wanted to be a good citizen by reporting the issue I encountered. If this incompatibility can't be resolved with a reasonable amount of work, please don't invest too much time in it (though I'm certainly willing to work further on this).

nov commented 8 years ago

your jwk's kty isn't RSA, but oct.

onli commented 8 years ago

I just stumbled about this thread again.

your jwk's kty isn't RSA, but oct.

The kty is handcoded to read 'RSA', please see https://github.com/callahad/authbackend/blob/e14f16899fc7a3dadf84651b897eac2eccc1d74d/server.rb#L387.

nakulpathak3 commented 7 years ago

Ran into same issue, better error message/general fix on the JSON JWT side for handling this case would be very helpful

mingca commented 7 years ago

@nov I ran into this issue while integrating omniauth-openid-connect with Auth0 as openid idp. I debugged and it seems like Auth0 does not send kid header in it's response.

json-jwt-1.7.2/lib/json/jose.rb:30
jwk[:kid] && jwk[:kid] == kid

so jwk[:kid] == kid is evaluated false all the time. Regrading to https://tools.ietf.org/html/rfc7515#section-4.1.4 kid header is optional. And i think they do not send kid header when there's only one JWK because it's obvious.

Appreciate to any comments in advance.

nov commented 7 years ago

you can specify exact jwk for signature verification, not jwk set. then nonkid is required.

f0ster commented 6 years ago

@ninjarails maybe this is helpful, I am manually using JWT

# lib/json_web_token.rb

# frozen_string_literal: true
require 'net/http'
require 'uri'

class JsonWebToken
  def self.verify(token)
    JWT.decode(token,  Rails.application.secrets.auth0_client_secret,
               true, # Verify the signature of this token
               algorithm: 'RS256') {  |header| jwks_hash[header['kid']] }
  end

  def self.jwks_hash
    jwks_raw = Net::HTTP.get URI("https://MYDOMAIN.auth0.com/.well-known/jwks.json")
    jwks_keys = Array(JSON.parse(jwks_raw)['keys'])
    Hash[
      jwks_keys
        .map do |k|
        [
          k['kid'],
          OpenSSL::X509::Certificate.new(
            Base64.decode64(k['x5c'].first)
          ).public_key
        ]
      end
    ]
  end
end
kofronpi commented 5 years ago

To whom it may be useful, when using openid_connect gem and having issues with json-jwt

Using OpenIDConnect::Discovery::Provider::Config.discover!, I had to use discover.jwks.first:

expected = { client_id: client_id, issuer: discover.issuer, nonce: session.delete(:nonce) }
id_token = OpenIDConnect::ResponseObject::IdToken.decode(access_token.id_token, discover.jwks.first)
id_token.verify!(expected)

otherwise I had a JWK::Set::KidNotFound error thrown.

pgiblock commented 5 years ago

@kofronpi - I did run into that issue as well, but I get a different error from IdToken.decode when applying the workaround: JSON::JWS::UnexpectedAlgorithm (no implicit conversion of OpenSSL::PKey::RSA into String)

The JWK looks like: {"kid"=>"kL2gw_im-dqAZPKXsI9DM1R2EH8XlePQ8AiwG8h1mRE", "kty"=>"RSA", "alg"=>"RS512", "use"=>"sig", "n"=>"zlzzXasliI5Dr8uJCSh3RpOw6j0L__4I7lTOHYeLTIoiMV2ba14hLVqarosLB4KCHMnS7g2AAoRi1FNpZzjqUgCD51SAVmEujQFuK-romD1kF6JzrmbzGS4JQKOdRH3zlBopcANhOHabfZ_ibtj_-lu70XUjkYoh4CI2ruv2zZMi9fpL-MgUD24Lsc_j0SxoCTNngQQT3xi6iqrID7u9yapluo4WVSP9yOROBxTQWidF3DB62fu8ar4XsTaSGBHBBFe9SngPgkYXg8iDLXkawAUgq7_WMsr0Fc_cnsRczPaWfCrKFmGWvVjF2ZHniPIUUwxFiInDfn-rVQl-J3CdNw", "e"=>"AQAB"}

The code is:

def discover
  @disco ||= OpenIDConnect::Discovery::Provider::Config.discover! @provider_uri
end

# Note: I've tried :basic, changing the scope, etc.. same error.
access_token = oidc_client.access_token!(
  scope: [:openid, :email, :profile],
  client_auth_method: :client_secret_basic
)

jwk = discover.jwks.first
id_token = OpenIDConnect::ResponseObject::IdToken.decode access_token.id_token, jwk

The IdP is KeyCloak. Thoughts? Thanks!

kofronpi commented 5 years ago

@pgiblock how do you initialize your client ? I do like this:

      @client ||= OpenIDConnect::Client.new(
        identifier: ENV['CLIENT'],
        secret: ENV['SECRET'],
        redirect_uri: ENV['REDIRECT_URI'],
        authorization_endpoint: discover.authorization_endpoint,
        token_endpoint: discover.token_endpoint,
        userinfo_endpoint: discover.userinfo_endpoint
      )

And then the only difference with your code, I simply call access_token!:

        expected = { client_id: ENV['CLIENT'], issuer: discover.issuer, nonce: session.delete(:nonce) }
        client.authorization_code = params[:code]
        access_token = client.access_token!
        # Validate the access token.
        id_token = OpenIDConnect::ResponseObject::IdToken.decode(access_token.id_token, discover.jwks.first)
        id_token.verify!(expected)

Hope it helps. I don't know Keycloak so I would contact its maintainer directly.