diff --git a/CHANGELOG.md b/CHANGELOG.md index af9b1903d5f..302f98a7c93 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -169,6 +169,8 @@ [#10160](https://github.com/Kong/kong/pull/10160) - **OAuth2**: `refresh_token_ttl` is now limited between `0` and `100000000` by schema validator. Previously numbers that are too large causes requests to fail. [#10068](https://github.com/Kong/kong/pull/10068) +- **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) ### Changed diff --git a/kong-3.2.0-0.rockspec b/kong-3.2.0-0.rockspec index 21e721af7cf..2e8070cf290 100644 --- a/kong-3.2.0-0.rockspec +++ b/kong-3.2.0-0.rockspec @@ -13,6 +13,7 @@ description = { } dependencies = { "inspect == 3.1.3", + "lpeg == 1.0.2", "luasec == 1.2.0", "luasocket == 3.0-rc1", "penlight == 1.13.1", @@ -301,6 +302,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.migrations.005_210_to_211"] = "kong/plugins/oauth2/migrations/005_210_to_211.lua", + ["kong.plugins.oauth2.migrations.006_220_to_221"] = "kong/plugins/oauth2/migrations/006_220_to_221.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 index 2c47fd12ed2..9a301f51805 100644 --- a/kong/plugins/oauth2/access.lua +++ b/kong/plugins/oauth2/access.lua @@ -87,6 +87,10 @@ do return ngx_decode_base64(value) end end +local mime_type = require "kong.tools.mime_type" +local function get_plugin_identifier(conf) + return conf.__key__ +end local function generate_token(conf, service, credential, authenticated_userid, @@ -365,7 +369,8 @@ local function authorize(conf) authenticated_userid = parameters[AUTHENTICATED_USERID], scope = scopes, challenge = challenge, - challenge_method = challenge_method + challenge_method = challenge_method, + plugin_identifier = get_plugin_identifier(conf), }, { ttl = 300 }) @@ -643,6 +648,16 @@ local function issue_token(conf) end end + if not response_params[ERROR] and conf.global_credentials then + local expected_identifier = get_plugin_identifier(conf) + if expected_identifier ~= auth_code.plugin_identifier 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 index a9a354e414a..e9749d6bbd1 100644 --- 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_identifier = { type = "string", required = false } }, }, } diff --git a/kong/plugins/oauth2/migrations/006_220_to_221.lua b/kong/plugins/oauth2/migrations/006_220_to_221.lua new file mode 100644 index 00000000000..c60af13629e --- /dev/null +++ b/kong/plugins/oauth2/migrations/006_220_to_221.lua @@ -0,0 +1,18 @@ +return { + postgres = { + up = [[ + DO $$ + BEGIN + ALTER TABLE IF EXISTS ONLY "oauth2_authorization_codes" ADD "plugin_identifier" TEXT; + EXCEPTION WHEN DUPLICATE_COLUMN THEN + -- Do nothing, accept existing state + END$$; + ]], + }, + + cassandra = { + up = [[ + ALTER TABLE oauth2_authorization_codes ADD plugin_identifier text; + ]], + }, +} diff --git a/kong/plugins/oauth2/migrations/init.lua b/kong/plugins/oauth2/migrations/init.lua index 287a19e9c5e..b965a90a74e 100644 --- a/kong/plugins/oauth2/migrations/init.lua +++ b/kong/plugins/oauth2/migrations/init.lua @@ -3,4 +3,5 @@ return { "003_130_to_140", "004_200_to_210", "005_210_to_211", + "006_220_to_221", } diff --git a/spec/03-plugins/25-oauth2/03-access_spec.lua b/spec/03-plugins/25-oauth2/03-access_spec.lua index c8095b93ab8..97df63b0050 100644 --- 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,30 @@ 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) end) describe("Making a request", function()