Add x5c header key finder

[bdewater]

This validates the certificate chain in accordance with RFC 5280, as described in RFC 7515 section 4.1.6.

To use this in your app, here are a couple of notes:

• you will want to cache the relevant CRL file(s) and make sure the cache is expired by the OpenSSL::X509::CRL#next_update timestamp
• in case you need to dynamically extract the CRL distribution point URIs from the x5c certificates, it is possible to use the X5cKeyFinder class directly in the keyfinder block argument passed to JWT.decode

Closes #59
Closes #308

[rabajaj0509]

@bdewater i am very naive at this and want to test this PR. Would you mind explaining me what CRL is and some detailed steps that would help me to test the PR?

[bdewater]

@rahulbajaj0509 a Certificate Revocation List is a list of certificates that should be considered invalid, even if the signature is correct, not before/not after timestamps are valid, etc. The list contains a reasonCode as to why a certificate was revoked.

My reason for opening this PR was because I missed this functionality for verifying the FIDO Alliance metadata files as part of cedarcode/webauthn-ruby#208 - I haven't had the chance yet to test this PR within the context of that work, but I'm planning to do that soon and make any adjustments if needed.

You can find out where to obtain the CRL file by seeing if any certificate (either part of the x5c header or the supplied root certificates) has a CRL Distribution Points extension. A helper method in the OpenSSL gem to get CRL Distribution Points URIs was merged. That PR builds on other unreleased work, so until the next release (hopefully soon) you can use this:

def crl_uris(certificate)
  ext = certificate.extensions.find { |ext| ext.oid == "crlDistributionPoints" }
  return nil if ext.nil?

  ext_value_der = OpenSSL::ASN1.decode(ext.to_der).value.last.value
  cdp_asn1 = OpenSSL::ASN1.decode(ext_value_der)
  if cdp_asn1.tag_class != :UNIVERSAL || cdp_asn1.tag != OpenSSL::ASN1::SEQUENCE
    raise OpenSSL::ASN1::ASN1Error "invalid extension"
  end

  crl_uris = cdp_asn1.map do |crl_distribution_point|
    distribution_point = crl_distribution_point.value.find do |v|
      v.tag_class == :CONTEXT_SPECIFIC && v.tag == 0
    end
    full_name = distribution_point&.value&.find do |v|
      v.tag_class == :CONTEXT_SPECIFIC && v.tag == 0
    end
    full_name&.value&.find do |v|
      v.tag_class == :CONTEXT_SPECIFIC && v.tag == 6 # uniformResourceIdentifier
    end
  end

  crl_uris&.map(&:value)
end

Let's take the example x5c header from the RFC. Once you have base64 decoded these, you'll see these certificates have been issued by GoDaddy. We'll assume here that you are certain whomever will be giving you JWTs will use GoDaddy as their root CA to sign their intermediate certificates, which will in turn be used to sign the JWTs.

Head over to https://certs.godaddy.com/repository to obtain the GoDaddy root certificates. Use the crl_uris method to extract the CRL URIs and download those files (1, 2) as well. Then you should be able to call

root_certificates = [] # Array of OpenSSL::X509::Certificate objects
crls = [] # Array of OpenSSL::X509::CRL objects
JWT.decode(jwt, nil, true, root_certificates: root_certificates, crls: crls)

Note that it was a conscious design design to not put any kind of downloading/caching logic in the gem. This is an application concern and I believe we shouldn't make too much assumptions about how the PKI is set up. If you need to do this on the fly you can use the keyfinder block still (like I did in my WebAuthn PR before I opened this PR), something like:

payload, _ = JWT.decode(response, nil, true) do |headers|
  root_certificates = # OpenSSL::X509::Certificate objects from somewhere
  jwt_certificates = headers["x5c"].map do |encoded|
    OpenSSL::X509::Certificate.new(::Base64.strict_decode64(encoded))
  end
  crls = (root_certificates + jwt_certificates).map do |cert|
    uris = cert.crl_uris
    # download `uris`, parse to OpenSSL::X509::CRL, write to cache
  end
  JWT::X5cKeyFinder.from(jwt_certificates, root_certificates, crls)
end

[cpb]

This looks like a great enhancement. If I get some time over the long weekend, how could a first-time-contributor to ruby-jwt help move this forward?

[bdewater]

@cpb if you can use this fork in your app and provide feedback if it works for you or have any other comments, that'd be great. I haven't had the time yet to do that myself.

[bdewater]

I verified this code works for my use case (verifying WebAuthn attestation via the FIDO Alliance Metadata Service). Code can be seen here: bdewater/fido_metadata@b157b5a - any feedback welcome 🙂

[anakinj]

This was a nice extraction!

[anakinj]

Hi, this looks really promising 👍. Gave a few comments here and there, hope it's of any value. A little something for the documentation would also be great.

[thomasdarde]

Any update ont his PR Review ?

[spiffistan]

Any chance this can get merged?

[sourcelevel-bot]

JWT::X5cKeyFinder#parse_certificates doesn't depend on instance state (maybe move it to another class?)

Read more about it here.

[sourcelevel-bot]

JWT::X5cKeyFinder#build_store doesn't depend on instance state (maybe move it to another class?)

Read more about it here.

[sourcelevel-bot]

JWT::X5cKeyFinder#from has approx 6 statements

Read more about it here.

[sourcelevel-bot]

JWT::X5cKeyFinder#build_store has approx 8 statements

Read more about it here.

[bdewater]

@anakinj I rebased and implemented your feedback. Curious how strong you feel about the remaining Reek comments - shoveling these private functions into the SecurityUtils module and/or breaking them up to lower statement count won't add much real value IMO.

[anakinj]

I agree with you that the suggestion of the robot should not be taken as the truth. I will take a look at this when I have a chance to fully concentrate on it. Too complicated after a long day :)

[anakinj]

Looking good.

[sourcelevel-bot]

JWT::X5cKeyFinder#build_store performs a nil-check

Read more about it here.

[sourcelevel-bot]

SourceLevel has finished reviewing this Pull Request and has found:

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

See more details about this review.

[anakinj]

@bdewater Is this ready to get merged?

[bdewater]

@anakinj it is now 😄 rebased to pick up the latest Rubocop stuff, addressed your most recent feedback, and added a test to ensure the x5c option in the JWT.decode call will actually call the X5cKeyFinder class.

[anakinj]

Thank you for your contribution (and patience) @bdewater.