From 3a63b47c759c8cc58ea1c272ccae349c6cc2adfb Mon Sep 17 00:00:00 2001 From: Shashi Ranjan Date: Fri, 3 Aug 2018 21:33:43 -0700 Subject: [PATCH] fix(ldap-auth) use username, password for cache key This change adds password as part of cache key, doing so we can cache invalid credential without blocking valid credential --- kong/plugins/ldap-auth/access.lua | 28 +++--- kong/plugins/ldap-auth/ldap.lua | 16 ++-- .../21-ldap-auth/01-access_spec.lua | 6 +- .../21-ldap-auth/01-access_spec.lua | 6 +- .../21-ldap-auth/02-invalidations_spec.lua | 85 ++++++++++++------- 5 files changed, 86 insertions(+), 55 deletions(-) diff --git a/kong/plugins/ldap-auth/access.lua b/kong/plugins/ldap-auth/access.lua index f4c4c365390f..64bbb5825886 100644 --- a/kong/plugins/ldap-auth/access.lua +++ b/kong/plugins/ldap-auth/access.lua @@ -12,7 +12,6 @@ local fmt = string.format local ngx_log = ngx.log local request = ngx.req local ngx_error = ngx.ERR -local ngx_debug = ngx.DEBUG local md5 = ngx.md5 local decode_base64 = ngx.decode_base64 local ngx_socket_tcp = ngx.socket.tcp @@ -31,7 +30,8 @@ local _M = {} local function retrieve_credentials(authorization_header_value, conf) local username, password if authorization_header_value then - local s, e = find(lower(authorization_header_value), "^%s*" .. lower(conf.header_type) .. "%s+") + local s, e = find(lower(authorization_header_value), "^%s*" .. + lower(conf.header_type) .. "%s+") if s == 1 then local cred = sub(authorization_header_value, e + 1) local decoded_cred = decode_base64(cred) @@ -49,7 +49,8 @@ local function ldap_authenticate(given_username, given_password, conf) sock:settimeout(conf.timeout) ok, err = sock:connect(conf.ldap_host, conf.ldap_port) if not ok then - ngx_log(ngx_error, "[ldap-auth] failed to connect to " .. conf.ldap_host .. ":" .. tostring(conf.ldap_port) .. ": ", err) + ngx_log(ngx_error, "[ldap-auth] failed to connect to ", conf.ldap_host, + ":", tostring(conf.ldap_port),": ", err) return nil, err end @@ -60,7 +61,8 @@ local function ldap_authenticate(given_username, given_password, conf) end local _, err = sock:sslhandshake(true, conf.ldap_host, conf.verify_ldap_host) if err ~= nil then - return false, "failed to do SSL handshake with " .. conf.ldap_host .. ":" .. tostring(conf.ldap_port) .. ": " .. err + return false, fmt("failed to do SSL handshake with %s:%s: %s", + conf.ldap_host, tostring(conf.ldap_port), err) end end @@ -69,14 +71,13 @@ local function ldap_authenticate(given_username, given_password, conf) ok, suppressed_err = sock:setkeepalive(conf.keepalive) if not ok then - ngx_log(ngx_error, "[ldap-auth] failed to keepalive to " .. conf.ldap_host .. ":" .. tostring(conf.ldap_port) .. ": ", suppressed_err) + ngx_log(ngx_error, "[ldap-auth] failed to keepalive to ", conf.ldap_host, ":", + tostring(conf.ldap_port), ": ", suppressed_err) end return is_authenticated, err end local function load_credential(given_username, given_password, conf) - ngx_log(ngx_debug, "[ldap-auth] authenticating user against LDAP server: " .. conf.ldap_host .. ":" .. conf.ldap_port) - local ok, err = ldap_authenticate(given_username, given_password, conf) if err ~= nil then ngx_log(ngx_error, err) @@ -92,7 +93,7 @@ local function load_credential(given_username, given_password, conf) end -local function cache_key(conf, username) +local function cache_key(conf, username, password) if not ldap_config_cache[conf] then ldap_config_cache[conf] = md5(fmt("%s:%u:%s:%s:%u", lower(conf.ldap_host), @@ -103,18 +104,21 @@ local function cache_key(conf, username) )) end - return fmt("ldap_auth_cache:%s:%s", ldap_config_cache[conf], username) + return fmt("ldap_auth_cache:%s:%s:%s", ldap_config_cache[conf], + username, password) end local function authenticate(conf, given_credentials) - local given_username, given_password = retrieve_credentials(given_credentials, conf) + local given_username, given_password = retrieve_credentials(given_credentials, + conf) if given_username == nil then return false end - local credential, err = singletons.cache:get(cache_key(conf, given_username), { - ttl = conf.cache_ttl + local credential, err = singletons.cache:get(cache_key(conf, given_username, given_password), { + ttl = conf.cache_ttl, + neg_ttl = conf.cache_ttl }, load_credential, given_username, given_password, conf) if err or credential == nil then return responses.send_HTTP_INTERNAL_SERVER_ERROR(err) diff --git a/kong/plugins/ldap-auth/ldap.lua b/kong/plugins/ldap-auth/ldap.lua index d2d4c364a799..8653253b3bb0 100644 --- a/kong/plugins/ldap-auth/ldap.lua +++ b/kong/plugins/ldap-auth/ldap.lua @@ -53,7 +53,8 @@ function _M.bind_request(socket, username, password) local ldapAuth = encoder:encode({ _ldaptype = 80, password }) local bindReq = encoder:encode(3) .. encoder:encode(username) .. ldapAuth - local ldapMsg = encoder:encode(ldapMessageId) .. encodeLDAPOp(encoder, APPNO.BindRequest, true, bindReq) + local ldapMsg = encoder:encode(ldapMessageId) .. + encodeLDAPOp(encoder, APPNO.BindRequest, true, bindReq) local packet local pos, packet_len, tmp, _ local response = {} @@ -71,7 +72,8 @@ function _M.bind_request(socket, username, password) response.protocolOp = asn1.intToBER(tmp) if response.protocolOp.number ~= APPNO.BindResponse then - return false, string_format("Received incorrect Op in packet: %d, expected %d", response.protocolOp.number, APPNO.BindResponse) + return false, string_format("Received incorrect Op in packet: %d, expected %d", + response.protocolOp.number, APPNO.BindResponse) end pos, response.resultCode = decoder:decode(packet, pos) @@ -95,7 +97,9 @@ function _M.unbind_request(socket) local encoder = asn1.ASN1Encoder:new() ldapMessageId = ldapMessageId +1 - ldapMsg = encoder:encode(ldapMessageId) .. encodeLDAPOp(encoder, APPNO.UnbindRequest, false, nil) + ldapMsg = encoder:encode(ldapMessageId) .. + encodeLDAPOp(encoder, APPNO.UnbindRequest, + false, nil) packet = encoder:encodeSeq(ldapMsg) socket:send(packet) return true, "" @@ -110,7 +114,8 @@ function _M.start_tls(socket) local method_name = encoder:encode({_ldaptype = 80, "1.3.6.1.4.1.1466.20037"}) ldapMessageId = ldapMessageId +1 - ldapMsg = encoder:encode(ldapMessageId) .. encodeLDAPOp(encoder, APPNO.ExtendedRequest, true, method_name) + ldapMsg = encoder:encode(ldapMessageId) .. + encodeLDAPOp(encoder, APPNO.ExtendedRequest, true, method_name) packet = encoder:encodeSeq(ldapMsg) socket:send(packet) packet = socket:receive(2) @@ -123,7 +128,8 @@ function _M.start_tls(socket) response.protocolOp = asn1.intToBER(tmp) if response.protocolOp.number ~= APPNO.ExtendedResponse then - return false, string_format("Received incorrect Op in packet: %d, expected %d", response.protocolOp.number, APPNO.ExtendedResponse) + return false, string_format("Received incorrect Op in packet: %d, expected %d", + response.protocolOp.number, APPNO.ExtendedResponse) end pos, response.resultCode = decoder:decode(packet, pos) diff --git a/spec-old-api/03-plugins/21-ldap-auth/01-access_spec.lua b/spec-old-api/03-plugins/21-ldap-auth/01-access_spec.lua index 89e631dbb030..a4fe8c30edac 100644 --- a/spec-old-api/03-plugins/21-ldap-auth/01-access_spec.lua +++ b/spec-old-api/03-plugins/21-ldap-auth/01-access_spec.lua @@ -7,7 +7,7 @@ local fmt = string.format local md5 = ngx.md5 -local function cache_key(conf, username) +local function cache_key(conf, username, password) local prefix = md5(fmt("%s:%u:%s:%s:%u", lower(conf.ldap_host), conf.ldap_port, @@ -16,7 +16,7 @@ local function cache_key(conf, username) conf.cache_ttl )) - return fmt("ldap_auth_cache:%s:%s", prefix, username) + return fmt("ldap_auth_cache:%s:%s:%s", prefix, username, password) end local ldap_host_aws = "ec2-54-172-82-117.compute-1.amazonaws.com" @@ -378,7 +378,7 @@ describe("Plugin: ldap-auth (access)", function() assert.response(r).has.status(200) -- Check that cache is populated - local key = cache_key(plugin2.config, "einstein") + local key = cache_key(plugin2.config, "einstein", "password") helpers.wait_until(function() local res = assert(client_admin:send { diff --git a/spec/03-plugins/21-ldap-auth/01-access_spec.lua b/spec/03-plugins/21-ldap-auth/01-access_spec.lua index 8e4726cdf9f8..9781490fa809 100644 --- a/spec/03-plugins/21-ldap-auth/01-access_spec.lua +++ b/spec/03-plugins/21-ldap-auth/01-access_spec.lua @@ -7,7 +7,7 @@ local fmt = string.format local md5 = ngx.md5 -local function cache_key(conf, username) +local function cache_key(conf, username, password) local prefix = md5(fmt("%s:%u:%s:%s:%u", lower(conf.ldap_host), conf.ldap_port, @@ -16,7 +16,7 @@ local function cache_key(conf, username) conf.cache_ttl )) - return fmt("ldap_auth_cache:%s:%s", prefix, username) + return fmt("ldap_auth_cache:%s:%s:%s", prefix, username, password) end @@ -414,7 +414,7 @@ for _, strategy in helpers.each_strategy() do assert.response(res).has.status(200) -- Check that cache is populated - local key = cache_key(plugin2.config, "einstein") + local key = cache_key(plugin2.config, "einstein", "password") helpers.wait_until(function() local res = assert(admin_client:send { diff --git a/spec/03-plugins/21-ldap-auth/02-invalidations_spec.lua b/spec/03-plugins/21-ldap-auth/02-invalidations_spec.lua index fd71b0a5ac13..a2986e1fed82 100644 --- a/spec/03-plugins/21-ldap-auth/02-invalidations_spec.lua +++ b/spec/03-plugins/21-ldap-auth/02-invalidations_spec.lua @@ -1,5 +1,4 @@ local helpers = require "spec.helpers" -local cjson = require "cjson" local fmt = string.format local lower = string.lower local md5 = ngx.md5 @@ -7,9 +6,9 @@ local md5 = ngx.md5 local ldap_host_aws = "ec2-54-172-82-117.compute-1.amazonaws.com" for _, strategy in helpers.each_strategy() do - describe("Plugin: ldap-auth (invalidations) [#" .. strategy .. "]", function() - local proxy_client + describe("Plugin: ldap-auth (invalidation) [#" .. strategy .. "]", function() local admin_client + local proxy_client local plugin setup(function() @@ -28,7 +27,8 @@ for _, strategy in helpers.each_strategy() do ldap_port = "389", start_tls = false, base_dn = "ou=scientists,dc=ldap,dc=mashape,dc=com", - attribute = "uid" + attribute = "uid", + cache_ttl = 1, } } @@ -36,21 +36,27 @@ for _, strategy in helpers.each_strategy() do database = strategy, nginx_conf = "spec/fixtures/custom_nginx.template", })) + end) - proxy_client = helpers.proxy_client() + before_each(function() admin_client = helpers.admin_client() + proxy_client = helpers.proxy_client() end) - teardown(function() - if proxy_client and admin_client then - proxy_client:close() + after_each(function() + if admin_client then admin_client:close() end + if proxy_client then + proxy_client:close() + end + end) + teardown(function() helpers.stop_kong(nil, true) end) - local function cache_key(conf, username) + local function cache_key(conf, username, password) local ldap_config_cache = md5(fmt("%s:%u:%s:%s:%u", lower(conf.ldap_host), conf.ldap_port, @@ -59,25 +65,24 @@ for _, strategy in helpers.each_strategy() do conf.cache_ttl )) - return fmt("ldap_auth_cache:%s:%s", ldap_config_cache, username) + return fmt("ldap_auth_cache:%s:%s:%s", ldap_config_cache, + username, password) end describe("authenticated LDAP user get cached", function() - it("should invalidate when Hmac Auth Credential entity is deleted", function() - -- It should work + it("should cache invalid credential", function() local res = assert(proxy_client:send { method = "GET", path = "/requests", body = {}, headers = { ["HOST"] = "ldapauth.com", - authorization = "ldap " .. ngx.encode_base64("einstein:password") + authorization = "ldap " .. ngx.encode_base64("einstein:wrongpassword") } }) - assert.res_status(200, res) + assert.res_status(403, res) - -- Check that cache is populated - local cache_key = cache_key(plugin.config, "einstein") + local cache_key = cache_key(plugin.config, "einstein", "wrongpassword") res = assert(admin_client:send { method = "GET", path = "/cache/" .. cache_key, @@ -85,38 +90,54 @@ for _, strategy in helpers.each_strategy() do }) assert.res_status(200, res) end) - it("should not do negative cache", function() + it("should invalidate negative cache once ttl expires", function() + local cache_key = cache_key(plugin.config, "einstein", "wrongpassword") + + helpers.wait_until(function() + local res = assert(admin_client:send { + method = "GET", + path = "/cache/" .. cache_key, + body = {}, + }) + res:read_body() + return res.status == 404 + end) + end) + it("should cache valid credential", function() + -- It should work local res = assert(proxy_client:send { method = "GET", path = "/requests", body = {}, headers = { ["HOST"] = "ldapauth.com", - authorization = "ldap " .. ngx.encode_base64("einstein:wrongpassword") + authorization = "ldap " .. ngx.encode_base64("einstein:password") } }) - assert.res_status(403, res) + assert.res_status(200, res) - local cache_key = cache_key(plugin.config, "einstein") + -- Check that cache is populated + local cache_key = cache_key(plugin.config, "einstein", "password") res = assert(admin_client:send { method = "GET", path = "/cache/" .. cache_key, body = {}, }) - local body = assert.res_status(200, res) - assert.is_equal("password", cjson.decode(body).password) - - local res = assert(proxy_client:send { - method = "GET", - path = "/requests", - body = {}, - headers = { - ["HOST"] = "ldapauth.com", - authorization = "ldap " .. ngx.encode_base64("einstein:password") - } - }) assert.res_status(200, res) end) + it("should invalidate cache once ttl expires", function() + local cache_key = cache_key(plugin.config, "einstein", "password") + + helpers.wait_until(function() + local res = assert(admin_client:send { + method = "GET", + path = "/cache/" .. cache_key, + body = {}, + }) + res:read_body() + return res.status == 404 + end) + end) end) end) end