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

http: servername === false should disable SNI #27316

Closed
wants to merge 5 commits into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Apr 19, 2019

There is no way to disable SNI extension when sending a request to HTTPS
server. Setting options.servername to a falsy value would make Node.js
core override it with either hostname or ip address.

This change introduces a way to disable SNI completely if this is
required for user's application. Setting options.servername to false
in https.request would disable overrides and thus disable the
extension.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

There is no way to disable SNI extension when sending a request to HTTPS
server. Setting `options.servername` to a falsy value would make Node.js
core override it with either hostname or ip address.

This change introduces a way to disable SNI completely if this is
required for user's application. Setting `options.servername` to `false`
in `https.request` would disable overrides and thus disable the
extension.
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Apr 19, 2019
@indutny
Copy link
Member Author

indutny commented Apr 19, 2019

cc @nodejs/http @nodejs/crypto

Copy link
Member

@srl295 srl295 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. (though I did not have this use case, it is plausible and would have simplified my testing)

doc/api/https.md Outdated Show resolved Hide resolved
doc/api/https.md Outdated Show resolved Hide resolved
@indutny
Copy link
Member Author

indutny commented Apr 20, 2019

Thank you for review everyone!

@BridgeAR BridgeAR added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 21, 2019
doc/api/https.md Outdated Show resolved Hide resolved
test/parallel/test-https-agent-sni.js Show resolved Hide resolved
Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but one nit re: documentation.

doc/api/https.md Outdated
@@ -29,6 +29,10 @@ An [`Agent`][] object for HTTPS similar to [`http.Agent`][]. See
Can have the same fields as for [`http.Agent(options)`][], and
* `maxCachedSessions` {number} maximum number of TLS cached sessions.
Use `0` to disable TLS session caching. **Default:** `100`.
* `servername` {string | boolean} the value of
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Boolean might be slightly misleading? It's really {string | false} since setting it to true won't do what one might expect...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beware that false will be the only type case here and will baffle doc tools (tools/doc/type-parser.js will throw).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was exactly my reasoning. We might want to use null as @bnoordhuis has suggested to fix this!

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM % nits others pointed out.

Observation: I don't think you can conclude from the test that no SNI extension is sent, just that the user's code doesn't set it explicitly. Maybe add a { SNICallback: cb } to the server's options object to verify?

Question: isn't null a better value than false for the opt-out when true doesn't work?

@indutny
Copy link
Member Author

indutny commented Apr 23, 2019

@bnoordhuis I think the test verifies it very well. The socket.servername is set to false only when there is no SNI present, AFAIK!

Is there a common practice for falsey values that we try to follow in core? In this PR's case - there are three possible types of values undefined, string, or null/false.

@vsemozhetbyt @apapirovski @srl295 @lpinca @BridgeAR may I ask y'all for an opinion on the topic?

@BridgeAR
Copy link
Member

@indutny

Is there a common practice for falsey values that we try to follow in core

Not that I am aware of. Using undefined seems a bad option, since it's often handled as not existing. So a property with undefined as value would be considered not set. Using an empty string in this specific case might be an alternative but AFAIC only if there's no valid use case for that so far.

Using null is somewhat ambiguous in core. It's often ignored while we mostly become stricter with type checks and often do not accept null as alternative to undefined anymore. In this specific case I would consider it a good option to indicate the wish to disable the functionality.

I can not think of any API that only accepts false but not true, so that would be something new in core.

Thus, I personally would go with either an empty string or null while not blocking false.

@sam-github
Copy link
Contributor

I agree that an empty string and/or null would be better than false.

https://tools.ietf.org/html/rfc6066#section-3 doesn't allow zero-length server names, so an empty string is not a valid value, and attempting to set one will error with SSL_R_SSL3_EXT_INVALID_SERVERNAME if a string of length zero is passed to SSL_set_tlsext_host_name().

@indutny
Copy link
Member Author

indutny commented Apr 27, 2019

Alright, updated the PR to use '' instead of false. PTAL @nodejs/collaborators

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 27, 2019
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 29, 2019

Trott pushed a commit to Trott/io.js that referenced this pull request Apr 30, 2019
There is no way to disable SNI extension when sending a request to HTTPS
server. Setting `options.servername` to a falsy value would make Node.js
core override it with either hostname or ip address.

This change introduces a way to disable SNI completely if this is
required for user's application. Setting `options.servername` to ``
in `https.request` would disable overrides and thus disable the
extension.

PR-URL: nodejs#27316
Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@Trott
Copy link
Member

Trott commented Apr 30, 2019

Landed in 98e9de7

@Trott Trott closed this Apr 30, 2019
targos pushed a commit that referenced this pull request Apr 30, 2019
There is no way to disable SNI extension when sending a request to HTTPS
server. Setting `options.servername` to a falsy value would make Node.js
core override it with either hostname or ip address.

This change introduces a way to disable SNI completely if this is
required for user's application. Setting `options.servername` to ``
in `https.request` would disable overrides and thus disable the
extension.

PR-URL: #27316
Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@targos targos mentioned this pull request May 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. http Issues or PRs related to the http subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.