Skip to content

crypto: simplify missing passphrase detection - #27089

Closed
tniessen wants to merge 2 commits into
nodejs:masterfrom
tniessen:crypto-simplify-missing-passphrase-detection
Closed

crypto: simplify missing passphrase detection#27089
tniessen wants to merge 2 commits into
nodejs:masterfrom
tniessen:crypto-simplify-missing-passphrase-detection

Conversation

@tniessen

Copy link
Copy Markdown
Member

This commit removes the PasswordCallbackInfo class introduced in #25208 and uses existing OpenSSL error handling APIs to detect missing passphrases instead. This should not cause any observable differences for users, but slightly simplifies and shortens our code.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. labels Apr 4, 2019
Comment threadsrc/node_crypto.cc Outdated

@sam-githubsam-github left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code looks good. Its completely backwards compatible? No test changes needed at all? If so, its nice to delete code!

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Comment threadsrc/node_crypto.cc Outdated
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@tniessen

Copy link
Copy Markdown
MemberAuthor

Its completely backwards compatible? No test changes needed at all? If so, its nice to delete code!

As far as I know, there should be no difference whatsoever.

@danbev

Copy link
Copy Markdown
Contributor

Landed in fadcb2d.

@danbevdanbev closed this Apr 8, 2019
danbev pushed a commit that referenced this pull request Apr 8, 2019
PR-URL: #27089
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for freeto 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++.cryptoIssues and PRs related to the crypto subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@tniessen@nodejs-github-bot@danbev@sam-github@bnoordhuis@jasnell