doc: general improvements to tls.md copy by jasnell · Pull Request #6933 · nodejs/node · GitHub

Checklist

documentation is changed or added
the commit message follows commit guidelines

Affected core subsystem(s)

doc (tls)

Description of change

General improvements to tls.md copy

@nodejs/documentation @nodejs/crypto

I think we do not support either of SSL2 or SSL3 by default anymore. Is it worth mentioning here?

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.

Should we at least add deprecated to SSL?

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.

Unmatched backtick

@eljefedelrodeodeljefe I totally agree. We should perhaps document the conventions followed in docs, somewhere.

Let's add a note about deprecation of NPN. It was never documented officially, and now is slowly going away.

Deprecation just in Node.js or deprecation in general use?

In general https://groups.google.com/a/chromium.org/forum/#!topic/security-dev/eJVxCZ92vvg

Ack

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 <aside>. But first I need to get all of our instances normalized.

@addaleax @indutny @thefourtheye ... nits addressed

@jasnell SGTM

Nit, but server's private key?

What about this?

Few more nits, otherwise LGTM. This is looking really great, thank you!

Nits updated!

I see it now, nvm.

LGTM, good job on this @jasnell

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 checkServerIdentity callback. Maybe it's because it's documented in several places.

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 checkServerIdentity text to the updated language.

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.
For the next round of edits I will work on pulling out the SSL references that are no longer relevant.

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

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 #6933 merged. #6933 (review) PR-URL: #10345 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Italo A. Casas <me@italoacasas.com>

jasnell added tls Issues and PRs related to the tls subsystem. crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations. labels May 23, 2016

indutny reviewed May 23, 2016
View reviewed changes

addaleax reviewed May 23, 2016
View reviewed changes

indutny reviewed May 23, 2016
View reviewed changes

mscdex removed the crypto Issues and PRs related to the crypto subsystem. label May 24, 2016

bnoordhuis reviewed May 24, 2016
View reviewed changes

shigeki reviewed May 25, 2016
View reviewed changes

doc: general improvements to tls.md copy

8ad0809

Restructuring and clarifications to the tls.md copy to improve readability and flow.

jasnell force-pushed the doc-tls-copy branch from 49e287f to 8ad0809 Compare May 26, 2016 15:08

jasnell closed this May 26, 2016

MylesBorins mentioned this pull request Jul 11, 2016

Audit commits not found on v4.x-staging #7661

Closed

MylesBorins added the dont-land-on-v4.x label Jul 11, 2016

zbinlin reviewed Dec 16, 2016

View reviewed changes

sam-github mentioned this pull request Dec 19, 2016

doc: fix typo in ecdhCurve, a tls property name #10345

Merged

2 tasks

cjihrig pushed a commit that referenced this pull request Dec 20, 2016

doc: fix typo in ecdhCurve, a tls property name

5654c83

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>

MylesBorins pushed a commit that referenced this pull request Jan 22, 2017

doc: fix typo in ecdhCurve, a tls property name

27dba16

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>

MylesBorins pushed a commit that referenced this pull request Jan 24, 2017

doc: fix typo in ecdhCurve, a tls property name

1da1af5

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>

MylesBorins pushed a commit that referenced this pull request Jan 31, 2017

doc: fix typo in ecdhCurve, a tls property name

24bf753

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>

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

doc: general improvements to tls.md copy by jasnell · Pull Request #6933 · nodejs/node · GitHub

Uh oh!

Uh oh!

doc: general improvements to tls.md copy - #6933

doc: general improvements to tls.md copy#6933jasnell wants to merge 1 commit intonodejs:masternodejs/node:masterfrom jasnell:doc-tls-copyjasnell/node:doc-tls-copyCopy head branch name to clipboard

Conversation

jasnell commented May 23, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eljefedelrodeodeljefe commented May 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thefourtheye commented May 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented May 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented May 23, 2016

Uh oh!

eljefedelrodeodeljefe commented May 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

indutny commented May 23, 2016

Uh oh!

jasnell commented May 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

indutny commented May 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented May 24, 2016

Uh oh!

bnoordhuis commented May 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shigeki commented May 25, 2016

Uh oh!

jasnell commented May 25, 2016

Uh oh!

jasnell commented May 25, 2016

Uh oh!

bnoordhuis commented May 25, 2016

Uh oh!

jasnell commented May 25, 2016

Uh oh!

bnoordhuis commented May 25, 2016

Uh oh!

jasnell commented May 26, 2016

Uh oh!

MylesBorins commented Jul 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

doc: general improvements to tls.md copy#6933
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:doc-tls-copy

jasnell commented May 23, 2016 •
edited
Loading