From bded47d6a9c7ee7c334aae6bcef1f47c3c8f7896 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20Nowak?= Date: Tue, 24 Oct 2023 18:07:50 +0200 Subject: [PATCH] fix(oauth2): add missing www-authenticate headers When server returns 401 Unauthorized response it should return WWW-Authenticate header as well with proper challenge. Not all oauth2 401 responses had this header. Fix: #7772 KAG-321 --- .../kong/oauth2_www_authenticate.yml | 4 + kong/clustering/compat/removed_fields.lua | 7 ++ kong/plugins/oauth2/access.lua | 100 +++++++++++------- kong/plugins/oauth2/schema.lua | 1 + .../09-hybrid_mode/09-config-compat_spec.lua | 17 +++ spec/03-plugins/25-oauth2/03-access_spec.lua | 83 ++++++++++++--- 6 files changed, 162 insertions(+), 50 deletions(-) create mode 100644 changelog/unreleased/kong/oauth2_www_authenticate.yml diff --git a/changelog/unreleased/kong/oauth2_www_authenticate.yml b/changelog/unreleased/kong/oauth2_www_authenticate.yml new file mode 100644 index 000000000000..3550ac0f1d48 --- /dev/null +++ b/changelog/unreleased/kong/oauth2_www_authenticate.yml @@ -0,0 +1,4 @@ +message: "**OAuth2**: Add WWW-Authenticate headers to all 401 responses and realm option." +type: bugfix +scope: Plugin + diff --git a/kong/clustering/compat/removed_fields.lua b/kong/clustering/compat/removed_fields.lua index 9893dd60ceff..6a17cfd82b97 100644 --- a/kong/clustering/compat/removed_fields.lua +++ b/kong/clustering/compat/removed_fields.lua @@ -136,4 +136,11 @@ return { "llm.model.options.upstream_path", }, }, + + -- Any dataplane older than 3.8.0 + [3008000000] = { + oauth2 = { + "realm", + }, + }, } diff --git a/kong/plugins/oauth2/access.lua b/kong/plugins/oauth2/access.lua index ca4864ed31de..2c9babb978ff 100644 --- a/kong/plugins/oauth2/access.lua +++ b/kong/plugins/oauth2/access.lua @@ -6,7 +6,7 @@ local secret = require "kong.plugins.oauth2.secret" local sha256_base64url = require "kong.tools.sha256".sha256_base64url - +local fmt = string.format local kong = kong local type = type local next = next @@ -811,7 +811,7 @@ local function load_token(access_token) end -local function retrieve_token(conf, access_token) +local function retrieve_token(conf, access_token, realm) local token_cache_key = kong.db.oauth2_tokens:cache_key(access_token) local token, err = kong.cache:get(token_cache_key, nil, load_token, access_token) if err then @@ -827,6 +827,11 @@ local function retrieve_token(conf, access_token) [ERROR] = "invalid_token", error_description = "The access token is global, but the current " .. "plugin is configured without 'global_credentials'", + }, + { + ["WWW-Authenticate"] = 'Bearer' .. realm .. ' error=' .. + '"invalid_token" error_description=' .. + '"The access token is invalid or has expired"' }) end @@ -951,6 +956,7 @@ end local function do_authentication(conf) local access_token = parse_access_token(conf); + local realm = conf.realm and fmt(' realm="%s"', conf.realm) or '' if not access_token or access_token == "" then return nil, { status = 401, @@ -959,12 +965,12 @@ local function do_authentication(conf) error_description = "The access token is missing" }, headers = { - ["WWW-Authenticate"] = 'Bearer realm="service"' + ["WWW-Authenticate"] = 'Bearer' .. realm } } end - local token = retrieve_token(conf, access_token) + local token = retrieve_token(conf, access_token, realm) if not token then return nil, { status = 401, @@ -973,7 +979,7 @@ local function do_authentication(conf) error_description = "The access token is invalid or has expired" }, headers = { - ["WWW-Authenticate"] = 'Bearer realm="service" error=' .. + ["WWW-Authenticate"] = 'Bearer' .. realm .. ' error=' .. '"invalid_token" error_description=' .. '"The access token is invalid or has expired"' } @@ -991,7 +997,7 @@ local function do_authentication(conf) error_description = "The access token is invalid or has expired" }, headers = { - ["WWW-Authenticate"] = 'Bearer realm="service" error=' .. + ["WWW-Authenticate"] = 'Bearer' .. realm .. ' error=' .. '"invalid_token" error_description=' .. '"The access token is invalid or has expired"' } @@ -1009,7 +1015,7 @@ local function do_authentication(conf) error_description = "The access token is invalid or has expired" }, headers = { - ["WWW-Authenticate"] = 'Bearer realm="service" error=' .. + ["WWW-Authenticate"] = 'Bearer' .. realm .. ' error=' .. '"invalid_token" error_description=' .. '"The access token is invalid or has expired"' } @@ -1043,7 +1049,7 @@ local function do_authentication(conf) return true end -local function invalid_oauth2_method(endpoint_name) +local function invalid_oauth2_method(endpoint_name, realm) return { status = 405, message = { @@ -1053,7 +1059,7 @@ local function invalid_oauth2_method(endpoint_name) " is invalid for the " .. endpoint_name .. " endpoint" }, headers = { - ["WWW-Authenticate"] = 'Bearer realm="service" error=' .. + ["WWW-Authenticate"] = 'Bearer' .. realm .. ' error=' .. '"invalid_method" error_description=' .. '"The HTTP method ' .. kong.request.get_method() .. ' is invalid for the ' .. @@ -1062,13 +1068,54 @@ local function invalid_oauth2_method(endpoint_name) } end +local function set_anonymous_consumer(anonymous) + local consumer_cache_key = kong.db.consumers:cache_key(anonymous) + local consumer, err = kong.cache:get(consumer_cache_key, nil, + kong.client.load_consumer, + anonymous, true) + if err then + return error(err) + end + + set_consumer(consumer) +end + +--- When conf.anonymous is enabled we are in "logical OR" authentication flow. +--- Meaning - either anonymous consumer is enabled or there are multiple auth plugins +--- and we need to passthrough on failed authentication. +local function logical_OR_authentication(conf) + if kong.client.get_credential() then + -- we're already authenticated and in "logical OR" between auth methods -- early exit + local clear_header = kong.service.request.clear_header + clear_header("X-Authenticated-Scope") + clear_header("X-Authenticated-UserId") + return + end + + local ok, _ = do_authentication(conf) + if not ok then + set_anonymous_consumer(conf.anonymous) + end +end + +--- When conf.anonymous is not set we are in "logical AND" authentication flow. +--- Meaning - if this authentication fails the request should not be authorized +--- even though other auth plugins might have successfully authorized user. +local function logical_AND_authentication(conf) + local ok, err = do_authentication(conf) + if not ok then + return kong.response.exit(err.status, err.message, err.headers) + end +end + function _M.execute(conf) local path = kong.request.get_path() local has_end_slash = string_byte(path, -1) == SLASH + local realm = conf.realm and fmt(' realm="%s"', conf.realm) or '' if string_find(path, "/oauth2/token", has_end_slash and -14 or -13, true) then if kong.request.get_method() ~= "POST" then - local err = invalid_oauth2_method("token") + local err = invalid_oauth2_method("token", realm) return kong.response.exit(err.status, err.message, err.headers) end @@ -1077,40 +1124,17 @@ function _M.execute(conf) if string_find(path, "/oauth2/authorize", has_end_slash and -18 or -17, true) then if kong.request.get_method() ~= "POST" then - local err = invalid_oauth2_method("authorization") + local err = invalid_oauth2_method("authorization", realm) return kong.response.exit(err.status, err.message, err.headers) end return authorize(conf) end - if conf.anonymous and kong.client.get_credential() then - -- we're already authenticated, and we're configured for using anonymous, - -- hence we're in a logical OR between auth methods and we're already done. - local clear_header = kong.service.request.clear_header - clear_header("X-Authenticated-Scope") - clear_header("X-Authenticated-UserId") - return - end - - - local ok, err = do_authentication(conf) - if not ok then - if conf.anonymous then - -- get anonymous user - local consumer_cache_key = kong.db.consumers:cache_key(conf.anonymous) - local consumer, err = kong.cache:get(consumer_cache_key, nil, - kong.client.load_consumer, - conf.anonymous, true) - if err then - return error(err) - end - - set_consumer(consumer) - - else - return kong.response.exit(err.status, err.message, err.headers) - end + if conf.anonymous then + return logical_OR_authentication(conf) + else + return logical_AND_authentication(conf) end end diff --git a/kong/plugins/oauth2/schema.lua b/kong/plugins/oauth2/schema.lua index 778cbfdab6f5..9849a3c6c846 100644 --- a/kong/plugins/oauth2/schema.lua +++ b/kong/plugins/oauth2/schema.lua @@ -36,6 +36,7 @@ return { { refresh_token_ttl = typedefs.ttl { default = 1209600, required = true }, }, { reuse_refresh_token = { description = "An optional boolean value that indicates whether an OAuth refresh token is reused when refreshing an access token.", type = "boolean", default = false, required = true }, }, { pkce = { description = "Specifies a mode of how the Proof Key for Code Exchange (PKCE) should be handled by the plugin.", type = "string", default = "lax", required = false, one_of = { "none", "lax", "strict" } }, }, + { realm = { description = "When authentication fails the plugin sends `WWW-Authenticate` header with `realm` attribute value.", type = "string", required = false }, }, }, custom_validator = validate_flows, entity_checks = { diff --git a/spec/02-integration/09-hybrid_mode/09-config-compat_spec.lua b/spec/02-integration/09-hybrid_mode/09-config-compat_spec.lua index 1c8f7bd26ee5..f8ca0759d905 100644 --- a/spec/02-integration/09-hybrid_mode/09-config-compat_spec.lua +++ b/spec/02-integration/09-hybrid_mode/09-config-compat_spec.lua @@ -587,6 +587,23 @@ describe("CP/DP config compat transformations #" .. strategy, function() admin.plugins:remove({ id = ai_response_transformer.id }) end) end) + + describe("www-authenticate header in plugins (realm config)", function() + it("[oauth2] removes realm for versions below 3.8", function() + local oauth2 = admin.plugins:insert { + name = "oauth2", + config = { + enable_password_grant = true, + realm = "test", + } + } + local expected_oauth2_prior_38 = cycle_aware_deep_copy(oauth2) + expected_oauth2_prior_38.config.realm = nil + do_assert(utils.uuid(), "3.7.0", expected_oauth2_prior_38) + -- cleanup + admin.plugins:remove({ id = oauth2.id }) + end) + end) end) end) diff --git a/spec/03-plugins/25-oauth2/03-access_spec.lua b/spec/03-plugins/25-oauth2/03-access_spec.lua index 48e1cf018a28..aada9897a71d 100644 --- a/spec/03-plugins/25-oauth2/03-access_spec.lua +++ b/spec/03-plugins/25-oauth2/03-access_spec.lua @@ -468,6 +468,7 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function() config = { enable_password_grant = true, enable_authorization_code = false, + realm = "test-oauth2", }, }) @@ -1422,7 +1423,7 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function() local json = cjson.decode(body) assert.same({ error_description = "Invalid client authentication", error = "invalid_client" }, json) end) - it("returns an error when empty client_id and empty client_secret is sent regardless of method", function() + it("returns an error when empty client_id and empty client_secret is sent regardless of method - without realm", function() local res = assert(proxy_ssl_client:send { method = "GET", path = "/oauth2/token?client_id&grant_type=client_credentials&client_secret", @@ -1436,6 +1437,23 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function() local json = cjson.decode(body) assert.same({ error_description = "The HTTP method GET is invalid for the token endpoint", error = "invalid_method" }, json) + assert.are.equal('Bearer error="invalid_method" error_description="The HTTP method GET is invalid for the token endpoint"', res.headers["www-authenticate"]) + end) + it("returns an error when empty client_id and empty client_secret is sent regardless of method - with realm", function() + local res = assert(proxy_ssl_client:send { + method = "GET", + path = "/oauth2/token?client_id&grant_type=client_credentials&client_secret", + body = {}, + headers = { + ["Host"] = "oauth2_5.test", + ["Content-Type"] = "application/json" + } + }) + local body = assert.res_status(405, res) + local json = cjson.decode(body) + assert.same({ error_description = "The HTTP method GET is invalid for the token endpoint", + error = "invalid_method" }, json) + assert.are.equal('Bearer realm="test-oauth2" error="invalid_method" error_description="The HTTP method GET is invalid for the token endpoint"', res.headers["www-authenticate"]) end) it("returns an error when grant_type is not sent", function() local res = assert(proxy_ssl_client:send { @@ -1761,7 +1779,20 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function() end) describe("Password Grant", function() - it("blocks unauthorized requests", function() + it("blocks unauthorized requests - with no realm set", function() + local res = assert(proxy_ssl_client:send { + method = "GET", + path = "/request", + headers = { + ["Host"] = "oauth2_4.test" + } + }) + local body = assert.res_status(401, res) + local json = cjson.decode(body) + assert.same({ error_description = "The access token is missing", error = "invalid_request" }, json) + assert.are.equal('Bearer', res.headers["www-authenticate"]) + end) + it("blocks unauthorized requests - with realm set", function() local res = assert(proxy_ssl_client:send { method = "GET", path = "/request", @@ -1772,6 +1803,7 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function() local body = assert.res_status(401, res) local json = cjson.decode(body) assert.same({ error_description = "The access token is missing", error = "invalid_request" }, json) + assert.are.equal('Bearer realm="test-oauth2"', res.headers["www-authenticate"]) end) it("returns an error when client_secret is not sent", function() local res = assert(proxy_ssl_client:send { @@ -2916,6 +2948,7 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function() local body = assert.res_status(401, res) local json = cjson.decode(body) assert.same({ error_description = "The access token is missing", error = "invalid_request" }, json) + assert.are.equal("Bearer", res.headers["www-authenticate"]) end) it("works when a correct access_token is being sent in the querystring", function() local token = provision_token() @@ -2965,6 +2998,7 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function() } }) assert.res_status(401, res) + assert.are.equal("Bearer", res.headers["www-authenticate"]) end) it("refreshing token fails when scope is mismatching", function () @@ -3119,8 +3153,9 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function() } }) assert.res_status(401, res) + assert.are.equal("Bearer", res.headers["www-authenticate"]) end) - it("does not work when requesting a different API", function() + it("does not work when requesting a different API - with no realm set", function() local token = provision_token() local res = assert(proxy_ssl_client:send { @@ -3133,6 +3168,22 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function() local body = assert.res_status(401, res) local json = cjson.decode(body) assert.same({ error_description = "The access token is invalid or has expired", error = "invalid_token" }, json) + assert.are.equal("Bearer error=\"invalid_token\" error_description=\"The access token is invalid or has expired\"", res.headers["www-authenticate"]) + end) + it("does not work when requesting a different API - with realm set", function() + local token = provision_token() + + local res = assert(proxy_ssl_client:send { + method = "GET", + path = "/request?access_token=" .. token.access_token, + headers = { + ["Host"] = "oauth2_4.test" + } + }) + local body = assert.res_status(401, res) + local json = cjson.decode(body) + assert.same({ error_description = "The access token is invalid or has expired", error = "invalid_token" }, json) + assert.are.equal("Bearer error=\"invalid_token\" error_description=\"The access token is invalid or has expired\"", res.headers["www-authenticate"]) end) it("works when a correct access_token is being sent in a form body", function() local token = provision_token() @@ -3399,6 +3450,7 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function() } }) assert.res_status(401, res) + assert.are.equal("Bearer error=\"invalid_token\" error_description=\"The access token is invalid or has expired\"", res.headers["WWW-Authenticate"]) end) it("does not access two different APIs that are not sharing global credentials 2", function() local token = provision_token("oauth2.test") @@ -3412,6 +3464,7 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function() } }) assert.res_status(401, res) + assert.are.equal('Bearer error="invalid_token" error_description="The access token is invalid or has expired"', res.headers['www-authenticate']) local res = assert(proxy_ssl_client:send { method = "POST", @@ -3461,7 +3514,7 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function() local body = assert.res_status(401, res) local json = cjson.decode(body) assert.same({ error_description = "The access token is missing", error = "invalid_request" }, json) - assert.are.equal('Bearer realm="service"', res.headers['www-authenticate']) + assert.are.equal('Bearer', res.headers['www-authenticate']) end) it("returns 401 Unauthorized when an invalid access token is being sent via url parameter", function() local res = assert(proxy_ssl_client:send { @@ -3474,7 +3527,7 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function() local body = assert.res_status(401, res) local json = cjson.decode(body) assert.same({ error_description = "The access token is invalid or has expired", error = "invalid_token" }, json) - assert.are.equal('Bearer realm="service" error="invalid_token" error_description="The access token is invalid or has expired"', res.headers['www-authenticate']) + assert.are.equal('Bearer error="invalid_token" error_description="The access token is invalid or has expired"', res.headers['www-authenticate']) end) it("returns 401 Unauthorized when an invalid access token is being sent via the Authorization header", function() local res = assert(proxy_ssl_client:send { @@ -3488,7 +3541,7 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function() local body = assert.res_status(401, res) local json = cjson.decode(body) assert.same({ error_description = "The access token is invalid or has expired", error = "invalid_token" }, json) - assert.are.equal('Bearer realm="service" error="invalid_token" error_description="The access token is invalid or has expired"', res.headers['www-authenticate']) + assert.are.equal('Bearer error="invalid_token" error_description="The access token is invalid or has expired"', res.headers['www-authenticate']) end) it("returns 401 Unauthorized when token has expired", function() local token = provision_token() @@ -3513,7 +3566,7 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function() return status == 401 end, 7) assert.same({ error_description = "The access token is invalid or has expired", error = "invalid_token" }, json) - assert.are.equal('Bearer realm="service" error="invalid_token" error_description="The access token is invalid or has expired"', headers['www-authenticate']) + assert.are.equal('Bearer error="invalid_token" error_description="The access token is invalid or has expired"', headers['www-authenticate']) end) end) @@ -3668,10 +3721,10 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function() assert.truthy(db.oauth2_tokens:select({ id = id })) -- But waiting after the cache expiration (5 seconds) should block the request - local status, json + local status, json, res2 helpers.wait_until(function() local client = helpers.proxy_client() - local res = assert(client:send { + res2 = assert(client:send { method = "POST", path = "/request", headers = { @@ -3679,12 +3732,13 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function() authorization = "bearer " .. token.access_token } }) - status = res.status - local body = res:read_body() + status = res2.status + local body = res2:read_body() json = body and cjson.decode(body) return status == 401 end, 7) assert.same({ error_description = "The access token is invalid or has expired", error = "invalid_token" }, json) + assert.are.equal("Bearer error=\"invalid_token\" error_description=\"The access token is invalid or has expired\"", res2.headers["WWW-Authenticate"]) -- Refreshing the token local res = assert(proxy_ssl_client:send { @@ -3755,7 +3809,7 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function() return status == 401 end, 7) assert.same({ error_description = "The access token is invalid or has expired", error = "invalid_token" }, json) - assert.are.equal('Bearer realm="service" error="invalid_token" error_description="The access token is invalid or has expired"', headers['www-authenticate']) + assert.are.equal('Bearer error="invalid_token" error_description="The access token is invalid or has expired"', headers['www-authenticate']) local final_refreshed_token = refresh_token("oauth2_13.test", refreshed_token.refresh_token) local last_res = assert(proxy_client:send { @@ -3979,6 +4033,7 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function() config = { scopes = { "email", "profile", "user.email" }, anonymous = anonymous.id, + realm = "test-oauth2" }, }) @@ -4060,6 +4115,7 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function() } }) assert.response(res).has.status(401) + assert.are.equal("Bearer", res.headers["www-authenticate"]) end) it("fails 401, with only the second credential provided", function() @@ -4076,6 +4132,7 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function() } }) assert.response(res).has.status(401) + assert.are.equal("Key", res.headers["www-authenticate"]) end) it("fails 401, with no credential provided", function() @@ -4087,6 +4144,7 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function() } }) assert.response(res).has.status(401) + assert.are.equal("Bearer", res.headers["www-authenticate"]) end) end) @@ -4352,6 +4410,7 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function() "plugin is configured without 'global_credentials'", error = "invalid_token", }, json) + assert.are.equal("Bearer error=\"invalid_token\" error_description=\"The access token is invalid or has expired\"", res.headers["www-authenticate"]) end) end) end)