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 Agent using Host header for SNI server_name #37104

Open
Nevon opened this issue Jan 27, 2021 · 3 comments
Open

HTTP Agent using Host header for SNI server_name #37104

Nevon opened this issue Jan 27, 2021 · 3 comments
Labels
http Issues or PRs related to the http subsystem.

Comments

@Nevon
Copy link

Nevon commented Jan 27, 2021

  • Version: v14.15.1`
  • Platform: Darwin C02DFH5SMD6M 19.6.0 Darwin Kernel Version 19.6.0: Tue Nov 10 00:10:30 PST 2020; root:xnu-6153.141.10~1/RELEASE_X86_64 x86_64
  • Subsystem: http

What steps will reproduce the bug?

I'm not 100% sure that this is a bug, but it was surprising behavior that is different from any other http clients I've looked at (curl, python's httplib, nginx).

If you make an HTTPS request specifying a host header that is different from the request host, the servername used for SNI will be set to the value of the host header, unless the host header contains an IP address or you explicitly pass a servername in the agent options.

node/lib/http.js

Lines 1086 to 1092 in b0c0111

options.servername = host;
if (req) {
var hostHeader = req.getHeader('host');
if (hostHeader) {
options.servername = hostHeader.replace(/:.*$/, '');
}
}

What is the expected behavior?

This was very surprising to me as the host header is part of the HTTP layer, and the servername belongs to the TLS layer, and I have never seen a client behave this way. I would have expected the host (not the host header) to be used as the default servername.

For example using curl:

$ curl -vvI https://www.google.com -H "Host: https://not-google.com"
*   Trying 172.217.21.164...
* TCP_NODELAY set
* Connected to www.google.com (172.217.21.164) port 443 (#0)
* ALPN, offering h2
* ALPN, offering http/1.1
* successfully set certificate verify locations:
*   CAfile: /etc/ssl/cert.pem
  CApath: none
* TLSv1.2 (OUT), TLS handshake, Client hello (1):
* TLSv1.2 (IN), TLS handshake, Server hello (2):
* TLSv1.2 (IN), TLS handshake, Certificate (11):
* TLSv1.2 (IN), TLS handshake, Server key exchange (12):
* TLSv1.2 (IN), TLS handshake, Server finished (14):
* TLSv1.2 (OUT), TLS handshake, Client key exchange (16):
* TLSv1.2 (OUT), TLS change cipher, Change cipher spec (1):
* TLSv1.2 (OUT), TLS handshake, Finished (20):
* TLSv1.2 (IN), TLS change cipher, Change cipher spec (1):
* TLSv1.2 (IN), TLS handshake, Finished (20):
* SSL connection using TLSv1.2 / ECDHE-ECDSA-CHACHA20-POLY1305
* ALPN, server accepted to use h2
* Server certificate:
*  subject: C=US; ST=California; L=Mountain View; O=Google LLC; CN=www.google.com
*  start date: Jan  5 12:13:00 2021 GMT
*  expire date: Mar 30 12:12:59 2021 GMT
*  subjectAltName: host "www.google.com" matched cert's "www.google.com"
*  issuer: C=US; O=Google Trust Services; CN=GTS CA 1O1
*  SSL certificate verify ok.
* Using HTTP2, server supports multi-use
* Connection state changed (HTTP/2 confirmed)
* Copying HTTP/2 data in stream buffer to connection buffer after upgrade: len=0
* Using Stream ID: 1 (easy handle 0x7fecd500f600)
> HEAD / HTTP/2
> Host: https://not-google.com
> User-Agent: curl/7.64.1
> Accept: */*
>

As you can see, the certificate is accepted:

subjectAltName: host "www.google.com" matched cert's "www.google.com"

If we do the same thing in Node, the certificate will be rejected as the certificate host name does not match the server name that we send:

const https = require('https');
const options = {
  headers: {
    host: 'https://not-google.com'
  }
};

https.get('https://google.com', options, (res) => {}).on('error', console.error);

This will result in [ERR_TLS_CERT_ALTNAME_INVALID]: Hostname/IP does not match certificate's altnames.

Changing this so that the host is used as the default servername instead of the host header is a small and simple change, but my question would be if this is done intentionally, and if so, why?

Additional information

  • I was not able to find any reference in the git history of why this decision was made - only that it was made back in 2012.
  • Having a mismatched servername and host header is domain fronting, but it doesn't seem like something the HTTP client should be responsible for preventing. And the current design still allows for it by just setting the servername yourself.
@schamberg97
Copy link
Contributor

The only reason I can think of is that this approach allows you to lazily query the server by its IP to avoid DNS resolution, without defining host and servername separately. Which sounds like a bit of a bad idea to me, tbh

@dnlup
Copy link
Contributor

dnlup commented Jan 28, 2021

SNI should be the conceptual equivalent of virtual hosting for HTTPS, so I imagine the reason could be for providing that behavior. I can't say if it's right or wrong, though.

@xuemengfei
Copy link

#22389 Found an issue that has been discussed since 2018.

@Lxxyx Lxxyx added the http Issues or PRs related to the http subsystem. label Jan 29, 2021
wmfgerrit pushed a commit to wikimedia/operations-deployment-charts that referenced this issue Jan 26, 2023
Due to how nodejs works, if you try to call an HTTPs endpoint
like inference-staging.discovery.wmnet with a custom HTTP Host header
(like enwiki-goodfaith.revscoring-editquality-goodfaith.wikimedia.org)
the TLS servername will be set with said value. This means
that without proper SANs, there will be TLS hostname validation
errors reported by nodejs. This is what's happening when ChangeProp
tries to contact LiftWing, due to this nodejs "feature":
nodejs/node#37104

This change is meant to test the new SANs on the staging endpoint
first.

Bug: T327302
Change-Id: I7822f4e41e111537ea2e6208d9b47b5192d488d4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Development

No branches or pull requests

6 participants
@Nevon @xuemengfei @Lxxyx @dnlup @schamberg97 and others