From f593c76b40358a501bafc8a98538339ed19d736a Mon Sep 17 00:00:00 2001 From: Thijs Schreijer Date: Wed, 25 May 2016 13:49:59 +0200 Subject: [PATCH] duplicate headers are no longer allowed. Test is pending because the setup does not allow duplicates --- kong/plugins/oauth2/access.lua | 22 +++------- kong/plugins/ssl/handler.lua | 14 +----- kong/tools/utils.lua | 29 ++++++++++++ spec/unit/02-utils_spec.lua | 80 ++++++++++++++++++++++++++++++++++ 4 files changed, 118 insertions(+), 27 deletions(-) diff --git a/kong/plugins/oauth2/access.lua b/kong/plugins/oauth2/access.lua index c76a9ca4ac0..1ede8af9ac3 100644 --- a/kong/plugins/oauth2/access.lua +++ b/kong/plugins/oauth2/access.lua @@ -9,6 +9,7 @@ local url = require "socket.url" local Multipart = require "multipart" local string_find = string.find local req_get_headers = ngx.req.get_headers +local check_https = utils.check_https local _M = {} @@ -83,17 +84,6 @@ local function get_redirect_uri(client_id) return client and client.redirect_uri or nil, client end -local HTTPS = "https" - -local function is_https(conf) - local result = ngx.var.scheme:lower() == HTTPS - if not result and conf.accept_http_if_already_terminated then - local forwarded_proto_header = ngx.req.get_headers()["x-forwarded-proto"] - result = forwarded_proto_header and forwarded_proto_header:lower() == HTTPS - end - return result -end - local function retrieve_parameters() ngx.req.read_body() -- OAuth2 parameters could be in both the querystring or body @@ -132,8 +122,9 @@ local function authorize(conf) local state = parameters[STATE] local allowed_redirect_uris, client, redirect_uri, parsed_redirect_uri - if not is_https(conf) then - response_params = {[ERROR] = "access_denied", error_description = "You must use HTTPS"} + local is_https, err = check_https(conf.accept_http_if_already_terminated) + if not is_https then + response_params = {[ERROR] = "access_denied", error_description = err or "You must use HTTPS"} else if conf.provision_key ~= parameters.provision_key then response_params = {[ERROR] = "invalid_provision_key", error_description = "Invalid Kong provision_key"} @@ -252,8 +243,9 @@ local function issue_token(conf) local parameters = retrieve_parameters() local state = parameters[STATE] - if not is_https(conf) then - response_params = {[ERROR] = "access_denied", error_description = "You must use HTTPS"} + local is_https, err = check_https(conf.accept_http_if_already_terminated) + if not is_https then + response_params = {[ERROR] = "access_denied", error_description = err or "You must use HTTPS"} else local grant_type = parameters[GRANT_TYPE] if not (grant_type == GRANT_AUTHORIZATION_CODE or diff --git a/kong/plugins/ssl/handler.lua b/kong/plugins/ssl/handler.lua index 7a6e3431157..2c8517b3fff 100644 --- a/kong/plugins/ssl/handler.lua +++ b/kong/plugins/ssl/handler.lua @@ -3,22 +3,12 @@ local BasePlugin = require "kong.plugins.base_plugin" local responses = require "kong.tools.responses" local cache = require "kong.tools.database_cache" +local check_https = require("kong.tools.utils").check_https local SSLHandler = BasePlugin:extend() SSLHandler.PRIORITY = 3000 -local HTTPS = "https" - -local function is_https(conf) - local result = ngx.var.scheme:lower() == HTTPS - if not result and conf.accept_http_if_already_terminated then - local forwarded_proto_header = ngx.req.get_headers()["x-forwarded-proto"] - result = forwarded_proto_header and forwarded_proto_header:lower() == HTTPS - end - return result -end - function SSLHandler:new() SSLHandler.super.new(self, "ssl") end @@ -50,7 +40,7 @@ end function SSLHandler:access(conf) SSLHandler.super.access(self) - if conf.only_https and not is_https(conf) then + if conf.only_https and not check_https(conf.accept_http_if_already_terminated) then ngx.header["connection"] = { "Upgrade" } ngx.header["upgrade"] = "TLS/1.0, HTTP/1.1" return responses.send(426, {message="Please use HTTPS protocol"}) diff --git a/kong/tools/utils.lua b/kong/tools/utils.lua index 969eaf8004a..e2241ca5d92 100644 --- a/kong/tools/utils.lua +++ b/kong/tools/utils.lua @@ -118,6 +118,35 @@ function _M.encode_args(args, raw) return table_concat(query, "&") end +--- Checks whether a request is https or was originally https (but already terminated). +-- It will check in the current request (global `ngx` table). If the header `X-Forwarded-Proto` exists +-- with value `https` then it will also be considered as an https connection. +-- @param allow_terminated if truthy, the `X-Forwarded-Proto` header will be checked as well. +-- @return boolean or nil+error in case the header exists multiple times +_M.check_https = function(allow_terminated) + if ngx.var.scheme:lower() == "https" then + return true + end + + if not allow_terminated then + return false + end + + local forwarded_proto_header = ngx.req.get_headers()["x-forwarded-proto"] + if tostring(forwarded_proto_header):lower() == "https" then + return true + end + + if type(forwarded_proto_header) == "table" then + -- we could use the first entry (lower security), or check the contents of each of them (slow). So for now defensive, and error + -- out on multiple entries for the x-forwarded-proto header. + return nil, "Only one X-Forwarded-Proto header allowed" + end + + return false +end + + --- Calculates a table size. -- All entries both in array and hash part. -- @param t The table to use diff --git a/spec/unit/02-utils_spec.lua b/spec/unit/02-utils_spec.lua index c3ddcbd3016..a2bee2fb11e 100644 --- a/spec/unit/02-utils_spec.lua +++ b/spec/unit/02-utils_spec.lua @@ -6,6 +6,86 @@ describe("Utils", function() assert.truthy(utils.get_hostname()) end) + describe("https_check", function() + + local old_ngx + local headers = {} + + setup(function() + old_ngx = ngx + _G.ngx = { + var = { + scheme = nil + }, + req = { + get_headers = function() return headers end + } + } + end) + + teardown(function() + _G.ngx = old_ngx + end) + + describe("without X-Forwarded-Proto header", function() + setup(function() + headers["x-forwarded-proto"] = nil + end) + + it("should validate an HTTPS scheme", function() + ngx.var.scheme = "hTTps" -- mixed casing to ensure case insensitiveness + assert.is.truthy(utils.check_https()) + end) + + it("should invalidate non-HTTPS schemes", function() + ngx.var.scheme = "hTTp" + assert.is.falsy(utils.check_https()) + ngx.var.scheme = "something completely different" + assert.is.falsy(utils.check_https()) + end) + + it("should invalidate non-HTTPS schemes with proto header allowed", function() + ngx.var.scheme = "hTTp" + assert.is.falsy(utils.check_https(true)) + end) + end) + + describe("with X-Forwarded-Proto header", function() + + teardown(function() + headers["x-forwarded-proto"] = nil + end) + + it("should validate any scheme with X-Forwarded_Proto as HTTPS", function() + headers["x-forwarded-proto"] = "hTTPs" -- check mixed casing for case insensitiveness + ngx.var.scheme = "hTTps" + assert.is.truthy(utils.check_https(true)) + ngx.var.scheme = "hTTp" + assert.is.truthy(utils.check_https(true)) + ngx.var.scheme = "something completely different" + assert.is.truthy(utils.check_https(true)) + end) + + it("should validate only https scheme with X-Forwarded_Proto as non-HTTPS", function() + headers["x-forwarded-proto"] = "hTTP" + ngx.var.scheme = "hTTps" + assert.is.truthy(utils.check_https(true)) + ngx.var.scheme = "hTTp" + assert.is.falsy(utils.check_https(true)) + ngx.var.scheme = "something completely different" + assert.is.falsy(utils.check_https(true)) + end) + + it("should return an error with multiple X-Forwarded_Proto headers", function() + headers["x-forwarded-proto"] = { "hTTP", "https" } + ngx.var.scheme = "hTTps" + assert.is.truthy(utils.check_https(true)) + ngx.var.scheme = "hTTp" + assert.are.same({ nil, "Only one X-Forwarded-Proto header allowed" }, { utils.check_https(true) }) + end) + end) + end) + describe("string", function() describe("random_string()", function() it("should return a random string", function()