From cb5d57156940a815b430ca929ee33bd25f3ad19e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20Nowak?= Date: Thu, 19 Oct 2023 15:11:28 +0200 Subject: [PATCH] fix(key-auth): add missing www-authenticate headers When serve returns 401 Unauthorized response it should return WWW-Authenticate header as well with proper challenge. Not all key-auth 401 responses had this header. Also this is removing the realm part from WWW-Authenticate header as it is a potential phishing attack vector. Fix: #7772 KAG-321 --- .../kong/key_auth_www_authenticate.yml | 3 ++ kong/plugins/key-auth/handler.lua | 31 ++++++++++--------- .../03-plugins/09-key-auth/02-access_spec.lua | 20 +++++++++++- 3 files changed, 39 insertions(+), 15 deletions(-) create mode 100644 changelog/unreleased/kong/key_auth_www_authenticate.yml diff --git a/changelog/unreleased/kong/key_auth_www_authenticate.yml b/changelog/unreleased/kong/key_auth_www_authenticate.yml new file mode 100644 index 00000000000..3d1e12b085d --- /dev/null +++ b/changelog/unreleased/kong/key_auth_www_authenticate.yml @@ -0,0 +1,3 @@ +message: Add WWW-Authenticate headers to all 401 response in key auth plugin. +type: bugfix +scope: Plugin diff --git a/kong/plugins/key-auth/handler.lua b/kong/plugins/key-auth/handler.lua index 0c711cca133..7a963aba2de 100644 --- a/kong/plugins/key-auth/handler.lua +++ b/kong/plugins/key-auth/handler.lua @@ -25,14 +25,11 @@ local KeyAuthHandler = { local EMPTY = {} -local _realm = 'Key realm="' .. _KONG._NAME .. '"' - - -local ERR_DUPLICATE_API_KEY = { status = 401, message = "Duplicate API key found" } -local ERR_NO_API_KEY = { status = 401, message = "No API key found in request" } -local ERR_INVALID_AUTH_CRED = { status = 401, message = "Invalid authentication credentials" } -local ERR_INVALID_PLUGIN_CONF = { status = 500, message = "Invalid plugin configuration" } -local ERR_UNEXPECTED = { status = 500, message = "An unexpected error occurred" } +local ERR_DUPLICATE_API_KEY = "Duplicate API key found" +local ERR_NO_API_KEY = "No API key found in request" +local ERR_INVALID_AUTH_CRED = "Invalid authentication credentials" +local ERR_INVALID_PLUGIN_CONF = "Invalid plugin configuration" +local ERR_UNEXPECTED = "An unexpected error occurred" local function load_credential(key) @@ -99,11 +96,18 @@ local function get_body() return body end +local function server_error(message) + return { status = 500, message = message } +end + +local function unauthorized(message) + return { status = 401, message = message, headers = { ["WWW-Authenticate"] = 'Key' } } +end local function do_authentication(conf) if type(conf.key_names) ~= "table" then kong.log.err("no conf.key_names set, aborting plugin execution") - return nil, ERR_INVALID_PLUGIN_CONF + return nil, server_error(ERR_INVALID_PLUGIN_CONF) end local headers = kong.request.get_headers() @@ -160,14 +164,13 @@ local function do_authentication(conf) elseif type(v) == "table" then -- duplicate API key - return nil, ERR_DUPLICATE_API_KEY + return nil, unauthorized(ERR_DUPLICATE_API_KEY) end end -- this request is missing an API key, HTTP 401 if not key or key == "" then - kong.response.set_header("WWW-Authenticate", _realm) - return nil, ERR_NO_API_KEY + return nil, unauthorized(ERR_NO_API_KEY) end -- retrieve our consumer linked to this API key @@ -188,7 +191,7 @@ local function do_authentication(conf) -- no credential in DB, for this key, it is invalid, HTTP 401 if not credential or hit_level == 4 then - return nil, ERR_INVALID_AUTH_CRED + return nil, unauthorized(ERR_INVALID_AUTH_CRED) end ----------------------------------------- @@ -203,7 +206,7 @@ local function do_authentication(conf) credential.consumer.id) if err then kong.log.err(err) - return nil, ERR_UNEXPECTED + return nil, server_error(ERR_UNEXPECTED) end set_consumer(consumer, credential) diff --git a/spec/03-plugins/09-key-auth/02-access_spec.lua b/spec/03-plugins/09-key-auth/02-access_spec.lua index 8135569a1f8..78093bbd348 100644 --- a/spec/03-plugins/09-key-auth/02-access_spec.lua +++ b/spec/03-plugins/09-key-auth/02-access_spec.lua @@ -249,6 +249,7 @@ for _, strategy in helpers.each_strategy() do local json = cjson.decode(body) assert.not_nil(json) assert.matches("No API key found in request", json.message) + assert.equal('Key', res.headers["WWW-Authenticate"]) end) it("returns WWW-Authenticate header on missing credentials", function() local res = assert(proxy_client:send { @@ -259,7 +260,7 @@ for _, strategy in helpers.each_strategy() do } }) res:read_body() - assert.equal('Key realm="' .. meta._NAME .. '"', res.headers["WWW-Authenticate"]) + assert.equal('Key', res.headers["WWW-Authenticate"]) end) end) @@ -286,6 +287,7 @@ for _, strategy in helpers.each_strategy() do local json = cjson.decode(body) assert.not_nil(json) assert.matches("Invalid authentication credentials", json.message) + assert.equal('Key', res.headers["WWW-Authenticate"]) end) it("handles duplicated key in querystring", function() local res = assert(proxy_client:send { @@ -299,6 +301,7 @@ for _, strategy in helpers.each_strategy() do local json = cjson.decode(body) assert.not_nil(json) assert.matches("Duplicate API key found", json.message) + assert.equal('Key', res.headers["WWW-Authenticate"]) end) end) @@ -360,6 +363,7 @@ for _, strategy in helpers.each_strategy() do local json = cjson.decode(body) assert.not_nil(json) assert.matches("Invalid authentication credentials", json.message) + assert.equal('Key', res.headers["WWW-Authenticate"]) end) -- lua-multipart doesn't currently handle duplicates at all. @@ -381,6 +385,7 @@ for _, strategy in helpers.each_strategy() do local json = cjson.decode(body) assert.not_nil(json) assert.matches("Duplicate API key found", json.message) + assert.equal('Key', res.headers["WWW-Authenticate"]) end) end @@ -397,6 +402,7 @@ for _, strategy in helpers.each_strategy() do local json = cjson.decode(body) assert.not_nil(json) assert.matches("Duplicate API key found", json.message) + assert.equal('Key', res.headers["WWW-Authenticate"]) end) it("does not identify apikey[] as api keys", function() @@ -411,6 +417,7 @@ for _, strategy in helpers.each_strategy() do local json = cjson.decode(body) assert.not_nil(json) assert.matches("No API key found in request", json.message) + assert.equal('Key', res.headers["WWW-Authenticate"]) end) it("does not identify apikey[1] as api keys", function() @@ -425,6 +432,7 @@ for _, strategy in helpers.each_strategy() do local json = cjson.decode(body) assert.not_nil(json) assert.matches("No API key found in request", json.message) + assert.equal('Key', res.headers["WWW-Authenticate"]) end) end end) @@ -456,6 +464,7 @@ for _, strategy in helpers.each_strategy() do local json = cjson.decode(body) assert.not_nil(json) assert.matches("Invalid authentication credentials", json.message) + assert.equal('Key', res.headers["WWW-Authenticate"]) end) end) @@ -516,6 +525,7 @@ for _, strategy in helpers.each_strategy() do local json = cjson.decode(body) assert.not_nil(json) assert.matches("Invalid authentication credentials", json.message) + assert.equal('Key', res.headers["WWW-Authenticate"]) res = assert(proxy_client:send { method = "GET", @@ -529,6 +539,7 @@ for _, strategy in helpers.each_strategy() do json = cjson.decode(body) assert.not_nil(json) assert.matches("Invalid authentication credentials", json.message) + assert.equal('Key', res.headers["WWW-Authenticate"]) end) end) @@ -659,6 +670,7 @@ for _, strategy in helpers.each_strategy() do local json = cjson.decode(body) assert.not_nil(json) assert.matches("No API key found in request", json.message) + assert.equal('Key', res.headers["WWW-Authenticate"]) end) end) @@ -838,6 +850,7 @@ for _, strategy in helpers.each_strategy() do } }) assert.response(res).has.status(401) + assert.equal('Basic realm="' .. meta._NAME .. '"', res.headers["WWW-Authenticate"]) end) it("fails 401, with only the second credential provided", function() @@ -850,6 +863,7 @@ for _, strategy in helpers.each_strategy() do } }) assert.response(res).has.status(401) + assert.equal('Key', res.headers["WWW-Authenticate"]) end) it("fails 401, with no credential provided", function() @@ -861,6 +875,7 @@ for _, strategy in helpers.each_strategy() do } }) assert.response(res).has.status(401) + assert.equal('Key', res.headers["WWW-Authenticate"]) end) end) @@ -1274,6 +1289,9 @@ for _, strategy in helpers.each_strategy() do }, }) assert.res_status(test[5], res) + if test[5] == 401 then + assert.equal('Key', res.headers["WWW-Authenticate"]) + end proxy_client:close() end) end