test/document TLS authentication, then add support for TRUSTED CERTIFICATE pem headers by sam-github · Pull Request #24733 · nodejs/node · GitHub

TLS client authentication should be tested, including failure scenarios.

There were tests for a couple success scenarios as side-effects of testing other things, but little coverage for the various ways intermediate and root CAs can be configured.

Support the same PEM certificate formats for the ca: option to
tls.createSecureContext() that are supported by openssl when loading a
CAfile.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

@sam-github build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1733/pipeline

CI: https://ci.nodejs.org/job/node-test-pull-request/19066/

Should this module maybe rather be in test/common/?

I don't know. It predates test/common, AFAICT, but perhaps it should have been moved there when test/common was created? Moving it would be a good code-n-learn activity.

While I was doing some support yesterday for people banging their head against node's tls, I found that node wasn't working with "TRUSTED CERATIFICATE", and @bnoordhuis pointed me towards the root cause.

I pushed the fix onto this PR so I can use the tests to prove it did not used to work, and now it does.

node/deps/openssl/openssl/crypto/x509/by_file.c

Line 93 in 01c5c16

x = PEM_read_bio_X509_AUX(in, NULL, NULL, "");

is what openssl's -CAfile option eventually leads to, so I believe calling PEM_read_bio_X509_AUX() is the right thing for Node.js to do as well.

@addaleax @bnoordhuis PTAL

Travis failed to find the first commit, as it often does.

ci: https://ci.nodejs.org/job/node-test-pull-request/19097/

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/19129/

Wrap in common.mustCall(...) here and below?

The common connect() function does that already.

The linux failures look unrelated but the windows failures do. As to why the connection is being established when it shouldn't, I have no idea.

ci: https://ci.nodejs.org/job/node-test-pull-request/19141/

ci: https://ci.nodejs.org/job/node-test-pull-request/19168/

Resume CI (scheduled): https://ci.nodejs.org/job/node-test-pull-request/19190/

Windows failures on CI are relevant:

https://ci.nodejs.org/job/node-test-binary-windows/22179/COMPILED_BY=vs2017,RUNNER=win10,RUN_SUBSET=3/console

03:11:38 not ok 456 parallel/test-tls-client-auth
03:11:38   ---
03:11:38   duration_ms: 0.239
03:11:38   severity: fail
03:11:38   exitcode: 1
03:11:38   stack: |-
03:11:38     c:\workspace\node-test-binary-windows\test\parallel\test-tls-client-auth.js:152
03:11:38       assert.strictEqual(err.code, 'UNABLE_TO_VERIFY_LEAF_SIGNATURE');
03:11:38                              ^
03:11:38     
03:11:38     TypeError: Cannot read property 'code' of undefined
03:11:38         at c:\workspace\node-test-binary-windows\test\parallel\test-tls-client-auth.js:152:26
03:11:38         at c:\workspace\node-test-binary-windows\test\common\index.js:346:15
03:11:38         at maybeCallback (c:\workspace\node-test-binary-windows\test\fixtures\tls-connect.js:97:7)
03:11:38         at TLSSocket.<anonymous> (c:\workspace\node-test-binary-windows\test\fixtures\tls-connect.js:68:13)
03:11:38         at TLSSocket.emit (events.js:189:13)
03:11:38         at TLSSocket.onConnectSecure (_tls_wrap.js:1168:10)
03:11:38         at TLSSocket.emit (events.js:189:13)
03:11:38         at TLSSocket._finishInit (_tls_wrap.js:629:8)
03:11:38   ...

Ah, Windows CRLF... I do not love thee.

ci: https://ci.nodejs.org/job/node-test-pull-request/19314/

linux failures: https://ci.nodejs.org/job/node-test-commit-linux/23842/

Look like exit code test failures, not related to TLS.

re-ci: https://ci.nodejs.org/job/node-test-commit-linux/23845/

more aix & linux exit code and CLI syntax failures, retry: https://ci.nodejs.org/job/node-test-commit/24172/

The only failures were test-cli-syntax.js, which is flaky. I'm re-running ci, but I think this is landable.

re-ci: https://ci.nodejs.org/job/node-test-commit/24210/

Landed in 83ec33b...2e4a163

TLS client authentication should be tested, including failure scenarios. PR-URL: #24733 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Support the same PEM certificate formats for the ca: option to tls.createSecureContext() that are supported by openssl when loading a CAfile. Fixes: #24761 PR-URL: #24733 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

TLS client authentication should be tested, including failure scenarios. PR-URL: #24733 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Support the same PEM certificate formats for the ca: option to tls.createSecureContext() that are supported by openssl when loading a CAfile. Fixes: #24761 PR-URL: #24733 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Notable changes: * **test**: * test TLS client authentication (Sam Roberts) [#24733](#24733) * **tls**: * support "BEGIN TRUSTED CERTIFICATE" for ca: (Sam Roberts) [#24733](#24733) * **tools**: * update ESLint to 5.10.0 (cjihrig) [#24903](#24903) * **util**: * add inspection getter option (Ruben Bridgewater) [#24852](#24852) PR-URL: #25102

Notable changes: * **tls**: * support "BEGIN TRUSTED CERTIFICATE" for ca: (Sam Roberts) [#24733](#24733) * **util**: * add inspection getter option (Ruben Bridgewater) [#24852](#24852) PR-URL: #25102

TLS client authentication should be tested, including failure scenarios. PR-URL: nodejs#24733 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Support the same PEM certificate formats for the ca: option to tls.createSecureContext() that are supported by openssl when loading a CAfile. Fixes: nodejs#24761 PR-URL: nodejs#24733 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Notable changes: * **tls**: * support "BEGIN TRUSTED CERTIFICATE" for ca: (Sam Roberts) [nodejs#24733](nodejs#24733) * **util**: * add inspection getter option (Ruben Bridgewater) [nodejs#24852](nodejs#24852) PR-URL: nodejs#25102

TLS client authentication should be tested, including failure scenarios. PR-URL: #24733 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Support the same PEM certificate formats for the ca: option to tls.createSecureContext() that are supported by openssl when loading a CAfile. Fixes: nodejs#24761 PR-URL: nodejs#24733 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

TLS client authentication should be tested, including failure scenarios. PR-URL: #24733 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 29, 2018

addaleax approved these changes Nov 30, 2018

View reviewed changes

addaleax added the tls Issues and PRs related to the tls subsystem. label Nov 30, 2018

sam-github force-pushed the test-client-auth branch from 479d168 to 2934a3d Compare November 30, 2018 19:33

sam-github added the semver-minor PRs that contain new features and should be released in the next minor version. label Nov 30, 2018

sam-github mentioned this pull request Nov 30, 2018

tls ca: option doesn't support the same formats as openssl -CAfile #24761

Closed

sam-github force-pushed the test-client-auth branch from 2934a3d to 4b48cf9 Compare November 30, 2018 19:40

sam-github changed the title test: test TLS client authentication test/document TLS authentication, then add support for TRUSTED CERTIFICATE pem headers Nov 30, 2018

sam-github force-pushed the test-client-auth branch 2 times, most recently from 3674751 to 61380b3 Compare November 30, 2018 19:48

sam-github requested a review from bnoordhuis November 30, 2018 20:05

bnoordhuis approved these changes Dec 2, 2018

View reviewed changes

sam-github force-pushed the test-client-auth branch from 61380b3 to a75dd8e Compare December 3, 2018 19:54

This was referenced Dec 5, 2018

CI failures: 20181201-20181205 nodejs/reliability#19

Closed

addon build failures on Windows machines nodejs/build#1622

Closed

sam-github force-pushed the test-client-auth branch 2 times, most recently from f838013 to 62e0408 Compare December 7, 2018 21:06

sam-github closed this Dec 11, 2018

sam-github deleted the test-client-auth branch December 11, 2018 23:29

BethGriggs mentioned this pull request Dec 18, 2018

V11.5.0 proposal #25102

Merged

beevelop mentioned this pull request Dec 18, 2018

2018-12-18 Version 11.5.0 (Current) @BethGriggs beevelop/docker-android-nodejs#272

Closed

BethGriggs mentioned this pull request May 1, 2019

v10.16.0 proposal #27514

Merged

BethGriggs added the land-on-v10.x label Sep 3, 2019

test/document TLS authentication, then add support for TRUSTED CERTIFICATE pem headers by sam-github · Pull Request #24733 · nodejs/node · GitHub

Uh oh!

Uh oh!

test/document TLS authentication, then add support for TRUSTED CERTIFICATE pem headers - #24733

test/document TLS authentication, then add support for TRUSTED CERTIFICATE pem headers#24733sam-github wants to merge 2 commits intonodejs:masternodejs/node:masterfrom sam-github:test-client-authsam-github/node:test-client-authCopy head branch name to clipboard

Conversation

sam-github commented Nov 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Nov 29, 2018

Uh oh!

Trott commented Nov 29, 2018

Uh oh!

addaleax Nov 30, 2018

Choose a reason for hiding this comment

Uh oh!

sam-github Nov 30, 2018

Choose a reason for hiding this comment

Uh oh!

sam-github commented Nov 30, 2018

Uh oh!

sam-github commented Nov 30, 2018

Uh oh!

Trott commented Dec 2, 2018

Uh oh!

bnoordhuis Dec 2, 2018

Choose a reason for hiding this comment

Uh oh!

sam-github Dec 3, 2018

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Dec 2, 2018

Uh oh!

sam-github commented Dec 3, 2018

Uh oh!

sam-github commented Dec 3, 2018

Uh oh!

addaleax commented Dec 4, 2018

Uh oh!

Trott commented Dec 4, 2018

Uh oh!

sam-github commented Dec 7, 2018

Uh oh!

sam-github commented Dec 7, 2018

Uh oh!

sam-github commented Dec 10, 2018

Uh oh!

sam-github commented Dec 11, 2018

Uh oh!

sam-github commented Dec 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

test/document TLS authentication, then add support for TRUSTED CERTIFICATE pem headers#24733
sam-github wants to merge 2 commits into
nodejs:masterfrom
sam-github:test-client-auth

sam-github commented Nov 29, 2018 •
edited
Loading