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

crypto: update root certificates #1833

Merged
merged 4 commits into from
Jun 2, 2015

Conversation

bnoordhuis
Copy link
Member

@mscdex mscdex added the crypto Issues and PRs related to the crypto subsystem. label May 29, 2015
@indutny
Copy link
Member

indutny commented May 31, 2015

@shigeki
Copy link
Contributor

shigeki commented Jun 2, 2015

opt_t and opt_s in mk-ca-bundle.pl are not necessary because they are used for the output of ca-bundle.crt.

$ ./mk-ca-bundle.pl -t
unable to load certificate
140323113272992:error:0906D06C:PEM routines:PEM_read_bio:no start line:pem_lib.c:703:Expecting: TRUSTED CERTIFICATE
Couldn't close openssl pipe:  at ./mk-ca-bundle.pl line 290, <TXT> line 195.

Otherwize, make test-internet is fine and I also made a test of tls connection to s3.amazonaws.com and it is no problem. LGTM.

I will remove test/internet/test-tls-connnect-melissadata.js that was created in d8c4a93 after updated.

As for this updating root certs, what shoudl we do for CNNNIC whitelist?
https://blog.mozilla.org/security/2015/04/02/distrusting-new-cnnic-certificates/

Firefox and Chrome obtain serials of issued certs and checking them during cert verifying.
Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1151512
Chrome: https://codereview.chromium.org/1042973002 , https://codereview.chromium.org/1067723002

I incline not to do it because

  • iojs is not a browser.
  • We can only maintain its white list by looking at Firefox/Chrome repository.
  • I worry about performance overhead for checking CNNNIC rootCA serials at every TLS handshake.

@bnoordhuis
Copy link
Member Author

@indutny Don't know how I ended up pasting a bad link... the right one is https://hg.mozilla.org/mozilla-central/raw-file/aa275ad846f1/security/nss/lib/ckfw/builtins/certdata.txt - I'll update the commit log before I land this.

@shigeki

opt_t and opt_s in mk-ca-bundle.pl are not necessary

I tried to keep the delta with upstream as small as possible, to make future updates easier, that's why I left in some cruft. Basically all I did was remove the auto-downloading code.

what shoudl we do for CNNNIC whitelist?

I personally wouldn't mind the whitelisting approach. I don't think the overhead is going to be terrible when it's implemented as a simple binary search over a static array.

The only issue I see is tracking whitelist updates. Either we do it manually from time to time like we do for the root certificates or it has to be scripted into the release process somehow.

Remove unneeded functionality and tweak the generated output so we
can #include it in C++ source code.

This commit essentially reapplies the changes from commit e159073
("tools: customize mk-ca-bundle.pl") to the updated script.

PR-URL: nodejs#1833
Reviewed-By: Shigeki Ohtsu <[email protected]>
This is the latest certdata.txt from [0], last updated on 2015-04-20.

[0] https://hg.mozilla.org/mozilla-central/raw-file/aa275ad846f1/security/nss/lib/ckfw/builtins/certdata.txt

PR-URL: nodejs#1833
Reviewed-By: Shigeki Ohtsu <[email protected]>
Update the list of root certificates in src/node_root_certs.h with
tools/mk-ca-bundle.pl.

PR-URL: nodejs#1833
Reviewed-By: Shigeki Ohtsu <[email protected]>
@bnoordhuis bnoordhuis force-pushed the update-root-certs branch from acc2040 to a4dbf45 Compare June 2, 2015 17:34
@bnoordhuis bnoordhuis closed this Jun 2, 2015
@bnoordhuis bnoordhuis merged commit a4dbf45 into nodejs:master Jun 2, 2015
@bnoordhuis bnoordhuis deleted the update-root-certs branch June 2, 2015 17:34
andrewdeandrade pushed a commit to andrewdeandrade/node that referenced this pull request Jun 3, 2015
andrewdeandrade pushed a commit to andrewdeandrade/node that referenced this pull request Jun 3, 2015
Remove unneeded functionality and tweak the generated output so we
can #include it in C++ source code.

This commit essentially reapplies the changes from commit e159073
("tools: customize mk-ca-bundle.pl") to the updated script.

PR-URL: nodejs/node#1833
Reviewed-By: Shigeki Ohtsu <[email protected]>
andrewdeandrade pushed a commit to andrewdeandrade/node that referenced this pull request Jun 3, 2015
This is the latest certdata.txt from [0], last updated on 2015-04-20.

[0] https://hg.mozilla.org/mozilla-central/raw-file/aa275ad846f1/security/nss/lib/ckfw/builtins/certdata.txt

PR-URL: nodejs/node#1833
Reviewed-By: Shigeki Ohtsu <[email protected]>
andrewdeandrade pushed a commit to andrewdeandrade/node that referenced this pull request Jun 3, 2015
Update the list of root certificates in src/node_root_certs.h with
tools/mk-ca-bundle.pl.

PR-URL: nodejs/node#1833
Reviewed-By: Shigeki Ohtsu <[email protected]>
@rvagg rvagg mentioned this pull request Jun 11, 2015
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants