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.

Sign up for GitHub

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

Jump to bottom

[v8.x backport] backport of tls-cnnic-whitelist fixes #20776

Closed
wants to merge 2 commits into from
Closed

[v8.x backport] backport of tls-cnnic-whitelist fixes #20776

wants to merge 2 commits into from

Commits on May 16, 2018

  1. test: set clientOpts.port property 

    Currently this test will overwrite the clientOpts object with the port,
instead of setting the port property on the clientOpts object which
looks like the original intent.

Doing this the test fails reporting that the fake-cnnic-root-cert has
expired. This is indeed true:
$ openssl x509 -in test/fixtures/keys/fake-cnnic-root-cert.pem \
-text -noout
Certificate:
        ...
        Validity
            Not Before: Jun  9 17:15:16 2015 GMT
            Not After : Mar 29 17:15:16 2018 GMT

This commit sets the errorCode to CERT_HAS_EXPIRED. I tried updating the
certificate using test/fixtures/keys/Makefile but then no error is
thrown and I'm currently looking into this.

PR-URL: nodejs#19767
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
    @danbev
    danbev committed May 16, 2018
    Configuration menu
    Copy the full SHA
    2e1544b View commit details
    Browse the repository at this point in the history

  2. test: remove test case 0 from tls-cnnic-whitelist 

    I looks like this test has not worked as expected since commit
2bc7841 ("test: use random ports
where possible"). The test in that commit checked for `CERT_REVOKED`
which was returned by CheckWhitelistedServerCert.

CheckWhitelistedServerCert was later removed in commit
6ee4228 ("src: drop CNNIC+StartCom
certificate whitelisting").

I'm suggesting that this test case be removed as I don't think it is
valid anymore.

PR-URL: nodejs#19767
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
    @danbev
    danbev committed May 16, 2018
    Configuration menu
    Copy the full SHA
    8766431 View commit details
    Browse the repository at this point in the history