fix(oauth2): prevent an authorization code created by one plugin inst…

…ance to be exchanged for an access token by a different plugin instance (#10011) * fix(oauth2): prevent an authorization code created by one plugin instance to be exchanged for an access token by a different plugin instance * verify only if plugin_id is present to avoid existing codes being fails * add test case * change plugin_id field type from text to uuid * make plugin_id to be foreign key * fix merge issue * fix cassandra script
Kong · May 8, 2023 · 8c4e5c6 · 8c4e5c6 · khcp-gha-bot · May 8, 2023
1 parent 31a7424
commit 8c4e5c6
Show file tree

Hide file tree

Showing 7 changed files with 107 additions and 9 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -190,7 +190,8 @@
   [#10663](https://github.com/Kong/kong/pull/10663)
 - **gRPC gateway**: `null` in the JSON payload caused an uncaught exception to be thrown during pb.encode.
   [#10687](https://github.com/Kong/kong/pull/10687)
-
+- **Oauth2**: prevent an authorization code created by one plugin instance to be exchanged for an access token by a different plugin instance.
+  [#10011](https://github.com/Kong/kong/pull/10011)
 
 #### PDK
 

diff --git a/kong-3.3.0-0.rockspec b/kong-3.3.0-0.rockspec
@@ -306,6 +306,7 @@ build = {
     ["kong.plugins.oauth2.migrations.004_200_to_210"] = "kong/plugins/oauth2/migrations/004_200_to_210.lua",
     ["kong.plugins.oauth2.migrations.005_210_to_211"] = "kong/plugins/oauth2/migrations/005_210_to_211.lua",
     ["kong.plugins.oauth2.migrations.006_320_to_330"] = "kong/plugins/oauth2/migrations/006_320_to_330.lua",
+    ["kong.plugins.oauth2.migrations.007_320_to_330"] = "kong/plugins/oauth2/migrations/007_320_to_330.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",

diff --git a/kong/plugins/oauth2/access.lua b/kong/plugins/oauth2/access.lua
@@ -365,7 +365,8 @@ local function authorize(conf)
             authenticated_userid = parameters[AUTHENTICATED_USERID],
             scope = scopes,
             challenge = challenge,
-            challenge_method = challenge_method
+            challenge_method = challenge_method,
+            plugin = { id  = kong.plugin.get_id() },
           }, {
             ttl = 300
           })
@@ -643,6 +644,17 @@ local function issue_token(conf)
           end
         end
 
+        if not response_params[ERROR] and conf.global_credentials then
+          -- verify only if plugin is present to avoid existing codes being fails
+          if auth_code.plugin and
+             (kong.plugin.get_id() ~= auth_code.plugin.id) then
+            response_params = {
+              [ERROR] = "invalid_request",
+              error_description = "Invalid " .. CODE
+            }
+          end
+        end
+
         if not response_params[ERROR] then
           if not auth_code or (service_id and service_id ~= auth_code.service.id)
           then

diff --git a/kong/plugins/oauth2/daos.lua b/kong/plugins/oauth2/daos.lua
@@ -79,6 +79,7 @@ local oauth2_authorization_codes = {
     { scope = { type = "string" }, },
     { challenge = { type = "string", required = false }},
     { challenge_method = { type = "string", required = false, one_of = { "S256" } }},
+    { plugin = { type = "foreign", reference = "plugins", default = ngx.null, on_delete = "cascade", }, },
   },
 }
 

diff --git a/kong/plugins/oauth2/migrations/007_320_to_330.lua b/kong/plugins/oauth2/migrations/007_320_to_330.lua
@@ -0,0 +1,19 @@
+return {
+  postgres = {
+    up = [[
+      DO $$
+      BEGIN
+        ALTER TABLE IF EXISTS ONLY "oauth2_authorization_codes" ADD "plugin_id" UUID REFERENCES "plugins" ("id") ON DELETE CASCADE;
+      EXCEPTION WHEN DUPLICATE_COLUMN THEN
+        -- Do nothing, accept existing state
+      END$$;
+    ]],
+  },
+
+  cassandra = {
+    up = [[
+      ALTER TABLE oauth2_authorization_codes ADD plugin_id uuid;
+      CREATE INDEX IF NOT EXISTS ON oauth2_authorization_codes(plugin_id);
+    ]],
+  },
+}
diff --git a/kong/plugins/oauth2/migrations/init.lua b/kong/plugins/oauth2/migrations/init.lua
@@ -4,4 +4,5 @@ return {
   "004_200_to_210",
   "005_210_to_211",
   "006_320_to_330",
+  "007_320_to_330",
 }
diff --git a/spec/03-plugins/25-oauth2/03-access_spec.lua b/spec/03-plugins/25-oauth2/03-access_spec.lua
@@ -261,6 +261,7 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function()
       local service14   = admin_api.services:insert()
       local service15   = admin_api.services:insert()
       local service16   = admin_api.services:insert()
+      local service17   = admin_api.services:insert()
 
       local route1 = assert(admin_api.routes:insert({
         hosts     = { "oauth2.com" },
@@ -370,6 +371,11 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function()
         service     = service16,
       }))
 
+      local route17 = assert(admin_api.routes:insert({
+        hosts       = { "oauth2_17.com" },
+        protocols   = { "http", "https" },
+        service     = service17,
+      }))
 
       local service_grpc = assert(admin_api.services:insert {
           name = "grpc",
@@ -542,6 +548,14 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function()
           pkce = "lax",
         }
       })
+
+      admin_api.oauth2_plugins:insert({
+        route = { id = route17.id },
+        config   = {
+          scopes                   = { "email", "profile", "user.email" },
+          global_credentials       = true,
+        }
+      })
     end)
 
     before_each(function ()
@@ -2360,7 +2374,7 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function()
         assert.same({ error_description = "code_verifier is required for PKCE authorization requests", error = "invalid_request" }, json)
       end)
       it("success when no code_verifier provided for public app without pkce when conf.pkce is none", function()
-        local code = provision_code()
+        local code = provision_code("oauth2_14.com")
         local res = assert(proxy_ssl_client:send {
           method  = "POST",
           path    = "/oauth2/token",
@@ -2587,7 +2601,7 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function()
       end)
       it("fails when code verifier does not match challenge for confidential app when conf.pkce is strict", function()
         local challenge, _ = get_pkce_tokens()
-        local code = provision_code(nil, nil, nil, challenge)
+        local code = provision_code("oauth2_15.com", nil, nil, challenge)
         local res = assert(proxy_ssl_client:send {
           method  = "POST",
           path    = "/oauth2/token",
@@ -2666,7 +2680,8 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function()
         assert.same({ error_description = "Invalid client authentication", error = "invalid_client" }, json)
       end)
       it("fails when no code verifier provided for confidential app when conf.pkce is strict", function()
-        local code = provision_code()
+        local challenge, _ = get_pkce_tokens()
+        local code = provision_code("oauth2_15.com", nil, nil, challenge)
         local res = assert(proxy_ssl_client:send {
           method  = "POST",
           path    = "/oauth2/token",
@@ -2687,7 +2702,7 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function()
       end)
       it("fails when no code verifier provided for confidential app with pkce when conf.pkce is lax", function()
         local challenge, _ = get_pkce_tokens()
-        local code = provision_code(nil, nil, nil, challenge)
+        local code = provision_code("oauth2_16.com", nil, nil, challenge)
         local res = assert(proxy_ssl_client:send {
           method  = "POST",
           path    = "/oauth2/token",
@@ -2708,7 +2723,7 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function()
       end)
       it("fails when no code verifier provided for confidential app with pkce when conf.pkce is none", function()
         local challenge, _ = get_pkce_tokens()
-        local code = provision_code(nil, nil, nil, challenge)
+        local code = provision_code("oauth2_14.com", nil, nil, challenge)
         local res = assert(proxy_ssl_client:send {
           method  = "POST",
           path    = "/oauth2/token",
@@ -2728,7 +2743,7 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function()
         assert.same({ error_description = "code_verifier is required for PKCE authorization requests", error = "invalid_request" }, json)
       end)
       it("suceeds when no code verifier provided for confidential app without pkce when conf.pkce is none", function()
-        local code = provision_code()
+        local code = provision_code("oauth2_14.com")
         local res = assert(proxy_ssl_client:send {
           method  = "POST",
           path    = "/oauth2/token",
@@ -2753,7 +2768,7 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function()
         assert.matches("%w+", json.refresh_token)
       end)
       it("suceeds when no code verifier provided for confidential app without pkce when conf.pkce is lax", function()
-        local code = provision_code()
+        local code = provision_code("oauth2_16.com")
         local res = assert(proxy_ssl_client:send {
           method  = "POST",
           path    = "/oauth2/token",
@@ -2777,6 +2792,54 @@ describe("Plugin: oauth2 [#" .. strategy .. "]", function()
         assert.equal(32, #json.refresh_token)
         assert.matches("%w+", json.refresh_token)
       end)
+
+      it("fails when exchanging a code created by a different plugin instance when both plugin instances set global_credentials to true", function()
+        local code = provision_code("oauth2_16.com") -- obtain a code from plugin oauth2_16.com
+        local res = assert(proxy_ssl_client:send {
+          method  = "POST",
+          path    = "/oauth2/token",
+          body    = {
+            code             = code,
+            client_id        = "clientid123",
+            client_secret    = "secret123",
+            grant_type       = "authorization_code",
+          },
+          headers = {
+            ["Host"]         = "oauth2_17.com", -- exchange the code from plugin oauth2_17.com
+            ["Content-Type"] = "application/json",
+          }
+        })
+        local body = assert.res_status(400, res)
+        local json = cjson.decode(body)
+        assert.same({
+          error = "invalid_request",
+          error_description = "Invalid code",
+        }, json)
+      end)
+
+      it("should not fail when plugin_id is not present which indicates it's an old code", function()
+        local code = provision_code("oauth2_16.com")
+        local db_code, err = db.oauth2_authorization_codes:select_by_code(code)
+        assert.is_nil(err)
+        db_code.plugin = ngx.null
+        local _, _, err = db.oauth2_authorization_codes:update({ id = db_code.id }, db_code)
+        assert.is_nil(err)
+        local res = assert(proxy_ssl_client:send {
+          method  = "POST",
+          path    = "/oauth2/token",
+          body    = {
+            code             = code,
+            client_id        = "clientid123",
+            client_secret    = "secret123",
+            grant_type       = "authorization_code",
+          },
+          headers = {
+            ["Host"]         = "oauth2_16.com",
+            ["Content-Type"] = "application/json",
+          }
+        })
+        assert.res_status(200, res)
+      end)
     end)
 
     describe("Making a request", function()