Skip to content

Commit

Permalink
fix(cache) avoid deadlocks upon callback errors
Browse files Browse the repository at this point in the history
Main cause is exiting early on error conditions, when the lock has not
yet been released. It occurs both in the `get_or_set` method itself as
well as in the callbacks all over the code base.

Fix Kong#2186
  • Loading branch information
Tieske authored and thibaultcha committed Mar 21, 2017
1 parent 768f006 commit 5c7e511
Show file tree
Hide file tree
Showing 12 changed files with 213 additions and 65 deletions.
7 changes: 5 additions & 2 deletions kong/core/plugins_iterator.lua
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ local function load_plugin_into_memory(api_id, consumer_id, plugin_name)
name = plugin_name
}
if err then
return responses.send_HTTP_INTERNAL_SERVER_ERROR(err)
return nil, err
end

if #rows > 0 then
Expand All @@ -37,8 +37,11 @@ end
-- @treturn table Plugin retrieved from the cache or database.
local function load_plugin_configuration(api_id, consumer_id, plugin_name)
local cache_key = cache.plugin_key(plugin_name, api_id, consumer_id)
local plugin = cache.get_or_set(cache_key, nil, load_plugin_into_memory,
local plugin, err = cache.get_or_set(cache_key, nil, load_plugin_into_memory,
api_id, consumer_id, plugin_name)
if err then
responses.send_HTTP_INTERNAL_SERVER_ERROR(err)
end
if plugin ~= nil and plugin.enabled then
return plugin.config or {}
end
Expand Down
8 changes: 5 additions & 3 deletions kong/plugins/acl/handler.lua
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ end
local function load_acls_into_memory(consumer_id)
local results, err = singletons.dao.acls:find_all {consumer_id = consumer_id}
if err then
return responses.send_HTTP_INTERNAL_SERVER_ERROR(err)
return nil, err
end
return results
end
Expand All @@ -37,9 +37,11 @@ function ACLHandler:access(conf)
end

-- Retrieve ACL
local acls = cache.get_or_set(cache.acls_key(consumer_id), nil,
local acls, err = cache.get_or_set(cache.acls_key(consumer_id), nil,
load_acls_into_memory, consumer_id)

if err then
responses.send_HTTP_INTERNAL_SERVER_ERROR(err)
end
if not acls then acls = {} end

local block
Expand Down
20 changes: 15 additions & 5 deletions kong/plugins/basic-auth/access.lua
Original file line number Diff line number Diff line change
Expand Up @@ -69,16 +69,20 @@ end
local function load_credential_into_memory(username)
local credentials, err = singletons.dao.basicauth_credentials:find_all {username = username}
if err then
return responses.send_HTTP_INTERNAL_SERVER_ERROR(err)
return nil, err
end
return credentials[1]
end

local function load_credential_from_db(username)
if not username then return end

return cache.get_or_set(cache.basicauth_credential_key(username),
local credential, err = cache.get_or_set(cache.basicauth_credential_key(username),
nil, load_credential_into_memory, username)
if err then
return responses.send_HTTP_INTERNAL_SERVER_ERROR(err)
end
return credential
end

local function load_consumer_into_memory(consumer_id, anonymous)
Expand All @@ -87,7 +91,7 @@ local function load_consumer_into_memory(consumer_id, anonymous)
if anonymous and not err then
err = 'anonymous consumer "'..consumer_id..'" not found'
end
return responses.send_HTTP_INTERNAL_SERVER_ERROR(err)
return nil, err
end
return result
end
Expand Down Expand Up @@ -132,8 +136,11 @@ local function do_authentication(conf)
end

-- Retrieve consumer
local consumer = cache.get_or_set(cache.consumer_key(credential.consumer_id),
local consumer, err = cache.get_or_set(cache.consumer_key(credential.consumer_id),
nil, load_consumer_into_memory, credential.consumer_id, false)
if err then
return responses.send_HTTP_INTERNAL_SERVER_ERROR(err)
end

set_consumer(consumer, credential)

Expand All @@ -145,8 +152,11 @@ function _M.execute(conf)
if not ok then
if conf.anonymous ~= "" then
-- get anonymous user
local consumer = cache.get_or_set(cache.consumer_key(conf.anonymous),
local consumer, err = cache.get_or_set(cache.consumer_key(conf.anonymous),
nil, load_consumer_into_memory, conf.anonymous, true)
if err then
return responses.send_HTTP_INTERNAL_SERVER_ERROR(err)
end
set_consumer(consumer, nil)
else
return responses.send(err.status, err.message)
Expand Down
23 changes: 17 additions & 6 deletions kong/plugins/hmac-auth/access.lua
Original file line number Diff line number Diff line change
Expand Up @@ -106,17 +106,22 @@ end
local function load_credential_into_memory(username)
local keys, err = singletons.dao.hmacauth_credentials:find_all { username = username }
if err then
return responses.send_HTTP_INTERNAL_SERVER_ERROR(err)
return nil, err
end
return keys[1]
end

local function load_credential(username)
local credential
local credential, err
if username then
credential = cache.get_or_set(cache.hmacauth_credential_key(username),
credential, err = cache.get_or_set(cache.hmacauth_credential_key(username),
nil, load_credential_into_memory, username)
end

if err then
return responses.send_HTTP_INTERNAL_SERVER_ERROR(err)
end

return credential
end

Expand Down Expand Up @@ -144,7 +149,7 @@ local function load_consumer_into_memory(consumer_id, anonymous)
if anonymous and not err then
err = 'anonymous consumer "'..consumer_id..'" not found'
end
return responses.send_HTTP_INTERNAL_SERVER_ERROR(err)
return nil, err
end
return result
end
Expand Down Expand Up @@ -197,8 +202,11 @@ local function do_authentication(conf)
end

-- Retrieve consumer
local consumer = cache.get_or_set(cache.consumer_key(credential.consumer_id),
local consumer, err = cache.get_or_set(cache.consumer_key(credential.consumer_id),
nil, load_consumer_into_memory, credential.consumer_id)
if err then
return responses.send_HTTP_INTERNAL_SERVER_ERROR(err)
end

set_consumer(consumer, credential)

Expand All @@ -210,8 +218,11 @@ function _M.execute(conf)
if not ok then
if conf.anonymous ~= "" then
-- get anonymous user
local consumer = cache.get_or_set(cache.consumer_key(conf.anonymous),
local consumer, err = cache.get_or_set(cache.consumer_key(conf.anonymous),
nil, load_consumer_into_memory, conf.anonymous, true)
if err then
return responses.send_HTTP_INTERNAL_SERVER_ERROR(err)
end
set_consumer(consumer, nil)
else
return responses.send(err.status, err.message)
Expand Down
19 changes: 14 additions & 5 deletions kong/plugins/jwt/handler.lua
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ end
local function load_credential(jwt_secret_key)
local rows, err = singletons.dao.jwt_secrets:find_all {key = jwt_secret_key}
if err then
return responses.send_HTTP_INTERNAL_SERVER_ERROR()
return nil, err
end
return rows[1]
end
Expand All @@ -64,7 +64,7 @@ local function load_consumer(consumer_id, anonymous)
if anonymous and not err then
err = 'anonymous consumer "'..consumer_id..'" not found'
end
return responses.send_HTTP_INTERNAL_SERVER_ERROR(err)
return nil, err
end
return result
end
Expand Down Expand Up @@ -114,8 +114,11 @@ local function do_authentication(conf)
end

-- Retrieve the secret
local jwt_secret = cache.get_or_set(cache.jwtauth_credential_key(jwt_secret_key),
local jwt_secret, err = cache.get_or_set(cache.jwtauth_credential_key(jwt_secret_key),
nil, load_credential, jwt_secret_key)
if err then
return responses.send_HTTP_INTERNAL_SERVER_ERROR(err)
end

if not jwt_secret then
return false, {status = 403, message = "No credentials found for given '"..conf.key_claim_name.."'"}
Expand Down Expand Up @@ -149,8 +152,11 @@ local function do_authentication(conf)
end

-- Retrieve the consumer
local consumer = cache.get_or_set(cache.consumer_key(jwt_secret_key),
local consumer, err = cache.get_or_set(cache.consumer_key(jwt_secret_key),
nil, load_consumer, jwt_secret.consumer_id)
if err then
return responses.send_HTTP_INTERNAL_SERVER_ERROR(err)
end

-- However this should not happen
if not consumer then
Expand All @@ -169,8 +175,11 @@ function JwtHandler:access(conf)
if not ok then
if conf.anonymous ~= "" then
-- get anonymous user
local consumer = cache.get_or_set(cache.consumer_key(conf.anonymous),
local consumer, err = cache.get_or_set(cache.consumer_key(conf.anonymous),
nil, load_consumer, conf.anonymous, true)
if err then
return responses.send_HTTP_INTERNAL_SERVER_ERROR(err)
end
set_consumer(consumer, nil)
else
return responses.send(err.status, err.message)
Expand Down
21 changes: 15 additions & 6 deletions kong/plugins/key-auth/handler.lua
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ local function load_credential(key)
key = key
}
if not creds then
return responses.send_HTTP_INTERNAL_SERVER_ERROR(err)
return nil, err
end
return creds[1]
end
Expand All @@ -37,7 +37,7 @@ local function load_consumer(consumer_id, anonymous)
if anonymous and not err then
err = 'anonymous consumer "'..consumer_id..'" not found'
end
return responses.send_HTTP_INTERNAL_SERVER_ERROR(err)
return nil, err
end
return result
end
Expand Down Expand Up @@ -98,8 +98,11 @@ local function do_authentication(conf)
end

-- retrieve our consumer linked to this API key
local credential = cache.get_or_set(cache.keyauth_credential_key(key),
local credential, err = cache.get_or_set(cache.keyauth_credential_key(key),
nil, load_credential, key)
if err then
return responses.send_HTTP_INTERNAL_SERVER_ERROR(err)
end

-- no credential in DB, for this key, it is invalid, HTTP 403
if not credential then
Expand All @@ -111,8 +114,11 @@ local function do_authentication(conf)
-----------------------------------------

-- retrieve the consumer linked to this API key, to set appropriate headers
local consumer = cache.get_or_set(cache.consumer_key(credential.consumer_id),
local consumer, err = cache.get_or_set(cache.consumer_key(credential.consumer_id),
nil, load_consumer, credential.consumer_id)
if err then
return responses.send_HTTP_INTERNAL_SERVER_ERROR(err)
end

set_consumer(consumer, credential)

Expand All @@ -126,8 +132,11 @@ function KeyAuthHandler:access(conf)
if not ok then
if conf.anonymous ~= "" then
-- get anonymous user
local consumer = cache.get_or_set(cache.consumer_key(conf.anonymous),
nil, load_consumer, conf.anonymous, true)
local consumer, err = cache.get_or_set(cache.consumer_key(conf.anonymous),
nil, load_consumer, conf.anonymous, true)
if err then
responses.send_HTTP_INTERNAL_SERVER_ERROR(err)
end
set_consumer(consumer, nil)
else
return responses.send(err.status, err.message)
Expand Down
15 changes: 10 additions & 5 deletions kong/plugins/ldap-auth/access.lua
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ local function ldap_authenticate(given_username, given_password, conf)
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)
return responses.send_HTTP_INTERNAL_SERVER_ERROR(err)
return nil, err, responses.status_codes.HTTP_INTERNAL_SERVER_ERROR
end

if conf.start_tls then
Expand All @@ -68,7 +68,8 @@ 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)
local ok, err, status = ldap_authenticate(given_username, given_password, conf)
if status ~= nil then return nil, err, status end
if err ~= nil then ngx_log(ngx_error, err) end
if not ok then
return nil
Expand All @@ -82,8 +83,9 @@ local function authenticate(conf, given_credentials)
return false
end

local credential = cache.get_or_set(cache.ldap_credential_key(ngx.ctx.api.id, given_username),
local credential, err, status = cache.get_or_set(cache.ldap_credential_key(ngx.ctx.api.id, given_username),
conf.cache_ttl, load_credential, given_username, given_password, conf)
if status then responses.send(status, err) end

return credential and credential.password == given_password, credential
end
Expand All @@ -94,7 +96,7 @@ local function load_consumer(consumer_id, anonymous)
if anonymous and not err then
err = 'anonymous consumer "'..consumer_id..'" not found'
end
return responses.send_HTTP_INTERNAL_SERVER_ERROR(err)
return nil, err
end
return result
end
Expand Down Expand Up @@ -151,8 +153,11 @@ function _M.execute(conf)
if not ok then
if conf.anonymous ~= "" then
-- get anonymous user
local consumer = cache.get_or_set(cache.consumer_key(conf.anonymous),
local consumer, err = cache.get_or_set(cache.consumer_key(conf.anonymous),
nil, load_consumer, conf.anonymous, true)
if err then
responses.send_HTTP_INTERNAL_SERVER_ERROR(err)
end
set_consumer(consumer, nil)
else
return responses.send(err.status, err.message)
Expand Down
Loading

0 comments on commit 5c7e511

Please sign in to comment.