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 · Aug 7, 2018 · 3a63b47 · 3a63b47
1 parent 54193bd
commit 3a63b47
Show file tree

Hide file tree

Showing 5 changed files with 86 additions and 55 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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