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(proxy) X-Forwarded-* upstream headers #2236

Closed
wants to merge 17 commits into from
Closed
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,23 @@
## [Unreleased][unreleased]

### Changed

- :warning: The default configuration for `X-Forwarded-*` and `X-Real-IP`
upstream headers was changed considerably. Previously Kong trusted and
forwarded those headers by default. With this release we do **not** trust
those headers anymore by default. This is all configurable by now. Please
read more about this change from our
[0.10.x Proxy Reference](https://getkong.org/docs/0.10.x/proxy/)
and
[0.10.x Configuration Reference](https://getkong.org/docs/0.10.x/configuration/)
Copy link
Member

Choose a reason for hiding this comment

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

note to self: slightly rephrase for merge


### Added

- :fireworks: Configurable `X-Forwarded-*` and `X-Real-IP` upstream headers.
[#2236](https://github.com/Mashape/kong/pull/2236)
- :fireworks: Support for the PROXY protocol.
[#2236](https://github.com/Mashape/kong/pull/2236)

## [0.10.1] - 2017/03/27

### Changed
Expand Down
2 changes: 2 additions & 0 deletions kong-0.10.1-0.rockspec
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ dependencies = {
"lua_pack == 1.0.4",
"lua-resty-dns-client == 0.3.2",
"lua-resty-worker-events == 0.3.0",
"lua-resty-mediador == 0.1.2",
}
build = {
type = "builtin",
Expand Down Expand Up @@ -79,6 +80,7 @@ build = {
["kong.api.routes.certificates"] = "kong/api/routes/certificates.lua",
["kong.api.routes.snis"] = "kong/api/routes/snis.lua",

["kong.tools.ip"] = "kong/tools/ip.lua",
["kong.tools.dns"] = "kong/tools/dns.lua",
["kong.tools.utils"] = "kong/tools/utils.lua",
["kong.tools.public"] = "kong/tools/public.lua",
Expand Down
19 changes: 19 additions & 0 deletions kong.conf.default
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,25 @@
# process. When this number is exceeded, the
# least recently used connections are closed.

#real_ip_header = X-Real-IP # Defines the request header field whose value
# will be used to replace the client address.
# Note: See http://nginx.org/en/docs/http/ngx_http_realip_module.html#real_ip_header
# for a description of this directive.

#real_ip_recursive = off # Sets the ngx_http_realip_module directive of
# the same name.
# Note: See http://nginx.org/en/docs/http/ngx_http_realip_module.html#real_ip_recursive
# for a description of this directive.

#trusted_ips = # Defines trusted addresses that are known
# to send correct replacement addresses.
# If the special value unix: is specified,
# all UNIX-domain sockets will be trusted.
# This directive accepts a comma-separated
# list of values.
# Note: See http://nginx.org/en/docs/http/ngx_http_realip_module.html for a
# list of accepted values.

#------------------------------------------------------------------------------
# DATASTORE
#------------------------------------------------------------------------------
Expand Down
27 changes: 20 additions & 7 deletions kong/conf_loader.lua
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ local pl_path = require "pl.path"
local tablex = require "pl.tablex"
local utils = require "kong.tools.utils"
local log = require "kong.cmd.utils.log"
local ip = require "kong.tools.ip"

local DEFAULT_PATHS = {
"/etc/kong/kong.conf",
Expand Down Expand Up @@ -62,6 +63,9 @@ local CONF_INFERENCES = {
nginx_user = {typ = "string"},
nginx_worker_processes = {typ = "string"},
upstream_keepalive = {typ = "number"},
real_ip_header = {typ = "string"},
real_ip_recursive = {typ = "ngx_boolean"},
trusted_ips = {typ = "array"},

database = {enum = {"postgres", "cassandra"}},
pg_port = {typ = "number"},
Expand Down Expand Up @@ -224,16 +228,16 @@ local function check_and_infer(conf)
end
end

local ip, port = utils.normalize_ipv4(conf.cluster_listen)
if not (ip and port) then
local address, port = utils.normalize_ipv4(conf.cluster_listen)
if not (address and port) then
errors[#errors+1] = "cluster_listen must be in the form of IPv4:port"
end
ip, port = utils.normalize_ipv4(conf.cluster_listen_rpc)
if not (ip and port) then
address, port = utils.normalize_ipv4(conf.cluster_listen_rpc)
if not (address and port) then
errors[#errors+1] = "cluster_listen_rpc must be in the form of IPv4:port"
end
ip, port = utils.normalize_ipv4(conf.cluster_advertise or "")
if conf.cluster_advertise and not (ip and port) then
address, port = utils.normalize_ipv4(conf.cluster_advertise or "")
if conf.cluster_advertise and not (address and port) then
errors[#errors+1] = "cluster_advertise must be in the form of IPv4:port"
end
if conf.cluster_ttl_on_failure < 60 then
Expand All @@ -243,6 +247,15 @@ local function check_and_infer(conf)
conf.lua_package_cpath = ""
end

-- Checking the trusted ips
for _, address in ipairs(conf.trusted_ips) do
if not ip.valid(address) and not address == "unix:" then
errors[#errors+1] = "trusted_ips must be a comma separated list in "..
"the form of IPv4 or IPv6 address or CIDR "..
"block or 'unix:', got '" .. address .. "'"
end
end

return #errors == 0, errors[1], errors
end

Expand Down Expand Up @@ -425,7 +438,7 @@ local function load(path, custom_conf)
-- initialize the dns client, so the globally patched tcp.connect method
-- will work from here onwards.
assert(require("kong.tools.dns")(conf))

return setmetatable(conf, nil) -- remove Map mt
end

Expand Down
20 changes: 19 additions & 1 deletion kong/core/handler.lua
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ local balancer_execute = require("kong.core.balancer").execute


local router, router_err
local tostring = tostring
local ngx = ngx
local ngx_now = ngx.now
local server_header = _KONG._NAME.."/".._KONG._VERSION

Expand Down Expand Up @@ -119,9 +121,25 @@ return {
end

-- if set `host_header` is the original header to be preserved
var.upstream_host = host_header or
var.upstream_host = host_header or
balancer_address.hostname..":"..balancer_address.port

-- X-Forwarded Headers
local realip_remote_addr = var.realip_remote_addr

if not singletons.ip.trusted(realip_remote_addr) then
var.upstream_x_forwarded_proto = var.scheme
var.upstream_x_forwarded_host = var.host
var.upstream_x_forwarded_port = var.server_port
end

local http_x_forwarded_for = var.http_x_forwarded_for
if http_x_forwarded_for then
var.upstream_x_forwarded_for = http_x_forwarded_for .. ", " .. realip_remote_addr

else
var.upstream_x_forwarded_for = var.remote_addr
Copy link
Member

Choose a reason for hiding this comment

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

I think some comments here would be very beneficial to avoid future confusion, regarding the usage of realip_remote_addr vs var.remove_addr, and why we need to build the X-Forwarded-For upstream header ourselves (due to the ngx_http_proxy_module limitation we discovered).

Copy link
Member Author

Choose a reason for hiding this comment

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

Comments were added.

end
end,
-- Only executed if the `router` module found an API and allows nginx to proxy it.
after = function()
Expand Down
2 changes: 2 additions & 0 deletions kong/kong.lua
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

require("kong.core.globalpatches")()

local ip = require "kong.tools.ip"
local dns = require "kong.tools.dns"
local core = require "kong.core.handler"
local Serf = require "kong.serf"
Expand Down Expand Up @@ -134,6 +135,7 @@ function Kong.init()
assert(dao:run_migrations()) -- migrating in case embedded in custom nginx

-- populate singletons
singletons.ip = ip.init(config)
singletons.dns = dns(config)
singletons.loaded_plugins = assert(load_plugins(config, dao, events))
singletons.serf = Serf.new(config, dao)
Expand Down
1 change: 1 addition & 0 deletions kong/singletons.lua
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ local _M = {
configuration = nil,
events = nil,
dao = nil,
ip = nil,
dns = nil,
serf = nil,
loaded_plugins = nil
Expand Down
3 changes: 3 additions & 0 deletions kong/templates/kong_defaults.lua
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ admin_ssl = on
admin_ssl_cert = NONE
admin_ssl_cert_key = NONE
upstream_keepalive = 60
real_ip_header = X-Real-IP
real_ip_recursive = off
trusted_ips = NONE

database = postgres
pg_host = 127.0.0.1
Expand Down
62 changes: 45 additions & 17 deletions kong/templates/nginx_kong.lua
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,6 @@ client_max_body_size 0;
proxy_ssl_server_name on;
underscores_in_headers on;

real_ip_header X-Forwarded-For;
set_real_ip_from 0.0.0.0/0;
real_ip_recursive on;

lua_package_path '${{LUA_PACKAGE_PATH}};;';
lua_package_cpath '${{LUA_PACKAGE_CPATH}};;';
lua_code_cache ${{LUA_CODE_CACHE}};
Expand Down Expand Up @@ -65,25 +61,48 @@ upstream kong_upstream {
}

map $http_upgrade $upstream_connection {
default keep-alive;
default keep-alive;
websocket upgrade;
}

map $http_upgrade $upstream_upgrade {
default '';
default '';
websocket websocket;
}

map $http_x_forwarded_proto $upstream_x_forwarded_proto {
default $http_x_forwarded_proto;
'' $scheme;
}

map $http_x_forwarded_host $upstream_x_forwarded_host {
default $http_x_forwarded_host;
'' $host;
}

map $http_x_forwarded_port $upstream_x_forwarded_port {
default $http_x_forwarded_port;
'' $server_port;
}
Copy link
Member

Choose a reason for hiding this comment

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

Any reason why we're not handling those in the Lua logic but need those maps again instead? I don't remember if there was a special case that they handle in lieu of an else in the Lua code?

Copy link
Member Author

Choose a reason for hiding this comment

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

We just initialize the vars with defaults. We can do it in Lua land as well.

Copy link
Member

Choose a reason for hiding this comment

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

That would avoid having pieces of the logic in two different places indeed


server {
server_name kong;
> if real_ip_header == "proxy_protocol" then
listen ${{PROXY_LISTEN}} proxy_protocol;
> else
listen ${{PROXY_LISTEN}};
> end
error_page 404 408 411 412 413 414 417 /kong_error_handler;
error_page 500 502 503 504 /kong_error_handler;

access_log logs/access.log;

> if ssl then
> if real_ip_header == "proxy_protocol" then
listen ${{PROXY_LISTEN_SSL}} proxy_protocol ssl;
> else
listen ${{PROXY_LISTEN_SSL}} ssl;
> end
ssl_certificate ${{SSL_CERT}};
ssl_certificate_key ${{SSL_CERT_KEY}};
ssl_protocols TLSv1.1 TLSv1.2;
Expand All @@ -92,24 +111,33 @@ server {
}
> end

real_ip_header ${{REAL_IP_HEADER}};
real_ip_recursive ${{REAL_IP_RECURSIVE}};
> for i = 1, #trusted_ips do
set_real_ip_from $(trusted_ips[i]);
> end

location / {
set $upstream_host nil;
set $upstream_scheme nil;
set $upstream_host nil;
set $upstream_scheme nil;
set $upstream_x_forwarded_for nil;

access_by_lua_block {
kong.access()
}

proxy_http_version 1.1;
proxy_set_header X-Real-IP $remote_addr;
proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
proxy_set_header X-Forwarded-Proto $scheme;
proxy_set_header Host $upstream_host;
proxy_set_header Upgrade $upstream_upgrade;
proxy_set_header Connection $upstream_connection;

proxy_pass_header Server;
proxy_pass $upstream_scheme://kong_upstream;
proxy_set_header X-Real-IP $remote_addr;
proxy_set_header X-Forwarded-For $upstream_x_forwarded_for;
proxy_set_header X-Forwarded-Proto $upstream_x_forwarded_proto;
proxy_set_header X-Forwarded-Host $upstream_x_forwarded_host;
proxy_set_header X-Forwarded-Port $upstream_x_forwarded_port;
proxy_set_header Host $upstream_host;
proxy_set_header Upgrade $upstream_upgrade;
proxy_set_header Connection $upstream_connection;
proxy_pass_header Server;
proxy_pass_header Date;
Copy link
Member

Choose a reason for hiding this comment

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

note: We should also mention this in the changelog and add it to the proxy guide list of forwarded headers from upstream

proxy_pass $upstream_scheme://kong_upstream;

header_filter_by_lua_block {
kong.header_filter()
Expand Down
56 changes: 56 additions & 0 deletions kong/tools/ip.lua
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
local ip = require "resty.mediador.ip"
local px = require "resty.mediador.proxy"


local function trust_all() return true end
local function trust_none() return false end


local _M = { valid = ip.valid }


function _M.init(config)
local ips = config.trusted_ips
local count = #ips
local trusted_ips = {}
local trust_all_ipv4, trust_all_ipv6

-- This is because we don't support unix: that the ngx_http_realip module supports.
-- Also as an optimization we will only compile trusted ips if the Kong is not run
-- with the default 0.0.0.0/0, ::/0 aka trust all ip addresses settings.
for i = 1, count do
local address = ips[i]

if ip.valid(address) then
trusted_ips[#trusted_ips+1] = address
if address == "0.0.0.0/0" then
trust_all_ipv4 = true

elseif address == "::/0" then
trust_all_ipv6 = true
end
end
end

if #trusted_ips == 0 then
return {
valid = ip.valid,
trusted = trust_none,
}
end

if trust_all_ipv4 and trust_all_ipv6 then
Copy link
Member

Choose a reason for hiding this comment

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

note: we need to make sure to document that one needs both ipv4 and ipv6 forms specified to "trust all"

(but tbh, I feel like we should respect the --with-ipv6 option of Nginx, and ignore all IPv6 settings/values if support is not enabled in Nginx)

Copy link
Member Author

Choose a reason for hiding this comment

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

Wasn't it so that --with-ipv6 was dropped already from Nginx mainline, or does OpenResty still continue having them somehow?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah sorry I wasn't very clear here. Yes, --with-ipv6 was dropped. What I meant is split in 2 parts:

  • I think that users should be able to enable/disable ipv6 support through Kong
  • Currently doable via the --with-nginx option check in the current OpenResty (I think 1.11.2still has it? not sure?), but in the future, the need for disabling ipv6 will be even greater, because the OS might support ipv6, but not the network, and we cannot rely on the --with-ipv6 option anymore to assume what the user wanted, hence, it simply increased the need for this behavior to be configurable.

Copy link
Member Author

Choose a reason for hiding this comment

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

return {
valid = ip.valid,
trusted = trust_all,
}
end

return {
valid = ip.valid,
trusted = px.compile(trusted_ips),
}
end


return _M
Loading