Implementation of HMAC Based Token pattern#155
Conversation
|
I can tell this was a major effort— the contribution offer is appreciated. There's a lot to dig into and consider here, but I wanted to followup to let you know that this has been seen 👍 |
|
@devinivy Thanks for your update. I'm looking into implementing the double submit pattern as described in the OWASP cheat sheet. Is it something the CRUMB community could be interested in? |
nlf
left a comment
nlf
left a comment
There was a problem hiding this comment.
first off, let me say thank you for implementing this. i love that the hapi community cares about security, and this is a fantastic example of that. your contribution is hugely appreciated!
i requested some changes, largely a lot of nit picking, but the sessionKey option is definitely worth discussing (see my code comments)
| method: (req, h) => { | ||
| // This extension method fakes an authentication | ||
| req.auth.credentials = { userId: 'john' }; | ||
| req.auth.isAuthenticated = true; |
There was a problem hiding this comment.
given the condition that isAuthenticated needs to be true for us to generate a valid crumb in hmac mode, but it is also possible for isAuthenticated to be false, i would very much like to see a test here that demonstrates and documents that behavior
| crumb = Cryptiles.randomString(settings.size); | ||
| } | ||
|
|
||
| if (settings.method === 'hmac' && (request.auth.isAuthenticated)) { |
There was a problem hiding this comment.
nit: extra parens
| if (settings.method === 'hmac' && (request.auth.isAuthenticated)) { | |
| if (settings.method === 'hmac' && request.auth.isAuthenticated) { |
| crumb = Hmac.encrypt(request.auth.credentials[settings.sessionKey], settings.secret); | ||
| } | ||
|
|
||
| h.state(settings.key, crumb, settings.cookieOptions); |
There was a problem hiding this comment.
as mentioned in another comment, it's possible at this point for crumb to be null. we need a test that shows what happens in this scenario
| } | ||
|
|
||
| if (settings.method === 'hmac' && | ||
| !Hmac.validate(content[request.route.settings.plugins._crumb.key], request.auth.credentials[settings.sessionKey], settings.secret) |
There was a problem hiding this comment.
nit: this line is super long, making it kind of hard to comprehend quickly. it might be best if we assigned the value of request.route.settings.plugins._crumb.key to a named variable and used that instead of the full reference.
| } | ||
|
|
||
| if (header !== getCrumbValue()) { | ||
| if (settings.method !== 'hmac' && header !== getCrumbValue()) { |
There was a problem hiding this comment.
if we're setting a default of random to settings.method, why not make this condition if (settings.method === 'random' to be more explicit?
|
|
||
| const Crypto = require('crypto'); | ||
|
|
||
| const ALGO = 'sha256'; |
There was a problem hiding this comment.
this should be configurable. sha256 is a reasonable default, but there's no good reason to not allow users to override it with their own choice
| * @param {string}key The secret | ||
| * @returns {string}token | ||
| */ | ||
| const encrypt = (userId, key) => { |
There was a problem hiding this comment.
nit: all instances of the variable userId should be sessionId since that more clearly indicates what the parameter is since it's not necessarily a user's id
| * @param {string}key The secret | ||
| * @returns {string}token | ||
| */ | ||
| const encrypt = (userId, key) => { |
There was a problem hiding this comment.
second nit: key variables should be renamed to secret, again to more clearly indicate what it's used for
| .update(token_timestamp, 'utf8') // Timestamp used at token generation | ||
| .digest('hex'); | ||
|
|
||
| return (digest === token_digest && token_timestamp <= timestamp); |
There was a problem hiding this comment.
given the comparison of token_timestamp and timestamp, i think it's more appropriate to convert the token_timestamp to a number after splitting the value out than it is to convert the timestamp to a string
| * `enforce` - defaults to true, using enforce with false will set the CSRF header cookie but won't execute the validation | ||
| * `logUnauthorized` - whether to add to the request log with tag 'crumb' and data 'validation failed' (defaults to false) | ||
|
|
||
| * `method` - The token generation method (and therefore how to validate), could take values `random` or `hmac`. Defaults to `random`, if set to `hmac` crumb will use the `Hmac Based Token pattern` [as described by OWASP here](https://github.com/OWASP/CheatSheetSeries/blob/083b4791c44ef28b105f66c82ffd83a86bf9fa16/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.md). |
There was a problem hiding this comment.
nit:
| * `method` - The token generation method (and therefore how to validate), could take values `random` or `hmac`. Defaults to `random`, if set to `hmac` crumb will use the `Hmac Based Token pattern` [as described by OWASP here](https://github.com/OWASP/CheatSheetSeries/blob/083b4791c44ef28b105f66c82ffd83a86bf9fa16/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.md). | |
| * `method` - The token generation and validation method, can be `'random'` or `'hmac'`. Defaults to `'random'`, if set to `'hmac'` crumb will use the `Hmac Based Token pattern` [as described by OWASP here](https://github.com/OWASP/CheatSheetSeries/blob/083b4791c44ef28b105f66c82ffd83a86bf9fa16/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.md). |
if this is something you're interested in implementing we would be happy to review and merge! |
3 participants
Hello,
In the frame of one of my project, I had to modify the Crumb plugin to implement an
HMAC based token patternto be used in a stateless server. I think it could valuable for the community to have it officially embedded in Crumb. The implementation follows the pattern described in the OWSAP cheatsheet. I have included a new option to specify the token generation to use: random (the method used by crumb as of now) or hmac (my current contribution)Unit tests have been updated to test 100% of the code base with both generation method. API markdown documentation has also been updated accordingly.