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

[Converge] SSLv2/3 related commits #2514

Closed
rvagg opened this issue Aug 24, 2015 · 7 comments
Closed

[Converge] SSLv2/3 related commits #2514

rvagg opened this issue Aug 24, 2015 · 7 comments
Assignees
Labels
tls Issues and PRs related to the tls subsystem.
Milestone

Comments

@rvagg
Copy link
Member

rvagg commented Aug 24, 2015

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?

@rvagg rvagg added this to the 4.0.0 milestone Aug 24, 2015
@ChALkeR ChALkeR added tls Issues and PRs related to the tls subsystem. doc Issues and PRs related to the documentations. test Issues and PRs related to the tests. and removed doc Issues and PRs related to the documentations. test Issues and PRs related to the tests. labels Aug 24, 2015
@rvagg rvagg mentioned this issue Aug 24, 2015
10 tasks
@Fishrock123
Copy link
Contributor

@indutny / @shigeki could you look at the first two commits there?

@indutny
Copy link
Member

indutny commented Aug 24, 2015

I'm fine with these changes.

@shigeki
Copy link
Contributor

shigeki commented Aug 25, 2015

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.

@rvagg
Copy link
Member Author

rvagg commented Aug 25, 2015

@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

@shigeki
Copy link
Contributor

shigeki commented Aug 25, 2015

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

@shigeki shigeki closed this as completed Aug 25, 2015
@Fishrock123
Copy link
Contributor

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?

@shigeki
Copy link
Contributor

shigeki commented Aug 26, 2015

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

No branches or pull requests

5 participants