-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
tls: update v0.10 and v0.12 root certificates? #3952
Comments
+1 for updating. This is a security-related fix, and the only setups this is going to break are those that are dangerously misconfigured. Also, if there are any problems on deployments, in most cases they would notice this during testing (before updating the actual deployment machine). |
cc also @nodejs/lts |
+1 |
IMHO such hardcoded will always hurt sooner or later. So IMHO using it as fallback might be ok, but first it should try to use the CA certs form the OS alias OpenSSL lib's default directory (optionally from $SSL_CERTS_DIR). E.g.: http://iws.cs.uni-magdeburg.de/~elkner/tmp/node5/os-ca-certs.patch |
no objections from me as long as we leave it till after this upcoming series of releases just so as to reduce the impact on stability with so much else going on, so ~2 weeks from now at least? |
@bnoordhuis Yeah, sorry, I don't use windows (IIRC Win95 was the last one, I used regularly)/don't know it anymore, so can't help here :(. |
This includes deprecation SHA-1 root certs. I've tested 0.12 and 0.10 that alt cert chain in openssl-1.0.1 working fine and we can update them. +1 |
Probably, but without testing these changes, it's actually difficult to know. Last time we did this in a v0.10.x version, there were issues with AWS' servers certificates. These servers had a certificate chain that exposed a limitation of the OpenSSL version we were using. In that case, even though one could argue their certificate chain was not necessarily very academic, we couldn't tell people to stop connecting to AWS, and we couldn't ask AWS to change their certificates' chains. Even though there's now a test that checks that a node client can connect to AWS, we still don't know what the impact of that upgrade would be because we don't have a tests suite we can run. So this change might break users for no good reason, it might not, but we don't know.
This assumes that users run systems with tests that cover all their use cases, but that is not true in practice. Also, we should have the goal to be able to test our own changes, and not rely on users to do that for us. A while ago I suggested that we should have a tests suite for changes to trusted root certificates, and that we could maybe borrow from Mozilla's tests suite. I still think we would benefit from having such a tests suite. Unfortunately I don't have the time to work on that right now, but if anyone has any time to do that, I think it would be worth exploring. |
@ChALkeR @bnoordhuis Let me say this beforehand, I am a noob in crypto/security. |
@ojss Not the built-in root certificates, not without recompiling. |
I like @misterdjules' suggestion of a test suite like Mozilla's to test these. We have this already about go in to the next Stable (v5.x) and it'll work its way back in to v4.x then v0.12 and v0.10. By the time it's in these older releases it should have plenty of exposure out there in the wild. It would be nice to have a way of testing but in lieu of that we have real users on Stable with a slightly lesser guarantee of things going wrong than on LTS so I imagine we'll be hearing about breakage sooner rather than later. |
Looks like this one was done but it may be time to update again (cc @bnoordhuis). Closing this issue. |
FWIW: Referred patch above updated to 6.x: http://iks.cs.ovgu.de/~elkner/tmp/node6/os-ca-certs.patch |
The root certificates were last updated in the v0.10 and v0.12 branches on December 4, 2014.
Do we want to upgrade them? There is a non-zero risk of breaking existing deployments. On the other hand, outdated root certificates are a potential security liability.
/cc @nodejs/crypto
The text was updated successfully, but these errors were encountered: