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

Improvements to tls API documentation, batch 1 #9800

Closed
wants to merge 19 commits into from

Conversation

sam-github
Copy link
Contributor

@sam-github sam-github commented Nov 25, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc,tls

Description of change

Improvements to tls API documentation.

@nodejs-github-bot nodejs-github-bot added the tls Issues and PRs related to the tls subsystem. label Nov 25, 2016
@sam-github sam-github changed the title Improvements to Improvements to tls API documentation, batch 1 Nov 25, 2016
@sam-github
Copy link
Contributor Author

@nodejs/crypto @nodejs/documentation @silverwind @jasnell

@sam-github sam-github added doc Issues and PRs related to the documentations. test Issues and PRs related to the tests. labels Nov 25, 2016
<!-- YAML
added: v0.3.2
-->

* `options` {Object}
* `pfx` {string|Buffer} A string or `Buffer` containing the private key,
certificate and CA certs of the server in PFX or PKCS12 format. (Mutually
exclusive with the `key`, `cert`, and `ca` options.)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

My tests and reading of code show it not mutually exclusive with key/cert, btw, and it would be quite strange if it was mutually exclusive with ca. I am still extending test suite to confirm this.

@sam-github sam-github force-pushed the notes-tls branch 2 times, most recently from dc2d08f to a05d3e8 Compare November 25, 2016 20:51
Copy link
Contributor

@silverwind silverwind left a comment

Choose a reason for hiding this comment

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

Few notes regarding docs.

[`tls.TLSSocket`]s.

Consult
<https://www.openssl.org/docs/man1.0.2/apps/ciphers.html#CIPHER-LIST-FORMAT> for
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe use a square bracket link here and move the actual link to the bottom?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, maybe In a follow on commit, because this style of link to openssl docs is pervasive here and in crypto.md, I can change them all in one go.

cb();
});
server.on('resumeSession', (id, cb) => {
cb(null, tlsSessionStore[id.toString('hex')] || null);
cb(null, tlsSessionStore[id.tostring('hex')] || null);
Copy link
Contributor

Choose a reason for hiding this comment

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

A mistake?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes. I was staring and staring at this, but I didn't see the case change. Now I do.

* `ca`{string|string[]|Buffer|Buffer[]} Optional CA certificates to trust.
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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a word that it will replace default CAs?

Copy link
Contributor Author

@sam-github sam-github Nov 25, 2016

Choose a reason for hiding this comment

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

Isn't that how all default values work? The default is used when you don't specify the option. If you specify the option, the default isn't used.

I can spell it out if necessary, but the other Optional x. Default is y.
I think the old text used to be confusing, which is why some people thought it could possibly be additive.

Copy link
Contributor Author

@sam-github sam-github Nov 25, 2016

Choose a reason for hiding this comment

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

sorry, meant to write: but the other Optional x. Default is y. don't spell out that x replaces y if you specify it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, you have a point. It's probably okay your way.

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 do feel there is a missing section describing the basic authentication model of TLS, which will make all the buttons and dials more clear. Also, the examples are terrible, but I ran out of time. Maybe on the plane next week I'll rewrite them.

* `honorCipherOrder` {boolean} Attempt to use the server's cipher suite
preferences instead of the client's. When `true`, causes
`SSL_OP_CIPHER_SERVER_PREFERENCE` to be set in `secureOptions`, see
[OpenSSL Options][] for more information.
Copy link
Contributor

Choose a reason for hiding this comment

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

Defaults to true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did you read the Note below? It doesn't default to true if you do new tls.TLSSocket({isServer: true, ...}).

Copy link
Contributor Author

@sam-github sam-github Nov 25, 2016

Choose a reason for hiding this comment

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

cf.

this.honorCipherOrder = true;

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I follow. What influence does isServer have over honorCipherOrder? The linked code seems pretty clear that it is true unless user-provided.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See my comment two above. The linked code is in tls.createServer(), that code isn't run when calling new tls.TLSSocket() (server or client variant), and of course not when calling tls.connect(), not that it would matter in that case, its a server only option.

Clearly you are finding this confusing, though, and the docs shouldn't be confusing. Any suggestions for making it clear that its true by default when using tls.createServer(), but not when using new tls.TLSSocket({isServer: true, ...})? In other words, the common way of making server sockets had a default value, but the other supported and documented way of making them does not set it?

I thought I said that pretty explicitly.

Copy link
Contributor

@silverwind silverwind Nov 26, 2016

Choose a reason for hiding this comment

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

Thanks, I see it now. I'd argue that it should be the default for all servers, at least that was the intention when I made that change. Should be a pretty low-impact change, no?

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 agree its odd. In the process of documenting the existing behaviour, I found many odd and irregular aspects of the TLS API, but this PR documents TLS "as is". I definitely encourage you to PR a fix to move the setting of the default out of function Server() and into _tls_common where it can apply to all TLS sockets, and be based on the .isServer property. If you don't, I will. I consider completing the docs to be step 1 to cleaning up the API. In the absence of docs, its hard for reviewers to understand why behaviour is inconsistent and needs to be fixed.

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 have started a PR to make it default for all servers, FYI, @silverwind, but might not PR it until I'm back from node interactive. Btw, look for me if you are down here.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 to that, I'm not too familiar around tls, so I welcome you doing that change. It was certainly an oversight when we flipped that option.

Oh, and I'm unfortunately not attending NI.

[Perfect Forward Secrecy][]. Use `openssl dhparam` to create the parameters.
The key length must be greater than or equal to 1024 bits, otherwise an
error will be thrown. It is strongly recommended to use 2048 bits or larger
for stronger security. If omitted or invalid, the parameters are silently
Copy link
Contributor

Choose a reason for hiding this comment

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

adequate security?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sam-github
Copy link
Contributor Author

Thanks, I'll fix the stuff you found.

@mscdex
Copy link
Contributor

mscdex commented Nov 26, 2016

The number of commits seems a bit much, can't some of the common changes (e.g. 'describe' commits) be squashed together?

@sam-github
Copy link
Contributor Author

@mscdex of course I can squash before merge, each commit would become a bullet point. squashing is easy, unsquashing is hard, which is why its like this for now. @jasnell suggested a series of small commits would make it easier to review than a mega-rewrite.

@@ -1653,8 +1653,8 @@ the `crypto`, `tls`, and `https` modules and are generally specific to OpenSSL.
</tr>
<tr>
<td><code>SSL_OP_CIPHER_SERVER_PREFERENCE</code></td>
<td>Uses the server's preferences instead of the clients when selecting a
cipher. See
<td>Attempt to use the server's preferences instead of the client's when
Copy link
Member

Choose a reason for hiding this comment

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

To use consistent verb conjugation with (most of) the other entries in this table, it would seem this should be Attempts rather than Attempt. (There's also an Allow below that should be Allows.)

* `secureOptions` {number} Optionally affect the OpenSSL protocol behaviour,
which is not usually necessary. This should be used carefully if at all!
Value is a numeric bitmask of the `SSL_OP_*` options from
[OpenSSL Options][].
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This may replace #9340

@sam-github sam-github force-pushed the notes-tls branch 2 times, most recently from 7c21a24 to 3c1b2e5 Compare December 8, 2016 21:45
@sam-github
Copy link
Contributor Author

@nodejs/security, one of you care to review for accuracy?

@Trott I addressed your feedback, do you have time to review again?

@jasnell or @Trott, opinion on merging as series of commits, or @mscdex 's suggestion of squashing them, perhaps into one per test:/doc:/tls: subsystem?

@mscdex
Copy link
Contributor

mscdex commented Dec 8, 2016

@sam-github For review it's fine as-is, but I just wanted to make sure if/when this PR is landed, that the like-commits get squashed together and not landed as-is.

var cert = fs.readFileSync(path.join(common.fixturesDir, 'pass-cert.pem'));

assert(Buffer.isBuffer(passKey));
assert(Buffer.isBuffer(cert));
assert.equal(typeof passKey.toString(), 'string');
Copy link
Member

Choose a reason for hiding this comment

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

Nit: assert.strictEqual() instead of assert.equal() here and anywhere else it shows up

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 did it, though I don't understand why. The only difference is that now the test will fail if typeof doesn't return a string, or if a string literal is not a string, both of which would be crash the world bugs in V8.

var cert = fs.readFileSync(path.join(common.fixturesDir, 'pass-cert.pem'));

assert(Buffer.isBuffer(passKey));
assert(Buffer.isBuffer(cert));
assert.equal(typeof passKey.toString(), 'string');
Copy link
Member

Choose a reason for hiding this comment

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

Nit: assert.strictEqual() instead of assert.equal() here and anywhere else it shows up

rejectUnauthorized: false
}, common.mustCall(function() {}));
*/
})).unref();

assert.throws(function() {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: While you're in here anyway, can you add a second argument to this assert.throws(), probably a RegExp that confirms that the error is the expected one?

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

@@ -11,11 +11,17 @@ var tls = require('tls');
var fs = require('fs');
var path = require('path');

var key = fs.readFileSync(path.join(common.fixturesDir, 'pass-key.pem'));
var passKey = fs.readFileSync(path.join(common.fixturesDir, 'pass-key.pem'));
Copy link
Member

Choose a reason for hiding this comment

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

Nit: While in here anyway, maybe take an opportunity to var->const some stuff? Up to you.

@@ -1,4 +1,5 @@
'use strict';
var assert = require('assert');
Copy link
Member

Choose a reason for hiding this comment

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

Nit: var->const in this file too if you are so inclined (and not if you are not).

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 subject to squashing and nits as pointed out by Rich.

Either the options or the listener argument to tls.createServer() was
optional, but not both. This makes no sense, so align the argument
checking and documentation with net.createServer(), which accepts the
same option sequence, and which tls.createServer() is modelled on.
Its confusing to have multiple names for the same thing, use
secureOptions consistently.
- All the secure context options are valid options
  to a secureContext
- isServer modifies the default value of requestCert
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
PR-URL: #9800
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
Either the options or the listener argument to tls.createServer() was
optional, but not both. This makes no sense, so align the argument
checking and documentation with net.createServer(), which accepts the
same option sequence, and which tls.createServer() is modelled on.

PR-URL: #9800
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
PR-URL: #9800
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
Its confusing to have multiple names for the same thing, use
secureOptions consistently.

PR-URL: #9800
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
Document all TLSSocket options:

- All the secure context options are valid options
  to a secureContext
- isServer modifies the default value of requestCert

Describe all tls.connect() variants:

- tls.connect(path) was undocumented
- tls.connect(port) was underdocumented, and its relationship to
  tls.connect(options) was obscure

Socket passed to tls.connect is user managed:

- Replace #8996

Add documentation to:

- describe and add tests for the pfx and key variants, and describe how
  and when passphrase is used.
- describe tls cert and ca options
- describe buffer forms of tls crl option
- describe tls cipher option and defaults
- fix link to Crypto Constants
- describe that honorCipherOrder sets SSL_OP_CIPHER_SERVER_PREFERENCE.
- describe tls ecdhCurve/dhparam options
- describe tls secureProtocol option
- describe tls secureOptions
- describe tls sessionIdContext

De-deduplicate secure context docs:

The secure context options were documented 4 times, making it difficult
to understand where the options come from, where they are supported,
and under what conditions they are used.

The multiple copies were inconsistent and contradictory in their
descriptions of the options, and also inconsistent in whether the
options would be documented at all.

Cut through this gordian knot by linking all APIs that use the
secureContext options to the single source of truth about the options.

PR-URL: #9800
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
PR-URL: #9800
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2017
@sam-github sam-github mentioned this pull request Jan 24, 2017
2 tasks
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
PR-URL: #9800
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
Either the options or the listener argument to tls.createServer() was
optional, but not both. This makes no sense, so align the argument
checking and documentation with net.createServer(), which accepts the
same option sequence, and which tls.createServer() is modelled on.

PR-URL: #9800
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
PR-URL: #9800
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
Its confusing to have multiple names for the same thing, use
secureOptions consistently.

PR-URL: #9800
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
Document all TLSSocket options:

- All the secure context options are valid options
  to a secureContext
- isServer modifies the default value of requestCert

Describe all tls.connect() variants:

- tls.connect(path) was undocumented
- tls.connect(port) was underdocumented, and its relationship to
  tls.connect(options) was obscure

Socket passed to tls.connect is user managed:

- Replace #8996

Add documentation to:

- describe and add tests for the pfx and key variants, and describe how
  and when passphrase is used.
- describe tls cert and ca options
- describe buffer forms of tls crl option
- describe tls cipher option and defaults
- fix link to Crypto Constants
- describe that honorCipherOrder sets SSL_OP_CIPHER_SERVER_PREFERENCE.
- describe tls ecdhCurve/dhparam options
- describe tls secureProtocol option
- describe tls secureOptions
- describe tls sessionIdContext

De-deduplicate secure context docs:

The secure context options were documented 4 times, making it difficult
to understand where the options come from, where they are supported,
and under what conditions they are used.

The multiple copies were inconsistent and contradictory in their
descriptions of the options, and also inconsistent in whether the
options would be documented at all.

Cut through this gordian knot by linking all APIs that use the
secureContext options to the single source of truth about the options.

PR-URL: #9800
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
PR-URL: #9800
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
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.

8 participants