Skip to content

Commit

Permalink
duplicate headers are no longer allowed. Test is pending because the …
Browse files Browse the repository at this point in the history
…setup does not allow duplicates
  • Loading branch information
Tieske authored and subnetmarco committed May 31, 2016
1 parent 95e24c2 commit f593c76
Show file tree
Hide file tree
Showing 4 changed files with 118 additions and 27 deletions.
22 changes: 7 additions & 15 deletions kong/plugins/oauth2/access.lua
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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"}
Expand Down Expand Up @@ -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
Expand Down
14 changes: 2 additions & 12 deletions kong/plugins/ssl/handler.lua
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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"})
Expand Down
29 changes: 29 additions & 0 deletions kong/tools/utils.lua
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
80 changes: 80 additions & 0 deletions spec/unit/02-utils_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down

0 comments on commit f593c76

Please sign in to comment.