-
Notifications
You must be signed in to change notification settings - Fork 28.8k
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: general improvements to tls.md copy #6933
Conversation
@@ -2,90 +2,138 @@ | |||
|
|||
Stability: 2 - Stable | |||
|
|||
Use `require('tls')` to access this module. | |||
The `tls` module provides an implementation of the Transport Layer Security | |||
(TLS) and Secure Socket Layer (SSL) protocols that is built on top of OpenSSL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we do not support either of SSL2
or SSL3
by default anymore. Is it worth mentioning here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I made a note of that for myself when I was going through this. There are a couple of outdated bits in this that should be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we at least add deprecated
to SSL
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps... let's do this separately. There are a couple places where the SSL stuff in this doc needs to be updated.
nit: we are very, very inconsistent about it, but I would prefer the "Note"-section preceded with an underscore and only ended with an underscore after/with the relevant section. |
|
||
The `tlsSocket.renegotiate()` method initiate TLS renegotiation process. | ||
Upon completion, the `callback` function will be passed a single argument | ||
that is either an `Error` (if the request failed) or `null. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unmatched backtick
@eljefedelrodeodeljefe I totally agree. We should perhaps document the conventions followed in docs, somewhere. |
|
||
## ALPN, NPN and SNI | ||
### ALPN, NPN and SNI | ||
|
||
<!-- type=misc --> | ||
|
||
ALPN (Application-Layer Protocol Negotiation Extension), NPN (Next | ||
Protocol Negotiation) and, SNI (Server Name Indication) are TLS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a note about deprecation of NPN. It was never documented officially, and now is slowly going away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deprecation just in Node.js or deprecation in general use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
@thefourtheye @eljefedelrodeodeljefe ... regarding the Note sections, I have something different in mind when I'm done going through each of these documents... I'd like to explore making them true aside callout in the rendered html, pulled out of the regular text flow using |
@addaleax @indutny @thefourtheye ... nits addressed |
@jasnell SGTM |
The term "[Forward Secrecy]" or "Perfect Forward Secrecy" describes a feature of | ||
key-agreement (i.e., key-exchange) methods. That is, the server and client keys | ||
are used to negotiate new keys that are used specifically and only for the | ||
current communication session. Practically, this means that even if a private |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, but server's private key
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about this?
Few more nits, otherwise LGTM. This is looking really great, thank you! |
Nits updated! |
key-agreement (i.e., key-exchange) methods. That is, the server and client keys | ||
are used to negotiate new temporary keys that are used specifically and only for | ||
the current communication session. Practically, this means that even if the | ||
server's private key is compromised, communication can only be decrypted by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see it now, nvm.
LGTM, good job on this @jasnell |
TLS/SSL is a public/private key infrastructure. Each client and each | ||
server must have a private key. A private key is created like this: | ||
The TLS/SSL is a public/private key infrastructure (PKI). Each client and | ||
server must have a *private key*. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I were a pedant, I'd point out that null ciphers (i.e., no-auth and no-encrypt ciphers) that don't require a key do exist.
Fortunately, I'm not a pedant!
@shigeki @bnoordhuis ... nits addressed! commits squashed and message updated. |
I couldn't spot whether you changed the description for the |
``` | ||
|
||
where `secure_socket` has the same API as `pair.cleartext`. | ||
|
||
[OpenSSL cipher list format documentation]: https://www.openssl.org/docs/apps/ciphers.html#CIPHER-LIST-FORMAT | ||
[Chrome's 'modern cryptography' setting]: https://www.chromium.org/Home/chromium-security/education/tls#TOC-Deprecation-of-TLS-Features-Algorithms-in-Chrome |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This link was already outdated. https://www.chromium.org/Home/chromium-security/education/tls#TOC-Cipher-Suites seems to be a new one.
@jasnell Just one minor nits, otherwise LGTM. Good work, thanks. |
@bnoordhuis ... made the change here => https://github.com/nodejs/node/pull/6933/files#diff-f6e3a86962eaf0897ab59e88b418e64fR724 @shigeki ... updated! |
@bnoordhuis ... updated both instances of the |
I only see one change in commit 49e287f. Did I overlook something? |
@bnoordhuis ... the other one was here -> 129be4b#diff-f6e3a86962eaf0897ab59e88b418e64fR724 It was squashed in with the other changes... |
Ah, alright. LGTM then. |
Restructuring and clarifications to the tls.md copy to improve readability and flow.
Restructuring and clarifications to the tls.md copy to improve readability and flow. PR-URL: #6933 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
Landed in 1b6a468. Thanks all. |
Restructuring and clarifications to the tls.md copy to improve readability and flow. PR-URL: nodejs#6933 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
Restructuring and clarifications to the tls.md copy to improve readability and flow. PR-URL: #6933 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
added don't land label please feel free to backport |
- `certfile`: all CA certs concatenated in one file like | ||
`cat ca1-cert.pem ca2-cert.pem > ca-cert.pem` | ||
If using Perfect Foward Secrecy using `ECDHE`, Diffie-Hellman parameters are | ||
not required and a default ECDHE curve will be used. The `ecdheCurve` property |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ecdheCurve
or ecdhCurve
?
Addresses comment after PR nodejs#6933 merged. nodejs#6933 (review) PR-URL: nodejs#10345 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Italo A. Casas <me@italoacasas.com>
Addresses comment after PR nodejs#6933 merged. nodejs#6933 (review) PR-URL: nodejs#10345 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Italo A. Casas <me@italoacasas.com>
Addresses comment after PR #6933 merged. #6933 (review) PR-URL: #10345 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Italo A. Casas <me@italoacasas.com>
Addresses comment after PR #6933 merged. #6933 (review) PR-URL: #10345 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Italo A. Casas <me@italoacasas.com>
Addresses comment after PR #6933 merged. #6933 (review) PR-URL: #10345 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Italo A. Casas <me@italoacasas.com>
Addresses comment after PR #6933 merged. #6933 (review) PR-URL: #10345 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Italo A. Casas <me@italoacasas.com>
Checklist
Affected core subsystem(s)
doc (tls)
Description of change
General improvements to tls.md copy
@nodejs/documentation @nodejs/crypto