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.

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.

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-24T14:39:43Z

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-24T14:46:35Z

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-24T14:47:14Z

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:07:15Z

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

2bd8ac7

sourcelevel-bot bot reviewed May 4, 2019

View reviewed changes

lib/jwt/x509/validator.rb

@@ -0,0 +1,30 @@

module JWT

module X509

Copy link

sourcelevel-bot bot May 4, 2019

Choose a reason for hiding this comment

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

JWT::X509 has the name 'X509'

Read more about it here.

sourcelevel-bot bot reviewed May 4, 2019

View reviewed changes

lib/jwt/x509/key_finder.rb

@@ -0,0 +1,17 @@

module JWT

module X509

Copy link

sourcelevel-bot bot May 4, 2019

Choose a reason for hiding this comment

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

JWT::X509 has the name 'X509'

Read more about it here.

sourcelevel-bot bot reviewed May 4, 2019

View reviewed changes

itstehkman changed the title ~~Part 1: x5c validator~~ Support x5c JWT header as specified in RFC-7515 (JWS) May 4, 2019

ebert fixes

97faa6c

sourcelevel-bot bot reviewed May 7, 2019

View reviewed changes

bdewater suggested changes May 24, 2019

View reviewed changes

bdewater mentioned this pull request Nov 4, 2019

Add x5c header key finder #338

Merged

excpt added enhancement feature labels Jul 7, 2020

excpt added this to the Version 2.3.0 milestone Jul 7, 2020

excpt self-requested a review July 7, 2020 00:31

anakinj closed this in #338 Dec 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

itstehkman commented May 4, 2019 •

edited by excpt

Loading

sourcelevel-bot bot commented May 4, 2019

sourcelevel-bot bot May 4, 2019

sourcelevel-bot bot May 4, 2019

sourcelevel-bot bot May 4, 2019

sourcelevel-bot bot May 4, 2019

sourcelevel-bot bot May 4, 2019

sourcelevel-bot bot May 4, 2019

sourcelevel-bot bot May 4, 2019

sourcelevel-bot bot May 4, 2019

sourcelevel-bot bot May 4, 2019

sourcelevel-bot bot May 4, 2019

sourcelevel-bot bot May 4, 2019

sourcelevel-bot bot May 4, 2019

sourcelevel-bot bot May 7, 2019

sourcelevel-bot bot commented May 7, 2019

bdewater May 24, 2019

bdewater May 24, 2019

bdewater May 24, 2019

bdewater May 24, 2019

itstehkman Jul 4, 2019 •

edited

Loading

rabajaj0509 Oct 3, 2019

bdewater Oct 6, 2019 •

edited

Loading

bdewater Nov 4, 2019

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

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

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

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

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

Conversation

itstehkman commented May 4, 2019 • edited by excpt Loading

sourcelevel-bot bot commented May 4, 2019

sourcelevel-bot bot May 4, 2019

Choose a reason for hiding this comment

sourcelevel-bot bot May 4, 2019

Choose a reason for hiding this comment

sourcelevel-bot bot May 4, 2019

Choose a reason for hiding this comment

sourcelevel-bot bot May 4, 2019

Choose a reason for hiding this comment

sourcelevel-bot bot May 4, 2019

Choose a reason for hiding this comment

sourcelevel-bot bot May 4, 2019

Choose a reason for hiding this comment

sourcelevel-bot bot May 4, 2019

Choose a reason for hiding this comment

sourcelevel-bot bot May 4, 2019

Choose a reason for hiding this comment

sourcelevel-bot bot May 4, 2019

Choose a reason for hiding this comment

sourcelevel-bot bot May 4, 2019

Choose a reason for hiding this comment

sourcelevel-bot bot May 4, 2019

Choose a reason for hiding this comment

sourcelevel-bot bot May 4, 2019

Choose a reason for hiding this comment

sourcelevel-bot bot May 7, 2019

Choose a reason for hiding this comment

sourcelevel-bot bot commented May 7, 2019

bdewater May 24, 2019

Choose a reason for hiding this comment

bdewater May 24, 2019

Choose a reason for hiding this comment

bdewater May 24, 2019

Choose a reason for hiding this comment

bdewater May 24, 2019

Choose a reason for hiding this comment

itstehkman Jul 4, 2019 • edited Loading

Choose a reason for hiding this comment

rabajaj0509 Oct 3, 2019

Choose a reason for hiding this comment

bdewater Oct 6, 2019 • edited Loading

Choose a reason for hiding this comment

bdewater Nov 4, 2019

Choose a reason for hiding this comment

itstehkman commented May 4, 2019 •

edited by excpt

Loading

itstehkman Jul 4, 2019 •

edited

Loading

bdewater Oct 6, 2019 •

edited

Loading