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.
This commit also adds an option to configure the realm
for protected resource. By default it is empty therefore it
is not displayed but it can be configured to
be present in www_authenticate header.

Fix: #7772
KAG-321
  • Loading branch information
nowNick committed Dec 5, 2023
1 parent 08d989c commit 296a965
Show file tree
Hide file tree
Showing 6 changed files with 82 additions and 28 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
33 changes: 19 additions & 14 deletions kong/plugins/key-auth/handler.lua
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ local type = type
local error = error
local ipairs = ipairs
local tostring = tostring
local fmt = string.format


local HEADERS_CONSUMER_ID = constants.HEADERS.CONSUMER_ID
Expand All @@ -25,14 +26,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 +97,19 @@ local function get_body()
return body
end

local function server_error(message)
return { status = 500, message = message }
end

local function unauthorized(message, www_auth_content)
return { status = 401, message = message, headers = { ["WWW-Authenticate"] = www_auth_content } }
end

local function do_authentication(conf)
local www_auth_content = conf.realm and fmt('Key realm="%s"', conf.realm) or 'Key'
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 +166,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, www_auth_content)
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, www_auth_content)
end

-- retrieve our consumer linked to this API key
Expand All @@ -188,7 +193,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, www_auth_content)
end

-----------------------------------------
Expand All @@ -203,7 +208,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
1 change: 1 addition & 0 deletions kong/plugins/key-auth/schema.lua
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ return {
{ key_in_query = { description = "If enabled (default), the plugin reads the query parameter in the request and tries to find the key in it.", type = "boolean", required = true, default = true }, },
{ key_in_body = { description = "If enabled, the plugin reads the request body. Supported MIME types: `application/www-form-urlencoded`, `application/json`, and `multipart/form-data`.", type = "boolean", required = true, default = false }, },
{ run_on_preflight = { description = "A boolean value that indicates whether the plugin should run (and try to authenticate) on `OPTIONS` preflight requests. If set to `false`, then `OPTIONS` requests are always allowed.", type = "boolean", required = true, default = true }, },
{ realm = { description = "When authentication or authorization fails, or there is an unexpected error, the plugin sends an `WWW-Authenticate` header with the `realm` attribute value.", type = "string", required = false }, },
},
}, },
},
Expand Down
1 change: 1 addition & 0 deletions spec/01-unit/01-db/01-schema/07-plugins_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ describe("plugins", function()
key_names = { "apikey" },
hide_credentials = false,
anonymous = ngx.null,
realm = ngx.null,
key_in_header = true,
key_in_query = true,
key_in_body = false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,7 @@ describe("declarative config: flatten", function()
config = {
anonymous = null,
hide_credentials = false,
realm = null,
key_in_header = true,
key_in_query = true,
key_in_body = false,
Expand Down Expand Up @@ -430,6 +431,7 @@ describe("declarative config: flatten", function()
config = {
anonymous = null,
hide_credentials = false,
realm = null,
key_in_header = true,
key_in_query = true,
key_in_body = false,
Expand Down Expand Up @@ -627,6 +629,7 @@ describe("declarative config: flatten", function()
config = {
anonymous = null,
hide_credentials = false,
realm = null,
key_in_header = true,
key_in_query = true,
key_in_body = false,
Expand Down Expand Up @@ -1142,6 +1145,7 @@ describe("declarative config: flatten", function()
config = {
anonymous = null,
hide_credentials = false,
realm = null,
key_in_header = true,
key_in_query = true,
key_in_body = false,
Expand Down
68 changes: 54 additions & 14 deletions spec/03-plugins/09-key-auth/02-access_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ for _, strategy in helpers.each_strategy() do
route = { id = route2.id },
config = {
hide_credentials = true,
realm = "test-realm"
},
}

Expand Down Expand Up @@ -229,20 +230,41 @@ for _, strategy in helpers.each_strategy() do
assert.not_nil(json)
assert.matches("No API key found in request", json.message)
end)
it("returns Unauthorized on empty key header", function()
local res = assert(proxy_client:send {
method = "GET",
path = "/status/200",
headers = {
["Host"] = "key-auth1.com",
["apikey"] = "",
}
})
local body = assert.res_status(401, res)
local json = cjson.decode(body)
assert.not_nil(json)
assert.matches("No API key found in request", json.message)
describe("when realm is not configured", function()
it("returns Unauthorized on empty key header with no realm", function()
local res = assert(proxy_client:send {
method = "GET",
path = "/status/200",
headers = {
["Host"] = "key-auth1.com",
["apikey"] = "",
}
})
local body = assert.res_status(401, res)
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)
describe("when realm is configured", function()
it("returns Unauthorized on empty key header with no realm", function()
local res = assert(proxy_client:send {
method = "GET",
path = "/status/200",
headers = {
["Host"] = "key-auth2.com",
["apikey"] = "",
}
})
local body = assert.res_status(401, res)
local json = cjson.decode(body)
assert.not_nil(json)
assert.matches("No API key found in request", json.message)
assert.equal('Key realm="test-realm"', res.headers["WWW-Authenticate"])
end)
end)

it("returns Unauthorized on empty key querystring", function()
local res = assert(proxy_client:send {
method = "GET",
Expand All @@ -255,6 +277,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 @@ -265,7 +288,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 @@ -292,6 +315,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 @@ -305,6 +329,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 @@ -366,6 +391,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 @@ -387,6 +413,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 @@ -403,6 +430,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 @@ -417,6 +445,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 @@ -431,6 +460,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 @@ -462,6 +492,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 @@ -522,6 +553,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 @@ -535,6 +567,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 @@ -665,6 +698,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 @@ -844,6 +878,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 @@ -856,6 +891,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 @@ -867,6 +903,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 @@ -1280,6 +1317,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 296a965

Please sign in to comment.