From 2d164ffb5bb04a435f8279f9832d1116fb389eb6 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.2-0.rockspec | 1 + kong/plugins/oauth2/access.lua | 42 ++- kong/plugins/oauth2/daos.lua | 20 + .../oauth2/migrations/004_200_to_210.lua | 8 + kong/plugins/oauth2/secret.lua | 347 ++++++++++++++++++ .../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 + 9 files changed, 537 insertions(+), 10 deletions(-) create mode 100644 kong/plugins/oauth2/secret.lua diff --git a/kong-2.0.2-0.rockspec b/kong-2.0.2-0.rockspec index 451607efb203..40d7543418c9 100644 --- a/kong-2.0.2-0.rockspec +++ b/kong-2.0.2-0.rockspec @@ -228,6 +228,7 @@ build = { ["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.secret"] = "kong/plugins/oauth2/secret.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", diff --git a/kong/plugins/oauth2/access.lua b/kong/plugins/oauth2/access.lua index dbd45baf7d4d..9bb738f99a71 100644 --- a/kong/plugins/oauth2/access.lua +++ b/kong/plugins/oauth2/access.lua @@ -3,6 +3,7 @@ local utils = require "kong.tools.utils" local constants = require "kong.constants" local sha256 = require "resty.sha256" local timestamp = require "kong.tools.timestamp" +local secret = require "kong.plugins.oauth2.secret" local kong = kong @@ -566,17 +567,40 @@ local function issue_token(conf) end if client then - if client.client_type == CLIENT_TYPE_CONFIDENTIAL and client.client_secret ~= client_secret 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\"" + if client.client_type == CLIENT_TYPE_CONFIDENTIAL 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 + + 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 + elseif client.client_type == CLIENT_TYPE_PUBLIC and strip(client_secret) ~= "" then response_params = { [ERROR] = "invalid_request", diff --git a/kong/plugins/oauth2/daos.lua b/kong/plugins/oauth2/daos.lua index e097f1429578..0a7b7a479edc 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, @@ -38,6 +43,21 @@ local oauth2_credentials = { { tags = typedefs.tags }, { client_type = { type = "string", required = true, default = "confidential", one_of = { "confidential", "public" }, }, }, }, + 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 index 24735a79b26f..192d6ccaa218 100644 --- a/kong/plugins/oauth2/migrations/004_200_to_210.lua +++ b/kong/plugins/oauth2/migrations/004_200_to_210.lua @@ -21,6 +21,13 @@ return { EXCEPTION WHEN DUPLICATE_COLUMN THEN -- Do nothing, accept existing state END$$; + + DO $$ + BEGIN + ALTER TABLE IF EXISTS ONLY oauth2_credentials ADD hash_secret BOOLEAN; + EXCEPTION WHEN DUPLICATE_COLUMN THEN + -- Do nothing, accept existing state + END$$; ]], }, @@ -29,6 +36,7 @@ return { ALTER TABLE oauth2_authorization_codes ADD challenge text; ALTER TABLE oauth2_authorization_codes ADD challenge_method text; ALTER TABLE oauth2_credentials ADD client_type text; + ALTER TABLE oauth2_credentials ADD hash_secret boolean; ]], }, } diff --git a/kong/plugins/oauth2/secret.lua b/kong/plugins/oauth2/secret.lua new file mode 100644 index 000000000000..015f944f9e1b --- /dev/null +++ b/kong/plugins/oauth2/secret.lua @@ -0,0 +1,347 @@ +local utils = require "kong.tools.utils" + + +local type = type +local fmt = string.format +local find = string.find +local pcall = pcall +local remove = table.remove +local concat = table.concat +local assert = assert +local tonumber = tonumber +local encode_base64 = ngx.encode_base64 +local decode_base64 = ngx.decode_base64 + + +local ENABLED_ALGORITHMS = { + ARGON2 = false, + BCRYPT = false, + PBKDF2 = true, + SCRYPT = false, +} + +--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 function infer(value) + value = utils.strip(value) + return tonumber(value, 10) or value +end + + +local function parse_phc(phc) + local parts = utils.split(phc, "$") + local count = #parts + if count < 2 or count > 5 then + return nil, "invalid phc string format" + end + + local id = parts[2] + local id_parts = utils.split(id, "-") + local id_count = #id_parts + + local prefix + local digest + if id_count == 1 then + prefix = id_parts[1] + else + prefix = id_parts[1] + remove(id_parts, 1) + digest = concat(id_parts, "-") + end + + local params = {} + local prms = parts[3] + if prms then + local prm_parts = utils.split(prms, ",") + for i = 1, #prm_parts do + local param = prm_parts[i] + local kv = utils.split(param, "=") + local kv_count = #kv + if kv_count == 1 then + params[#params + 1] = infer(kv[1]) + elseif kv_count == 2 then + local k = utils.strip(kv[1]) + params[k] = infer(kv[2]) + else + return nil, "invalid phc string format for parameter" + end + end + end + + local salt = parts[4] + if salt then + local decoded_salt = decode_base64(salt) + if decoded_salt then + salt = decoded_salt + end + end + + local hash = parts[5] + if hash then + local decoded_hash = decode_base64(hash) + if decoded_hash then + hash = decoded_hash + end + end + + return { + id = utils.strip(id), + prefix = utils.strip(prefix), + digest = utils.strip(digest), + params = params, + salt = salt, + hash = hash, + } +end + + +local PREFIX = nil -- currently chosen algorithm (nil means that we try to find one) + + +local ARGON2 +local ARGON2_ID = "$argon2" +if ENABLED_ALGORITHMS.ARGON2 then + 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 crypt = {} + + function crypt.hash(secret) + return argon2.hash_encoded(secret, utils.get_rand_bytes(ARGON2_SALT_LEN), ARGON2_OPTIONS) + end + + function crypt.verify(secret, hash) + return argon2.verify(hash, secret) + end + + return crypt + end) + + if ok then + ARGON2 = crypt + PREFIX = PREFIX or ARGON2_PREFIX + end +end + + +local BCRYPT +local BCRYPT_ID = "$2" +if ENABLED_ALGORITHMS.BCRYPT then + 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 crypt = {} + + function crypt.hash(secret) + return bcrypt.digest(secret, BCRYPT_ROUNDS) + end + + function crypt.verify(secret, hash) + return bcrypt.verify(secret, hash) + end + + return crypt + end) + + if ok then + BCRYPT = crypt + PREFIX = PREFIX or BCRYPT_PREFIX + end +end + + +local PBKDF2 +local PBKDF2_ID = "$pbkdf2" +if ENABLED_ALGORITHMS.PBKDF2 then + local PBKDF2_PREFIX + + local ok, crypt = pcall(function() + local kdf = require "resty.openssl.kdf" + + -- pbkdf2 settings + local PBKDF2_DIGEST = "sha512" + local PBKDF2_ITERATIONS = 10000 + local PBKDF2_HASH_LEN = 32 + local PBKDF2_SALT_LEN = 16 + + local EMPTY = {} + + local function derive(secret, opts) + opts = opts or EMPTY + local salt = opts.salt or utils.get_rand_bytes(PBKDF2_SALT_LEN) + local hash, err = kdf.derive({ + type = kdf.PBKDF2, + outlen = opts.outlen or PBKDF2_HASH_LEN, + pass = secret, + salt = salt, + md = opts.md or PBKDF2_DIGEST, + pbkdf2_iter = opts.pbkdf2_iter or PBKDF2_ITERATIONS, + }) + if not hash then + return nil, err + end + + local HASH = encode_base64(hash, true) + local SALT = encode_base64(salt, true) + + return fmt("%s-%s$i=%u,l=%u$%s$%s", + PBKDF2_ID, PBKDF2_DIGEST, + PBKDF2_ITERATIONS, PBKDF2_HASH_LEN, + SALT, HASH) + end + + do + local hash = derive("") + local parts = utils.split(hash, "$") + remove(parts) + remove(parts) + PBKDF2_PREFIX = concat(parts, "$") + end + + local crypt = {} + + function crypt.hash(secret) + return derive(secret) + end + + function crypt.verify(secret, hash) + local phc, err = parse_phc(hash) + if not phc then + return nil, err + end + + local outlen = phc.params.l + if not outlen and phc.hash then + outlen = #phc.hash + end + + local calculated_hash, err = derive(secret, { + outlen = outlen, + salt = phc.salt, + md = phc.digest, + pbkdf2_iter = phc.params.i + }) + if not calculated_hash then + return nil, err + end + + return calculated_hash == hash + end + + return crypt + end) + + + if ok then + PBKDF2 = crypt + PREFIX = PREFIX or PBKDF2_PREFIX + end +end + + +local crypt = {} + + +function crypt.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 + + if PBKDF2 then + return PBKDF2.hash(secret) + end + + return nil, "no suitable password hashing algorithm found" +end + + +function crypt.verify(secret, hash) + if type(secret) ~= "string" then + return false, "secret needs to be a string" + end + + if type(hash) ~= "string" then + return false, "hash needs to be a string" + end + + 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 + + if PBKDF2 and find(hash, PBKDF2_ID, 1, true) == 1 then + return PBKDF2.verify(secret, hash) + end + + return false, "no suitable password hashing algorithm found" +end + + +function crypt.needs_rehash(hash) + if type(hash) ~= "string" then + return true + end + + if PREFIX then + return find(hash, PREFIX, 1, true) ~= 1 + end + + return true +end + + +return crypt 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 007acadc26a5..d6bb0a3cf280 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 @@ -701,12 +701,14 @@ describe("declarative config: process_auto_fields", function() client_type = "confidential", name = "my-credential", redirect_uris = { "https://example.com" }, + hash_secret = false, }, { client_type = "confidential", name = "another-credential", consumer = "foo", redirect_uris = { "https://example.test" }, + hash_secret = false, }, } }, config) @@ -758,11 +760,13 @@ describe("declarative config: process_auto_fields", function() client_type = "confidential", name = "my-credential", redirect_uris = { "https://example.com" }, + hash_secret = false, }, { client_type = "confidential", name = "another-credential", redirect_uris = { "https://example.test" }, + hash_secret = false, }, } } @@ -789,7 +793,8 @@ describe("declarative config: process_auto_fields", function() client_type = "confidential", name = "my-credential", redirect_uris = { "https://example.com" }, - oauth2_tokens = {} + oauth2_tokens = {}, + hash_secret = false, }, } }, config) @@ -815,6 +820,7 @@ describe("declarative config: process_auto_fields", function() client_type = "confidential", 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 346a69bf2f74..afc4debfb32c 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 @@ -1548,6 +1548,7 @@ describe("declarative config: flatten", function() client_id = "RANDOM", client_secret = "RANDOM", client_type = "confidential", + hash_secret = false, consumer = { id = "UUID" }, @@ -1560,6 +1561,7 @@ describe("declarative config: flatten", function() client_id = "RANDOM", client_secret = "RANDOM", client_type = "confidential", + hash_secret = false, consumer = { id = "UUID", }, @@ -1810,6 +1812,7 @@ describe("declarative config: flatten", function() client_id = "RANDOM", client_secret = "RANDOM", client_type = "confidential", + hash_secret = false, consumer = { id = "UUID" }, @@ -1822,6 +1825,7 @@ describe("declarative config: flatten", function() client_id = "RANDOM", client_secret = "RANDOM", client_type = "confidential", + hash_secret = false, consumer = { id = "UUID" }, @@ -1855,6 +1859,7 @@ describe("declarative config: flatten", function() client_id = "RANDOM", client_secret = "RANDOM", client_type = "confidential", + hash_secret = false, consumer = { id = "UUID" }, @@ -1890,6 +1895,7 @@ describe("declarative config: flatten", function() client_id = "RANDOM", client_secret = "RANDOM", client_type = "confidential", + 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 f91c51a63be2..0774b350f334 100644 --- a/spec/03-plugins/25-oauth2/03-access_spec.lua +++ b/spec/03-plugins/25-oauth2/03-access_spec.lua @@ -175,6 +175,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 }, @@ -191,6 +192,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 }, @@ -207,6 +209,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 },