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

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

Merged
merged 4 commits into from Nov 9, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions kong/constants.lua
Expand Up @@ -56,6 +56,7 @@ local protocols_with_subsystem = {
tcp = "stream",
tls = "stream",
udp = "stream",
tls_passthrough = "stream",
grpc = "http",
grpcs = "http",
}
Expand Down
7 changes: 4 additions & 3 deletions kong/db/schema/entities/routes.lua
Expand Up @@ -19,7 +19,8 @@ return {
elements = typedefs.protocol,
mutually_exclusive_subsets = {
{ "http", "https" },
{ "tcp", "tls", "udp", },
{ "tcp", "tls", "udp" },
{ "tls_passthrough" },
fffonion marked this conversation as resolved.
Show resolved Hide resolved
{ "grpc", "grpcs" },
},
default = { "http", "https" }, -- TODO: different default depending on service's scheme
Expand Down Expand Up @@ -57,10 +58,10 @@ return {

entity_checks = {
{ conditional = { if_field = "protocols",
if_match = { elements = { type = "string", not_one_of = { "grpcs", "https", "tls" }}},
if_match = { elements = { type = "string", not_one_of = { "grpcs", "https", "tls", "tls_passthrough" }}},
then_field = "snis",
then_match = { len_eq = 0 },
then_err = "'snis' can only be set when 'protocols' is 'grpcs', 'https' or 'tls'",
then_err = "'snis' can only be set when 'protocols' is 'grpcs', 'https', 'tls' or 'tls_passthrough'",
}},
},
}
14 changes: 10 additions & 4 deletions kong/db/schema/entities/routes_subschemas.lua
Expand Up @@ -24,17 +24,22 @@ local stream_subschema = {
name = "tcp",

fields = {
{ methods = typedefs.no_methods { err = "cannot set 'methods' when 'protocols' is 'tcp', 'tls' or 'udp'" } },
{ hosts = typedefs.no_hosts { err = "cannot set 'hosts' when 'protocols' is 'tcp', 'tls' or 'udp'" } },
{ paths = typedefs.no_paths { err = "cannot set 'paths' when 'protocols' is 'tcp', 'tls' or 'udp'" } },
{ headers = typedefs.no_headers { err = "cannot set 'headers' when 'protocols' is 'tcp', 'tls' or 'udp'" } },
{ methods = typedefs.no_methods { err = "cannot set 'methods' when 'protocols' is 'tcp', 'tls', 'tls_passthrough' or 'udp'" } },
{ hosts = typedefs.no_hosts { err = "cannot set 'hosts' when 'protocols' is 'tcp', 'tls', 'tls_passthrough' or 'udp'" } },
{ paths = typedefs.no_paths { err = "cannot set 'paths' when 'protocols' is 'tcp', 'tls', 'tls_passthrough' or 'udp'" } },
{ headers = typedefs.no_headers { err = "cannot set 'headers' when 'protocols' is 'tcp', 'tls', 'tls_passthrough' or 'udp'" } },
},
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'",
}},
{conditional_at_least_one_of = { if_field = "protocols",
if_match = { elements = { type = "string", one_of = { "tls_passthrough" } } },
then_at_least_one_of = { "snis" },
then_err = "must set snis when 'protocols' is 'tls_passthrough'",
}},
},
}

Expand Down Expand Up @@ -67,6 +72,7 @@ return {
tcp = stream_subschema,
tls = stream_subschema,
udp = stream_subschema,
tls_passthrough = stream_subschema,
grpc = grpc_subschema,
grpcs = grpc_subschema,
}
6 changes: 6 additions & 0 deletions kong/init.lua
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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 :)

return
end

local plugins_iterator = runloop.get_updated_plugins_iterator()
execute_plugins_iterator(plugins_iterator, "preread", ctx)

Expand Down
18 changes: 18 additions & 0 deletions kong/pdk/client.lua
Expand Up @@ -25,6 +25,8 @@ local AUTH_AND_LATER = phase_checker.new(PHASES.access,
PHASES.log)
local TABLE_OR_NIL = { ["table"] = true, ["nil"] = true }

local stream_subsystem = ngx.config.subsystem == "stream"


local function new(self)
local _CLIENT = {}
Expand All @@ -48,6 +50,14 @@ local function new(self)
function _CLIENT.get_ip()
check_not_phase(PHASES.init_worker)

-- when proxying TLS request in second layer or doing TLS passthrough
-- realip_remote_addr is always the previous layer of nginx thus always unix:
local tls_passthrough_block = ngx.var.kong_tls_passthrough_block
if stream_subsystem and
((tls_passthrough_block and #tls_passthrough_block > 0) or ngx.var.ssl_protocol) then
return ngx.var.remote_addr
end

return ngx.var.realip_remote_addr or ngx.var.remote_addr
end

Expand Down Expand Up @@ -99,6 +109,14 @@ local function new(self)
function _CLIENT.get_port()
check_not_phase(PHASES.init_worker)

-- when proxying TLS request in second layer or doing TLS passthrough
-- realip_remote_addr is always the previous layer of nginx thus always unix:
local tls_passthrough_block = ngx.var.kong_tls_passthrough_block
if stream_subsystem and
((tls_passthrough_block and #tls_passthrough_block > 0) or ngx.var.ssl_protocol) then
return tonumber(ngx.var.remote_port)
end

return tonumber(ngx.var.realip_remote_port or ngx.var.remote_port)
end

Expand Down
12 changes: 12 additions & 0 deletions kong/router.lua
Expand Up @@ -1897,6 +1897,10 @@ function _M.new(routes)
or tonumber(var.server_port, 10)
-- error value for non-TLS connections ignored intentionally
local sni, _ = server_name()
-- fallback to preread SNI if current connection doesn't terminate TLS
if not sni then
sni = var.ssl_preread_server_name
end

local scheme
if var.protocol == "UDP" then
Expand All @@ -1906,6 +1910,14 @@ function _M.new(routes)
scheme = sni and "tls" or "tcp"
end

-- when proxying TLS request in second layer or doing TLS passthrough
-- rewrite the dst_ip,port back to what specified in proxy_protocol
local tls_passthrough_block = var.kong_tls_passthrough_block
if (tls_passthrough_block and #tls_passthrough_block > 0) or var.ssl_protocol then
dst_ip = var.proxy_protocol_server_addr
dst_port = tonumber(var.proxy_protocol_server_port)
end

return find_route(nil, nil, nil, scheme,
src_ip, src_port,
dst_ip, dst_port,
Expand Down
25 changes: 24 additions & 1 deletion kong/runloop/handler.lua
Expand Up @@ -1113,9 +1113,32 @@ return {
return exit(500)
end

local route = match_t.route
-- if matched route doesn't do tls_passthrough and we are in the preread server block
-- this request should be TLS terminated; return immediately and not run further steps
-- (even bypassing the balancer)
local tls_preread_block = var.kong_tls_preread_block
if tls_preread_block and #tls_preread_block > 0 then
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"
Copy link
Member

Choose a reason for hiding this comment

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

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


elseif protocols and protocols.tls_passthrough then
var.kong_tls_preread_block_upstream = "unix:" .. kong.configuration.prefix .. "/stream_tls_passthrough.sock"

else
log(ERR, "unexpected protocols in matched Route")
return exit(500)
end

ctx.stream_proxy_preread_terminate = true
return
end


ctx.workspace = match_t.route and match_t.route.ws_id

local route = match_t.route
local service = match_t.service
local upstream_url_t = match_t.upstream_url_t

Expand Down
71 changes: 71 additions & 0 deletions kong/templates/nginx_kong_stream.lua
Expand Up @@ -86,17 +86,25 @@ upstream kong_upstream {
}

> if #stream_listeners > 0 then
# non-SSL listeners, and the SSL terminator
server {
> for _, entry in ipairs(stream_listeners) do
> if not entry.ssl then
listen $(entry.listener);
> end
> end

> if stream_proxy_ssl_enabled then
listen unix:${{PREFIX}}/stream_tls_terminate.sock ssl proxy_protocol;
> end

access_log ${{PROXY_STREAM_ACCESS_LOG}};
error_log ${{PROXY_STREAM_ERROR_LOG}} ${{LOG_LEVEL}};

> for _, ip in ipairs(trusted_ips) do
set_real_ip_from $(ip);
> end
set_real_ip_from unix:;

# injected nginx_sproxy_* directives
> for _, el in ipairs(nginx_sproxy_directives) do
Expand Down Expand Up @@ -131,6 +139,69 @@ server {
}
}

> if stream_proxy_ssl_enabled then
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

# SSL listeners, but only preread the handshake here
server {
> for _, entry in ipairs(stream_listeners) do
> if entry.ssl then
listen $(entry.listener:gsub(" ssl", ""));
> end
> end

access_log ${{PROXY_STREAM_ACCESS_LOG}};
error_log ${{PROXY_STREAM_ERROR_LOG}} ${{LOG_LEVEL}};

> for _, ip in ipairs(trusted_ips) do
set_real_ip_from $(ip);
> end

# injected nginx_sproxy_* directives
> for _, el in ipairs(nginx_sproxy_directives) do
$(el.name) $(el.value);
> end

preread_by_lua_block {
Kong.preread()
}

ssl_preread on;

proxy_protocol on;

set $kong_tls_preread_block 1;
set $kong_tls_preread_block_upstream '';
proxy_pass $kong_tls_preread_block_upstream;
}

server {
listen unix:${{PREFIX}}/stream_tls_passthrough.sock proxy_protocol;

access_log ${{PROXY_STREAM_ACCESS_LOG}};
error_log ${{PROXY_STREAM_ERROR_LOG}} ${{LOG_LEVEL}};

set_real_ip_from unix:;

# injected nginx_sproxy_* directives
> for _, el in ipairs(nginx_sproxy_directives) do
$(el.name) $(el.value);
> end

preread_by_lua_block {
Kong.preread()
}

ssl_preread on;

set $kong_tls_passthrough_block 1;

proxy_pass kong_upstream;

log_by_lua_block {
Kong.log()
}
}
> end -- stream_proxy_ssl_enabled

> if database == "off" then
server {
listen unix:${{PREFIX}}/stream_config.sock;
Expand Down
36 changes: 32 additions & 4 deletions spec/01-unit/01-db/01-schema/06-routes_spec.lua
Expand Up @@ -794,7 +794,7 @@ describe("routes schema", function()
local ok, errs = Routes:validate(route)
assert.falsy(ok)
assert.same({
paths = "cannot set 'paths' when 'protocols' is 'tcp', 'tls' or 'udp'",
paths = "cannot set 'paths' when 'protocols' is 'tcp', 'tls', 'tls_passthrough' or 'udp'",
}, errs)
end
end)
Expand All @@ -811,7 +811,7 @@ describe("routes schema", function()
local ok, errs = Routes:validate(route)
assert.falsy(ok)
assert.same({
methods = "cannot set 'methods' when 'protocols' is 'tcp', 'tls' or 'udp'",
methods = "cannot set 'methods' when 'protocols' is 'tcp', 'tls', 'tls_passthrough' or 'udp'",
}, errs)
end
end)
Expand Down Expand Up @@ -1010,7 +1010,7 @@ describe("routes schema", function()
end
end)

it("rejects specifying 'snis' if 'protocols' does not have 'https' or 'tls'", function()
it("rejects specifying 'snis' if 'protocols' does not have 'https', 'tls' or 'tls_passthrough'", function()
local route = Routes:process_auto_fields({
protocols = { "tcp", "udp" },
snis = { "example.org" },
Expand All @@ -1020,7 +1020,7 @@ describe("routes schema", function()
assert.falsy(ok)
assert.same({
["@entity"] = {
"'snis' can only be set when 'protocols' is 'grpcs', 'https' or 'tls'",
"'snis' can only be set when 'protocols' is 'grpcs', 'https', 'tls' or 'tls_passthrough'",
},
snis = "length must be 0",
}, errs)
Expand Down Expand Up @@ -1179,4 +1179,32 @@ describe("routes schema", function()
strip_path = "cannot set 'strip_path' when 'protocols' is 'grpc' or 'grpcs'"
}, errs)
end)

it("errors if tls and tls_passthrough set on a same route", function()
local s = { id = "a4fbd24e-6a52-4937-bd78-2536713072d2" }
local route = Routes:process_auto_fields({
snis = { "foo.grpc.com" },
protocols = { "tls", "tls_passthrough" },
service = s,
}, "insert")
local ok, errs = Routes:validate(route)
assert.falsy(ok)
assert.same({
protocols = "these sets are mutually exclusive: ('tcp', 'tls', 'udp'), ('tls_passthrough')",
}, errs)
end)

it("errors if snis is not set on tls_pasthrough", function()
local s = { id = "a4fbd24e-6a52-4937-bd78-2536713072d2" }
local route = Routes:process_auto_fields({
sources = {{ ip = "127.0.0.1" }},
protocols = { "tls_passthrough" },
service = s,
}, "insert")
local ok, errs = Routes:validate(route)
assert.falsy(ok)
assert.same({
["@entity"] = { "must set snis when 'protocols' is 'tls_passthrough'" },
}, errs)
end)
end)
Expand Up @@ -190,7 +190,7 @@ describe("declarative config: validate", function()
["host"] = "expected a string",
["path"] = "must not have empty segments",
["port"] = "value should be between 0 and 65535",
["protocol"] = "expected one of: grpc, grpcs, http, https, tcp, tls, udp",
["protocol"] = "expected one of: grpc, grpcs, http, https, tcp, tls, tls_passthrough, udp",
["retries"] = "value should be between 0 and 32767",
}
}
Expand Down
2 changes: 1 addition & 1 deletion spec/02-integration/02-cmd/02-start_stop_spec.lua
Expand Up @@ -532,7 +532,7 @@ describe("kong start/stop #" .. strategy, function()
})

assert.falsy(ok)
assert.matches("in 'protocol': expected one of: grpc, grpcs, http, https, tcp, tls, udp", err, nil, true)
assert.matches("in 'protocol': expected one of: grpc, grpcs, http, https, tcp, tls, tls_passthrough, udp", err, nil, true)
assert.matches("in 'name': invalid value '@gobo': the only accepted ascii characters are alphanumerics or ., -, _, and ~", err, nil, true)
assert.matches("in entry 2 of 'hosts': invalid hostname: \\\\99", err, nil, true)
end)
Expand Down
4 changes: 2 additions & 2 deletions spec/02-integration/03-db/02-db_core_entities_spec.lua
Expand Up @@ -1207,13 +1207,13 @@ for _, strategy in helpers.each_strategy() do
strategy = strategy,
message = unindent([[
3 schema violations
('snis' can only be set when 'protocols' is 'grpcs', 'https' or 'tls';
('snis' can only be set when 'protocols' is 'grpcs', 'https', 'tls' or 'tls_passthrough';
must set one of 'methods', 'hosts', 'headers', 'paths' when 'protocols' is 'http';
snis: length must be 0)
]], true, true),
fields = {
["@entity"] = {
"'snis' can only be set when 'protocols' is 'grpcs', 'https' or 'tls'",
"'snis' can only be set when 'protocols' is 'grpcs', 'https', 'tls' or 'tls_passthrough'",
"must set one of 'methods', 'hosts', 'headers', 'paths' when 'protocols' is 'http'",
},
["snis"] = "length must be 0",
Expand Down