feat(router) allow TLS passthrough in stream router #6757

fffonion · 2021-01-21T08:48:15Z

Summary

Adds the ability in route TLS traffic based on SNI without terminating the connection.

To let Kong does normal TLS termination

kong.conf:
stream_listen=0.0.0.0:9000 ssl

kong.yaml
routes:
- protocol: tls
  snis: something

To let Kong does TLS passthrough

kong.conf:
stream_listen=0.0.0.0:9000 ssl

kong.yaml
routes:
- protocol: tls_passthrough
  snis: something

Full changelog

feat(router) allow TLS passthrough in stream router

Issues resolved

Fix #5902 #6694

TODO:

preserve client address when doing second layer proxing
not run plugins in log phase in the preread server block

hbagdi · 2021-01-21T15:30:25Z

I love it when such features can be implemented with a small patch.
Question: Is it possible to mix TLS termination and pass-through (based on the SNI) on the same port? If not, what is the limitation? If yes, how much effort is that?

fffonion · 2021-01-21T17:28:11Z

@hbagdi Having them on the same port is not likely something Nginx support directly, but can be hacked by configuration. If one configures two "layers" of proxy, the first layer doesn't do TLS termination and when it matches certain SNI, it forward it a local port, which is a TLS port. Then theoritically you can have the port in the first layer accepts both type of traffic.

hbagdi · 2021-01-22T08:22:58Z

That sounds like an uncomfortable and a risky hack to me that we should best avoid.

fffonion · 2021-01-22T08:32:20Z

It's actually not that terrible as it looks 😄 and it will be transparent to client. By configuration I was referring to two Routes in Kong, the nginx configuration doesn't need to be changed. Though it's indeed not a common case to use Kong itself as a upstream, but it's a common pattern in Nginx as I heard of.

dndx · 2021-01-22T09:39:10Z

@hbagdi In reality proxying to itself does not impact performance that much. The bulk of the overhead is always inside the TLS termination which has to run expensive cryptography operations. Proxy to self, in reality is just few memcpys and can easily saturate the memory bandwidth without breaking a sweat of the CPU.

I very much feel that we need to support mixing terminated and non-terminated connections on the same port, otherwise the usefulness of Kong TLS proxy will become very limited.

hbagdi · 2021-01-23T20:21:09Z

I very much feel that we need to support mixing terminated and non-terminated connections on the same port, otherwise the usefulness of Kong TLS proxy will become very limited.

I do too. But it has to be transparent to the user, meaning, a user shouldn't need to create two routes in Kong.
If I create 3 routes on the same port in kong:

a route performs TCP termination
a route that does TLS termination for an SNI
a route that does SNI-based proxying without TLS termination (SNI sniffing)

it should just work.

fffonion · 2021-01-25T12:33:16Z

Seems like we need to add more lines in this PR : ) Turning on ssl_preread, or if there's any plugin or Kong logic that tries to peek data in preread phase will block current request in preread phase for protocols where "server sends data before client" (for example SMTP). Kong/Nginx only starts to proxy data from upstream to downstream in phase after preread.

https://github.com/Kong/kong/blob/master/spec/02-integration/05-proxy/22-reports_spec.lua#L41 is an example for this use case.

So I'm proposing to make ssl_preread gated by a new Kong configuration so that user can choose the behaviour of stream proxy. This will not affect HTTP subsystem.

dndx · 2021-01-26T08:13:32Z

Discussed with @fffonion, the following design are proposed:

We keep the L4 configuration exactly the same as today.
When user specifies stream_listen, ssl can be specified for a port as needed (this is the same as today). If the user wants to perform any special TLS handling, like SNI proxy or TLS termination, then the ssl flag must be present.
When ssl flag is present, the preread module will be enabled for that server block. At the same time, another server that listens on a unix domain socket will be created that will handle TLS termination if later needed.
When request comes into a port with ssl flag, the incoming SNI server_name is examined. If termination is required, the request is forwarded via the domain socket to the terminating server block.

The advantage of this method is:

Support the current user config as well as mixed terminating and non-terminating TLS proxy, without need to introduce additional config options
Minimum loss of performance

Note that we do not want to support mixing TLS and cleartext traffic on the same port, for reasons @fffonion already mentioned above. It will inevitable break a lot of cleartext protocol if we enable preread module for cleartext ports. User must always specify in stream_listen whether a port proxies TLS traffic or not, but we can decide whether to terminate the TLS.

hbagdi · 2021-01-26T09:08:06Z

+1 to that.
One question, based on the SNI, how will Kong decide to terminate or TLS-passthrough the request?

fffonion · 2021-01-27T10:30:57Z

@hbagdi there will be two server blocks, since currently we can't control ssl termination dynamically in Lua

match route based on SNI  --- Service is tls --> unix domain socket -> local server block with tls termination -> Upstream
                        \ ___ Service is tcp -> Upstream

hbagdi · 2021-01-27T14:08:58Z

So if I'm understanding correctly, weather to terminate TLS or not will be based on the protocol of Service? That seems odd. Routing decisions should be based on routes only, isn't it?

…

On Wed, Jan 27, 2021, 4:01 PM Wangchong Zhou ***@***.***> wrote: @hbagdi <https://github.com/hbagdi> there will be two server blocks, since currently we can't control ssl termination dynamically in Lua match route based on SNI --- Service is tls --> unix domain socket -> local server block with tls termination -> Upstream \ ___ Service is tcp -> Upstream — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6757 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABD2EOKGLUI7BVWM52C636TS37TPHANCNFSM4WMPNRKA> .

fffonion · 2021-02-01T12:02:34Z

@hbagdi That has similar pattern in http subsystem, Service will determine how Kong "treat" the request.
For example you can route an SNI route to a http service, but Kong will complain it should only be used in https service.
Then whether to serve certificate is based on the protocol of Service, it shouldn't be regarded as a routing decision.

hbagdi · 2021-02-01T12:36:48Z

For example you can route an SNI route to a http service, but Kong will complain it should only be used in https service.

AFAIK, that error is generated at the time of route/service creation and not at runtime.

Then whether to serve certificate is based on the protocol of Service, it shouldn't be regarded as a routing decision.

This doesn't seem correct. In HTTP sub-system, the certificate presented to downstream has nothing to do with Service resource because the route is not known during TLS (and hence Service is not known). I think the stream sub-system works similarly but I don't have enough experience with it to be sure.

In Routes, we have the protocol - tcp or tls, SNI and port. I think what is missing is a field which dictates whether to terminate TLS or not.

haroldoproenca · 2021-05-12T02:04:47Z

Hi, How are you?
I'm facing the same problem with my Kong Ingress Controller and I would like to know when you intend to release this fix.
Thank's,

Haroldo

mayocream · 2021-06-14T05:08:46Z

Any updates on this PR?

fffonion · 2021-06-16T08:57:34Z

@haroldoproenca @mayocream We will revisit this PR in the near future releases, please hold for updates!

ignacioli · 2021-08-09T07:47:16Z

Any updates on this PR?

Thanks

mayocream · 2021-10-25T11:20:41Z

I was waiting for this moment that this PR have new commits!
Great works! 😋

fffonion · 2021-10-25T12:57:34Z

kong/templates/nginx_kong_stream.lua

@@ -131,6 +139,71 @@ server {
    }
 }

+> if stream_proxy_ssl_enabled then


normal request -> first server block -> upstream
tls passthrough request -> second server blocke - proxy_protocol -> third server block -> upstream
tls termination request -> second server block - proxy_protocol -> first server block -> upstream

ideally, when doing tls passthrough, we can directly go from second server block to upstream
but currently we can't turn proxy_protocol on/off from lua side, so there's an extra third
server block to offload the proxy_protocol. we can have something like .kong.tls.disable_proxy_protocol to make this better.

hbagdi · 2021-10-26T16:24:11Z

kong/db/schema/entities/routes_subschemas.lua

  },
  entity_checks = {
    { conditional_at_least_one_of = { if_field = "protocols",
                                      if_match = { elements = { type = "string", one_of = { "tcp", "tls", "udp", } } },
                                      then_at_least_one_of = { "sources", "destinations", "snis" },
-                                      then_err = "must set one of %s when 'protocols' is 'tcp', 'tls' or 'udp'",
+                                      then_err = "must set one of %s when 'protocols' is 'tcp', 'tls', 'tls_passthrough' or 'udp'",


Add tls_pasthrough to the if_match statement (line 34)

need to add some test in schema unit test as well

kong/db/schema/entities/routes.lua

dndx · 2021-11-08T12:40:33Z

kong/runloop/handler.lua

+        local protocols = route.protocols
+        if protocols and protocols.tls then
+          log(DEBUG, "TLS termination required, return to second layer proxying")
+          var.kong_tls_preread_block_upstream = "unix:" .. kong.configuration.prefix .. "/stream_tls_terminate.sock"


This seems to be something we can cache once at load and no need to create new strings for each connection.

dndx · 2021-11-08T12:45:17Z

kong/init.lua

@@ -742,6 +742,12 @@ function Kong.preread()

  runloop.preread.before(ctx)

+  -- if proxying to a second layer TLS terminator is required
+  -- abort further execution and return back to Nginx
+  if ctx.stream_proxy_preread_terminate then


Instead of checking the flag, if we add a return value for runloop.preread.before does that makes more sense? Flags are slower but more important it makes the control flow less clear :)

fffonion added the pr/wip A work in progress PR opened to receive feedback label Jan 21, 2021

fffonion added pr/please review and removed pr/wip A work in progress PR opened to receive feedback labels Jan 25, 2021

fffonion force-pushed the feat/tls-passthrough branch 2 times, most recently from 1534716 to 2b74669 Compare January 25, 2021 11:51

fffonion added pr/wip A work in progress PR opened to receive feedback and removed pr/please review labels Jan 25, 2021

dndx added the pr/discussion This PR is being debated. Probably just a few details. label Jan 26, 2021

gszr mentioned this pull request Oct 13, 2021

Stream Listen Not working as expected #6694

Closed

fffonion force-pushed the feat/tls-passthrough branch 3 times, most recently from 673c96e to c998e72 Compare October 25, 2021 11:16

fffonion force-pushed the feat/tls-passthrough branch 2 times, most recently from 7ee8101 to d705c3d Compare October 25, 2021 12:53

fffonion commented Oct 25, 2021

View reviewed changes

fffonion added pr/please review core/router core/streams Refers to the streams subsystem and removed pr/wip A work in progress PR opened to receive feedback pr/discussion This PR is being debated. Probably just a few details. labels Oct 25, 2021

fffonion force-pushed the feat/tls-passthrough branch 7 times, most recently from 70b90d6 to 52507a2 Compare October 26, 2021 11:59

hbagdi reviewed Oct 26, 2021

View reviewed changes

fffonion added 2 commits November 1, 2021 16:39

feat(router) allow to route TLS request without terminating it

3567008

add more tests

bdec9c2

fffonion force-pushed the feat/tls-passthrough branch from df018d3 to bdec9c2 Compare November 1, 2021 08:39

dndx reviewed Nov 8, 2021

View reviewed changes

fffonion added 2 commits November 8, 2021 21:17

use return value for preread

d2ee8b1

cache socket path

eb665ec

dndx approved these changes Nov 9, 2021

View reviewed changes

fffonion merged commit 8c46e10 into master Nov 9, 2021

fffonion deleted the feat/tls-passthrough branch November 9, 2021 13:22

fffonion mentioned this pull request Nov 11, 2021

doc(router) add docs for TLS passthrough Kong/docs.konghq.com#3373

Merged

fffonion mentioned this pull request Nov 23, 2021

doc(admin-api) add tls_passthrough in route #8099

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(router) allow TLS passthrough in stream router #6757

feat(router) allow TLS passthrough in stream router #6757

fffonion commented Jan 21, 2021 •

edited

hbagdi commented Jan 21, 2021

fffonion commented Jan 21, 2021

hbagdi commented Jan 22, 2021

fffonion commented Jan 22, 2021

dndx commented Jan 22, 2021

hbagdi commented Jan 23, 2021

fffonion commented Jan 25, 2021 •

edited

dndx commented Jan 26, 2021

hbagdi commented Jan 26, 2021

fffonion commented Jan 27, 2021

hbagdi commented Jan 27, 2021 via email

fffonion commented Feb 1, 2021

hbagdi commented Feb 1, 2021

haroldoproenca commented May 12, 2021

mayocream commented Jun 14, 2021

fffonion commented Jun 16, 2021

ignacioli commented Aug 9, 2021

mayocream commented Oct 25, 2021

fffonion Oct 25, 2021

hbagdi Oct 26, 2021

fffonion Oct 26, 2021

dndx Nov 8, 2021

dndx Nov 8, 2021

feat(router) allow TLS passthrough in stream router #6757

feat(router) allow TLS passthrough in stream router #6757

Conversation

fffonion commented Jan 21, 2021 • edited

Summary

Full changelog

Issues resolved

hbagdi commented Jan 21, 2021

fffonion commented Jan 21, 2021

hbagdi commented Jan 22, 2021

fffonion commented Jan 22, 2021

dndx commented Jan 22, 2021

hbagdi commented Jan 23, 2021

fffonion commented Jan 25, 2021 • edited

dndx commented Jan 26, 2021

hbagdi commented Jan 26, 2021

fffonion commented Jan 27, 2021

hbagdi commented Jan 27, 2021 via email

fffonion commented Feb 1, 2021

hbagdi commented Feb 1, 2021

haroldoproenca commented May 12, 2021

mayocream commented Jun 14, 2021

fffonion commented Jun 16, 2021

ignacioli commented Aug 9, 2021

mayocream commented Oct 25, 2021

fffonion Oct 25, 2021

Choose a reason for hiding this comment

hbagdi Oct 26, 2021

Choose a reason for hiding this comment

fffonion Oct 26, 2021

Choose a reason for hiding this comment

dndx Nov 8, 2021

Choose a reason for hiding this comment

dndx Nov 8, 2021

Choose a reason for hiding this comment

fffonion commented Jan 21, 2021 •

edited

fffonion commented Jan 25, 2021 •

edited