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 ROCA weak key checking #3189

Merged
merged 5 commits into from Nov 2, 2017
Merged

Add ROCA weak key checking #3189

merged 5 commits into from Nov 2, 2017

Conversation

jsha
Copy link
Contributor

@jsha jsha commented Oct 20, 2017

Fixes #3183.

Thanks to @titanous for the library!

@titanous
Copy link
Contributor

FYI I just pushed a rewrite of the algorithm that halves the time it takes to check a key.

@rolandshoemaker
Copy link
Contributor

LGTM if we pull in the updated lib.

Copy link
Contributor

@cpu cpu left a comment

Choose a reason for hiding this comment

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

I agree with @rolandshoemaker that we should update to the more performant version of this code before deploying.

@rolandshoemaker
Copy link
Contributor

@titanous looks like the full paper was just pushed to a preprint, haven't had a chance to look at it yet but any chance there will be further perf improvements based on their explanation of the fingerprinting process?

@titanous
Copy link
Contributor

titanous commented Oct 30, 2017

Based on my understanding of the fingerprint, I don't believe that there are obvious substantial improvements to the current algorithm, though someone with a better math/perf background may be able to put something better together.

The current 1μs for a good key and 18μs for a weak key on my i7-4850HQ (a 2013 2.3GHz laptop CPU) seems good enough.

@jsha
Copy link
Contributor Author

jsha commented Oct 30, 2017

Yep, that's plenty good enough, thanks!

Copy link
Contributor

@cpu cpu left a comment

Choose a reason for hiding this comment

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

Sorry to realize this last minute, but I think we should probably gate this with a feature flag. After that I'm 👍

Copy link
Contributor

@cpu cpu left a comment

Choose a reason for hiding this comment

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

Thanks @jsha, @titanous ! 💪 🗝️ 💪

@cpu cpu merged commit 5df083a into master Nov 2, 2017
@cpu cpu deleted the rocacheck branch November 2, 2017 12:43
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.

None yet

4 participants