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

tls: update v0.10 and v0.12 root certificates? #3952

Closed
bnoordhuis opened this issue Nov 21, 2015 · 14 comments
Closed

tls: update v0.10 and v0.12 root certificates? #3952

bnoordhuis opened this issue Nov 21, 2015 · 14 comments
Labels
crypto Issues and PRs related to the crypto subsystem. tls Issues and PRs related to the tls subsystem.

Comments

@bnoordhuis
Copy link
Member

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

@bnoordhuis bnoordhuis added tls Issues and PRs related to the tls subsystem. crypto Issues and PRs related to the crypto subsystem. labels Nov 21, 2015
@ChALkeR
Copy link
Member

ChALkeR commented Nov 21, 2015

+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).

@Fishrock123
Copy link
Contributor

cc also @nodejs/lts

@jasnell
Copy link
Member

jasnell commented Nov 21, 2015

+1

@jelmd
Copy link

jelmd commented Nov 22, 2015

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

@rvagg
Copy link
Member

rvagg commented Nov 22, 2015

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
Copy link
Member Author

@rvagg Sounds reasonable. I've put a note on my calendar to revisit in the week of December 6.

@jelmd Old discussion. The reason node works the way it does is for consistency across platforms.

@jelmd
Copy link

jelmd commented Nov 22, 2015

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

@shigeki
Copy link
Contributor

shigeki commented Nov 23, 2015

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

@misterdjules
Copy link

+1 for updating. This is a security-related fix, and the only setups this is going to break are those that are dangerously misconfigured.

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.

Also, if there are any problems on deployments, in most cases they would notice this during testing (before updating the actual deployment machine).

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.

@ojss
Copy link
Contributor

ojss commented Nov 28, 2015

@ChALkeR @bnoordhuis Let me say this beforehand, I am a noob in crypto/security.
But isn't it possible for the users/deployers to modify and/or upgrade the root certificate store themselves? Am I wrong in thinking like this?

@bnoordhuis
Copy link
Member Author

@ojss Not the built-in root certificates, not without recompiling.

@rvagg
Copy link
Member

rvagg commented Dec 5, 2015

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.

@jasnell
Copy link
Member

jasnell commented May 17, 2016

Looks like this one was done but it may be time to update again (cc @bnoordhuis). Closing this issue.

@jasnell jasnell closed this as completed May 17, 2016
@jelmd
Copy link

jelmd commented Aug 3, 2016

FWIW: Referred patch above updated to 6.x: http://iks.cs.ovgu.de/~elkner/tmp/node6/os-ca-certs.patch

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

No branches or pull requests

9 participants