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 · Feb 27, 2020 · 238987b · 238987b
1 parent 19f3d2d
commit 238987b
Show file tree

Hide file tree

Showing 10 changed files with 361 additions and 11 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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"
                 },