Add support for x5t and x5t#S256 header #669
Conversation
hieuk09
force-pushed
the
feature/add-x5t
branch
3 times, most recently
from
f6e2067 to
28a5e72
Compare
|
@hieuk09 Did you have some reference for how this should work? Could take a look at that to educate myself a bit. |
Now when looking closer at the changes I found some links... |
|
Thank you for the detailed review. I'll take a look and fix all the comments tomorrow. |
|
@anakinj could you have another review? |
| @@ -37,22 +36,36 @@ def key_for(kid) | |||
| # Returns the key for the given token | |||
| # @param [JWT::EncodedToken] token the token | |||
| def call(token) | |||
| key_for(token.header['kid']) | |||
| kid = token.header['kid'] | |||
There was a problem hiding this comment.
If the token happens to have both a kid and for example a x5t header it will never attempt using the x5t.
Not sure if this is a real scenario and could for sure be a later addition.
There was a problem hiding this comment.
I think this is a real scenario: here is a payload and header of token from Azure:
[
{
"aud": "https://management.core.windows.net",
"iss": "https://sts.windows.net/57c0dc11-c207-49dd-9bec-76a2d395ea76/",
"iat": 1745152327,
"nbf": 1745152327,
"exp": 1745157876,
"acr": "1",
// some more data
},
{
"typ": "JWT",
"alg": "RS256",
"x5t": "CNv0OI3RwqlHFEVnaoMAshCH2XE",
"kid": "CNv0OI3RwqlHFEVnaoMAshCH2XE"
}
]The keys can be found in https://login.windows.net/common/discovery/keys.
| @@ -51,6 +51,9 @@ def verify_key | |||
| def export(options = {}) | |||
| exported = parameters.clone | |||
| exported.reject! { |k, _| RSA_PRIVATE_KEY_ELEMENTS.include? k } unless private? && options[:include_private] == true | |||
|
|
|||
| exported[:x5t] = Base64.url_encode(OpenSSL::Digest::SHA1.new(rsa_key.to_der).digest) if options[:include_x5t] | |||
There was a problem hiding this comment.
Im still a bit uncertain if we should make the decision here what the thumbprint is, the x5t is the thumbprint for the certificate, the RSA public key is just a part of the certificate.
Think we could have a way to export a X509 certificate that could be adding these fields directly from the certificate itself.
Also with this approach the thumbprint will differ if rsa_key is a private key or just the public key.
Also what about the SHA256 version of the thumbprint?
There was a problem hiding this comment.
That's a good point, thanks for pointing that out. Let me think a bit more about this.
Description
Close #562
Checklist
Before the PR can be merged be sure the following are checked: