fix(oauth2): prevent an authorization code created by one plugin instance to be exchanged for an access token by a different plugin instance #10011

vm-001 · 2022-12-28T04:04:13Z

Summary

Forbid an authorization code can be exchanged to access_token in different plugin instance.

Checklist

The Pull Request has tests
There's an entry in the CHANGELOG
There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Issue reference

FTI-3983

vm-001 · 2022-12-28T04:07:33Z

Related PR: #9312

vm-001 · 2022-12-28T04:10:25Z

spec/03-plugins/25-oauth2/03-access_spec.lua

@@ -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")


Fix the test to use the same oauth2 plugin instance

hanshuebner

Some grammar and spelling issues to be fixed.

kong/plugins/oauth2/access.lua

CHANGELOG.md

kong/plugins/oauth2/access.lua

requested changes were made

kong/plugins/oauth2/access.lua

hbagdi · 2023-04-17T21:55:10Z

This is most certainly a breaking change since it is very much possible that a user could be relying on such buggy behavior.
Please do the due diligence on the impact of it and make the decision, @kikito.
And if we decide to proceed, please call it out in changelog to alert the user. If they find this in production, they will be quite sour (for good reasons).

vm-001 · 2023-04-23T02:48:16Z

@hbagdi @kikito I can add an entry to the "breaking change" section to warning this behavior change. Could this solve your concern?

StarlightIbuki · 2023-04-25T02:55:20Z

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_id = { type = "string", required = false } },


Should we make this a foreign key?

Makes sense, and sounds good to me.

StarlightIbuki · 2023-04-25T05:03:09Z

kong/plugins/oauth2/access.lua

@@ -643,6 +644,15 @@ local function issue_token(conf)
          end
        end

+        if not response_params[ERROR] and conf.global_credentials then
+          if kong.plugin.get_id() ~= auth_code.plugin_id then


How do we handle entries added before the migration?

Yeah, that would be a problem. I updated it to become only validate if auth_code.plugin_id is present to make the old code pass the validation.

hbagdi · 2023-04-25T19:58:30Z

Discussed this today with @kikito. He will circle back on this one.

hanshuebner · 2023-05-03T14:16:18Z

@hbagdi I agree with @vm-001 that the current behavior is buggy. I cannot imagine a customer relying on the current behavior. As a reminder, here is a depiction of the OAuth2 authorization flow:

The user first creates an authorization grant (authentication code) using the service of the OAuth2 identity provider. It then presents the authentication code to the authorization server, which exchanges it for an access token. That token can then be used to authenticate against the resource server.

I believe that we can assume that our users use the OAuth2 plugin in the "normal way": A user hits a service that is protected by OAuth2 and that redirects them to the IdP. The IdP issues the authentication code and redirects the user back to the same service, in which Kong plays the authentication server role. Kong exchanges the authentication code for an access token and returns it to the client for further interaction with the resource server.

I cannot imagine a situation in which a user would willingly want to be able to have Kong exchange the authentication code issued when trying to access one service by another service. It seems to me that this would only happen if there is an error in the application (i.e. the OAuth2 related responses are not properly interpreted, making the token request go to a different service) or by malicious intent (i.e. an attacker has access to a less-privileged service and uses the bug to exchange an authentication code by that less-privileged service to a token issued by a more-privileged service).

I am not confident that the fix in this PR will solve all such problems, but I'm certain that we don't want to leave such gaps open if we know about them.

The PR does not currently pass checks because of an incompatibility with Cassandra. This will need to be fixed. I also have the question whether config.global_credentials does not imply that Kong treats access tokens from one plugin to be equivalent to access tokens from other plugins. If that is the case, then how does this change actually improve the situation? Would we not need to say that if config.global_credentials is true, all plugins that share credentials need to be configured identically anyway?

…ance to be exchanged for an access token by a different plugin instance

vm-001 · 2023-05-08T07:46:45Z

I also have the question whether config.global_credentials does not imply that Kong treats access tokens from one plugin to be equivalent to access tokens from other plugins. If that is the case, then how does this change actually improve the situation? Would we not need to say that if config.global_credentials is true, all plugins that share credentials need to be configured identically anyway?

I think the config.global_credentials is a setting indicating whether allow an oauth2 access token(not code) to across accessing different services when they both have set global_credentials to true. I'm not seeing Kong has any test cases against that if it was considered as a feature

Questions have been addressed

…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 (cherry picked from commit 8c4e5c6)

pull-request-size bot added the size/M label Dec 28, 2022

vm-001 requested a review from bungle December 28, 2022 04:04

github-actions bot added changelog plugins/oauth2 labels Dec 28, 2022

vm-001 had a problem deploying to release December 28, 2022 04:04 — with GitHub Actions Failure

vm-001 force-pushed the fix/oauth2-code-exchange-across-plugin-instances branch from 03e33cf to 8af6631 Compare December 28, 2022 04:05

vm-001 had a problem deploying to release December 28, 2022 04:05 — with GitHub Actions Failure

vm-001 marked this pull request as ready for review December 28, 2022 04:05

vm-001 force-pushed the fix/oauth2-code-exchange-across-plugin-instances branch from 8af6631 to f4d82a7 Compare December 28, 2022 04:08

vm-001 requested a review from fffonion December 28, 2022 04:09

vm-001 added this to the 3.2 milestone Dec 28, 2022

vm-001 had a problem deploying to release December 28, 2022 04:09 — with GitHub Actions Failure

vm-001 commented Dec 28, 2022

View reviewed changes

vm-001 force-pushed the fix/oauth2-code-exchange-across-plugin-instances branch from f4d82a7 to e531f38 Compare December 28, 2022 05:25

vm-001 had a problem deploying to release December 28, 2022 05:25 — with GitHub Actions Failure

vm-001 added the pr/please review label Dec 30, 2022

hanshuebner previously requested changes Jan 3, 2023

View reviewed changes

kong/plugins/oauth2/access.lua Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

vm-001 force-pushed the fix/oauth2-code-exchange-across-plugin-instances branch from d4708e5 to c7cd10e Compare January 4, 2023 02:37

vm-001 changed the title ~~fix(oauth2): forbid an authorization code can be exchanged to access_token in different plugin instance~~ fix(oauth2): prevent an authorization code created by one plugin instance to be exchanged for an access token by a different plugin instance Jan 4, 2023

flrgh requested a review from hanshuebner February 1, 2023 20:03

flrgh reviewed Feb 1, 2023

View reviewed changes