[Converge] SSLv2/3 related commits
rvagg opened this issue · 7 comments
Continuing from nodejs/node-convergence-archive#20, marking against 4.0.0 milestone. Summary of that discussion is that SSLv2 and SSLv3 will not be reintroduced to v4, but there are some outstanding items raised by @misterdjules:
If I remember correctly, nodejs/node-v0.x-archive@69080f5 is a bug fix that is independent of SSLv2/SSLv3 support, and I would think we'd want to keep it.
nodejs/node-v0.x-archive@8d045a3 (and subsequent changes to the tests that it adds) has proven to be very valuable to catch issues when upgrading OpenSSL. It's actually what helped us find out about the problen that nodejs/node-v0.x-archive@69080f5 fixes, and others. So I would suggest to keep that too.
nodejs/node-v0.x-archive@d230fa9 fixes a minor but still real issue with documentation. I would also keep that.
@jasnell can you look back through that thread to see if there are any other commits missing from this list that should be included?
I'm fine with these changes.
nodejs/node-v0.x-archive@69080f5 is the fix only for node-v0.10 and is no longer needed for 0.12 and 4.0. nodejs/node-v0.x-archive@8d045a3 has large test cases, give me sometime for check them.
@shigeki I'm assigning this to you, can you make sure those land in master by the end of this week please and then close this issue
@rvagg Okay, no problem.
Checking all test cases in nodejs/node-v0.x-archive@8d045a3, I think this is not necessary to be merged into 4.0 because 4.0 has no --enable-ssl2/ssl3 command line options and configurations of SSL options/methods for SSLv2/v3 are explicitly disabled so that the remaining test cases are only for TLSv1_methods.
The doc fix of nodejs/node-v0.x-archive@d230fa9 is for the description that was introduced only in joyent/node as nodejs/node-v0.x-archive@1349b68 to allow SSLv2/v3. This description is not included in 4.0 so it's no longer is needed neither.
so that the remaining test cases are only for TLSv1_methods.
Are we sure those aren't useful? @misterdjules said they were in finding regressions, unless we already test all of that stuff?
We already have test/parallel/test-tls-no-sslv23.js and test/parallel/test-tls-no-sslv3.js that can check regressions. They are not included in joyent/node.