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

src: split CryptoPemCallback into two functions #12827

Closed

Conversation

@danbev
Copy link
Member

commented May 4, 2017

Currently the function CryptoPemCallback is used for two things:

  1. As a passphrase callback.
  2. To avoid the default OpenSSL passphrase routine.

The default OpenSSL passphase routine would apply if both
the callback and the passphrase are null pointers and the typical
behaviour is to prompt for the passphase which is not appropriate in
node.

This commit suggests that the PasswordCallback function only handle
passphrases, and that an additional function named NoPasswordCallback
used for the second case to avoid OpenSSL's passphase routine.

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

src

src: split CryptoPemCallback into two functions
Currently the function CryptoPemCallback is used for two things:
1. As a passphrase callback.
2. To avoid the default OpenSSL passphrase routine.

The default OpenSSL passphase routine would apply if both
the callback and the passphrase are null pointers and the typical
behaviour is to prompt for the passphase which is not appropriate in
node.

This commit suggests that the PasswordCallback function only handle
passphrases, and that an additional function named NoPasswordCallback
used for the second case to avoid OpenSSL's passphase routine.
@danbev

This comment has been minimized.

Copy link
Member Author

commented May 4, 2017

@cjihrig
cjihrig approved these changes May 4, 2017
Copy link
Contributor

left a comment

LGTM, but @shigeki should probably take a look.

@jasnell
jasnell approved these changes May 4, 2017
@mscdex

This comment has been minimized.

Copy link
Contributor

commented May 4, 2017

@indutny
Copy link
Member

left a comment

I like this direction, let's do it! Few nits, though.

len = len > buflen ? buflen : len;
memcpy(buf, u, len);
return len;
}

This comment has been minimized.

Copy link
@indutny

indutny May 4, 2017

Member

Please add one more newline here, please.

This comment has been minimized.

Copy link
@danbev

danbev May 5, 2017

Author Member

I'll fix that.

@@ -473,7 +475,7 @@ void SecureContext::SetKey(const FunctionCallbackInfo<Value>& args) {

if (len == 2) {
if (args[1]->IsUndefined() || args[1]->IsNull())
len = 1;
has_password = false;
else
THROW_AND_RETURN_IF_NOT_STRING(args[1], "Pass phrase");
}

This comment has been minimized.

Copy link
@indutny

indutny May 4, 2017

Member

What if len == 1 from the start? It looks like has_password will be true?

This comment has been minimized.

Copy link
@danbev

danbev May 5, 2017

Author Member

Oh yes, sorry. Let me fix that. Thanks

@danbev

This comment has been minimized.

Copy link
Member Author

commented May 5, 2017

memcpy(buf, u, len);
return len;
}
CHECK_NE(u, nullptr);

This comment has been minimized.

Copy link
@sam-github

sam-github May 5, 2017

Member

consider bringing back the conditional... EDIT: ... because it simplifies code, see below.

This comment has been minimized.

Copy link
@indutny
@@ -482,12 +485,13 @@ void SecureContext::SetKey(const FunctionCallbackInfo<Value>& args) {
if (!bio)
return;

node::Utf8Value passphrase(env->isolate(), args[1]);

auto callback = has_password ? PasswordCallback : NoPasswordCallback;

This comment has been minimized.

Copy link
@sam-github

sam-github May 5, 2017

Member

... because it really simplifies this code.

This function uses the password cb because it may have a password.

The other examples below that use NoPasswordCallback I agree are more clear because the usage makes it really clear that there is no password, but I think here it is unnecessarily complex, caused only because the PasswordCallback stopped supporting an empty password.

This comment has been minimized.

Copy link
@indutny

indutny May 5, 2017

Member

Ah, I see. 👍

This comment has been minimized.

Copy link
@sam-github

sam-github May 5, 2017

Member

Sorry, for the unnecessarily obscure comments.

This comment has been minimized.

Copy link
@danbev

danbev May 6, 2017

Author Member

Fair point. Let me restore the original allowing a null passphrase but keep the NoPasswordCallback for this.

@@ -243,6 +241,13 @@ static int PasswordCallback(char *buf, int size, int rwflag, void *u) {
}


// This callback is used to avoid the default passphrase callback in OpenSSL
// which will typically prompt for the passphrase.

This comment has been minimized.

Copy link
@sam-github

sam-github May 6, 2017

Member

"the prompting is designed for the openssl CLI, but works poorly for Node.js because it involves synchronous interaction with the controlling terminal, something we never want, and use this function to avoid." <--- Maybe add something like that?

This comment has been minimized.

Copy link
@danbev

danbev May 6, 2017

Author Member

I've added that now, thanks!

@addaleax
Copy link
Member

left a comment

Just reaffirming my LGTM for the latest changes

@danbev

This comment has been minimized.

Copy link
Member Author

commented May 9, 2017

@indutny Would you mind taking a look and see what you think now? Your requested changes have been added (really only became the extra line as the other was reverted following the suggestions by sam-github). Thanks

@danbev

This comment has been minimized.

Copy link
Member Author

commented May 12, 2017

@indutny Sorry about nagging, just wanted to ask again if you have anything against this landing?

addressed

@addaleax

This comment has been minimized.

Copy link
Member

commented May 18, 2017

Landed in 29d89c9

@addaleax addaleax closed this May 18, 2017

addaleax added a commit that referenced this pull request May 18, 2017
src: split CryptoPemCallback into two functions
Currently the function CryptoPemCallback is used for two things:
1. As a passphrase callback.
2. To avoid the default OpenSSL passphrase routine.

The default OpenSSL passphase routine would apply if both
the callback and the passphrase are null pointers and the typical
behaviour is to prompt for the passphase which is not appropriate in
node.

This commit suggests that the PasswordCallback function only handle
passphrases, and that an additional function named NoPasswordCallback
used for the second case to avoid OpenSSL's passphase routine.

PR-URL: #12827
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@danbev

This comment has been minimized.

Copy link
Member Author

commented May 19, 2017

@addaleax Thanks!

anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
src: split CryptoPemCallback into two functions
Currently the function CryptoPemCallback is used for two things:
1. As a passphrase callback.
2. To avoid the default OpenSSL passphrase routine.

The default OpenSSL passphase routine would apply if both
the callback and the passphrase are null pointers and the typical
behaviour is to prompt for the passphase which is not appropriate in
node.

This commit suggests that the PasswordCallback function only handle
passphrases, and that an additional function named NoPasswordCallback
used for the second case to avoid OpenSSL's passphase routine.

PR-URL: nodejs#12827
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@jasnell jasnell referenced this pull request May 28, 2017
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete

@danbev danbev deleted the danbev:crypto-password-callback-refactor branch Jun 28, 2017

MylesBorins added a commit that referenced this pull request Jul 17, 2017
src: split CryptoPemCallback into two functions
Currently the function CryptoPemCallback is used for two things:
1. As a passphrase callback.
2. To avoid the default OpenSSL passphrase routine.

The default OpenSSL passphase routine would apply if both
the callback and the passphrase are null pointers and the typical
behaviour is to prompt for the passphase which is not appropriate in
node.

This commit suggests that the PasswordCallback function only handle
passphrases, and that an additional function named NoPasswordCallback
used for the second case to avoid OpenSSL's passphase routine.

PR-URL: #12827
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.