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

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

Closed
wants to merge 2 commits into from

Conversation

sam-github
Copy link
Contributor

@sam-github sam-github commented Nov 29, 2018

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 29, 2018
@Trott
Copy link
Member

Trott commented Nov 29, 2018


const {
assert, connect, keys
} = require(fixtures.path('tls-connect'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@addaleax addaleax added the tls Issues and PRs related to the tls subsystem. label Nov 30, 2018
@sam-github 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 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 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
Copy link
Contributor Author

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.

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

@sam-github
Copy link
Contributor Author

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

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

@Trott
Copy link
Member

Trott commented Dec 2, 2018

ca: client.ca,
requestCert: true,
},
}, function(err, pair, cleanup) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The common connect() function does that already.

@bnoordhuis
Copy link
Member

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.

@sam-github
Copy link
Contributor Author

@sam-github
Copy link
Contributor Author

@addaleax
Copy link
Member

addaleax commented Dec 4, 2018

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

@Trott
Copy link
Member

Trott commented Dec 4, 2018

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

@sam-github
Copy link
Contributor Author

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

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

@sam-github
Copy link
Contributor Author

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/

@sam-github
Copy link
Contributor Author

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

@sam-github
Copy link
Contributor Author

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/

@sam-github
Copy link
Contributor Author

Landed in 83ec33b...2e4a163

@sam-github sam-github closed this Dec 11, 2018
@sam-github sam-github deleted the test-client-auth branch December 11, 2018 23:29
sam-github added a commit that referenced this pull request Dec 11, 2018
TLS client authentication should be tested, including failure scenarios.

PR-URL: #24733
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
sam-github added a commit that referenced this pull request Dec 11, 2018
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
BethGriggs pushed a commit that referenced this pull request Dec 17, 2018
TLS client authentication should be tested, including failure scenarios.

PR-URL: #24733
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
BethGriggs pushed a commit that referenced this pull request Dec 17, 2018
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Dec 18, 2018
BethGriggs added a commit that referenced this pull request Dec 18, 2018
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
MylesBorins pushed a commit that referenced this pull request Dec 18, 2018
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
BethGriggs added a commit that referenced this pull request Dec 18, 2018
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
BethGriggs added a commit that referenced this pull request Dec 18, 2018
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
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
TLS client authentication should be tested, including failure scenarios.

PR-URL: nodejs#24733
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
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
BethGriggs pushed a commit that referenced this pull request Apr 17, 2019
TLS client authentication should be tested, including failure scenarios.

PR-URL: #24733
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
BethGriggs pushed a commit that referenced this pull request Apr 28, 2019
TLS client authentication should be tested, including failure scenarios.

PR-URL: #24733
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
sam-github added a commit to sam-github/node that referenced this pull request Apr 29, 2019
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@BethGriggs BethGriggs mentioned this pull request May 1, 2019
MylesBorins pushed a commit that referenced this pull request May 16, 2019
TLS client authentication should be tested, including failure scenarios.

PR-URL: #24733
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor PRs that contain new features and should be released in the next minor version. test Issues and PRs related to the tests. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants