Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for JWKs with HMAC key type. #372

Merged
merged 8 commits into from Sep 28, 2020
Merged

Conversation

@phlegx
Copy link
Contributor

@phlegx phlegx commented Sep 24, 2020

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::HMAC adheres closely to the pattern set by JWT::JWK::RSA and PR #371 of @richardlarocque JWT::JWK::EC. It keeps the same coding style and method names.

Like specified in the RFC:

  • It emits the secret key ("k") value when exporting JWKs.
  • An import followed by an export preserve the "kid" value.
  • Instance method private? returns always true because "k" is always exported.
@sourcelevel-bot
Copy link

@sourcelevel-bot sourcelevel-bot bot commented Sep 24, 2020

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.

@phlegx
Copy link
Contributor Author

@phlegx phlegx commented Sep 24, 2020

@anakinj happy to see your review of my code. 😄

lib/jwt/jwk/hmac.rb Outdated Show resolved Hide resolved
@anakinj
Copy link
Member

@anakinj anakinj commented Sep 25, 2020

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 :)

@phlegx
Copy link
Contributor Author

@phlegx phlegx commented Sep 25, 2020

@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)

This comment has been minimized.

@sourcelevel-bot

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.

raise NotImplementedError, "#{self.class} has not implemented method '#{__method__}'"
end

def export(options = {})

This comment has been minimized.

@sourcelevel-bot

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.


module JWT
module JWK
class HMAC < Factory

This comment has been minimized.

@sourcelevel-bot

sourcelevel-bot bot Sep 25, 2020

JWT::JWK::HMAC assumes too much for instance variable '@keypair'

This comment has been minimized.

@phlegx

phlegx Sep 25, 2020
Author Contributor

What does this mean "assumes too much"?

This comment has been minimized.

@anakinj

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.

This comment has been minimized.

@anakinj

anakinj Sep 25, 2020
Member

:) there is a @ keypair person on GitHub. Im so sorry for tagging you :)

@excpt excpt requested a review from anakinj Sep 25, 2020
@sourcelevel-bot
Copy link

@sourcelevel-bot sourcelevel-bot bot commented Sep 25, 2020

SourceLevel has finished reviewing this Pull Request and has found:

  • 1 possible new issue (including those that may have been commented here).

See more details about this review.

@excpt excpt self-requested a review Sep 25, 2020

module JWT
module JWK
class Factory

This comment has been minimized.

@anakinj

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.

This comment has been minimized.

@phlegx

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?

This comment has been minimized.

@anakinj

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 :)

This comment has been minimized.

@phlegx

phlegx Sep 25, 2020
Author Contributor

Thx! I commit the changes with JWT::JWK::KeyAbstract.

@phlegx
Copy link
Contributor Author

@phlegx phlegx commented Sep 25, 2020

Travis CI has failed because it has experienced an network timeout. I don't know how to re-start Travis without a commit.

@excpt
Copy link
Member

@excpt excpt commented Sep 25, 2020

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.

@phlegx
Copy link
Contributor Author

@phlegx phlegx commented Sep 25, 2020

So, what do you think @excpt and @anakinj? Ready to add the abstract code, include private option also to JWK RSA and EC?

@excpt
Copy link
Member

@excpt excpt commented Sep 28, 2020

So, what do you think @excpt and @anakinj? Ready to add the abstract code, include private option also to JWK RSA and EC?

It is good to go. 👍

@excpt
excpt approved these changes Sep 28, 2020
@excpt excpt merged commit c5bf337 into jwt:master Sep 28, 2020
3 checks passed
3 checks passed
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sourcelevel-bot
sourcelevel SourceLevel has found 1 possible new issue.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants