-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Dual stack HTTP+HTTPS routes cannot contain SNI match criteria while still honoring their protocols criteria #6425
Comments
@rainest definitely sounds like a bug. Given that Thanks for the detailed description! |
Schema rejection alone won't handle it effectively, unfortunately--both cases where this really matters (non-mTLS dual-stack route that has SNI criteria to avoid the catch-all certificate prompt and HTTPS-only route that has redirect configuration) won't be solved by that. On the opposite end, we could consider requiring SNI criteria on routes with mTLS plugins and remove the catch-all (i.e. if a request does not include SNI, or includes SNI for any hostname other than one known to require mTLS, mTLS doesn't activate and does not request a client cert), though I don't know whether we'd have a good mechanism to enforce it. Those routes could still benefit from HTTP-to-HTTPS redirects, and taking that option while preserving the existing router logic means they can't have it. |
@rainest After reading through the issue description I think I understand the issue described here. IMO ignoring the SNI matching criteria for HTTP sounds like the reasonable route to me, I wouldn't expect SNI to be used as a way of distinguishing HTTP/HTTPS requests ( @hishamhm what's your thoughts on this? |
@dndx I agree with the general approach. My only piece of advice is to watch out for corner cases to avoid changing existing behavior. After playing a bit with this topic, this was one interesting scenario I bumped into, for instance:
Current behavior is that hitting Another behavior which we need to take care not to change is this:
Here, if we have routes I suspect there may be other scenarios to watch out for, but I think this gives an idea of the kind of tricky situations that may arise. |
Disable automatic SNI criteria addition by commenting out the code that adds it. This is a useful convenience feature, but would cause breakage because of Kong/kong#6425 Once that issue is fixed, we should re-enable this functionality, but gate it with a flag, to avoid issues with older Kong versions.
…fixes #6425 Co-authored-by: Enrique García Cota <kikito@gmail.com>
Disable automatic SNI criteria addition by commenting out the code that adds it. This is a useful convenience feature, but would cause breakage because of Kong/kong#6425 Once that issue is fixed, we should re-enable this functionality, but gate it with a flag, to avoid issues with older Kong versions.
Disable automatic SNI criteria addition by commenting out the code that adds it. This is a useful convenience feature, but would cause breakage because of Kong/kong#6425 Once that issue is fixed, we should re-enable this functionality, but gate it with a flag, to avoid issues with older Kong versions.
Disable automatic SNI criteria addition by commenting out the code that adds it. This is a useful convenience feature, but would cause breakage because of Kong/kong#6425 Once that issue is fixed, we should re-enable this functionality, but gate it with a flag, to avoid issues with older Kong versions.
* feat(transl) add SNI match criteria to routes Add SNI match criteria to Kong routes when the source Ingress has a TLS rule with hostnames. * feat(override) add SNI annotation and KongIngress Add an Ingress annotation and KongIngress functionality for setting route SNI match criteria. Although the controller adds SNI criteria automatically for Ingresses that contain rules with hostnames and have TLS information, there are some scenarios that require setting SNI manually: * You wish to strip SNI match criteria from the route, in order to support SNI-incapable clients. SNI annotation processing is unusual to accommodate this: providing a "konghq.com/snis: ''" annotation is distinct from not providing the annotation at all, and instructs the controller to strip SNI information that it'd normally add automatically. * You use wildcard hostnames in your rules. Kong does not allow wildcard match criteria as of 2.1, and as such you must enumerate the specific SNI matches you want when using a wildcard hostname. * You wish to use a completely different SNI match criteria that does not match your rule hostnames at all. This is highly atypical, and should only be done if you know exactly why you need to do it. * feat(transl) disable automatic SNI handling Disable automatic SNI criteria addition by commenting out the code that adds it. This is a useful convenience feature, but would cause breakage because of Kong/kong#6425 Once that issue is fixed, we should re-enable this functionality, but gate it with a flag, to avoid issues with older Kong versions. * chore(deploy) update manifests * pr(anns) use consistent pluralization * fix(anns) use nil in place of empty string slice * pr(parser) use consistent caps and label unsafe Use SNI (rather than sni) throughout, and prefix which version is unsafe rather than which version is safe. * pr(parser) remove temporary commented code * test(anns) expect nil slice for empty annotations * test(transl) add E2E test for SNI annotation * chore(deploy) update single manifests * pr: add 404 test * pr: fix magic number * test(sni): add cases: 404 and server cert Co-authored-by: Michał Flendrich <michal@flendrich.pro>
Summary
Given a route that supports both HTTP and HTTPS requests, and has both hostname and SNI match criteria (for the same hostname), an HTTP request with the correct hostname will not match the given route. There is no way for any HTTP request to match that route, because plaintext HTTP requests will never contain SNI because they have no TLS envelope by definition.
Steps To Reproduce
sni.kong.example
, and one with hostname criteria only, fornosni.kong.example
. As no protocol set is explicitly specified, these routes will default toprotocols=http,https
curl -sv localhost:8000/anything -H "Host: sni.kong.example"
. Kong will return a 404.curl -sv localhost:8000/anything -H "Host: nosni.kong.example"
. The route will proxy normally, and return a 200 (or whatever status the upstream service normally returns).sni-404-example.yaml.txt is a deck dump that covers the above. Github is silly and forced me to append the
.txt
extension.Note that this also applies to HTTPS-only routes with an HTTPS redirect status code: once you add SNI criteria, those routes will no longer redirect to HTTPS when receiving an otherwise-matching HTTP request, and clients sending those requests will instead receive a 404.
Additional Details & Logs
Tested on kong/2.2.0beta.1, Alpine Docker. Should affect any Kong version/platform that supports SNI match criteria.
Why this matters
See previous internal discussion in FTI-1881 and comments on Kong/kubernetes-ingress-controller#863. On the controller side, we would like to handle adding SNI criteria for users automatically, as we can reasonably assume that Ingresses with a hostname rule and TLS information will be HTTPS-accessible, and will receive HTTPS requests with SNI information that have a matching HTTP hostname. There are some caveats to this, but I won't re-tread them here.
However, many of those rules will also be dual-stack, i.e. they will allow HTTP traffic alongside HTTPS traffic. As-is, adding SNI criteria to these routes forces them to single-stack HTTPS, because adding SNI criteria to a route means any request that wants to match that route must include a TLS client handshake with SNI, even when that is impossible. This also comes up in the context of SNI-incapable clients, which will do TLS when using HTTPS, but will not use SNI.
In the context of FTI-1881 (and the previous requests that spurred development of the current mTLS behavior), this doesn't matter for the routes that actually have an mTLS plugin: if you require client certificate authentication to access a route, that route must be single-stack anyway--even assuming this issue didn't exist, clients would not be able to successfully authenticate over HTTP. This does matter for all the routes that do not have an mTLS plugin, where we want to avoid prompting for a client certificate because we know that route does not have an mTLS plugin, and we know the route based on SNI information. Routes in the latter category can quite reasonably also allow HTTP traffic, so as-is we force users into a choice of "accept the certificate prompts for HTTPS, which are confusing for your end users" or "don't allow HTTP traffic to these routes at all".
There's arguably a third way where we split dual-stack routes into separate HTTPS and HTTP routes for the same service, but that's cumbersome and brings in a whole host of other issues (wanna use a stateful plugin, like rate-limiting-advanced or session? you're in for a fun time).
The text was updated successfully, but these errors were encountered: