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

Use constant time string comparison #31

Merged
merged 2 commits into from
Jun 4, 2020
Merged

Use constant time string comparison #31

merged 2 commits into from
Jun 4, 2020

Conversation

jamescook
Copy link
Contributor

Both of the popular Ruby and Go JWT implementations use constant-time string comparisons to some extent. Both use constant time comparison of a HMAC signature, but only Go uses constant time comparison for the 'aud' and 'iss'.

Copy link
Member

@veelenga veelenga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Could you please describe the usecase this change has an impact on? Also it would be nice to have some catching tests.

src/jwt.cr Outdated Show resolved Hide resolved
as suggested

Co-authored-by: Vitalii Elenhaupt <[email protected]>
@jamescook
Copy link
Contributor Author

@veelenga The general use case is to prevent timing attacks on HMAC signatures. In terms of tests, there is already coverage around signature and claim verification. Is there a test you can think of that needs to be added?

@veelenga
Copy link
Member

veelenga commented Jun 4, 2020

@jamescook I see, thanks. Well, I just try to follow the rule that every change should be enforced by the test, otherwise, if someone accidentally (or intensionally) changes it back, the suite will pass and there will not be any reason not to merge that as well.

However, I would rely on @stakach's input here.

Copy link
Member

@stakach stakach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks @jamescook

@stakach stakach merged commit 8e44ac9 into crystal-community:master Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants