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

Allow configuration of the algorithm to be used for encoding/decoding #19

Merged

Conversation

hrakotobe
Copy link

Allow the use of different signing algorithm when encoding/decoding JWTs.

HS256 is currently hard coded for the encoding and decoding of the token.
This is a proposition to allow overriding the default and specify more robust (in particular RS256) algorithms.

Please tell me if there is any coding style/formatting issues or comment on this approach.

@waiting-for-dev
Copy link
Owner

@hrakotobe thanks for your work on that. I'm ok with it. Having the option of configuring the algorithm makes a lot of sense. But, just out of curiosity, why do you need to use an asymmetric algorithm? Usually this gem is used in a server which acts both as identity provider and resource provider. Do you have any other actor who needs to verify the token signature? In terms of robustness, if I'm not wrong, there is no difference between HS256 & RS256. You could have more security with HS512 while keeping things symmetric.

About the implementation, I think everything is perfect in the library code. However, I think there is no need to change anything in the test suite. It is just a setting to allow other kind of algorithms, which are all tested in ruby-jwt library. From our end, we are save testing everything works fine with the default algorithm.

@hrakotobe hrakotobe force-pushed the allow-algorithm-override branch from 7423c27 to 643754e Compare July 31, 2019 11:20
@hrakotobe
Copy link
Author

@waiting-for-dev thanks for looking at it.
For the choice of RS256, yes, we plan to share the token between several system, some of which will only validate the tokens without generating new ones (typically they'll only have the public key)

For the tests, ok, that makes sense. I removed them.

@waiting-for-dev
Copy link
Owner

Could you just rename again e to exception in the strategy? This makes the build to fail.

@hrakotobe hrakotobe force-pushed the allow-algorithm-override branch from 643754e to 9671531 Compare August 1, 2019 08:15
@hrakotobe
Copy link
Author

It seems there is a configuration conflict on the code style checking between Reek and Rubocop.
Should I change the rubocop rule to match Reek's ?

https://travis-ci.org/waiting-for-dev/warden-jwt_auth/jobs/565924531#L649

Analyze with Reek..............................................[Reek] FAILED
650Errors on modified lines:
651  /home/travis/build/waiting-for-dev/warden-jwt_auth/lib/warden/jwt_auth/strategy.rb:24: UncommunicativeVariableName: Warden::JWTAuth::Strategy#authenticate! has the variable name 'e' [https://github.com/troessner/reek/blob/v4.8.2/docs/Uncommunicative-Variable-Name.md]

https://travis-ci.org/waiting-for-dev/warden-jwt_auth/jobs/566357274

Analyze with RuboCop........................................[RuboCop] FAILED
652Errors on modified lines:
653/home/travis/build/waiting-for-dev/warden-jwt_auth/lib/warden/jwt_auth/strategy.rb:24:34: C: Naming/RescuedExceptionsVariableName: Use `e` instead of `exception`.

@waiting-for-dev waiting-for-dev merged commit 8280785 into waiting-for-dev:master Aug 1, 2019
@waiting-for-dev
Copy link
Owner

Thanks @hrakotobe for your great work on this. I merged it and released as v0.4.0. If you are using devise-jwt, you'll need v0.6.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants