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

Doc and test cert apis #10389

Merged
merged 6 commits into from
Jan 12, 2017
Merged

Conversation

sam-github
Copy link
Contributor

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 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
@sam-github
Copy link
Contributor Author

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

@mscdex 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
@sam-github
Copy link
Contributor Author

/cc @nodejs/crypto

@@ -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?
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, I'll investigate

Copy link
Member

@gibfahn gibfahn left a comment

Choose a reason for hiding this comment

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

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


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`
Copy link
Member

Choose a reason for hiding this comment

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

If the chain was requested

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

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.
Copy link
Member

Choose a reason for hiding this comment

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

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?

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

// of the connection.

const common = require('../common');
const fs = require('fs');
Copy link
Member

@gibfahn gibfahn Jan 1, 2017

Choose a reason for hiding this comment

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

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');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

@sam-github Why not submit a PR 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

// Test interaction of compiled-in CAs with user-provided CAs.
'use strict';

const common = require('../common');
Copy link
Member

Choose a reason for hiding this comment

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

Order (as above)

@sam-github
Copy link
Contributor Author

@gibfahn @jasnell addressed your comments, thanks, PTAL

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@sam-github sam-github mentioned this pull request Jan 4, 2017
2 tasks
@jasnell
Copy link
Member

jasnell commented Jan 6, 2017

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

Copy link
Member

@gibfahn gibfahn left a comment

Choose a reason for hiding this comment

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

Some more nits

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

// tls.createServer() and tls.connect(), so assertions can be made on both ends
// of the connection.

const common = require('../common');
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@sam-github sam-github Jan 9, 2017

Choose a reason for hiding this comment

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

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.

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
Copy link
Member

Choose a reason for hiding this comment

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

it's -> its

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
Copy link
Member

@gibfahn gibfahn Jan 6, 2017

Choose a reason for hiding this comment

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

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

any is confusing here (for me)

Copy link
Contributor Author

@sam-github sam-github Jan 9, 2017

Choose a reason for hiding this comment

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

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

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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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
Copy link
Contributor Author

@gibfahn PTAL

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
Copy link
Member

Choose a reason for hiding this comment

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

strings and Buffers -> strings and/or Buffers

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
Copy link
Member

Choose a reason for hiding this comment

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

specify->override

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.
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

sam-github added a commit to sam-github/node that referenced this pull request Apr 17, 2017
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]>
sam-github added a commit to sam-github/node that referenced this pull request Apr 17, 2017
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 added a commit to sam-github/node that referenced this pull request Apr 17, 2017
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 added a commit to sam-github/node that referenced this pull request Apr 17, 2017
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]>
sam-github added a commit to sam-github/node that referenced this pull request Apr 17, 2017
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]>
sam-github added a commit to sam-github/node that referenced this pull request Apr 17, 2017
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
Copy link
Contributor Author

sorry @MylesBorins , it did not, see #12468

MylesBorins pushed a commit that referenced this pull request Apr 17, 2017
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]>
MylesBorins pushed a commit that referenced this pull request Apr 17, 2017
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]>
MylesBorins pushed a commit that referenced this pull request Apr 17, 2017
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]>
MylesBorins pushed a commit that referenced this pull request Apr 17, 2017
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]>
MylesBorins pushed a commit that referenced this pull request Apr 17, 2017
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]>
MylesBorins pushed a commit that referenced this pull request Apr 17, 2017
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]>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
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]>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
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]>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
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]>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
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]>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
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]>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
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]>
@MylesBorins MylesBorins mentioned this pull request Apr 19, 2017
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
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]>
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
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]>
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
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]>
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
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]>
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
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]>
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants