crypto,tls: remove SSLv2 support #5529
Conversation
bnoordhuis
added
tls
|
lgtm pending CI outcome, thanks for this @bnoordhuis |
|
Second try, accounting for Windows line endings this time: https://ci.nodejs.org/job/node-test-pull-request/1815/ |
| @@ -2675,8 +2674,8 @@ static void ParseArgs(int argc, char **argv) { | |||
| argv[i] = const_cast<char*>(""); | |||
| #if HAVE_OPENSSL | |||
| } else if (strcmp(arg, "--enable-ssl2") == 0) { | |||
| SSL2_ENABLE = true; | |||
| argv[i] = const_cast<char*>(""); | |||
| fprintf(stderr, "Error: --enable-ssl2 is no longer supported.\n"); | |||
There was a problem hiding this comment.
Maybe print CVE # here?
|
CI with CVE added to error message: https://ci.nodejs.org/job/node-test-pull-request/1818/ |
Remove support for SSLv2 because of DROWN (CVE-2016-0800). Use of the `--enable-ssl2` flag is now an error; node will print an error message and exit. Fixes: nodejs/Release#80 PR-URL: nodejs#5529 Reviewed-By: Rod Vagg <rod@vagg.org>
|
This solution LGTM but I would rather more people sign off on the code itself if possible. (Since I would rather not sign-off on c++ yet) |
bnoordhuis
force-pushed
the
v0.10-remove-sslv2
branch
from
f23aecc to
f8cb0dc
Compare
|
@bnoordhuis can you copy this to v0.12? It should be the same there right? |
|
Working on it. It's not quite the same though because of our custom clienthello parser. |
|
I have to go to medical checkup today. I'll take a look at this after back. |
Constants and doc descriptions related to SSLv2 are no longer needed. Fixes: nodejs#5529
|
This LGTM but want to get @shigeki's feedback when he's able. |
|
ha! just realized it was already merged... ;-) |
Doc descriptions related to SSLv2 are no longer needed. Fixes: nodejs/node#5529 PR-URL: nodejs/node#5541 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
R=@jasnell? @shigeki?
CI: https://ci.nodejs.org/job/node-test-pull-request/1810/