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.

Sign up for GitHub

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

Jump to bottom

doc: Improve checkServerIdentity docs #17203

Closed

Hannes-Magnusson-CK wants to merge 2 commits into nodejs:master from Hannes-Magnusson-CK:patch-2

Contributor

Hannes-Magnusson-CK commented Nov 21, 2017

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

tls doc

nodejs-github-bot added doc tls labels

Contributor Author

Hannes-Magnusson-CK commented Nov 21, 2017

See also #11467

Trott previously requested changes

View reviewed changes

Member

Trott left a comment •

edited

Loading

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

doc/api/tls.md Outdated

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

Member

Trott Nov 22, 2017

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!

Contributor Author

Hannes-Magnusson-CK Nov 22, 2017

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

Hannes-Magnusson-CK force-pushed the patch-2 branch from 4bf98c7 to ea0cd07 Compare

November 22, 2017 05:07

lpinca approved these changes

View reviewed changes

doc/api/tls.md Outdated

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

Member

lpinca Nov 22, 2017

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

Contributor Author

Hannes-Magnusson-CK Nov 22, 2017

of course! :) Updated


          doc: Improve checkServerIdentity docs

15a4b95

Hannes-Magnusson-CK force-pushed the patch-2 branch from ea0cd07 to 15a4b95 Compare

November 22, 2017 15:27

Contributor Author

Hannes-Magnusson-CK commented Nov 22, 2017

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?

Member

bnoordhuis commented Nov 22, 2017

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

Contributor Author

Hannes-Magnusson-CK commented Nov 22, 2017

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.


          doc: Document tls.checkServerIdentity

5bfa449

The funciton was added in eb2ca10

Contributor Author

Hannes-Magnusson-CK commented Nov 22, 2017 •

edited

Loading

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

jasnell reviewed

View reviewed changes

lib/tls.js

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

Member

jasnell Nov 22, 2017

why remove this?

Contributor Author

Hannes-Magnusson-CK Nov 22, 2017

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

jasnell approved these changes

View reviewed changes

Trott dismissed their stale review

November 24, 2017 22:01

requested change was made

addaleax approved these changes

View reviewed changes

addaleax added the author ready label

Member

addaleax commented Dec 1, 2017

Landed in da429c3, df63e53

Thanks for the PR! 🎉 ✨

addaleax closed this

addaleax pushed a commit that referenced this pull request


          doc: improve checkServerIdentity docs

da429c3

PR-URL: #17203
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

addaleax pushed a commit that referenced this pull request


          doc: document tls.checkServerIdentity

df63e53

The funciton was added in eb2ca10

PR-URL: #17203
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

addaleax removed the author ready label

Hannes-Magnusson-CK mentioned this pull request

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

Merged

6 tasks

MylesBorins pushed a commit that referenced this pull request


          doc: improve checkServerIdentity docs

fc435ab

PR-URL: #17203
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

MylesBorins pushed a commit that referenced this pull request


          doc: document tls.checkServerIdentity

ab5d7fe

The funciton was added in eb2ca10

PR-URL: #17203
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

MylesBorins pushed a commit that referenced this pull request


          doc: improve checkServerIdentity docs

a596577

PR-URL: #17203
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

MylesBorins pushed a commit that referenced this pull request


          doc: document tls.checkServerIdentity

7849d53

The funciton was added in eb2ca10

PR-URL: #17203
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

MylesBorins mentioned this pull request

v9.3.0 proposal #17631

Merged

gibfahn pushed a commit that referenced this pull request


          doc: improve checkServerIdentity docs

88ae46d

PR-URL: #17203
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

gibfahn pushed a commit that referenced this pull request


          doc: document tls.checkServerIdentity

89c1959

The funciton was added in eb2ca10

PR-URL: #17203
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

gibfahn added land-on-v6.x labels

gibfahn pushed a commit that referenced this pull request


          doc: improve checkServerIdentity docs

0fa2f39

PR-URL: #17203
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

Member

gibfahn commented Dec 19, 2017

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

gibfahn mentioned this pull request

v8.9.4 proposal #17772

Closed

gibfahn pushed a commit that referenced this pull request


          doc: improve checkServerIdentity docs

e786fde

PR-URL: #17203
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

gibfahn pushed a commit that referenced this pull request


          doc: document tls.checkServerIdentity

a3d218d

The funciton was added in eb2ca10

PR-URL: #17203
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

gibfahn mentioned this pull request

v8.9.4 proposal #17774

Merged

MylesBorins added the backported-to-v8.x label

MylesBorins mentioned this pull request

v6.12.3 proposal #17776

Merged

MylesBorins added backported-to-v6.x and removed land-on-v6.x labels

apapirovski mentioned this pull request

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

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc tls