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

@tniessen tniessen commented Apr 4, 2019

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-bot nodejs-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 thread src/node_crypto.cc Outdated

@sam-github sam-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 thread src/node_crypto.cc Outdated
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@tniessen

tniessen commented Apr 6, 2019

Copy link
Copy Markdown
Member Author

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

danbev commented Apr 8, 2019

Copy link
Copy Markdown
Contributor

Landed in fadcb2d.

@danbev danbev 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 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