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

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

Closed
lockhart-raphael opened this issue Feb 20, 2017 · 4 comments
Labels
doc Issues and PRs related to the documentations. tls Issues and PRs related to the tls subsystem.

Comments

@lockhart-raphael
Copy link

lockhart-raphael commented Feb 20, 2017

  • Version: 6.7.0 - 7.5.0 (at least)
  • Platform: All? tested on MacOS / Linux
  • Subsystem: TLS

The documentation for the checkServerIdentity function at https://nodejs.org/api/tls.html#tls_tls_connect_options_callback
states that the provided function "should throw an error if verification fails. "

However, the implementation (https://github.com/nodejs/node/blob/master/lib/_tls_wrap.js#L1083) expects a truthy value returned from the function as the error, and has no try / catch logic to catch an error if thrown, with the result that if the checkServerIdentity function throws an error the whole process will likely exit. Either the docs or the implementation should be corrected to reflect the intended behavior.

@mscdex mscdex added tls Issues and PRs related to the tls subsystem. doc Issues and PRs related to the documentations. labels Feb 20, 2017
@sam-github
Copy link
Contributor

fixed in #10846, but no one spoke up for it

I'll try to factor pieces of those docs out and get them accepted

sam-github added a commit to sam-github/node that referenced this issue Feb 20, 2017
Direct use of tls.TLSSocket to start a TLS session over an existing TCP connection was documented.

However, to use this connection securely it is necessary to validate and
authenticate the peer's certificate, and the documented events and
properties are implemented only for TLSSockets returned by
tls.connect(). In order to create secure connections, additional
undocumented APIs must be used, and these APIs are being called right
now by npm modules.

Fix: nodejs#10555
Fix: nodejs#11467
@Trott
Copy link
Member

Trott commented Jul 26, 2017

@sam-github Should this remain open?

@sam-github
Copy link
Contributor

yes, the docs are wrong

@apapirovski
Copy link
Member

Fixed by #17203

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. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

No branches or pull requests

5 participants