From 2b250485eaa957f37ac487f7c65e56d6ffe348dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20Nowak?= Date: Mon, 23 Oct 2023 16:54:21 +0200 Subject: [PATCH] fix(ldap): add missing www-authenticate headers When server returns 401 Unauthorized response it should return WWW-Authenticate header as well with proper challenge. Not all ldap-auth 401 responses had this header. Fix: #7772 KAG-321 --- .../unreleased/kong/ldap_www_authenticate.yml | 3 + kong/clustering/compat/removed_fields.lua | 5 +- kong/plugins/ldap-auth/access.lua | 84 ++++++++++++------- kong/plugins/ldap-auth/schema.lua | 1 + .../09-hybrid_mode/09-config-compat_spec.lua | 17 ++++ .../20-ldap-auth/01-access_spec.lua | 70 ++++++++++++++-- 6 files changed, 142 insertions(+), 38 deletions(-) create mode 100644 changelog/unreleased/kong/ldap_www_authenticate.yml diff --git a/changelog/unreleased/kong/ldap_www_authenticate.yml b/changelog/unreleased/kong/ldap_www_authenticate.yml new file mode 100644 index 000000000000..bd1fbe096d9b --- /dev/null +++ b/changelog/unreleased/kong/ldap_www_authenticate.yml @@ -0,0 +1,3 @@ +message: "**ldap-auth**: Add WWW-Authenticate headers to all 401 responses." +type: bugfix +scope: Plugin diff --git a/kong/clustering/compat/removed_fields.lua b/kong/clustering/compat/removed_fields.lua index 359d9b763797..6dbad36758a7 100644 --- a/kong/clustering/compat/removed_fields.lua +++ b/kong/clustering/compat/removed_fields.lua @@ -150,6 +150,9 @@ return { }, aws_lambda = { "empty_arrays_mode", - } + }, + ldap_auth = { + "realm", + }, }, } diff --git a/kong/plugins/ldap-auth/access.lua b/kong/plugins/ldap-auth/access.lua index fd79e6f2dccc..e75d92344860 100644 --- a/kong/plugins/ldap-auth/access.lua +++ b/kong/plugins/ldap-auth/access.lua @@ -232,25 +232,29 @@ local function set_consumer(consumer, credential) end end +local function unauthorized(message, authorization_scheme) + return { + status = 401, + message = message, + headers = { ["WWW-Authenticate"] = authorization_scheme } + } +end local function do_authentication(conf) local authorization_value = kong.request.get_header(AUTHORIZATION) local proxy_authorization_value = kong.request.get_header(PROXY_AUTHORIZATION) + local scheme = conf.header_type + if scheme == "ldap" then + -- ensure backwards compatibility (see GH PR #3656) + -- TODO: provide migration to capitalize older configurations + scheme = upper(scheme) + end + + local www_auth_content = conf.realm and fmt('%s realm="%s"', scheme, conf.realm) or scheme -- If both headers are missing, return 401 if not (authorization_value or proxy_authorization_value) then - local scheme = conf.header_type - if scheme == "ldap" then - -- ensure backwards compatibility (see GH PR #3656) - -- TODO: provide migration to capitalize older configurations - scheme = upper(scheme) - end - - return false, { - status = 401, - message = "Unauthorized", - headers = { ["WWW-Authenticate"] = scheme .. ' realm="kong"' } - } + return false, unauthorized("Unauthorized", www_auth_content) end local is_authorized, credential @@ -263,7 +267,7 @@ local function do_authentication(conf) end if not is_authorized then - return false, {status = 401, message = "Unauthorized" } + return false, unauthorized("Unauthorized", www_auth_content) end if conf.hide_credentials then @@ -277,30 +281,50 @@ local function do_authentication(conf) 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. +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 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 +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/ldap-auth/schema.lua b/kong/plugins/ldap-auth/schema.lua index afd6c1acc25f..a5738d471cd9 100644 --- a/kong/plugins/ldap-auth/schema.lua +++ b/kong/plugins/ldap-auth/schema.lua @@ -24,6 +24,7 @@ return { { keepalive = { description = "An optional value in milliseconds that defines how long an idle connection to LDAP server will live before being closed.", type = "number", default = 60000 }, }, { 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 fails with an authentication failure `4xx`.", type = "string" }, }, { header_type = { description = "An optional string to use as part of the Authorization header", type = "string", default = "ldap" }, }, + { realm = { description = "When authentication fails the plugin sends `WWW-Authenticate` header with `realm` attribute value.", type = "string", required = false }, }, }, entity_checks = { { conditional = { 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 18465456a086..67ed236d227a 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 @@ -659,6 +659,23 @@ describe("CP/DP config compat transformations #" .. strategy, function() -- cleanup admin.plugins:remove({ id = key_auth.id }) end) + + it("[ldap-auth] removes realm for versions below 3.8", function() + local ldap_auth = admin.plugins:insert { + name = "ldap-auth", + config = { + ldap_host = "localhost", + base_dn = "test", + attribute = "test", + realm = "test", + } + } + local expected_ldap_auth_prior_38 = cycle_aware_deep_copy(ldap_auth) + expected_ldap_auth_prior_38.config.realm = nil + do_assert(utils.uuid(), "3.7.0", expected_ldap_auth_prior_38) + -- cleanup + admin.plugins:remove({ id = ldap_auth.id }) + end) end) describe("compatibility test for response-transformer plugin", function() diff --git a/spec/03-plugins/20-ldap-auth/01-access_spec.lua b/spec/03-plugins/20-ldap-auth/01-access_spec.lua index f106076e58b6..af5dd2a6cd83 100644 --- a/spec/03-plugins/20-ldap-auth/01-access_spec.lua +++ b/spec/03-plugins/20-ldap-auth/01-access_spec.lua @@ -74,6 +74,10 @@ for _, ldap_strategy in pairs(ldap_strategies) do hosts = { "ldap7.test" }, } + local route8 = bp.routes:insert { + hosts = { "ldap8.test" }, + } + assert(bp.routes:insert { protocols = { "grpc" }, paths = { "/hello.HelloService/" }, @@ -110,6 +114,7 @@ for _, ldap_strategy in pairs(ldap_strategies) do attribute = "uid", hide_credentials = true, cache_ttl = 2, + realm = "test-ldap", } } @@ -177,6 +182,20 @@ for _, ldap_strategy in pairs(ldap_strategies) do } } + bp.plugins:insert { + route = { id = route8.id }, + name = "ldap-auth", + config = { + ldap_host = ldap_host_aws, + ldap_port = 389, + start_tls = ldap_strategy.start_tls, + base_dn = "ou=scientists,dc=ldap,dc=mashape,dc=com", + attribute = "uid", + header_type = "Basic", + realm = "test-ldap", + } + } + assert(helpers.start_kong({ database = strategy, nginx_conf = "spec/fixtures/custom_nginx.template", @@ -211,8 +230,7 @@ for _, ldap_strategy in pairs(ldap_strategies) do } }) assert.response(res).has.status(401) - local value = assert.response(res).has.header("www-authenticate") - assert.are.equal('LDAP realm="kong"', value) + assert.equal('LDAP', res.headers["WWW-Authenticate"]) local json = assert.response(res).has.jsonbody() assert.equal("Unauthorized", json.message) end) @@ -249,6 +267,7 @@ for _, ldap_strategy in pairs(ldap_strategies) do } }) assert.response(res).has.status(401) + assert.equal('LDAP', res.headers["WWW-Authenticate"]) local json = assert.response(res).has.jsonbody() assert.equal("Unauthorized", json.message) end) @@ -262,6 +281,7 @@ for _, ldap_strategy in pairs(ldap_strategies) do } }) assert.response(res).has.status(401) + assert.equal('LDAP', res.headers["WWW-Authenticate"]) local json = assert.response(res).has.jsonbody() assert.equal("Unauthorized", json.message) end) @@ -291,7 +311,7 @@ for _, ldap_strategy in pairs(ldap_strategies) do end) it("fails if credential type is invalid in post request", function() - local r = assert(proxy_client:send { + local res = assert(proxy_client:send { method = "POST", path = "/request", body = {}, @@ -301,7 +321,8 @@ for _, ldap_strategy in pairs(ldap_strategies) do ["content-type"] = "application/x-www-form-urlencoded", } }) - assert.response(r).has.status(401) + assert.response(res).has.status(401) + assert.equal('LDAP', res.headers["WWW-Authenticate"]) end) it("passes if credential is valid and starts with space in post request", function() local res = assert(proxy_client:send { @@ -349,6 +370,7 @@ for _, ldap_strategy in pairs(ldap_strategies) do } }) assert.response(res).has.status(401) + assert.equal('LDAP', res.headers["WWW-Authenticate"]) end) it("authorization fails with correct status with wrong very long password", function() local res = assert(proxy_client:send { @@ -360,6 +382,7 @@ for _, ldap_strategy in pairs(ldap_strategies) do } }) assert.response(res).has.status(401) + assert.equal('LDAP', res.headers["WWW-Authenticate"]) end) it("authorization fails if credential has multiple encoded usernames or passwords separated by ':' in get request", function() local res = assert(proxy_client:send { @@ -371,6 +394,7 @@ for _, ldap_strategy in pairs(ldap_strategies) do } }) assert.response(res).has.status(401) + assert.equal('LDAP', res.headers["WWW-Authenticate"]) end) it("does not pass if credential is invalid in get request", function() local res = assert(proxy_client:send { @@ -382,6 +406,7 @@ for _, ldap_strategy in pairs(ldap_strategies) do } }) assert.response(res).has.status(401) + assert.equal('LDAP', res.headers["WWW-Authenticate"]) end) it("does not hide credential sent along with authorization header to upstream server", function() local res = assert(proxy_client:send { @@ -408,6 +433,18 @@ for _, ldap_strategy in pairs(ldap_strategies) do assert.response(res).has.status(200) assert.request(res).has.no.header("authorization") end) + it("does not pass if credential is invalid in get request and passes www-authenticate realm information", function() + local res = assert(proxy_client:send { + method = "GET", + path = "/request", + headers = { + host = "ldap2.test", + authorization = "ldap " .. ngx.encode_base64("einstein:wrong_password") + } + }) + assert.response(res).has.status(401) + assert.equal('LDAP realm="test-ldap"', res.headers["WWW-Authenticate"]) + end) it("passes if custom credential type is given in post request", function() local r = assert(proxy_client:send { method = "POST", @@ -432,12 +469,27 @@ for _, ldap_strategy in pairs(ldap_strategies) do assert.response(res).has.status(401) local value = assert.response(res).has.header("www-authenticate") - assert.equal('Basic realm="kong"', value) + assert.equal('Basic', value) + local json = assert.response(res).has.jsonbody() + assert.equal("Unauthorized", json.message) + end) + it("injects conf.header_type in WWW-Authenticate header and realm if provided", function() + local res = assert(proxy_client:send { + method = "GET", + path = "/get", + headers = { + host = "ldap8.test", + } + }) + assert.response(res).has.status(401) + + local value = assert.response(res).has.header("www-authenticate") + assert.equal('Basic realm="test-ldap"', value) local json = assert.response(res).has.jsonbody() assert.equal("Unauthorized", json.message) end) it("fails if custom credential type is invalid in post request", function() - local r = assert(proxy_client:send { + local res = assert(proxy_client:send { method = "POST", path = "/request", body = {}, @@ -447,7 +499,8 @@ for _, ldap_strategy in pairs(ldap_strategies) do ["content-type"] = "application/x-www-form-urlencoded", } }) - assert.response(r).has.status(401) + assert.response(res).has.status(401) + assert.equal('Basic', res.headers["WWW-Authenticate"]) end) it("passes if credential is valid in get request using global plugin", function() local res = assert(proxy_client:send { @@ -676,6 +729,7 @@ for _, ldap_strategy in pairs(ldap_strategies) do } }) assert.response(res).has.status(401) + assert.equal('LDAP', res.headers["WWW-Authenticate"]) end) it("fails 401, with only the second credential provided", function() @@ -688,6 +742,7 @@ for _, ldap_strategy in pairs(ldap_strategies) do } }) assert.response(res).has.status(401) + assert.equal('Key', res.headers["WWW-Authenticate"]) end) it("fails 401, with no credential provided", function() @@ -699,6 +754,7 @@ for _, ldap_strategy in pairs(ldap_strategies) do } }) assert.response(res).has.status(401) + assert.equal('Key', res.headers["WWW-Authenticate"]) end) end)