Support x5c JWT header as specified in RFC-7515 (JWS) #308

itstehkman · 2019-05-04T21:39:16Z

Taking a stab at supporting x5c (#59)

https://tools.ietf.org/html/rfc7515#section-4.1.6

The "x5c" (X.509 certificate chain) Header Parameter contains the
X.509 public key certificate or certificate chain [RFC5280]
corresponding to the key used to digitally sign the JWS. The
certificate or certificate chain is represented as a JSON array of
certificate value strings. Each string in the array is a
base64-encoded (Section 4 of [RFC4648] -- not base64url-encoded) DER
[ITU.X690.2008] PKIX certificate value. The certificate containing
the public key corresponding to the key used to digitally sign the
JWS MUST be the first certificate. This MAY be followed by
additional certificates, with each subsequent certificate being the
one used to certify the previous one. The recipient MUST validate
the certificate chain according to RFC 5280 [RFC5280] and consider
the certificate or certificate chain to be invalid if any validation
failure occurs. Use of this Header Parameter is OPTIONAL.

I'll fix the ebert issues later, I just want the idea validated so I can move forward.


        Part 1: x5c validator

sourcelevel-bot · 2019-05-04T21:39:18Z

Hello, @itstehkman! This is your first Pull Request that will be reviewed by Ebert, 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.

sourcelevel-bot · 2019-05-04T21:39:41Z

lib/jwt/x509/validator.rb

+      end
+
+      def valid?
+        return true if @cert_chain.nil? || @cert_chain.empty?


Use @cert_chain.blank? instead of @cert_chain.nil? || @cert_chain.empty?.

sourcelevel-bot · 2019-05-04T21:39:41Z

lib/jwt/x509/validator.rb

+      # @option opts [String] 'x5t' (not yet supported)
+      def initialize(opts)
+        unless opts['x5c'].nil?
+          signing_der = ::Base64.decode64((opts['x5c'].first))


Don't use parentheses around a method call.

sourcelevel-bot · 2019-05-04T21:39:42Z

lib/jwt/decode.rb


      raise(JWT::IncorrectAlgorithm, 'An algorithm must be specified') if allowed_algorithms.empty?
      raise(JWT::IncorrectAlgorithm, 'Expected a different algorithm') unless options_includes_algo_in_header?

+      if @options[:validate_cert] == true && !!@header['x5c']


Avoid the use of double negation (!!).

sourcelevel-bot · 2019-05-04T21:39:43Z

lib/jwt/decode.rb


      raise(JWT::IncorrectAlgorithm, 'An algorithm must be specified') if allowed_algorithms.empty?
      raise(JWT::IncorrectAlgorithm, 'Expected a different algorithm') unless options_includes_algo_in_header?

+      if @options[:validate_cert] == true && !!@header['x5c']
+        return false unless ::JWT::X509::Validator.new({ 'x5c' => header['x5c'] }).valid?


Redundant curly braces around a hash parameter.

sourcelevel-bot · 2019-05-04T21:39:43Z

lib/jwt/decode.rb

@@ -35,10 +35,15 @@ def decode_segments
    def verify_signature
      @key = find_key(&@keyfinder) if @keyfinder
      @key = ::JWT::JWK::KeyFinder.new(jwks: @options[:jwks]).key_for(header['kid']) if @options[:jwks]
+      @key = ::JWT::X509::KeyFinder.new({ 'x5c' => header['x5c'] }).public_key if header['x5c']


Redundant curly braces around a hash parameter.

sourcelevel-bot · 2019-05-04T21:39:44Z

lib/jwt/x509/validator.rb

@@ -0,0 +1,30 @@
+module JWT
+  module X509


JWT::X509 has the name 'X509'

Read more about it here.

sourcelevel-bot · 2019-05-04T21:39:44Z

lib/jwt/x509/validator.rb

+      end
+
+      def valid?
+        return true if @cert_chain.nil? || @cert_chain.empty?


JWT::X509::Validator#valid? performs a nil-check

Read more about it here.

sourcelevel-bot · 2019-05-04T21:39:45Z

lib/jwt/x509/validator.rb

+      # @option opts [Array<String>] 'x5c' x509 cert chain, where first is the cert used to sign
+      # @option opts [String] 'x5t' (not yet supported)
+      def initialize(opts)
+        unless opts['x5c'].nil?


JWT::X509::Validator#initialize performs a nil-check

Read more about it here.

sourcelevel-bot · 2019-05-04T21:39:45Z

lib/jwt/x509/validator.rb

+      # @option opts [Array<String>] 'x5c' x509 cert chain, where first is the cert used to sign
+      # @option opts [String] 'x5t' (not yet supported)
+      def initialize(opts)
+        unless opts['x5c'].nil?


JWT::X509::Validator#initialize calls 'opts['x5c']' 4 times

Read more about it here.

sourcelevel-bot · 2019-05-04T21:39:46Z

lib/jwt/x509/key_finder.rb

@@ -0,0 +1,17 @@
+module JWT
+  module X509


JWT::X509 has the name 'X509'

Read more about it here.

sourcelevel-bot · 2019-05-04T21:39:47Z

lib/jwt/x509/key_finder.rb

+      # @option opts [Array<String>] 'x5c' x509 cert chain, where first is the cert used to sign
+      # @option opts [String] 'x5t' (not yet supported)
+      def initialize(opts)
+        unless opts['x5c'].nil?


JWT::X509::KeyFinder#initialize performs a nil-check

Read more about it here.

sourcelevel-bot · 2019-05-04T21:39:47Z

lib/jwt/decode.rb

@@ -35,10 +35,15 @@ def decode_segments
    def verify_signature
      @key = find_key(&@keyfinder) if @keyfinder
      @key = ::JWT::JWK::KeyFinder.new(jwks: @options[:jwks]).key_for(header['kid']) if @options[:jwks]
+      @key = ::JWT::X509::KeyFinder.new({ 'x5c' => header['x5c'] }).public_key if header['x5c']


JWT::Decode#verify_signature calls 'header['x5c']' 3 times

Read more about it here.


        ebert fixes

sourcelevel-bot · 2019-05-07T01:33:54Z

lib/jwt/x509/validator.rb

+      def initialize(opts)
+        x5c = opts[:x5c]
+
+        unless x5c.nil?


JWT::X509::Validator#initialize performs a nil-check

Read more about it here.

sourcelevel-bot · 2019-05-07T01:33:55Z

Ebert has finished reviewing this Pull Request and has found:

10 possible new issues (including those that may have been commented here).

You can see more details about this review at https://ebertapp.io/github/jwt/ruby-jwt/pulls/308.

bdewater · 2019-05-24T16:10:38Z

lib/jwt/x509/validator.rb

+          signing_der = ::Base64.decode64 x5c.first
+          @signing_cert = OpenSSL::X509::Certificate.new signing_der
+          len = x5c.length
+          @cert_chain = x5c[1...len].map do |b64der|


Remove len = x5c.length and use:

Suggested change

@cert_chain = x5c[1...len].map do |b64der|

@cert_chain = x5c[1..-1].map do |b64der|

bdewater · 2019-05-24T16:10:38Z

lib/jwt/x509/validator.rb

+          @signing_cert = OpenSSL::X509::Certificate.new signing_der
+          len = x5c.length
+          @cert_chain = x5c[1...len].map do |b64der|
+            OpenSSL::X509::Certificate.new(::Base64.decode64(b64der))


See comment above about RFC 4648.

Suggested change

OpenSSL::X509::Certificate.new(::Base64.decode64(b64der))

OpenSSL::X509::Certificate.new(::Base64.strict_decode64(b64der))

bdewater · 2019-05-24T16:10:38Z

lib/jwt/x509/validator.rb

+        x5c = opts[:x5c]
+
+        unless x5c.nil?
+          signing_der = ::Base64.decode64 x5c.first


We should use the strict variant here, as documented this method complies with RFC 4648 mentioned by the JWT spec.

Suggested change

signing_der = ::Base64.decode64 x5c.first

signing_der = ::Base64.strict_decode64(x5c.first)

bdewater · 2019-05-24T16:10:38Z

lib/jwt/x509/validator.rb

+          store.add_cert cert
+        end
+        store.verify @signing_cert
+      end


This block of code has security problems. From RFC 5280 section 3.2:

In general, a chain of multiple certificates may be needed, comprising a certificate of the public key owner (the end entity) signed by one CA, and zero or more additional certificates of CAs signed by other CAs. Such chains, called certification paths, are required because a public key user is only initialized with a limited number of assured CA public keys.

More concretely:

Chain validation can not be skipped: this would allow an attacker to just strip the chain and inject their own certificate and use it to sign, bypassing any security check. Even a single certificate must chain correctly to a trust anchor.

The trust anchor should be configurable by the consuming app. It may be tempting to default to OpenSSL::X509::Store#set_defaults_path but instead of allowing all the certificate authorities to sign, the user should specify this themselves and preferably only limit to root certificate(s) that apply for their use case. E.g. when validating a JWT from Google, use only the cert(s) from https://pki.goog/

RFC 5280 section 3.3 also mentions revocation checking, which is absent in this PR.

I've been implementing all of the above in the WebAuthn gem, you can find my code here: cedarcode/webauthn-ruby#208 - feel free to use it, I'd appreciate a mention as co-author if you do 🙂

@bdewater sorry for the late response here! Thank you for all the feedback, definitely seems like I was missing some important parts. And that webauthn PR looks impressive.

Unfortunately I am no longer working on this PR, as my project's requirements changed and I don't need to use x5c for JWTs anymore. Please feel free to take my work and continue it if you'd like!

@bdewater would you be interested in taking over this PR, because it looks like a important feature. Newer versions of the IDP's are supporting the x5c header and I think we should support it in this gem too.

Let me know if you would be able to work on this one, else I can pick it up and resolve :)

I'm currently on vacation but was planning to pick this up again after I'm back next week :)

Part 1: x5c validator

Verified

This commit was signed with a verified signature.

itstehkman Kareem Moussa

GPG key ID: DA7CB0133A1A3BA7 Learn about signing commits

Loading status checks…

2bd8ac7