hotfix(ldap-auth) distinguish valid and invalid credentials in cache

This change adds the password as part of cache key, doing so
we can cache invalid credential without blocking valid credential

This fixes issues described in this commit:

Kong@5c53051

From Kong#3677

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)

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)

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 {

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 {

spec/03-plugins/21-ldap-auth/02-invalidations_spec.lua

@@ -1,15 +1,14 @@
 local helpers = require "spec.helpers"
-local cjson = require "cjson"
 local fmt = string.format
 local lower = string.lower
 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,29 +27,36 @@ 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,
         }
       }
 
       assert(helpers.start_kong({
         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,64 +65,79 @@ 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,
           body   = {},
         })
         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