From cbc3846129086f918b8a915b91b1638c685e2b06 Mon Sep 17 00:00:00 2001 From: Thijs Schreijer Date: Tue, 28 Feb 2017 21:14:58 +0100 Subject: [PATCH 1/3] fix(key-auth): validate the configured headernames fixes #2013 adds validation of header names (was completely absent) due to nginx/openresty config the '_' is also considered an invalid character. --- kong/plugins/key-auth/schema.lua | 28 +++++++++-- kong/tools/utils.lua | 24 +++++++++ spec/01-unit/04-utils_spec.lua | 26 +++++++++- spec/03-plugins/02-key-auth/01-api_spec.lua | 55 +++++++++++++++++++-- 4 files changed, 126 insertions(+), 7 deletions(-) diff --git a/kong/plugins/key-auth/schema.lua b/kong/plugins/key-auth/schema.lua index 23484fb7923..6d12a9c4486 100644 --- a/kong/plugins/key-auth/schema.lua +++ b/kong/plugins/key-auth/schema.lua @@ -8,6 +8,16 @@ local function check_user(anonymous) return false, "the anonymous user must be empty or a valid uuid" end +local function check_keys(keys) + for _, key in ipairs(keys) do + local res, err = utils.validate_header_name(key, false) + if not res then + return false, "key_name '"..key.."' is illegal, "..tostring(err) + end + end + return true +end + local function default_key_names(t) if not t.key_names then return {"apikey"} @@ -17,8 +27,20 @@ end return { no_consumer = true, fields = { - key_names = {required = true, type = "array", default = default_key_names}, - hide_credentials = {type = "boolean", default = false}, - anonymous = {type = "string", default = "", func = check_user}, + key_names = { + required = true, + type = "array", + default = default_key_names, + func = check_keys, + }, + hide_credentials = { + type = "boolean", + default = false, + }, + anonymous = { + type = "string", + default = "", + func = check_user, + }, } } diff --git a/kong/tools/utils.lua b/kong/tools/utils.lua index e3219db1a94..a2c08f54073 100644 --- a/kong/tools/utils.lua +++ b/kong/tools/utils.lua @@ -24,6 +24,7 @@ local concat = table.concat local insert = table.insert local lower = string.lower local fmt = string.format +local match = string.match local find = string.find local gsub = string.gsub local split = pl_stringx.split @@ -583,4 +584,27 @@ _M.format_host = function(p1, p2) end end +-- patterns for allowed header characters +local header_chars = [[!#$%%&'%*%+%-%.0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ^`abcdefghijklmnopqrstuvwxyz|~]] +local header_allowed = "^["..header_chars.."]+$" +local header_allowed_u = "^["..header_chars.."_]+$" -- adds underscore + +--- Validates a header name. +-- Checks characters used in a header name to be valid. +-- @param name (string) the header name to verify +-- @param allow_underscore (boolean, optional) if thruthy and `_` (underscore) is +-- accepted, otherwise it will fail. +-- @return the valid header name, or `nil+error` +_M.validate_header_name = function(name, allow_underscore) + -- allow_underscore is used because openresty by default changes "_" into "-" + if name == nil or name == "" then + return nil, "no header name provided" + end + if match(name, allow_underscore and header_allowed_u or header_allowed) then + return name + end + return nil, "only the following characters are allowed for header names: ".. + (allow_underscore and header_allowed_u or header_allowed) +end + return _M diff --git a/spec/01-unit/04-utils_spec.lua b/spec/01-unit/04-utils_spec.lua index 287505c71bb..8f08641b1df 100644 --- a/spec/01-unit/04-utils_spec.lua +++ b/spec/01-unit/04-utils_spec.lua @@ -458,5 +458,29 @@ describe("Utils", function() end) end) end) - + + describe("validate_header_name()", function() + it("validates header names without underscore", function() + local header_chars = [[%!#$&'*+-.0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ^`abcdefghijklmnopqrstuvwxyz|~]] + for i = 1, 255 do + local c = string.char(i) + if string.find(header_chars, c, nil, true) then + assert(utils.validate_header_name(c) == c, "ascii character '"..c.."' ("..i..") should have been allowed") + else + assert(utils.validate_header_name(c) == nil, "ascii character "..i.." should not have been allowed") + end + end + end) + it("validates header names with underscore", function() + local header_chars = [[%!#$&'*+-.0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ^`abcdefghijklmnopqrstuvwxyz|~_]] + for i = 1, 255 do + local c = string.char(i) + if string.find(header_chars, c, nil, true) then + assert(utils.validate_header_name(c, true) == c, "ascii character '"..c.."' ("..i..") should have been allowed") + else + assert(utils.validate_header_name(c, true) == nil, "ascii character "..i.." should not have been allowed") + end + end + end) + end) end) diff --git a/spec/03-plugins/02-key-auth/01-api_spec.lua b/spec/03-plugins/02-key-auth/01-api_spec.lua index 0e291795289..93dd35f3229 100644 --- a/spec/03-plugins/02-key-auth/01-api_spec.lua +++ b/spec/03-plugins/02-key-auth/01-api_spec.lua @@ -5,12 +5,21 @@ describe("Plugin: key-auth (API)", function() local consumer local admin_client setup(function() - assert(helpers.start_kong()) - admin_client = helpers.admin_client() - + assert(helpers.dao.apis:insert { + name = "keyauth1", + upstream_url = "http://mockbin.com", + hosts = { "keyauth1.test" }, + }) + assert(helpers.dao.apis:insert { + name = "keyauth2", + upstream_url = "http://mockbin.com", + hosts = { "keyauth2.test" }, + }) consumer = assert(helpers.dao.consumers:insert { username = "bob" }) + assert(helpers.start_kong()) + admin_client = helpers.admin_client() end) teardown(function() if admin_client then admin_client:close() end @@ -254,4 +263,44 @@ describe("Plugin: key-auth (API)", function() end) end) end) + describe("/apis/:api/plugins", function() + it("fails with invalid key_names", function() + local key_name = "hello\\world" + local res = assert(admin_client:send { + method = "POST", + path = "/apis/keyauth1/plugins", + body = { + name = "key-auth", + config = { + key_names = {key_name}, + }, + }, + headers = { + ["Content-Type"] = "application/json" + } + }) + assert.response(res).has.status(400) + local body = assert.response(res).has.jsonbody() + assert.equal("key_name 'hello\\world' is illegal, only the following characters are allowed for header names: ^[!#$%%&'%*%+%-%.0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ^`abcdefghijklmnopqrstuvwxyz|~]+$", body["config.key_names"]) + end) + it("succeeds with valid key_names", function() + local key_name = "hello-world" + local res = assert(admin_client:send { + method = "POST", + path = "/apis/keyauth2/plugins", + body = { + name = "key-auth", + config = { + key_names = {key_name}, + }, + }, + headers = { + ["Content-Type"] = "application/json" + } + }) + assert.response(res).has.status(201) + local body = assert.response(res).has.jsonbody() + assert.equal(key_name, body.config.key_names[1]) + end) + end) end) From b274121ffb5b358d04afc8117b2ff9264697cbfb Mon Sep 17 00:00:00 2001 From: Thijs Schreijer Date: Tue, 28 Feb 2017 21:51:30 +0100 Subject: [PATCH 2/3] added changelog entry --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 255008ee01e..8fd7dab87c5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -118,6 +118,8 @@ perform significantly better than any previous version. - CORS: Properly return `Access-Control-Allow-Credentials: false` if `Access-Control-Allow-Origin: *`. [#2104](https://github.com/Mashape/kong/pull/2104) + - key-auth: validate the `key_names` to be proper header names + [#2142](https://github.com/Mashape/kong/pull/2142) ## [0.9.7] - 2016/12/21 From ae5cc86810c9bf7ae383a3f4fa519aa3aa4715e5 Mon Sep 17 00:00:00 2001 From: Thijs Schreijer Date: Thu, 2 Mar 2017 10:12:33 +0100 Subject: [PATCH 3/3] fix(review): updated review comments * switched to nginx validation instead of rfc * using PCRE instead of build in matching * added whitespace --- kong/plugins/key-auth/schema.lua | 15 ++++++++++ kong/tools/utils.lua | 24 +++++++--------- spec/01-unit/04-utils_spec.lua | 31 ++++++--------------- spec/03-plugins/02-key-auth/01-api_spec.lua | 2 +- 4 files changed, 35 insertions(+), 37 deletions(-) diff --git a/kong/plugins/key-auth/schema.lua b/kong/plugins/key-auth/schema.lua index 6d12a9c4486..881b8da3ae6 100644 --- a/kong/plugins/key-auth/schema.lua +++ b/kong/plugins/key-auth/schema.lua @@ -1,6 +1,8 @@ local utils = require "kong.tools.utils" + local function check_user(anonymous) + if anonymous == "" or utils.is_valid_uuid(anonymous) then return true end @@ -8,24 +10,34 @@ local function check_user(anonymous) return false, "the anonymous user must be empty or a valid uuid" end + local function check_keys(keys) + for _, key in ipairs(keys) do local res, err = utils.validate_header_name(key, false) + if not res then return false, "key_name '"..key.."' is illegal, "..tostring(err) end end + return true end + local function default_key_names(t) + if not t.key_names then return {"apikey"} end + end + return { + no_consumer = true, + fields = { key_names = { required = true, @@ -33,14 +45,17 @@ return { default = default_key_names, func = check_keys, }, + hide_credentials = { type = "boolean", default = false, }, + anonymous = { type = "string", default = "", func = check_user, }, } + } diff --git a/kong/tools/utils.lua b/kong/tools/utils.lua index a2c08f54073..0383e9470c2 100644 --- a/kong/tools/utils.lua +++ b/kong/tools/utils.lua @@ -24,12 +24,12 @@ local concat = table.concat local insert = table.insert local lower = string.lower local fmt = string.format -local match = string.match local find = string.find local gsub = string.gsub local split = pl_stringx.split local strip = pl_stringx.strip local re_find = ngx.re.find +local re_match = ngx.re.match ffi.cdef[[ typedef unsigned char u_char; @@ -584,27 +584,23 @@ _M.format_host = function(p1, p2) end end --- patterns for allowed header characters -local header_chars = [[!#$%%&'%*%+%-%.0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ^`abcdefghijklmnopqrstuvwxyz|~]] -local header_allowed = "^["..header_chars.."]+$" -local header_allowed_u = "^["..header_chars.."_]+$" -- adds underscore - --- Validates a header name. --- Checks characters used in a header name to be valid. +-- Checks characters used in a header name to be valid, as per nginx only +-- a-z, A-Z, 0-9 and '-' are allowed. -- @param name (string) the header name to verify --- @param allow_underscore (boolean, optional) if thruthy and `_` (underscore) is --- accepted, otherwise it will fail. -- @return the valid header name, or `nil+error` -_M.validate_header_name = function(name, allow_underscore) - -- allow_underscore is used because openresty by default changes "_" into "-" +_M.validate_header_name = function(name) + if name == nil or name == "" then return nil, "no header name provided" end - if match(name, allow_underscore and header_allowed_u or header_allowed) then + + if re_match(name, "^[a-zA-Z0-9-]+$", "jo") then return name end - return nil, "only the following characters are allowed for header names: ".. - (allow_underscore and header_allowed_u or header_allowed) + + return nil, "bad header name '" .. name .. + "', allowed characters are A-Z, a-z, 0-9 and '-'" end return _M diff --git a/spec/01-unit/04-utils_spec.lua b/spec/01-unit/04-utils_spec.lua index 8f08641b1df..db1590ee01f 100644 --- a/spec/01-unit/04-utils_spec.lua +++ b/spec/01-unit/04-utils_spec.lua @@ -459,28 +459,15 @@ describe("Utils", function() end) end) - describe("validate_header_name()", function() - it("validates header names without underscore", function() - local header_chars = [[%!#$&'*+-.0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ^`abcdefghijklmnopqrstuvwxyz|~]] - for i = 1, 255 do - local c = string.char(i) - if string.find(header_chars, c, nil, true) then - assert(utils.validate_header_name(c) == c, "ascii character '"..c.."' ("..i..") should have been allowed") - else - assert(utils.validate_header_name(c) == nil, "ascii character "..i.." should not have been allowed") - end + it("validate_header_name() validates header names", function() + local header_chars = [[-0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz]] + for i = 1, 255 do + local c = string.char(i) + if string.find(header_chars, c, nil, true) then + assert(utils.validate_header_name(c) == c, "ascii character '"..c.."' ("..i..") should have been allowed") + else + assert(utils.validate_header_name(c) == nil, "ascii character "..i.." should not have been allowed") end - end) - it("validates header names with underscore", function() - local header_chars = [[%!#$&'*+-.0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ^`abcdefghijklmnopqrstuvwxyz|~_]] - for i = 1, 255 do - local c = string.char(i) - if string.find(header_chars, c, nil, true) then - assert(utils.validate_header_name(c, true) == c, "ascii character '"..c.."' ("..i..") should have been allowed") - else - assert(utils.validate_header_name(c, true) == nil, "ascii character "..i.." should not have been allowed") - end - end - end) + end end) end) diff --git a/spec/03-plugins/02-key-auth/01-api_spec.lua b/spec/03-plugins/02-key-auth/01-api_spec.lua index 93dd35f3229..da1c183f41d 100644 --- a/spec/03-plugins/02-key-auth/01-api_spec.lua +++ b/spec/03-plugins/02-key-auth/01-api_spec.lua @@ -281,7 +281,7 @@ describe("Plugin: key-auth (API)", function() }) assert.response(res).has.status(400) local body = assert.response(res).has.jsonbody() - assert.equal("key_name 'hello\\world' is illegal, only the following characters are allowed for header names: ^[!#$%%&'%*%+%-%.0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ^`abcdefghijklmnopqrstuvwxyz|~]+$", body["config.key_names"]) + assert.equal("key_name 'hello\\world' is illegal, bad header name 'hello\\world', allowed characters are A-Z, a-z, 0-9 and '-'", body["config.key_names"]) end) it("succeeds with valid key_names", function() local key_name = "hello-world"