Doc and test cert apis #10389

sam-github · 2016-12-21T18:00:39Z

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

Affected core subsystem(s)

doc,test,tls

Description of change

doc,test: tls .ca option supports multi-PEM files
test: tls cert chain completion scenarios
doc: use correct tls certificate property name
test: check tls server verification with addCACert
test: move common tls connect setup into fixtures

sam-github · 2016-12-21T18:42:30Z

That last, test: getgroups() may contain duplicate GIDs is technically unrelated, though I need it for make test to pass on my system, I could move it to another PR. @Trott

sam-github · 2016-12-30T18:17:31Z

/cc @nodejs/crypto

jasnell · 2016-12-30T18:21:43Z

doc/api/tls.md

@@ -621,7 +623,7 @@ For example:
 ```

 If the peer does not provide a certificate, `null` or an empty object will be
-returned.
+returned. XXX uh, which one is it?


Just marking this so this doesn't accidentally get merged without this being resolved.

good catch, I'll investigate

gibfahn

Mostly looks great. Couple of issues, and if you could add a comment explaining purpose to the tests that don't have them that'd be great (not required).

gibfahn · 2017-01-01T09:22:57Z

doc/api/tls.md


 Returns an object representing the peer's certificate. The returned object has
 some properties corresponding to the fields of the certificate.

+If the chain was requested, each certificate will include a `issuerCertificate`


If the chain was requested

Maybe use the detailed chain or the full certificate chain? It's already unambiguous, but this might be clearer.

gibfahn · 2017-01-01T09:30:01Z

doc/api/tls.md

+    chain must be signed by a trusted CA for the connection to be authenticated.
+    When using certificates that are not signed by a well-known CA, the
+    certificate's root CA or the self-signed certificate must be specified as a
+    trusted CA or the connection will fail to authenticate.


The peer's certificate chain must be signed by a trusted CA

the certificate's root CA or the self-signed certificate must be specified as a trusted CA

I don't really understand this, are you saying that one of these three things must be true? Aren't these sentences contradictory?

What's the difference between trusted and well-known? Do they mean the same thing?

the certificate's root CA

Should this be the certificate chain's root CA?

I attempted to clarify, by using less terminology, trying to use the terminology more consistently, and breaking out the self-signed clause as its own special case/sententce.

PTAL to see if your questions above are answered. I don't want to answer them inline in github, because if the documentation doesn't answer the questions, its not suceeding as documentation.

gibfahn · 2017-01-01T09:44:08Z

test/fixtures/tls-connect.js

+// of the connection.
+
+const common = require('../common');
+const fs = require('fs');


According to the test writing guide, the order should be:

use strict

require common

comment explaining test purpose (not required)

other requires.

1 'use strict'; 2 const common = require('../common'); 3 4 // This test ensures that the http-parser can handle UTF-8 characters 5 // in the http header. 6 7 const http = require('http'); 8 const assert = require('assert');

reorganized as above, except that I always sort require statements lexicographically, I'm a bit surprised the test guide doesn't.

@sam-github Why not submit a PR 😉

gibfahn · 2017-01-01T09:44:30Z

test/internet/test-tls-add-ca-cert.js

+// Test interaction of compiled-in CAs with user-provided CAs.
+'use strict';
+
+const common = require('../common');


Order (as above)

sam-github · 2017-01-03T21:06:24Z

@gibfahn @jasnell addressed your comments, thanks, PTAL

mhdawson

LGTM

jasnell · 2017-01-06T18:00:39Z

@gibfahn ... are you good with this now?

gibfahn

Some more nits

I don't know much about how this stuff works, so can provide new user feedback!

gibfahn · 2017-01-06T22:10:00Z

test/fixtures/tls-connect.js

+// tls.createServer() and tls.connect(), so assertions can be made on both ends
+// of the connection.
+
+const common = require('../common');


I think the order is still wrong here (also doesn't it need a 'use strict'?)

Order is consistent with test/fixtures/*.js, the folder its in, and also with lib/**/*.js

This is not a unit test, notice - and I'm not sure why unit tests don't put their descriptive comment as the first lines, seems more useful. Maybe existing practice?

I added a use strict, thanks.

gibfahn · 2017-01-06T22:11:59Z

doc/api/tls.md

+    authenticated.  When using certificates that are not chainable to a
+    well-known CA, the certificate's CA must be explicitly specified as a
+    trusted or the connection will fail to authenticate. For self-signed
+    certificates, the certificate is it's own CA, and must be explicitly


it's -> its

gibfahn · 2017-01-06T22:13:31Z

doc/api/tls.md

+    certificates. Default is to trust the well-known CAs curated by Mozilla.
+    Mozilla's CAs are completely replaced when CAs are explicitly specified
+    using this option.
+    Multiple CAs can be provided using an array, and any string or Buffer can


and any string or Buffer can contain -> or a string or Buffer containing

any is confusing here (for me)

"and" is intended, its not an "or". broke the sentence up into two.

gibfahn · 2017-01-06T22:16:06Z

doc/api/tls.md

+    using this option.
+    Multiple CAs can be provided using an array, and any string or Buffer can
+    contain multiple PEM CAs concatenated together. The peer's certificate must
+    be chainable to a CA trusted by the server for the connection to be


The peer's certificate must be chainable to a CA trusted by the server

Does a CA trusted by the server mean one of the CAs provided in this option? If so maybe:

to a CA trusted by the server -> to one of the provided CAs

@gibfahn Can you please re-read the first two sentences in the paragraph again? They define a trusted CA, and they define the defaults. Your suggested replacement ignores that there are defaults, it appears to me.

sam-github · 2017-01-09T20:50:47Z

@gibfahn PTAL

gibfahn · 2017-01-11T18:02:18Z

doc/api/tls.md

+    certificates. Default is to trust the well-known CAs curated by Mozilla.
+    Mozilla's CAs are completely replaced when CAs are explicitly specified
+    using this option.  The value can be a string or Buffer, or an Array of
+    strings and Buffers.  Any string or Buffer can contain multiple PEM CAs


strings and Buffers -> strings and/or Buffers

gibfahn · 2017-01-11T18:03:21Z

doc/api/tls.md

-    Default is the well-known CAs from Mozilla. When connecting to peers that
-    use certificates issued privately, or self-signed, the private root CA or
-    self-signed certificate must be provided to verify the peer.
+  * `ca` {string|string[]|Buffer|Buffer[]} Optionally specify the trusted CA


specify->override

gibfahn · 2017-01-11T18:17:49Z

doc/api/tls.md

+    certificates that are not chainable to a well-known CA, the certificate's CA
+    must be explicitly specified as a trusted or the connection will fail to
+    authenticate. For self-signed certificates, the certificate is its own CA,
+    and must be explicitly specified as trusted.


Suggestion:

When using certificates that are not chainable to a well-known CA, the certificate's CA must be explicitly specified as a trusted or the connection will fail to authenticate. For self-signed certificates, the certificate is its own CA, and must be explicitly specified as trusted.

If a connection uses a certificate that doesn't match or chain to one of the default CAs, you need to use the ca option to provide an alternate CA that the connection certificate can match or chain to. For self-signed certificates, the certificate is its own CA, and must be provided.

I don't love the wording I'm suggesting, but I think it's a little clearer.

@gibfahn PTAL, I replaced use of "you" (not allowed by our doc style), and also "connection certificate" with "peer certificate", because there are up to two certs in a connection, and the .ca option should be used for the peers cert, not the local cert.

Some systems may have multiple group names with the same group ID, in which case getgroups() returns duplicate values, where `id -G` will filter the duplicates. Unique and sort the arrays so they can be compared. PR-URL: nodejs#10389 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

PR-URL: nodejs#10389 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Docs referred to an `issuer` property being optionally present, when it should have referred to the `issuerCertificate` property. PR-URL: nodejs#10389 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

SecureContext.addCACert() adds to the existing root store, preserving root cert entries. option.ca is applied without calling SecureContext.addRootCerts() so should add to the default, empty, root store. This test confirms that the built-in root CAs are not included when options.ca is used. Based on: shigeki@acd5837 PR-URL: nodejs#10389 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

TLS connection setup boilerplate is common to many TLS tests, factor it into a test fixture so tests are clearer to read and faster to write. PR-URL: nodejs#10389 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

sam-github · 2017-04-17T15:40:00Z

sorry @MylesBorins , it did not, see #12468

Some systems may have multiple group names with the same group ID, in which case getgroups() returns duplicate values, where `id -G` will filter the duplicates. Unique and sort the arrays so they can be compared. Backport-PR-URL: #12468 PR-URL: #10389 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Backport-PR-URL: #12468 PR-URL: #10389 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Docs referred to an `issuer` property being optionally present, when it should have referred to the `issuerCertificate` property. Backport-PR-URL: #12468 PR-URL: #10389 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

SecureContext.addCACert() adds to the existing root store, preserving root cert entries. option.ca is applied without calling SecureContext.addRootCerts() so should add to the default, empty, root store. This test confirms that the built-in root CAs are not included when options.ca is used. Based on: shigeki@acd5837 Backport-PR-URL: #12468 PR-URL: #10389 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

TLS connection setup boilerplate is common to many TLS tests, factor it into a test fixture so tests are clearer to read and faster to write. Backport-PR-URL: #12468 PR-URL: #10389 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Some systems may have multiple group names with the same group ID, in which case getgroups() returns duplicate values, where `id -G` will filter the duplicates. Unique and sort the arrays so they can be compared. Backport-PR-URL: #12468 PR-URL: #10389 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Backport-PR-URL: #12468 PR-URL: #10389 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Docs referred to an `issuer` property being optionally present, when it should have referred to the `issuerCertificate` property. Backport-PR-URL: #12468 PR-URL: #10389 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

SecureContext.addCACert() adds to the existing root store, preserving root cert entries. option.ca is applied without calling SecureContext.addRootCerts() so should add to the default, empty, root store. This test confirms that the built-in root CAs are not included when options.ca is used. Based on: shigeki@acd5837 Backport-PR-URL: #12468 PR-URL: #10389 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

TLS connection setup boilerplate is common to many TLS tests, factor it into a test fixture so tests are clearer to read and faster to write. Backport-PR-URL: #12468 PR-URL: #10389 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Some systems may have multiple group names with the same group ID, in which case getgroups() returns duplicate values, where `id -G` will filter the duplicates. Unique and sort the arrays so they can be compared. Backport-PR-URL: nodejs/node#12468 PR-URL: nodejs/node#10389 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Backport-PR-URL: nodejs/node#12468 PR-URL: nodejs/node#10389 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Docs referred to an `issuer` property being optionally present, when it should have referred to the `issuerCertificate` property. Backport-PR-URL: nodejs/node#12468 PR-URL: nodejs/node#10389 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

SecureContext.addCACert() adds to the existing root store, preserving root cert entries. option.ca is applied without calling SecureContext.addRootCerts() so should add to the default, empty, root store. This test confirms that the built-in root CAs are not included when options.ca is used. Based on: shigeki/node@acd5837 Backport-PR-URL: nodejs/node#12468 PR-URL: nodejs/node#10389 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

TLS connection setup boilerplate is common to many TLS tests, factor it into a test fixture so tests are clearer to read and faster to write. Backport-PR-URL: nodejs/node#12468 PR-URL: nodejs/node#10389 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

sam-github force-pushed the doc-and-test-cert-apis branch 2 times, most recently from 2a6e9a6 to 95c5769 Compare December 21, 2016 18:40

mscdex mentioned this pull request Dec 21, 2016

Bot failed to label PR nodejs/github-bot#88

Closed

mscdex added doc Issues and PRs related to the documentations. test Issues and PRs related to the tests. tls Issues and PRs related to the tls subsystem. labels Dec 21, 2016

jasnell requested changes Dec 30, 2016

View reviewed changes

gibfahn suggested changes Jan 1, 2017

View reviewed changes

sam-github force-pushed the doc-and-test-cert-apis branch from 95c5769 to a70f9e0 Compare January 3, 2017 20:16

nodejs-github-bot added the dont-land-on-v7.x label Jan 3, 2017

sam-github force-pushed the doc-and-test-cert-apis branch from a70f9e0 to 8a9a532 Compare January 3, 2017 21:06

nodejs-github-bot added the dont-land-on-v7.x label Jan 3, 2017

sam-github removed the dont-land-on-v7.x label Jan 3, 2017

sam-github force-pushed the doc-and-test-cert-apis branch from 8a9a532 to 8506566 Compare January 3, 2017 21:46

nodejs-github-bot added the dont-land-on-v7.x label Jan 3, 2017

mhdawson approved these changes Jan 3, 2017

View reviewed changes

jasnell approved these changes Jan 3, 2017

View reviewed changes

sam-github mentioned this pull request Jan 4, 2017

Sort requires #10616

Merged

2 tasks

gibfahn suggested changes Jan 6, 2017

View reviewed changes

nodejs-github-bot added the dont-land-on-v7.x label Jan 9, 2017

gibfahn suggested changes Jan 11, 2017

View reviewed changes

sam-github force-pushed the doc-and-test-cert-apis branch from 6d3f850 to d62a8a0 Compare January 11, 2017 20:54

nodejs-github-bot added the dont-land-on-v7.x label Jan 11, 2017

sam-github force-pushed the doc-and-test-cert-apis branch from d62a8a0 to 1873c84 Compare January 11, 2017 20:54

nodejs-github-bot added the dont-land-on-v7.x label Jan 11, 2017

sam-github mentioned this pull request Apr 17, 2017

(v6.x backport) Doc and test cert apis #12468

Closed

MylesBorins added backported-to-v6.x and removed backport-requested-v6.x labels Apr 17, 2017

MylesBorins mentioned this pull request Apr 19, 2017

v6.10.3 proposal #12498

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Doc and test cert apis #10389

Doc and test cert apis #10389

sam-github commented Dec 21, 2016

sam-github commented Dec 21, 2016

sam-github commented Dec 30, 2016

jasnell Dec 30, 2016

sam-github Dec 30, 2016

gibfahn left a comment

gibfahn Jan 1, 2017

sam-github Jan 3, 2017

gibfahn Jan 1, 2017

sam-github Jan 3, 2017

gibfahn Jan 1, 2017 •

edited

Loading

sam-github Jan 3, 2017

gibfahn Jan 4, 2017

sam-github Jan 9, 2017

gibfahn Jan 1, 2017

sam-github commented Jan 3, 2017

mhdawson left a comment

jasnell commented Jan 6, 2017

gibfahn left a comment

gibfahn Jan 6, 2017

sam-github Jan 9, 2017 •

edited

Loading

gibfahn Jan 6, 2017

gibfahn Jan 6, 2017 •

edited

Loading

sam-github Jan 9, 2017 •

edited

Loading

gibfahn Jan 6, 2017

sam-github Jan 9, 2017

sam-github commented Jan 9, 2017

gibfahn Jan 11, 2017

gibfahn Jan 11, 2017

gibfahn Jan 11, 2017

sam-github Jan 11, 2017

sam-github commented Apr 17, 2017

Doc and test cert apis #10389

Doc and test cert apis #10389

Conversation

sam-github commented Dec 21, 2016

Checklist

Affected core subsystem(s)

Description of change

sam-github commented Dec 21, 2016

sam-github commented Dec 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibfahn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibfahn Jan 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-github commented Jan 3, 2017

mhdawson left a comment

Choose a reason for hiding this comment

jasnell commented Jan 6, 2017

gibfahn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-github Jan 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibfahn Jan 6, 2017 • edited Loading

Choose a reason for hiding this comment

sam-github Jan 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-github commented Jan 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-github commented Apr 17, 2017

gibfahn Jan 1, 2017 •

edited

Loading

sam-github Jan 9, 2017 •

edited

Loading

gibfahn Jan 6, 2017 •

edited

Loading

sam-github Jan 9, 2017 •

edited

Loading