From 238987b24c8ffcec3905329579915dde91cf193d Mon Sep 17 00:00:00 2001 From: Aapo Talvensaari Date: Wed, 26 Feb 2020 01:38:27 +0200 Subject: [PATCH] feat(oauth2) optionally hash client secret ### Summary This was originally implemented with #4866 by @janza. After some review comments, and the introduction of DAO transformations, I decided to make changes, thus opening this new PR. This PR adds a new `boolean` column `hash_secret` to `oauth2_credentials` that is used to determine whether or not the `client_secret` will be hashed. The PR adds support for `argon2`, and `bcrypt` and it uses `argon2` if the system library for argon2 is installed. Otherwise it will fallback to `bcrypt`. The plugin will also check if the `client_secret` needs to be rehashed (on usage). One caveat. If you run this on cluster and some nodes have `argon2` and some don't, it is possible that you cannot use the credentials on those that don't. So keep your environments similar. ### Issues resolved Close #4866, Fix #1237 (at least on OAuth2) --- kong-2.0.0-0.rockspec | 5 +- kong/plugins/oauth2/access.lua | 40 ++++- kong/plugins/oauth2/daos.lua | 20 +++ .../oauth2/migrations/004_200_to_210.lua | 18 ++ kong/plugins/oauth2/migrations/init.lua | 1 + kong/plugins/oauth2/secret.lua | 159 ++++++++++++++++++ .../02-process_auto_fields_spec.lua | 8 +- .../11-declarative_config/03-flatten_spec.lua | 6 + spec/03-plugins/25-oauth2/02-api_spec.lua | 112 ++++++++++++ spec/03-plugins/25-oauth2/03-access_spec.lua | 3 + 10 files changed, 361 insertions(+), 11 deletions(-) create mode 100644 kong/plugins/oauth2/migrations/004_200_to_210.lua create mode 100644 kong/plugins/oauth2/secret.lua diff --git a/kong-2.0.0-0.rockspec b/kong-2.0.0-0.rockspec index 006984467a52..54eea74586f8 100644 --- a/kong-2.0.0-0.rockspec +++ b/kong-2.0.0-0.rockspec @@ -11,6 +11,8 @@ description = { license = "Apache 2.0" } dependencies = { + "bcrypt == 2.1", + "argon2-ffi == 3.0.1", "inspect == 3.1.1", "luasec == 0.9", "luasocket == 3.0-rc1", @@ -224,11 +226,12 @@ build = { ["kong.plugins.oauth2.migrations"] = "kong/plugins/oauth2/migrations/init.lua", ["kong.plugins.oauth2.migrations.000_base_oauth2"] = "kong/plugins/oauth2/migrations/000_base_oauth2.lua", ["kong.plugins.oauth2.migrations.003_130_to_140"] = "kong/plugins/oauth2/migrations/003_130_to_140.lua", + ["kong.plugins.oauth2.migrations.004_200_to_210"] = "kong/plugins/oauth2/migrations/004_200_to_210.lua", ["kong.plugins.oauth2.handler"] = "kong/plugins/oauth2/handler.lua", ["kong.plugins.oauth2.access"] = "kong/plugins/oauth2/access.lua", ["kong.plugins.oauth2.schema"] = "kong/plugins/oauth2/schema.lua", ["kong.plugins.oauth2.daos"] = "kong/plugins/oauth2/daos.lua", - + ["kong.plugins.oauth2.secret"] = "kong/plugins/oauth2/secret.lua", ["kong.plugins.log-serializers.basic"] = "kong/plugins/log-serializers/basic.lua", diff --git a/kong/plugins/oauth2/access.lua b/kong/plugins/oauth2/access.lua index 363ee5d4aedd..ef77c25a6c10 100644 --- a/kong/plugins/oauth2/access.lua +++ b/kong/plugins/oauth2/access.lua @@ -2,6 +2,7 @@ local url = require "socket.url" local utils = require "kong.tools.utils" local constants = require "kong.constants" local timestamp = require "kong.tools.timestamp" +local secret = require "kong.plugins.oauth2.secret" local kong = kong @@ -431,17 +432,38 @@ local function issue_token(conf) end end - if client and client.client_secret ~= client_secret then - response_params = { - [ERROR] = "invalid_client", - error_description = "Invalid client authentication" - } + if client then + local authenticated + if client.hash_secret then + authenticated = secret.verify(client_secret, client.client_secret) + if authenticated and secret.needs_rehash(client.client_secret) then + local pk = kong.db.oauth2_credentials.schema:extract_pk_values(client) + local ok, err = kong.db.oauth2_credentials:update(pk, { + client_secret = client_secret, + hash_secret = true, + }) + + if not ok then + kong.log.warn(err) + end + end - if from_authorization_header then - invalid_client_properties = { - status = 401, - www_authenticate = "Basic realm=\"OAuth2.0\"" + else + authenticated = client.client_secret == client_secret + end + + if not authenticated then + response_params = { + [ERROR] = "invalid_client", + error_description = "Invalid client authentication" } + + if from_authorization_header then + invalid_client_properties = { + status = 401, + www_authenticate = "Basic realm=\"OAuth2.0\"" + } + end end end diff --git a/kong/plugins/oauth2/daos.lua b/kong/plugins/oauth2/daos.lua index f21e98682b8b..30de7934dc17 100644 --- a/kong/plugins/oauth2/daos.lua +++ b/kong/plugins/oauth2/daos.lua @@ -1,5 +1,9 @@ local url = require "socket.url" local typedefs = require "kong.db.schema.typedefs" +local secret = require "kong.plugins.oauth2.secret" + + +local assert = assert local function validate_uri(uri) @@ -28,6 +32,7 @@ local oauth2_credentials = { { name = { type = "string", required = true }, }, { client_id = { type = "string", required = false, unique = true, auto = true }, }, { client_secret = { type = "string", required = false, auto = true }, }, + { hash_secret = { type = "boolean", required = true, default = false }, }, { redirect_uris = { type = "array", required = false, @@ -37,6 +42,21 @@ local oauth2_credentials = { }, }, }, { tags = typedefs.tags }, }, + transformations = { + { + input = { "hash_secret" }, + needs = { "client_secret" }, + on_write = function(hash_secret, client_secret) + if not hash_secret then + return {} + end + local hash = assert(secret.hash(client_secret)) + return { + client_secret = hash, + } + end, + }, + }, } diff --git a/kong/plugins/oauth2/migrations/004_200_to_210.lua b/kong/plugins/oauth2/migrations/004_200_to_210.lua new file mode 100644 index 000000000000..9bb173b05db4 --- /dev/null +++ b/kong/plugins/oauth2/migrations/004_200_to_210.lua @@ -0,0 +1,18 @@ +return { + postgres = { + up = [[ + DO $$ + BEGIN + ALTER TABLE IF EXISTS ONLY oauth2_credentials ADD hash_secret BOOLEAN DEFAULT FALSE; + EXCEPTION WHEN DUPLICATE_COLUMN THEN + -- Do nothing, accept existing state + END$$; + ]], + }, + + cassandra = { + up = [[ + ALTER TABLE oauth2_credentials ADD hash_secret boolean; + ]], + }, +} diff --git a/kong/plugins/oauth2/migrations/init.lua b/kong/plugins/oauth2/migrations/init.lua index b258bb4f88da..05b3b17258ff 100644 --- a/kong/plugins/oauth2/migrations/init.lua +++ b/kong/plugins/oauth2/migrations/init.lua @@ -1,4 +1,5 @@ return { "000_base_oauth2", "003_130_to_140", + "004_200_to_210", } diff --git a/kong/plugins/oauth2/secret.lua b/kong/plugins/oauth2/secret.lua new file mode 100644 index 000000000000..0b761bcd0425 --- /dev/null +++ b/kong/plugins/oauth2/secret.lua @@ -0,0 +1,159 @@ +local utils = require "kong.tools.utils" + + +local type = type +local find = string.find +local pcall = pcall +local remove = table.remove +local concat = table.concat +local assert = assert + + +--local CORES +--do +-- local infos = utils.get_system_infos() +-- if type(infos) == "table" then +-- CORES = infos.cores +-- end +-- if not CORES then +-- CORES = ngx.worker.count() or 1 +-- end +--end + + +local PREFIX = nil -- currently chosen algorithm + + +local ARGON2 +local ARGON2_ID = "$argon2" +do + local ARGON2_PREFIX + local ok, crypt = pcall(function() + local argon2 = require "argon2" + + -- argon2 settings + local ARGON2_VARIANT = argon2.variants.argon2_id + local ARGON2_PARALLELISM = 1 --CORES + local ARGON2_T_COST = 1 + local ARGON2_M_COST = 4096 + local ARGON2_HASH_LEN = 32 + local ARGON2_SALT_LEN = 16 + + local ARGON2_OPTIONS = { + variant = ARGON2_VARIANT, + parallelism = ARGON2_PARALLELISM, + hash_len = ARGON2_HASH_LEN, + t_cost = ARGON2_T_COST, + m_cost = ARGON2_M_COST, + } + do + local hash = argon2.hash_encoded("", utils.get_rand_bytes(ARGON2_SALT_LEN), ARGON2_OPTIONS) + local parts = utils.split(hash, "$") + remove(parts) + remove(parts) + ARGON2_PREFIX = concat(parts, "$") + end + + local secret = {} + + function secret.hash(secret) + return argon2.hash_encoded(secret, utils.get_rand_bytes(ARGON2_SALT_LEN), ARGON2_OPTIONS) + end + + function secret.verify(secret, hash) + return argon2.verify(hash, secret) + end + + return secret + end) + + if ok then + ARGON2 = crypt + PREFIX = PREFIX or ARGON2_PREFIX + end +end + + +local BCRYPT +local BCRYPT_ID = "$2" +do + local BCRYPT_PREFIX + local ok, crypt = pcall(function() + local bcrypt = require "bcrypt" + + -- bcrypt settings + local BCRYPT_ROUNDS = 12 + + do + local hash = bcrypt.digest("", BCRYPT_ROUNDS) + local parts = utils.split(hash, "$") + remove(parts) + BCRYPT_PREFIX = concat(parts, "$") + end + + local secret = {} + + function secret.hash(secret) + return bcrypt.digest(secret, BCRYPT_ROUNDS) + end + + function secret.verify(secret, hash) + return bcrypt.verify(secret, hash) + end + + return secret + end) + + if ok then + BCRYPT = crypt + PREFIX = PREFIX or BCRYPT_PREFIX + end +end + + +local secret = {} + + +function secret.hash(secret) + assert(type(secret) == "string", "secret needs to be a string") + + if ARGON2 then + return ARGON2.hash(secret) + end + + if BCRYPT then + return BCRYPT.hash(secret) + end + + return nil, "no suitable password hashing algorithm found" +end + + +function secret.verify(secret, hash) + assert(type(secret) == "string", "secret needs to be a string") + assert(type(hash) == "string", "hash needs to be a string") + + if ARGON2 and find(hash, ARGON2_ID, 1, true) == 1 then + return ARGON2.verify(secret, hash) + end + + if BCRYPT and find(hash, BCRYPT_ID, 1, true) == 1 then + return BCRYPT.verify(secret, hash) + end + + return false, "no suitable password hashing algorithm found" +end + + +function secret.needs_rehash(hash) + assert(type(hash) == "string", "hash needs to be a string") + + if PREFIX then + return find(hash, PREFIX, 1, true) ~= 1 + end + + return true +end + + +return secret diff --git a/spec/01-unit/01-db/01-schema/11-declarative_config/02-process_auto_fields_spec.lua b/spec/01-unit/01-db/01-schema/11-declarative_config/02-process_auto_fields_spec.lua index f3196209483d..5633125e715f 100644 --- a/spec/01-unit/01-db/01-schema/11-declarative_config/02-process_auto_fields_spec.lua +++ b/spec/01-unit/01-db/01-schema/11-declarative_config/02-process_auto_fields_spec.lua @@ -700,11 +700,13 @@ describe("declarative config: process_auto_fields", function() { name = "my-credential", redirect_uris = { "https://example.com" }, + hash_secret = false, }, { name = "another-credential", consumer = "foo", redirect_uris = { "https://example.test" }, + hash_secret = false, }, } }, config) @@ -755,10 +757,12 @@ describe("declarative config: process_auto_fields", function() { name = "my-credential", redirect_uris = { "https://example.com" }, + hash_secret = false, }, { name = "another-credential", redirect_uris = { "https://example.test" }, + hash_secret = false, }, } } @@ -784,7 +788,8 @@ describe("declarative config: process_auto_fields", function() { name = "my-credential", redirect_uris = { "https://example.com" }, - oauth2_tokens = {} + oauth2_tokens = {}, + hash_secret = false, }, } }, config) @@ -809,6 +814,7 @@ describe("declarative config: process_auto_fields", function() { name = "my-credential", redirect_uris = { "https://example.com" }, + hash_secret = false, oauth2_tokens = { { expires_in = 1, diff --git a/spec/01-unit/01-db/01-schema/11-declarative_config/03-flatten_spec.lua b/spec/01-unit/01-db/01-schema/11-declarative_config/03-flatten_spec.lua index 0f0d2323ed32..37f262d4efae 100644 --- a/spec/01-unit/01-db/01-schema/11-declarative_config/03-flatten_spec.lua +++ b/spec/01-unit/01-db/01-schema/11-declarative_config/03-flatten_spec.lua @@ -1547,6 +1547,7 @@ describe("declarative config: flatten", function() oauth2_credentials = { { client_id = "RANDOM", client_secret = "RANDOM", + hash_secret = false, consumer = { id = "UUID" }, @@ -1558,6 +1559,7 @@ describe("declarative config: flatten", function() }, { client_id = "RANDOM", client_secret = "RANDOM", + hash_secret = false, consumer = { id = "UUID", }, @@ -1807,6 +1809,7 @@ describe("declarative config: flatten", function() oauth2_credentials = { { client_id = "RANDOM", client_secret = "RANDOM", + hash_secret = false, consumer = { id = "UUID" }, @@ -1818,6 +1821,7 @@ describe("declarative config: flatten", function() }, { client_id = "RANDOM", client_secret = "RANDOM", + hash_secret = false, consumer = { id = "UUID" }, @@ -1850,6 +1854,7 @@ describe("declarative config: flatten", function() oauth2_credentials = { { client_id = "RANDOM", client_secret = "RANDOM", + hash_secret = false, consumer = { id = "UUID" }, @@ -1884,6 +1889,7 @@ describe("declarative config: flatten", function() oauth2_credentials = { { client_id = "RANDOM", client_secret = "RANDOM", + hash_secret = false, consumer = { id = "UUID" }, diff --git a/spec/03-plugins/25-oauth2/02-api_spec.lua b/spec/03-plugins/25-oauth2/02-api_spec.lua index 96fe0e3c0166..46e5cb6ea154 100644 --- a/spec/03-plugins/25-oauth2/02-api_spec.lua +++ b/spec/03-plugins/25-oauth2/02-api_spec.lua @@ -1,6 +1,7 @@ local cjson = require "cjson" local helpers = require "spec.helpers" local admin_api = require "spec.fixtures.admin_api" +local secret = require "kong.plugins.oauth2.secret" for _, strategy in helpers.each_strategy() do @@ -148,6 +149,79 @@ for _, strategy in helpers.each_strategy() do }) assert.res_status(201, res) end) + it("creates oauth2 credential with a hashed auto-generated client_secret", function() + -- this is quite useless as nobody knows the client_secret + local res = assert(admin_client:send { + method = "POST", + path = "/consumers/bob/oauth2", + body = { + name = "Test APP", + redirect_uris = { "http://google.com/" }, + hash_secret = true, + }, + headers = { + ["Content-Type"] = "application/json" + } + }) + local body = cjson.decode(assert.res_status(201, res)) + assert.equal(true, body.hash_secret) + assert.equal(false, secret.needs_rehash(body.client_secret)) + end) + it("creates oauth2 credential with a hashed client_secret", function() + local res = assert(admin_client:send { + method = "POST", + path = "/consumers/bob/oauth2", + body = { + name = "Test APP", + redirect_uris = { "http://google.com/" }, + client_secret = "test", + hash_secret = true, + }, + headers = { + ["Content-Type"] = "application/json" + } + }) + local body = cjson.decode(assert.res_status(201, res)) + assert.equal(true, body.hash_secret) + assert.equal(false, secret.needs_rehash(body.client_secret)) + assert.equal(true, secret.verify("test", body.client_secret)) + assert.equal(false, secret.verify("invalid", body.client_secret)) + end) + it("creates oauth2 credential without hashing the auto-generated client_secret", function() + local res = assert(admin_client:send { + method = "POST", + path = "/consumers/bob/oauth2", + body = { + name = "Test APP", + redirect_uris = { "http://google.com/" }, + }, + headers = { + ["Content-Type"] = "application/json" + } + }) + local body = cjson.decode(assert.res_status(201, res)) + assert.equal(false, body.hash_secret) + assert.equal(true, secret.needs_rehash(body.client_secret)) + end) + it("creates oauth2 credential without hashing the client_secret", function() + local res = assert(admin_client:send { + method = "POST", + path = "/consumers/bob/oauth2", + body = { + name = "Test APP", + redirect_uris = { "http://google.com/" }, + client_secret = "test", + }, + headers = { + ["Content-Type"] = "application/json" + } + }) + local body = cjson.decode(assert.res_status(201, res)) + assert.equal(false, body.hash_secret) + assert.equal(true, secret.needs_rehash(body.client_secret)) + assert.equal(false, secret.verify("test", body.client_secret)) + assert.equal(false, secret.verify("invalid", body.client_secret)) + end) describe("errors", function() it("returns bad request", function() local res = assert(admin_client:send { @@ -445,6 +519,44 @@ for _, strategy in helpers.each_strategy() do local json = cjson.decode(body) assert.same({ redirect_uris = { "cannot parse 'not-valid'" } }, json.fields) end) + it("updating client_secret requires hash_secret", function() + local res = assert(admin_client:send { + method = "PATCH", + path = "/consumers/bob/oauth2/" .. credential.id, + body = { + client_secret = "test", + }, + headers = { + ["Content-Type"] = "application/json" + } + }) + local body = assert.res_status(400, res) + local json = cjson.decode(body) + assert.same({ + ["@entity"] = { + "all or none of these fields must be set: 'hash_secret', 'client_secret'" + } + }, json.fields) + end) + it("updating hash_secret requires client_secret", function() + local res = assert(admin_client:send { + method = "PATCH", + path = "/consumers/bob/oauth2/" .. credential.id, + body = { + hash_secret = true, + }, + headers = { + ["Content-Type"] = "application/json" + } + }) + local body = assert.res_status(400, res) + local json = cjson.decode(body) + assert.same({ + ["@entity"] = { + "all or none of these fields must be set: 'hash_secret', 'client_secret'" + } + }, json.fields) + end) end) end) diff --git a/spec/03-plugins/25-oauth2/03-access_spec.lua b/spec/03-plugins/25-oauth2/03-access_spec.lua index 81a5e85776d3..f71473ea3f10 100644 --- a/spec/03-plugins/25-oauth2/03-access_spec.lua +++ b/spec/03-plugins/25-oauth2/03-access_spec.lua @@ -135,6 +135,7 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function() client1 = admin_api.oauth2_credentials:insert { client_id = "clientid123", client_secret = "secret123", + hash_secret = true, redirect_uris = { "http://google.com/kong" }, name = "testapp", consumer = { id = consumer.id }, @@ -151,6 +152,7 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function() admin_api.oauth2_credentials:insert { client_id = "clientid333", client_secret = "secret333", + hash_secret = true, redirect_uris = { "http://google.com/kong" }, name = "testapp3", consumer = { id = consumer.id }, @@ -167,6 +169,7 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function() admin_api.oauth2_credentials:insert { client_id = "clientid1011", client_secret = "secret1011", + hash_secret = true, redirect_uris = { "http://google.com/kong", }, name = "testapp31", consumer = { id = consumer.id },