Add support for JWKs with HMAC key type. #372
Conversation
|
Hello, @phlegx! This is your first Pull Request that will be reviewed by SourceLevel, 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. |
|
@anakinj happy to see your review of my code. |
|
After looking at this I understand your question about why we do not export the private key a little better. Did not even know that it's a thing to present HMAC secrets as JWK :) |
|
@anakinj the RFC describes JWK with: A JSON Web Key (JWK) is a JavaScript Object Notation (JSON) data structure that represents a cryptographic key. So, JWK is only a data structure that represents a cryptographic key. How a user expose a JWK to a web endpoint is not part of the RFC7517 and should not affect the behavior or structure of a JWK. |
| end | ||
|
|
||
| class << self | ||
| def import(jwk_data) |
sourcelevel-bot
bot
Sep 25, 2020
Unused method argument - jwk_data. If it's necessary, use _ or _jwk_data as an argument name to indicate that it won't be used. You can also write as import(*) if you want the method to accept any arguments but don't care about them.
Unused method argument - jwk_data. If it's necessary, use _ or _jwk_data as an argument name to indicate that it won't be used. You can also write as import(*) if you want the method to accept any arguments but don't care about them.
| raise NotImplementedError, "#{self.class} has not implemented method '#{__method__}'" | ||
| end | ||
|
|
||
| def export(options = {}) |
sourcelevel-bot
bot
Sep 25, 2020
Unused method argument - options. If it's necessary, use _ or _options as an argument name to indicate that it won't be used. You can also write as export(*) if you want the method to accept any arguments but don't care about them.
Unused method argument - options. If it's necessary, use _ or _options as an argument name to indicate that it won't be used. You can also write as export(*) if you want the method to accept any arguments but don't care about them.
|
|
||
| module JWT | ||
| module JWK | ||
| class HMAC < Factory |
sourcelevel-bot
bot
Sep 25, 2020
JWT::JWK::HMAC assumes too much for instance variable '@keypair'
JWT::JWK::HMAC assumes too much for instance variable '@keypair'
phlegx
Sep 25, 2020
Author
Contributor
What does this mean "assumes too much"?
What does this mean "assumes too much"?
anakinj
Sep 25, 2020
•
Member
I think it's refers to the keypair instance variable used later in the class, not totally sure. Maybe try to access instance variables from the parent via accessors.
I think it's refers to the keypair instance variable used later in the class, not totally sure. Maybe try to access instance variables from the parent via accessors.
anakinj
Sep 25, 2020
Member
:) there is a @ keypair person on GitHub. Im so sorry for tagging you :)
:) there is a @ keypair person on GitHub. Im so sorry for tagging you :)
|
SourceLevel has finished reviewing this Pull Request and has found:
|
|
|
||
| module JWT | ||
| module JWK | ||
| class Factory |
anakinj
Sep 25, 2020
Member
Is this a Factory or just a Base?
Otherwise this looks good. We could then refactor the other classes to use this when they reach master.
Is this a Factory or just a Base?
Otherwise this looks good. We could then refactor the other classes to use this when they reach master.
phlegx
Sep 25, 2020
•
Author
Contributor
It is an Abstract! What was I thinking here? OK, I need a break. So, we need to rename Factory to some other name. Suggestions?
It is an Abstract! What was I thinking here? OK, I need a break. So, we need to rename Factory to some other name. Suggestions?
anakinj
Sep 25, 2020
Member
Maybe something with key as the RFC states "structure that represents a cryptographic key".
:: JWT::JWK::KeyBase or :: JWT::JWK::KeyAbstract would be my choices. I have no better suggestions :)
Maybe something with key as the RFC states "structure that represents a cryptographic key".
:: JWT::JWK::KeyBase or :: JWT::JWK::KeyAbstract would be my choices. I have no better suggestions :)
phlegx
Sep 25, 2020
Author
Contributor
Thx! I commit the changes with JWT::JWK::KeyAbstract.
Thx! I commit the changes with JWT::JWK::KeyAbstract.
|
Travis CI has failed because it has experienced an network timeout. I don't know how to re-start Travis without a commit. |
Build restarted. |
Adds support for JWKs with "kty" value "oct" (HMAC).
For additional details on these JWKs and their contents, see https://tools.ietf.org/html/rfc7517#appendix-A.3.
This implementation of
JWT::JWK::HMACadheres closely to the pattern set byJWT::JWK::RSAand PR #371 of @richardlarocqueJWT::JWK::EC. It keeps the same coding style and method names.Like specified in the RFC:
private?returns always true because "k" is always exported.