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: whitelist CNNIC certificates? #1871

Closed
bnoordhuis opened this issue Jun 2, 2015 · 6 comments
Closed

tls: whitelist CNNIC certificates? #1871

bnoordhuis opened this issue Jun 2, 2015 · 6 comments
Labels
tls Issues and PRs related to the tls subsystem.

Comments

@bnoordhuis
Copy link
Member

See comment here:

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

/cc @nodejs/crypto

@bnoordhuis bnoordhuis added the tls Issues and PRs related to the tls subsystem. label Jun 2, 2015
@bnoordhuis
Copy link
Member Author

Allow me to copy/paste my reply. :-)

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.

@shigeki
Copy link
Contributor

shigeki commented Jun 4, 2015

Mozilla has an independent file of whitelist in https://hg.mozilla.org/mozilla-central/raw-file/98820360ab66/security/certverifier/CNNICHashWhitelist.inc . We have to watch it periodically if we take this.

I made a patch in shigeki@1459820 for adding CNNIC whitelist check and tried tests for evaluating its performance degradation. Most of tls connection is to access to TLS servers that is not issued by CNNIC. I made tls connection benchmarks to a local server by using my cert issued by GlobalSign.

There seems about 2% performance drop in tls connection on my tests. But in general case, RTT gets more than that in the test so that the ratio would be more small.
Do we take it or not?

## No CNNIC WhiteList
ohtsu@ubuntu:~/github/io.js$ ./iojs benchmark/tls/iij-connect.js
tls/iij-connect.js concurrency=1 dur=5: 183.98879
tls/iij-connect.js concurrency=10 dur=5: 193.84971
ohtsu@ubuntu:~/github/io.js$ ./iojs benchmark/tls/iij-connect.js
tls/iij-connect.js concurrency=1 dur=5: 177.17927
tls/iij-connect.js concurrency=10 dur=5: 187.91020
ohtsu@ubuntu:~/github/io.js$ ./iojs benchmark/tls/iij-connect.js
tls/iij-connect.js concurrency=1 dur=5: 184.38805
tls/iij-connect.js concurrency=10 dur=5: 197.62056
ave: 181.85203
ave: 193.12682

## With CNNIC WhiteList
ohtsu@ubuntu:~/github/io.js$ ./iojs benchmark/tls/iij-connect.js
tls/iij-connect.js concurrency=1 dur=5: 170.58091
tls/iij-connect.js concurrency=10 dur=5: 188.01853
ohtsu@ubuntu:~/github/io.js$ ./iojs benchmark/tls/iij-connect.js
tls/iij-connect.js concurrency=1 dur=5: 184.03144
tls/iij-connect.js concurrency=10 dur=5: 189.79172
ohtsu@ubuntu:~/github/io.js$ ./iojs benchmark/tls/iij-connect.js
tls/iij-connect.js concurrency=1 dur=5: 178.79860
tls/iij-connect.js concurrency=10 dur=5: 189.40764
ave: 177.80365 (97.77%)
ave: 189.07263 (97.90%)

@bnoordhuis
Copy link
Member Author

Can you file it as a PR? I have a couple of comments but it's easier to do that on a PR.

@shigeki
Copy link
Contributor

shigeki commented Jun 4, 2015

Okay, I will do it right now.

@shigeki
Copy link
Contributor

shigeki commented Jun 4, 2015

submitted #1895

@brendanashworth
Copy link
Contributor

AFAIK this has been fixed by #1895 (merged as 3beb880).

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

3 participants