From 2517d203ed11ccd3669538da632563757d0ed6c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20Nowak?= Date: Fri, 13 Oct 2023 15:36:19 +0200 Subject: [PATCH] fix(hmac): add missing www-authenticate headers When server returns 401 Unauthorized response it should return WWW-Authenticate header as well with proper challenge. HMAC auth was missing this header. Fix: #7772 KAG-321 --- .../unreleased/kong/hmac_www_authenticate.yml | 3 + kong/clustering/compat/removed_fields.lua | 3 + kong/plugins/hmac-auth/access.lua | 81 +++++++++++------- kong/plugins/hmac-auth/schema.lua | 1 + .../09-hybrid_mode/09-config-compat_spec.lua | 14 ++++ .../19-hmac-auth/03-access_spec.lua | 83 +++++++++++++++---- 6 files changed, 142 insertions(+), 43 deletions(-) create mode 100644 changelog/unreleased/kong/hmac_www_authenticate.yml diff --git a/changelog/unreleased/kong/hmac_www_authenticate.yml b/changelog/unreleased/kong/hmac_www_authenticate.yml new file mode 100644 index 000000000000..23e0e20ab91c --- /dev/null +++ b/changelog/unreleased/kong/hmac_www_authenticate.yml @@ -0,0 +1,3 @@ +message: "**hmac-auth**: Add WWW-Authenticate headers to 401 responses." +type: bugfix +scope: Plugin diff --git a/kong/clustering/compat/removed_fields.lua b/kong/clustering/compat/removed_fields.lua index 6dbad36758a7..ef370447bc9f 100644 --- a/kong/clustering/compat/removed_fields.lua +++ b/kong/clustering/compat/removed_fields.lua @@ -154,5 +154,8 @@ return { ldap_auth = { "realm", }, + hmac_auth = { + "realm", + }, }, } diff --git a/kong/plugins/hmac-auth/access.lua b/kong/plugins/hmac-auth/access.lua index 4df53921d525..39e3acb90fe4 100644 --- a/kong/plugins/hmac-auth/access.lua +++ b/kong/plugins/hmac-auth/access.lua @@ -275,24 +275,29 @@ local function set_consumer(consumer, credential) end 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 authorization = kong_request.get_header(AUTHORIZATION) local proxy_authorization = kong_request.get_header(PROXY_AUTHORIZATION) + local www_auth_content = conf.realm and fmt('hmac realm="%s"', conf.realm) or 'hmac' -- If both headers are missing, return 401 if not (authorization or proxy_authorization) then - return false, { status = 401, message = "Unauthorized" } + return false, unauthorized("Unauthorized", www_auth_content) end -- validate clock skew if not (validate_clock_skew(X_DATE, conf.clock_skew) or validate_clock_skew(DATE, conf.clock_skew)) then - return false, { - status = 401, - message = "HMAC signature cannot be verified, a valid date or " .. - "x-date header is required for HMAC Authentication" - } + return false, unauthorized( + "HMAC signature cannot be verified, a valid date or " .. + "x-date header is required for HMAC Authentication", + www_auth_content + ) end -- retrieve hmac parameter from Proxy-Authorization header @@ -312,26 +317,26 @@ local function do_authentication(conf) local ok, err = validate_params(hmac_params, conf) if not ok then kong.log.debug(err) - return false, { status = 401, message = SIGNATURE_NOT_VALID } + return false, unauthorized(SIGNATURE_NOT_VALID, www_auth_content) end -- validate signature local credential = load_credential(hmac_params.username) if not credential then kong.log.debug("failed to retrieve credential for ", hmac_params.username) - return false, { status = 401, message = SIGNATURE_NOT_VALID } + return false, unauthorized(SIGNATURE_NOT_VALID, www_auth_content) end hmac_params.secret = credential.secret if not validate_signature(hmac_params) then - return false, { status = 401, message = SIGNATURE_NOT_SAME } + return false, unauthorized(SIGNATURE_NOT_SAME, www_auth_content) end -- If request body validation is enabled, then verify digest. if conf.validate_request_body and not validate_body() then kong.log.debug("digest validation failed") - return false, { status = 401, message = SIGNATURE_NOT_SAME } + return false, unauthorized(SIGNATURE_NOT_SAME, www_auth_content) end -- Retrieve consumer @@ -349,34 +354,52 @@ local function do_authentication(conf) return true 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 -local _M = {} - + set_consumer(consumer) +end -function _M.execute(conf) - 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. +--- 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 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 - 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 + return kong.response.error(err.status, err.message, err.headers) + end +end - set_consumer(consumer) - else - return kong.response.error(err.status, err.message, err.headers) - end +local _M = {} + +function _M.execute(conf) + + if conf.anonymous then + return logical_OR_authentication(conf) + else + return logical_AND_authentication(conf) end end diff --git a/kong/plugins/hmac-auth/schema.lua b/kong/plugins/hmac-auth/schema.lua index a95b53bd62f9..3e83acf7d5cb 100644 --- a/kong/plugins/hmac-auth/schema.lua +++ b/kong/plugins/hmac-auth/schema.lua @@ -29,6 +29,7 @@ return { elements = { type = "string", one_of = ALGORITHMS }, default = ALGORITHMS, }, }, + { realm = { description = "When authentication fails the plugin sends `WWW-Authenticate` header with `realm` attribute value.", type = "string", required = false }, }, }, }, }, 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 43e23d84645f..8bdc0d8d63ef 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 @@ -676,6 +676,20 @@ describe("CP/DP config compat transformations #" .. strategy, function() -- cleanup admin.plugins:remove({ id = ldap_auth.id }) end) + + it("[hmac-auth] removes realm for versions below 3.8", function() + local hmac_auth = admin.plugins:insert { + name = "hmac-auth", + config = { + realm = "test" + } + } + local expected_hmac_auth_prior_38 = cycle_aware_deep_copy(hmac_auth) + expected_hmac_auth_prior_38.config.realm = nil + do_assert(utils.uuid(), "3.7.0", expected_hmac_auth_prior_38) + -- cleanup + admin.plugins:remove({ id = hmac_auth.id }) + end) end) describe("compatibility test for response-transformer plugin", function() diff --git a/spec/03-plugins/19-hmac-auth/03-access_spec.lua b/spec/03-plugins/19-hmac-auth/03-access_spec.lua index 4e3a1920d0fe..c771500426a4 100644 --- a/spec/03-plugins/19-hmac-auth/03-access_spec.lua +++ b/spec/03-plugins/19-hmac-auth/03-access_spec.lua @@ -47,7 +47,8 @@ for _, strategy in helpers.each_strategy() do name = "hmac-auth", route = { id = route1.id }, config = { - clock_skew = 3000 + clock_skew = 3000, + realm = "test-realm" } } @@ -175,19 +176,40 @@ for _, strategy in helpers.each_strategy() do end) describe("HMAC Authentication", function() - it("should not be authorized when the hmac credentials are missing", function() - local date = os.date("!%a, %d %b %Y %H:%M:%S GMT") - local res = assert(proxy_client:send { - method = "POST", - body = {}, - headers = { - ["HOST"] = "hmacauth.test", - date = date - } - }) - local body = assert.res_status(401, res) - body = cjson.decode(body) - assert.equal("Unauthorized", body.message) + describe("when realm is set", function () + it("should not be authorized when the hmac credentials are missing", function() + local date = os.date("!%a, %d %b %Y %H:%M:%S GMT") + local res = assert(proxy_client:send { + method = "POST", + body = {}, + headers = { + ["HOST"] = "hmacauth.test", + date = date + } + }) + local body = assert.res_status(401, res) + assert.equal('hmac realm="test-realm"', res.headers["WWW-Authenticate"]) + body = cjson.decode(body) + assert.equal("Unauthorized", body.message) + end) + end) + + describe("when realm is not set", function () + it("should return a 401 with an invalid authorization header", function() + local date = os.date("!%a, %d %b %Y %H:%M:%S GMT") + local res = assert(proxy_client:send { + method = "GET", + path = "/request", + body = {}, + headers = { + ["HOST"] = "hmacauth6.test", + date = date, + ["proxy-authorization"] = "this is no hmac token at all is it?", + }, + }) + assert.res_status(401, res) + assert.equal('hmac', res.headers["WWW-Authenticate"]) + end) end) it("rejects gRPC call without credentials", function() @@ -211,6 +233,7 @@ for _, strategy in helpers.each_strategy() do } }) local body = assert.res_status(401, res) + assert.equal('hmac realm="test-realm"', res.headers["WWW-Authenticate"]) body = cjson.decode(body) assert.equal(SIGNATURE_NOT_VALID, body.message) end) @@ -228,6 +251,7 @@ for _, strategy in helpers.each_strategy() do } }) local body = assert.res_status(401, res) + assert.equal('hmac realm="test-realm"', res.headers["WWW-Authenticate"]) body = cjson.decode(body) assert.equal("HMAC signature does not match", body.message) end) @@ -242,6 +266,7 @@ for _, strategy in helpers.each_strategy() do } }) local body = assert.res_status(401, res) + assert.equal('hmac realm="test-realm"', res.headers["WWW-Authenticate"]) body = cjson.decode(body) assert.equal([[HMAC signature cannot be verified, ]] .. [[a valid date or x-date header is]] @@ -260,6 +285,7 @@ for _, strategy in helpers.each_strategy() do } }) local body = assert.res_status(401, res) + assert.equal('hmac realm="test-realm"', res.headers["WWW-Authenticate"]) body = cjson.decode(body) assert.equal(SIGNATURE_NOT_VALID, body.message) end) @@ -276,6 +302,7 @@ for _, strategy in helpers.each_strategy() do } }) local body = assert.res_status(401, res) + assert.equal('hmac realm="test-realm"', res.headers["WWW-Authenticate"]) body = cjson.decode(body) assert.equal(SIGNATURE_NOT_VALID, body.message) end) @@ -293,6 +320,7 @@ for _, strategy in helpers.each_strategy() do } }) local body = assert.res_status(401, res) + assert.equal('hmac realm="test-realm"', res.headers["WWW-Authenticate"]) body = cjson.decode(body) assert.equal(SIGNATURE_NOT_VALID, body.message) end) @@ -310,6 +338,7 @@ for _, strategy in helpers.each_strategy() do } }) local body = assert.res_status(401, res) + assert.equal('hmac realm="test-realm"', res.headers["WWW-Authenticate"]) body = cjson.decode(body) assert.equal(SIGNATURE_NOT_VALID, body.message) end) @@ -326,6 +355,7 @@ for _, strategy in helpers.each_strategy() do } }) local body = assert.res_status(401, res) + assert.equal('hmac realm="test-realm"', res.headers["WWW-Authenticate"]) body = cjson.decode(body) assert.equal(SIGNATURE_NOT_VALID, body.message) end) @@ -341,6 +371,7 @@ for _, strategy in helpers.each_strategy() do } }) local body = assert.res_status(401, res) + assert.equal('hmac realm="test-realm"', res.headers["WWW-Authenticate"]) body = cjson.decode(body) assert.equal("Unauthorized", body.message) end) @@ -361,6 +392,7 @@ for _, strategy in helpers.each_strategy() do }, }) local body = assert.res_status(401, res) + assert.equal('hmac realm="test-realm"', res.headers["WWW-Authenticate"]) body = cjson.decode(body) assert.not_nil(body.message) assert.matches("HMAC signature cannot be verified", body.message) @@ -381,6 +413,7 @@ for _, strategy in helpers.each_strategy() do }, }) local body = assert.res_status(401, res) + assert.equal('hmac realm="test-realm"', res.headers["WWW-Authenticate"]) body = cjson.decode(body) assert.not_nil(body.message) assert.matches("HMAC signature cannot be verified", body.message) @@ -633,6 +666,7 @@ for _, strategy in helpers.each_strategy() do }) local body = assert.res_status(401, res) + assert.equal('hmac realm="test-realm"', res.headers["WWW-Authenticate"]) body = cjson.decode(body) assert.equal(SIGNATURE_NOT_VALID, body.message) end) @@ -659,6 +693,7 @@ for _, strategy in helpers.each_strategy() do }) local body = assert.res_status(401, res) + assert.equal('hmac realm="test-realm"', res.headers["WWW-Authenticate"]) body = cjson.decode(body) assert.equal(SIGNATURE_NOT_VALID, body.message) end) @@ -686,6 +721,7 @@ for _, strategy in helpers.each_strategy() do }) local body = assert.res_status(401, res) + assert.equal('hmac realm="test-realm"', res.headers["WWW-Authenticate"]) body = cjson.decode(body) assert.equal(SIGNATURE_NOT_VALID, body.message) end) @@ -711,6 +747,7 @@ for _, strategy in helpers.each_strategy() do }, }) local body = assert.res_status(401, res) + assert.equal('hmac realm="test-realm"', res.headers["WWW-Authenticate"]) body = cjson.decode(body) assert.equal(SIGNATURE_NOT_VALID, body.message) end) @@ -736,6 +773,7 @@ for _, strategy in helpers.each_strategy() do }, }) local body = assert.res_status(401, res) + assert.equal('hmac realm="test-realm"', res.headers["WWW-Authenticate"]) body = cjson.decode(body) assert.equal(SIGNATURE_NOT_VALID, body.message) end) @@ -761,6 +799,7 @@ for _, strategy in helpers.each_strategy() do }, }) local body = assert.res_status(401, res) + assert.equal('hmac realm="test-realm"', res.headers["WWW-Authenticate"]) body = cjson.decode(body) assert.equal(SIGNATURE_NOT_VALID, body.message) end) @@ -786,6 +825,7 @@ for _, strategy in helpers.each_strategy() do }, }) local body = assert.res_status(401, res) + assert.equal('hmac realm="test-realm"', res.headers["WWW-Authenticate"]) body = cjson.decode(body) assert.equal(SIGNATURE_NOT_VALID, body.message) end) @@ -834,6 +874,7 @@ for _, strategy in helpers.each_strategy() do }, }) assert.res_status(401, res) + assert.equal('hmac realm="test-realm"', res.headers["WWW-Authenticate"]) end) it("should pass the right headers to the upstream server", function() @@ -905,6 +946,7 @@ for _, strategy in helpers.each_strategy() do }, }) local body = assert.res_status(401, res) + assert.equal('hmac realm="test-realm"', res.headers["WWW-Authenticate"]) body = cjson.decode(body) assert.equal([[HMAC signature cannot be verified, a valid date or]] .. [[ x-date header is required for HMAC Authentication]], body.message) @@ -930,6 +972,7 @@ for _, strategy in helpers.each_strategy() do }, }) local body = assert.res_status(401, res) + assert.equal('hmac realm="test-realm"', res.headers["WWW-Authenticate"]) body = cjson.decode(body) assert.equal([[HMAC signature cannot be verified, a valid date or]] .. [[ x-date header is required for HMAC Authentication]], body.message) @@ -1071,6 +1114,7 @@ for _, strategy in helpers.each_strategy() do } }) local body = assert.res_status(401, res) + assert.equal('hmac', res.headers["WWW-Authenticate"]) body = cjson.decode(body) assert.equal("HMAC signature does not match", body.message) end) @@ -1253,6 +1297,7 @@ for _, strategy in helpers.each_strategy() do }, }) local body = assert.res_status(401, res) + assert.equal('hmac', res.headers["WWW-Authenticate"]) body = cjson.decode(body) assert.equal("HMAC signature does not match", body.message) end) @@ -1280,6 +1325,7 @@ for _, strategy in helpers.each_strategy() do }, }) local body = assert.res_status(401, res) + assert.equal('hmac', res.headers["WWW-Authenticate"]) body = cjson.decode(body) assert.equal("HMAC signature does not match", body.message) end) @@ -1307,6 +1353,7 @@ for _, strategy in helpers.each_strategy() do } }) local body = assert.res_status(401, res) + assert.equal('hmac', res.headers["WWW-Authenticate"]) body = cjson.decode(body) assert.equal("HMAC signature does not match", body.message) end) @@ -1354,6 +1401,7 @@ for _, strategy in helpers.each_strategy() do }, }) assert.res_status(401, res) + assert.equal('hmac', res.headers["WWW-Authenticate"]) encodedSignature = ngx.encode_base64( hmac_sha1_binary("secret", "date: " @@ -1373,6 +1421,7 @@ for _, strategy in helpers.each_strategy() do }, }) assert.res_status(401, res) + assert.equal('hmac', res.headers["WWW-Authenticate"]) end) it("should pass with GET with request-line having query param", function() @@ -1587,6 +1636,7 @@ for _, strategy in helpers.each_strategy() do }, }) assert.res_status(401, res) + assert.equal('hmac', res.headers["WWW-Authenticate"]) end) it("should pass with GET with hmac-sha384", function() @@ -1653,6 +1703,7 @@ for _, strategy in helpers.each_strategy() do }, }) assert.res_status(401, res) + assert.equal('hmac', res.headers["WWW-Authenticate"]) end) it("should return a 401 with an invalid authorization header", function() @@ -1668,6 +1719,7 @@ for _, strategy in helpers.each_strategy() do }, }) assert.res_status(401, res) + assert.equal('hmac', res.headers["WWW-Authenticate"]) end) it("should pass with hmac-sha1", function() @@ -1831,6 +1883,7 @@ for _, strategy in helpers.each_strategy() do }, }) assert.response(res).has.status(401) + assert.equal('hmac', res.headers["WWW-Authenticate"]) end) it("fails 401, with only the second credential provided", function() @@ -1844,6 +1897,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() @@ -1855,6 +1909,7 @@ for _, strategy in helpers.each_strategy() do }, }) assert.response(res).has.status(401) + assert.equal('Key', res.headers["WWW-Authenticate"]) end) end)