doc: Improve checkServerIdentity docs #17203

Hannes-Magnusson-CK · 2017-11-21T21:36:37Z

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

tls doc

Hannes-Magnusson-CK · 2017-11-21T21:37:13Z

See also #11467

Trott

Seems good to me with one requested change. Thanks for the pull request!

Trott · 2017-11-22T00:45:45Z

doc/api/tls.md

@@ -792,8 +792,9 @@ changes:
  * `servername`: {string} Server name for the SNI (Server Name Indication) TLS
    extension.
  * `checkServerIdentity(servername, cert)` {Function} A callback function
-    to be used when checking the server's hostname against the certificate.
-    This should throw an error if verification fails. The method should return
+    to be used (instead of the builtin {tls.checkServerIdentity} function)


Can you please change this line to this?:

to be used instead of tls.checkServerIdentity()

Backticks are appropriate here. Curly braces are for types. Thanks!

Ahh. That makes sense. Thanks for the comment!
Fixed :)

lpinca · 2017-11-22T06:35:31Z

doc/api/tls.md

@@ -792,8 +792,9 @@ changes:
  * `servername`: {string} Server name for the SNI (Server Name Indication) TLS
    extension.
  * `checkServerIdentity(servername, cert)` {Function} A callback function
-    to be used when checking the server's hostname against the certificate.
-    This should throw an error if verification fails. The method should return
+    to be used (instead of the builtin `tls.checkServerIdentity` function)


Tiny nit: can you use tls.checkServerIdentity()? It seems to be the dominant style.

of course! :) Updated

Hannes-Magnusson-CK · 2017-11-22T15:43:48Z

Probably a dumb question, I'm new around here =):
The "Public API" comment in https://github.com/nodejs/node/blob/master/lib/tls.js#L239

What makes those export more public then say https://github.com/nodejs/node/blob/master/lib/tls.js#L168 ?

If thats purely source code readability, should I document the checkServerIdentity function?

bnoordhuis · 2017-11-22T15:48:36Z

@Hannes-Magnusson-CK The comment is a bit misguided / misleading, it should just be removed.

You could read it as "these are the public and documented APIs" but some of the exports elsewhere are mentioned in the docs too, if only in passing.

Hannes-Magnusson-CK · 2017-11-22T15:52:03Z

Ok. Thanks @bnoordhuis !
I'll see if I can create another PR for documenting the checkServerIdentity function, removing the comment, and maybe couple of other related things.

The funciton was added in eb2ca10

Hannes-Magnusson-CK · 2017-11-22T20:31:34Z

Added a commit documenting tls.checkServerIdentity to this PR :)

jasnell · 2017-11-22T21:09:51Z

lib/tls.js

@@ -236,7 +236,6 @@ exports.parseCertString = internalUtil.deprecate(
  'Please use querystring.parse() instead.',
  'DEP0076');

-// Public API


why remove this?

Its a misleading comment. These aren't the only exported functions.

requested change was made

addaleax · 2017-12-01T19:53:14Z

Landed in da429c3, df63e53

Thanks for the PR! 🎉 ✨

PR-URL: #17203 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

The funciton was added in eb2ca10 PR-URL: #17203 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

PR-URL: #17203 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

The funciton was added in eb2ca10 PR-URL: #17203 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

PR-URL: #17203 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

The funciton was added in eb2ca10 PR-URL: #17203 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

PR-URL: #17203 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

The funciton was added in eb2ca10 PR-URL: #17203 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

PR-URL: #17203 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

gibfahn · 2017-12-19T23:30:54Z

Backported da429c3 to v6.x, df63e53 had conflicts, so choosing to leave it.

PR-URL: #17203 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

The funciton was added in eb2ca10 PR-URL: #17203 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

nodejs-github-bot added doc Issues and PRs related to the documentations. tls Issues and PRs related to the tls subsystem. labels Nov 21, 2017

Trott previously requested changes Nov 22, 2017

View reviewed changes

Hannes-Magnusson-CK force-pushed the patch-2 branch from 4bf98c7 to ea0cd07 Compare November 22, 2017 05:07

lpinca approved these changes Nov 22, 2017

View reviewed changes

doc: Improve checkServerIdentity docs

15a4b95

Hannes-Magnusson-CK force-pushed the patch-2 branch from ea0cd07 to 15a4b95 Compare November 22, 2017 15:27

doc: Document tls.checkServerIdentity

5bfa449

The funciton was added in eb2ca10

jasnell reviewed Nov 22, 2017

View reviewed changes

jasnell approved these changes Nov 22, 2017

View reviewed changes

addaleax approved these changes Nov 30, 2017

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 30, 2017

addaleax closed this Dec 1, 2017

addaleax pushed a commit that referenced this pull request Dec 1, 2017

doc: improve checkServerIdentity docs

da429c3

PR-URL: #17203 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 1, 2017

Hannes-Magnusson-CK mentioned this pull request Dec 1, 2017

Add missing native node tls.checkServerIdentity function DefinitelyTyped/DefinitelyTyped#21897

Merged

6 tasks

MylesBorins pushed a commit that referenced this pull request Dec 12, 2017

doc: improve checkServerIdentity docs

fc435ab

PR-URL: #17203 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

MylesBorins pushed a commit that referenced this pull request Dec 12, 2017

doc: improve checkServerIdentity docs

a596577

PR-URL: #17203 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

MylesBorins mentioned this pull request Dec 12, 2017

v9.3.0 proposal #17631

Merged

gibfahn pushed a commit that referenced this pull request Dec 19, 2017

doc: improve checkServerIdentity docs

88ae46d

PR-URL: #17203 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

gibfahn added land-on-v6.x labels Dec 19, 2017

gibfahn pushed a commit that referenced this pull request Dec 19, 2017

doc: improve checkServerIdentity docs

0fa2f39

PR-URL: #17203 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

gibfahn mentioned this pull request Dec 20, 2017

v8.9.4 proposal #17772

Closed

gibfahn pushed a commit that referenced this pull request Dec 20, 2017

doc: improve checkServerIdentity docs

e786fde

PR-URL: #17203 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

gibfahn mentioned this pull request Dec 20, 2017

v8.9.4 proposal #17774

Merged

MylesBorins added the backported-to-v8.x label Dec 20, 2017

MylesBorins mentioned this pull request Dec 20, 2017

v6.12.3 proposal #17776

Merged

MylesBorins added backported-to-v6.x and removed land-on-v6.x labels Jan 19, 2018

apapirovski mentioned this pull request Apr 13, 2018

tls documentation says that checkServerIdentity function should throw an error. It should not. #11467

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: Improve checkServerIdentity docs #17203

doc: Improve checkServerIdentity docs #17203

Hannes-Magnusson-CK commented Nov 21, 2017

Hannes-Magnusson-CK commented Nov 21, 2017

Trott left a comment •

edited

Loading

Trott Nov 22, 2017

Hannes-Magnusson-CK Nov 22, 2017

lpinca Nov 22, 2017

Hannes-Magnusson-CK Nov 22, 2017

Hannes-Magnusson-CK commented Nov 22, 2017

bnoordhuis commented Nov 22, 2017

Hannes-Magnusson-CK commented Nov 22, 2017

Hannes-Magnusson-CK commented Nov 22, 2017 •

edited

Loading

jasnell Nov 22, 2017

Hannes-Magnusson-CK Nov 22, 2017

addaleax commented Dec 1, 2017

gibfahn commented Dec 19, 2017

doc: Improve checkServerIdentity docs #17203

doc: Improve checkServerIdentity docs #17203

Conversation

Hannes-Magnusson-CK commented Nov 21, 2017

Checklist

Affected core subsystem(s)

Hannes-Magnusson-CK commented Nov 21, 2017

Trott left a comment • edited Loading

Choose a reason for hiding this comment

Trott Nov 22, 2017

Choose a reason for hiding this comment

Hannes-Magnusson-CK Nov 22, 2017

Choose a reason for hiding this comment

lpinca Nov 22, 2017

Choose a reason for hiding this comment

Hannes-Magnusson-CK Nov 22, 2017

Choose a reason for hiding this comment

Hannes-Magnusson-CK commented Nov 22, 2017

bnoordhuis commented Nov 22, 2017

Hannes-Magnusson-CK commented Nov 22, 2017

Hannes-Magnusson-CK commented Nov 22, 2017 • edited Loading

jasnell Nov 22, 2017

Choose a reason for hiding this comment

Hannes-Magnusson-CK Nov 22, 2017

Choose a reason for hiding this comment

addaleax commented Dec 1, 2017

gibfahn commented Dec 19, 2017

Trott left a comment •

edited

Loading

Hannes-Magnusson-CK commented Nov 22, 2017 •

edited

Loading