Skip to content

Commit

Permalink
fix(key-auth): add missing www-authenticate headers
Browse files Browse the repository at this point in the history
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
  • Loading branch information
nowNick committed Oct 19, 2023
1 parent 30d90f3 commit cb5d571
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 15 deletions.
3 changes: 3 additions & 0 deletions changelog/unreleased/kong/key_auth_www_authenticate.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
message: Add WWW-Authenticate headers to all 401 response in key auth plugin.
type: bugfix
scope: Plugin
31 changes: 17 additions & 14 deletions kong/plugins/key-auth/handler.lua
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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
Expand All @@ -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

-----------------------------------------
Expand All @@ -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)
Expand Down
20 changes: 19 additions & 1 deletion spec/03-plugins/09-key-auth/02-access_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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)

Expand All @@ -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 {
Expand All @@ -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)

Expand Down Expand Up @@ -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.
Expand 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

Expand All @@ -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()
Expand All @@ -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()
Expand All @@ -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)
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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",
Expand All @@ -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)

Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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()
Expand All @@ -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()
Expand All @@ -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)
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit cb5d571

Please sign in to comment.