From b04aa72ec0172ea946d01d5489ab84ca1881598a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20Nowak?= Date: Mon, 22 Jan 2024 13:23:27 +0100 Subject: [PATCH] fix(basic-auth): add missing www-authenticate headers (#11795) When server returns 401 Unauthorized response it should return WWW-Authenticate header as well with proper challenge. Not all basic auth 401 responses had this header. It also allows to configure the protected resource realm via plugin config. Fix: #7772 KAG-321 --- .../kong/basic_www_authenticate.yml | 3 + kong/plugins/basic-auth/access.lua | 21 ++---- kong/plugins/basic-auth/schema.lua | 1 + .../02-process_auto_fields_spec.lua | 2 + .../11-declarative_config/03-flatten_spec.lua | 6 +- .../10-basic-auth/03-access_spec.lua | 64 ++++++++++++------- .../10-basic-auth/05-declarative_spec.lua | 1 + 7 files changed, 58 insertions(+), 40 deletions(-) create mode 100644 changelog/unreleased/kong/basic_www_authenticate.yml diff --git a/changelog/unreleased/kong/basic_www_authenticate.yml b/changelog/unreleased/kong/basic_www_authenticate.yml new file mode 100644 index 000000000000..630747f005dc --- /dev/null +++ b/changelog/unreleased/kong/basic_www_authenticate.yml @@ -0,0 +1,3 @@ +message: Add missing WWW-Authenticate headers to 401 response in basic auth plugin. +type: bugfix +scope: Plugin diff --git a/kong/plugins/basic-auth/access.lua b/kong/plugins/basic-auth/access.lua index 8c76b526a536..43fec7990cc1 100644 --- a/kong/plugins/basic-auth/access.lua +++ b/kong/plugins/basic-auth/access.lua @@ -17,9 +17,6 @@ local HEADERS_CREDENTIAL_IDENTIFIER = constants.HEADERS.CREDENTIAL_IDENTIFIER local HEADERS_ANONYMOUS = constants.HEADERS.ANONYMOUS -local realm = 'Basic realm="' .. _KONG._NAME .. '"' - - local _M = {} @@ -154,21 +151,17 @@ local function set_consumer(consumer, credential) end -local function fail_authentication() - return false, { status = 401, message = "Invalid authentication credentials" } +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_authenticate = "Basic realm=\"" .. conf.realm .. "\"" + -- If both headers are missing, return 401 if not (kong.request.get_header("authorization") or kong.request.get_header("proxy-authorization")) then - return false, { - status = 401, - message = "Unauthorized", - headers = { - ["WWW-Authenticate"] = realm - } - } + return false, unauthorized("Unauthorized", www_authenticate) end local credential @@ -183,12 +176,12 @@ local function do_authentication(conf) if given_username and given_password then credential = load_credential_from_db(given_username) else - return fail_authentication() + return false, unauthorized("Invalid authentication credentials", www_authenticate) end end if not credential or not validate_credentials(credential, given_password) then - return fail_authentication() + return false, unauthorized("Invalid authentication credentials", www_authenticate) end -- Retrieve consumer diff --git a/kong/plugins/basic-auth/schema.lua b/kong/plugins/basic-auth/schema.lua index 9f99a1c59770..e16c61b2e3f1 100644 --- a/kong/plugins/basic-auth/schema.lua +++ b/kong/plugins/basic-auth/schema.lua @@ -11,6 +11,7 @@ return { fields = { { anonymous = { description = "An optional string (Consumer UUID or username) value to use as an “anonymous” consumer if authentication fails. If empty (default null), the request will fail with an authentication failure `4xx`. Please note that this value must refer to the Consumer `id` or `username` attribute, and **not** its `custom_id`.", type = "string" }, }, { hide_credentials = { description = "An optional boolean value telling the plugin to show or hide the credential from the upstream service. If `true`, the plugin will strip the credential from the request (i.e. the `Authorization` header) before proxying it.", type = "boolean", required = true, default = false }, }, + { 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 = true, default = "service" }, }, }, }, }, }, } diff --git a/spec/01-unit/01-db/01-schema/11-declarative_config/02-process_auto_fields_spec.lua b/spec/01-unit/01-db/01-schema/11-declarative_config/02-process_auto_fields_spec.lua index 98f20bef84b4..f12359a8aa5a 100644 --- a/spec/01-unit/01-db/01-schema/11-declarative_config/02-process_auto_fields_spec.lua +++ b/spec/01-unit/01-db/01-schema/11-declarative_config/02-process_auto_fields_spec.lua @@ -375,6 +375,7 @@ describe("declarative config: process_auto_fields", function() protocols = { "grpc", "grpcs", "http", "https" }, config = { hide_credentials = false, + realm = "service", } }, { @@ -709,6 +710,7 @@ describe("declarative config: process_auto_fields", function() protocols = { "grpc", "grpcs", "http", "https" }, config = { hide_credentials = false, + realm = "service", } }, { diff --git a/spec/01-unit/01-db/01-schema/11-declarative_config/03-flatten_spec.lua b/spec/01-unit/01-db/01-schema/11-declarative_config/03-flatten_spec.lua index 4883b76dca5c..324a86fe4f48 100644 --- a/spec/01-unit/01-db/01-schema/11-declarative_config/03-flatten_spec.lua +++ b/spec/01-unit/01-db/01-schema/11-declarative_config/03-flatten_spec.lua @@ -573,7 +573,8 @@ describe("declarative config: flatten", function() plugins = { { config = { anonymous = null, - hide_credentials = false + hide_credentials = false, + realm = "service" }, consumer = null, created_at = 1234567890, @@ -1088,7 +1089,8 @@ describe("declarative config: flatten", function() plugins = { { config = { anonymous = null, - hide_credentials = false + hide_credentials = false, + realm = "service" }, consumer = null, created_at = 1234567890, diff --git a/spec/03-plugins/10-basic-auth/03-access_spec.lua b/spec/03-plugins/10-basic-auth/03-access_spec.lua index 097943753f3a..8a6c76014d07 100644 --- a/spec/03-plugins/10-basic-auth/03-access_spec.lua +++ b/spec/03-plugins/10-basic-auth/03-access_spec.lua @@ -57,6 +57,9 @@ for _, strategy in helpers.each_strategy() do bp.plugins:insert { name = "basic-auth", route = { id = route1.id }, + config = { + realm = "test-realm", + } } bp.plugins:insert { @@ -132,33 +135,39 @@ for _, strategy in helpers.each_strategy() do end) describe("Unauthorized", function() - - it("returns Unauthorized on missing credentials", function() - local res = assert(proxy_client:send { - method = "GET", - path = "/status/200", - headers = { - ["Host"] = "basic-auth1.test" - } - }) - local body = assert.res_status(401, res) - local json = cjson.decode(body) - assert.not_nil(json) - assert.matches("Unauthorized", json.message) + describe("when realm is configured", function() + it("returns Unauthorized on missing credentials", function() + local res = assert(proxy_client:send { + method = "GET", + path = "/status/200", + headers = { + ["Host"] = "basic-auth1.test" + } + }) + local body = assert.res_status(401, res) + local json = cjson.decode(body) + assert.not_nil(json) + assert.matches("Unauthorized", json.message) + assert.equal('Basic realm="test-realm"', res.headers["WWW-Authenticate"]) + end) end) - it("returns WWW-Authenticate header on missing credentials", function() - local res = assert(proxy_client:send { - method = "GET", - path = "/status/200", - headers = { - ["Host"] = "basic-auth1.test" - } - }) - assert.res_status(401, res) - assert.equal('Basic realm="' .. meta._NAME .. '"', res.headers["WWW-Authenticate"]) + describe("when realm is default", function() + it("returns Unauthorized on missing credentials", function() + local res = assert(proxy_client:send { + method = "GET", + path = "/status/200", + headers = { + ["Host"] = "basic-auth2.test" + } + }) + local body = assert.res_status(401, res) + local json = cjson.decode(body) + assert.not_nil(json) + assert.matches("Unauthorized", json.message) + assert.equal('Basic realm="service"', res.headers["WWW-Authenticate"]) + end) end) - end) describe("Unauthorized", function() @@ -176,6 +185,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('Basic realm="test-realm"', res.headers["WWW-Authenticate"]) end) it("returns 401 Unauthorized on invalid credentials in Proxy-Authorization", function() @@ -191,6 +201,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('Basic realm="test-realm"', res.headers["WWW-Authenticate"]) end) it("returns 401 Unauthorized on password only", function() @@ -206,6 +217,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('Basic realm="test-realm"', res.headers["WWW-Authenticate"]) end) it("returns 401 Unauthorized on username only", function() @@ -221,6 +233,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('Basic realm="test-realm"', res.headers["WWW-Authenticate"]) end) it("rejects gRPC call without credentials", function() @@ -296,6 +309,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('Basic realm="test-realm"', res.headers["WWW-Authenticate"]) end) it("authenticates valid credentials in Proxy-Authorization", function() @@ -564,6 +578,7 @@ for _, strategy in helpers.each_strategy() do } }) assert.response(res).has.status(401) + assert.equal('Key realm="' .. meta._NAME .. '"', res.headers["WWW-Authenticate"]) end) it("fails 401, with no credential provided", function() @@ -575,6 +590,7 @@ for _, strategy in helpers.each_strategy() do } }) assert.response(res).has.status(401) + assert.equal('Key realm="' .. meta._NAME .. '"', res.headers["WWW-Authenticate"]) end) end) diff --git a/spec/03-plugins/10-basic-auth/05-declarative_spec.lua b/spec/03-plugins/10-basic-auth/05-declarative_spec.lua index c7a3de114857..db93e1fe3760 100644 --- a/spec/03-plugins/10-basic-auth/05-declarative_spec.lua +++ b/spec/03-plugins/10-basic-auth/05-declarative_spec.lua @@ -86,6 +86,7 @@ for _, strategy in helpers.each_strategy() do name = "basic-auth", config = { hide_credentials = true, + realm = "service", } }