fix(core) set SNI server name for upstream SSL connections

[konrade]

Made sure server name for upstream SSL connections match Host header.

Summary

Fixes problem when having configured upstream server using https://... where Kong initiates upstream SSL connection using SNI with server name "kong_upstream".
This does not match the "Host: " header forwarded from the incoming request and is seen by most web servers as an error and they won't process such request.

Full changelog

• Added NGINX proxy_ssl_name directive to nginx-kong.conf template

Issues resolved

Fix #2129

[Tieske]

I think this warrants a test.

[konrade]

Good idea to write a test for it.
I'm not sure how to do it for Kong and a simulate a HTTPS upstream and verify that SNI server name is set correctly.
My fix adds "proxy_ssl_name" to nginx-kong.conf in section "location /" so it should apply for all proxied requests if if they're not to a HTTPS upstream but I guess NGINX just ignores it when not using SSL for upstream connection.

An existing test spec/02-integration/05-proxy/01-router_spec.lua Router preserve_host preserves downstream host if enabled seems to fail on my PR.
I'm not sure why.

[p0pr0ck5]

Should this PR also consider the SNI sent in the downstream Client Hello? If Kong has received a TLS connection from the downstream client, should we not consider that SNI as well, rather than just the Host header/upstream_url (at least on a configurable basis)?

[thibaultcha]

@konrade Thanks for raising this issue. We also experienced the same one internally, and were about to implement your fix in #2129. I am glad it is coming from the community instead, at your initiative :)

@p0pr0ck5 I feel like such a use-case would be handled already when configuring the preserve_host property on the API object. Ex: Assuming (maybe wrongly) with enough confidence that the Client Hello SNI is the request Host header value as well, and if the API is configured with preserve_host = true, then $upstream_host will provide that value for the Client Hello made from Kong. Thus the upstream Client Hello and HTTP Host header effectively come from the downstream client. Thoughts?

[p0pr0ck5]

@thibaultcha my question was centered around your presented assumption indeed being incorrect. While I don't think this is a common case, there's certainly nothing preventing the (layer 4) SNI value from being different from the (layer 7) Host HTTP header. Indeed, this is something Kong currently allows for, which is why I brought it up for the sake of completeness.

Certainly I don't want to be objectionist for objections' sake; I just want to make sure we're covering all cases we want to support. My biggest concern is that we're not offering the option to configure the upstream SNI value; indeed, in other cases, the HTTP Host value is not directly tied to the SNI (curl/curl#607). Not saying we need all the flexibility the curl does ;)

[thibaultcha]

there's certainly nothing preventing the (layer 4) SNI value from being different from the (layer 7) Host HTTP header.

This was certainly very clear already, but my assumption was that "in most places, this isn't the case". As of now, I think such a configuration would be outside the scope of this hotfix though, and would be considered if requested, which hasn't happened so far.

[p0pr0ck5]

Agreed, I don't think it's a common use case. My thought is, rather than treating this as a quick hotfix only, is it an opportunity to add some additional flexibility to meet future use cases. It seems like the answer to that question is 'no', which is okay with me- just wanted to explore the opportunity.

@konrade what are your thoughts on the above discussion?

[konrade]

Based on what I've seen other web servers doing I think Kong should reject incoming requests where SNI server name does not match Host header (see: TLS Virtual Host Confusion).

I guess there are two scenarios regarding upstream SNI:

1. Use same server name from downstream in SNI for upstream (kind of passing TLS straight through Kong)
   • Should be the behaviour when configured API with preserve_host=true
2. Map requests on external FQDN to another TLS upstream server name

Scenario 2 could potentially require an option to configure what HTTP Host and SNI server name to use when upstream is specified by IP.

A very real case for me when starting to evaluate Kong was that I already had an older web gateway that did all kinds of reverse proxying (some with upstream TLS). Instead of trying to replicate all mappings directly I tried to put Kong in front of the old gateway with a default routing sending all that didn't match any configured APIs to the old gateway.

In this scenario I ran into another problem with upstream SNI and it was due to Kong/NGINX keeping the upstream connection alive between multiple requests even if they downstream request was on different FQDNs. This caused HTTP Host to missmatch with SNI for subsequent request that wasn't on same FQDN that opened upstream TLS connection in the first place.

[p0pr0ck5]

Following 5d86d09, it should be fairly easy to test this functionality by using an ssl_by_lua handler to examine the SNI field (https://github.com/openresty/lua-resty-core/blob/master/lib/ngx/ssl.md#server_name).

[p0pr0ck5]

I mocked up some tests related to this PR here: 3f3b476

Also, @konrade one point that's come up in discussion of this is the handling of the SNI header when preserve_host is false, and the upstream_url element of an API is built by an IP address. In such a case, the value of upstream_host is an IP/port string, which is not valid SNI (https://tools.ietf.org/html/rfc6066#section-3). So we should find a way to handle this case by either using the downstream Host header when preserve_host is false and the upstream_url uses an IP address, or explicitly defining an alternative elsewhere (I'm a fan of the latter, since that's what this PR tries to do already).

[konrade]

A bit tricky since Host header allows for IP but not SNI.
Interestingly curl seems to try to put IP in SNI when doing request towards https address but when I tried on my Mac (curl 7.51.0) it printed:

WARNING: using IP address, SNI is being disabled by the OS.

I have the feeling that the correct way to handle it is to not include SNI for upstream TLS when upstream_url uses an IP and preserve_host is false. This should probably also be the case if downstream Host is an IP and preserve_host is true.

[p0pr0ck5]

I have the feeling that the correct way to handle it is to not include SNI for upstream TLS when upstream_url uses an IP and preserve_host is false.

Any reason we couldn't use the downstream Host header here?

Also, this PR should still have tests :)

[konrade]

Should I take in your tests @p0pr0ck5 ?
Honestly I've never dug into the test suite/setup for Kong so would require a bit startup time for me to be able to contribute to the test suite.

[p0pr0ck5]

@thibaultcha what are you thoughts here? Can we merge this PR as-is and bring in the tests later, or is having the tests in the same PR are blocker?

@konrade do you strongly disagree that we should use the downstream Host header as the value of proxy_ssl_name when preserve_host is false and the upstream_url uses an IP?

[thibaultcha]

I would think that the tests here are quite the blocker, since we don't want to have an edge-case indeed... That said, they could be unit for now and simply testing the value of ngx.var.upstream_host after running the access handler... Although I don't think our components are atomic enough for such testing.

[p0pr0ck5]

@thibaultcha fwiw, there are integration (ish) tests here: 3f3b476

[konrade]

It should be possible to merge in your tests @p0pr0ck5 in patch-1 branch in my fork so the PR gets updated right?

[konrade]

Regarding the edge case when preserve_host is false and upstream is https and an IP then the only problem I see with passing on Host header value as SNI is when downstream request is done with IP. This would wrongly put an IP in SNI.

[p0pr0ck5]

Regarding the edge case when preserve_host is false and upstream is https and an IP then the only problem I see with passing on Host header value as SNI is when downstream request is done with IP. This would wrongly put an IP in SNI.

In such a case, Nginx would simply silently strip it from the Client Hello (https://github.com/nginx/nginx/blob/master/src/http/ngx_http_upstream.c#L1792). We could add in our own logic to handle this, but it would just be a waste of CPU cycles.

[konrade]

Would Host header also be passed on to upstream in this case? (I would guess not due to preserve_host being false)
Otherwise there could still be a scenario where upstream web server rejects request due to missmatch between SNI and Host header value.

[p0pr0ck5]

As your PR sits now, there would still be a mismatch between the Host header seen by the upstream, and the SNI seen by the upstream, when preserve_host is false and the upstream_url is built with an IP- the Host header would contain the IP (and port), and the SNI would be blank.

I spose in such a case (IP upstream_url and preserve_host false), we will always have a Host <-> SNI mismatch, because the desired value for the SNI (the Host header) is forbidden by an RFC. Thus I would say that we should document such a case, saying we simply can't support such a configuration at this time.

As for the tests, I'd imagine you can just merge that branch into your PR if you like. No promises someone else won't want some changes though ;)

[p0pr0ck5]

@konrade can you rebase this? We recently made some additions to the Nginx template that are resulting in a conflict here.

BTW we'd like to get this merged soon, thanks again for the contribution; any movements on tests?

[konrade]

Sorry for the delay but I've been traveling the last weeks so haven't been able to look into the tests.
I've rebased my PR branch patch-1 to latest next from Mashape.

[p0pr0ck5]

@konrade thanks for your work and discussion here! We will be merging this in via #2457. Thank you again!

[konrade]

No problem. Happy to help and to see that it got fixed.