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

Inaccurate https.request options docs #9324

Closed
papandreou opened this issue Oct 27, 2016 · 4 comments
Closed

Inaccurate https.request options docs #9324

papandreou opened this issue Oct 27, 2016 · 4 comments
Labels
doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors. https Issues or PRs related to the https subsystem.

Comments

@papandreou
Copy link
Contributor

papandreou commented Oct 27, 2016

  • Version: 7.0.0
  • Platform: All
  • Subsystem: Docs

I needed to explore specifying the SNI hostname independently of the hostname and the Host header, but the docs list the servername among these bullets (my emphasis):

The following options from tls.connect() can also be specified. However, a
globalAgent silently ignores these.

  • pfx: Certificate, Private key and CA certificates to use for SSL. Default null.
  • key: Private key to use for SSL. Default null.
  • passphrase: A string of passphrase for the private key or pfx. Default null.
  • cert: Public x509 certificate to use. Default null.
  • ca: A string, [Buffer][] or array of strings or [Buffer][]s of trusted
    certificates in PEM format. If this is omitted several well known "root"
    CAs will be used, like VeriSign. These are used to authorize connections.
  • ciphers: A string describing the ciphers to use or exclude. Consult
    https://www.openssl.org/docs/apps/ciphers.html#CIPHER-LIST-FORMAT for
    details on the format.
  • rejectUnauthorized: If true, the server certificate is verified against
    the list of supplied CAs. An 'error' event is emitted if verification
    fails. Verification happens at the connection level, before the HTTP
    request is sent. Default true.
  • secureProtocol: The SSL method to use, e.g. SSLv3_method to force
    SSL version 3. The possible values depend on your installation of
    OpenSSL and are defined in the constant [SSL_METHODS][].
  • servername: Servername for SNI (Server Name Indication) TLS extension.

In order to specify these options, use a custom [Agent][].

That doesn't seem to be correct, though. If I do an HTTPS request without providing an Agent instance (thus utilizing the global agent), it does use the servername as the SNI hostname:

require('https').request({
    hostname: 'www.github.com',
    servername: 'sniname.com'
}).end();
// Error: Hostname/IP doesn't match certificate's altnames: "Host: sniname.com. is not in the cert's altnames: DNS:github.com, DNS:www.github.com"

The rejectUnauthorized option also works fine without an agent.

It seems like the docs should be fixed to match reality? I'd whip up a PR, but I'm not sure exactly which of the remaining options also work without an agent, and I'm not familiar enough with TLS etc. to find out.

Looks like the inaccuracy was introduced in 8ba5631, while rejectUnauthorized was added to the list way back in f8c335d (0.6.6).

@mscdex mscdex added https Issues or PRs related to the https subsystem. doc Issues and PRs related to the documentations. labels Oct 27, 2016
@bnoordhuis
Copy link
Member

I'm not sure if the documentation was correct even at the time it was written. I remember quietly fixing lib/https.js to behave like the documentation says it does in 35607f3 in 2012. Seems Isaac changed it again in 49519f1 from 2013 but without updating the documentation.

The global agent respects connection-level properties. It simply creates a new connection when it doesn't find an existing connection in the pool with those exact same properties.

It seems .passphrase and .secureProtocol are the exceptions, those aren't checked for. That should be alright for .passphrase because it doesn't materially affect the connection (it's only used to unlock the key or the PFX) but .secureProtocol seems like an oversight.

@sam-github
Copy link
Contributor

@papandreou can you PR corrections to the docs? That would be very helpful if you have the time.

@sam-github sam-github added the good first issue Issues that are suitable for first-time contributors. label Nov 2, 2016
@papandreou
Copy link
Contributor Author

@sam-github, sure thing: #9453

@bnoordhuis, thanks for weighing in. I tried to fix the secureProtocol oversight here: #9452

papandreou added a commit to assetgraph/node that referenced this issue Nov 7, 2016
@papandreou
Copy link
Contributor Author

Everything has been addressed by PRs now, so I'll close this issue.

jasnell pushed a commit that referenced this issue Jan 11, 2017
Refs: #9324
PR-URL: #9452
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this issue Jan 18, 2017
Refs: nodejs#9324
PR-URL: nodejs#9452
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this issue Jan 23, 2017
Refs: nodejs#9324
PR-URL: nodejs#9452
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this issue Jan 25, 2017
Refs: nodejs#9324
PR-URL: nodejs#9452
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this issue Jan 27, 2017
Refs: nodejs#9324
PR-URL: nodejs#9452
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this issue Nov 14, 2017
Refs: #9324
PR-URL: #9452
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
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. good first issue Issues that are suitable for first-time contributors. https Issues or PRs related to the https subsystem.
Projects
None yet
Development

No branches or pull requests

4 participants