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

Revert "crypto: add crypto.timingSafeEqual" #8225

Closed
wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Aug 22, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

crypto

Description of change

This reverts commit 0fc5e0d.

Additional testing indicates that there may still be timing issues with this implementation. Revert in order to give more time for testing before this goes out into a release...

Refs: #8040
Refs: #8203

/cc @Trott @not-an-aardvark

This reverts commit 0fc5e0d.

Additional testing indicates that there may still be timing issues
with this implementation. Revert in order to give more time for
testing before this goes out into a release...

Refs: nodejs#8040
Refs: nodejs#8203
@nodejs-github-bot nodejs-github-bot added crypto Issues and PRs related to the crypto subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Aug 22, 2016
@Trott
Copy link
Member

Trott commented Aug 22, 2016

LGTM

@nodejs/crypto

@Trott
Copy link
Member

Trott commented Aug 22, 2016

@bnoordhuis
Copy link
Member

LGTM. This looks like it was a clean revert, no nudging necessary?

@Trott
Copy link
Member

Trott commented Aug 22, 2016

Given the rather large impact this has on our CI results, I'd be OK with this landing sooner rather than waiting the usual 48 hours as long as it's a clean revert and there's no concerns from @nodejs/crypto.

@jasnell
Copy link
Member Author

jasnell commented Aug 23, 2016

+1 for sooner rather than later.

@jasnell
Copy link
Member Author

jasnell commented Aug 23, 2016

It is a clean revert. There were no conflicts.

jasnell added a commit that referenced this pull request Aug 23, 2016
This reverts commit 0fc5e0d.

Additional testing indicates that there may still be timing issues
with this implementation. Revert in order to give more time for
testing before this goes out into a release...

Refs: #8040
Refs: #8203
PR-URL: #8225
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@jasnell
Copy link
Member Author

jasnell commented Aug 23, 2016

Landed in 0764bc4

@Fishrock123
Copy link
Contributor

#8428 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants