fix: authz-keycloak plugin should not be marked as auth-type and should run in access phase, basic-auth plugin should run in rewrite phase. #2859

jenskeiner · 2020-11-26T11:30:06Z

What this PR does / why we need it:

See #2857.

Pre-submission checklist:

Did you explain what problem does this PR solve? Or what new features have been added?
Have you added corresponding test cases?
Have you modified the corresponding document?
Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

…ess phase. (apache#2857)

membphis · 2020-11-26T11:41:13Z

Authentication type plug-ins should run in the rewrite phase

jenskeiner · 2020-11-26T12:07:07Z

Authentication type plug-ins should run in the rewrite phase

How do you suggest this get's fixed then when openid-connect and other authentication plugins currently run in the access phase?

The plugin development guidelines are currently not in line with the actual code base.

Also, what's the rationale for running authentication/authorization in the rewrite phase when the access phase is the one that specifically caters for that purpose? It's not obvious why you would run in a different phase.

membphis · 2020-11-26T13:55:54Z

reply you later

need more time to read your issue and pr first

spacewander · 2020-11-27T01:22:21Z

@membphis @jenskeiner
authz-keycloak is for authorization but not authentication. It doesn't interact with consumers, so I think this change is fine.

spacewander · 2020-11-27T01:23:09Z

doc/plugin-develop.md

@@ -168,8 +168,8 @@ local _M = {
 Determine which phase to run, generally access or rewrite. If you don't know the [Openresty life cycle](https://openresty-reference.readthedocs.io/en/latest/Directives/), it's
 recommended to know it in advance. For example key-auth is an authentication plugin, thus the authentication should be completed
 before forwarding the request to any upstream service. Therefore, the plugin can be executed in the rewrite and access phases.
-In APISIX, the authentication logic is implemented in the rewrite phase. Generally, IP access and interface
-permission are completed in the access phase.
+In APISIX, the authentication logic is implemented in the access phase. Generally, IP access and interface


We don't need to change it as authz-keycloak is for authorization.

Ok, I can revert this one. However, note that some authentication plugins currently run in the access phase, not the rewrite one. So the document doesn’t reflect the current state correctly, I believe.

@jenskeiner
We can fix it later, but in those plugins, not the documentation.

@jenskeiner
Thank you for the problem you point out. I just search for all plugins which has type = 'auth' (excluding the authz-keycloak) and find out basic-auth doesn't run the phase correctly.

The reason why we need to run authentication plugins in the rewrite phases:

apisix/apisix/init.lua

Lines 508 to 520 in f4161d3

run_plugin("rewrite", plugins, api_ctx)

if api_ctx.consumer then

local changed

route, changed = plugin.merge_consumer_route(

route,

api_ctx.consumer,

api_ctx

)

if changed then

core.table.clear(api_ctx.plugins)

api_ctx.plugins = plugin.filter(route, api_ctx.plugins)

end

end

And:

apisix/apisix/admin/consumers.lua

Line 50 in f4161d3

if plugin_obj.type == 'auth' then

Plugins has type = 'auth' should run in rewrite phase and set up the consumer for current api_ctx.

Ok, understood why you need to run in the rewrite phase. Then I would propose to change the phase for all authentication-type plugins to rewrite, instead of adjusting the phase for authz-keycloak from rewrite to access. Also, I think it would make the plugin development guidelines clearer if the reason they need to run in the rewrite phase was pointed out more clearly.

If you're fine with this, I can update this PR accordingly. Just let me know what you think.

We don't need to change it as authz-keycloak is for authorization.

Reverted.

membphis · 2020-11-27T01:38:12Z

@membphis @jenskeiner
authz-keycloak is for authorization but not authentication. It doesn't interact with consumers, so I think this change is fine.

many thx for your explain. @spacewander
we can continue this PR

… authz-keycloak as authentication plugin anymore, since it's only concerned with authorization.

spacewander · 2020-11-28T01:09:42Z

apisix/plugins/basic-auth.lua

@@ -125,7 +125,7 @@ do
    end
 end

-function _M.access(conf, ctx)
+function _M.rewrite(conf, ctx)


Need a test case to check if the ctx.consumer set by this plugin can be used by other plugin runs in access phase.

Happy to add a test case. I don't have much experience writing them yet, so if you could point to a similar example, that would be helpful.

You can add a stub:

diff --git apisix/init.lua apisix/init.lua index 7333dd1..411cdd4 100644 --- apisix/init.lua +++ apisix/init.lua @@ -514,6 +514,10 @@ function _M.http_access_phase() api_ctx.consumer, api_ctx ) + + core.log.info("find consumer ", api_ctx.consumer.username, + ", config changed: ", changed) + if changed then core.table.clear(api_ctx.plugins) api_ctx.plugins = plugin.filter(route, api_ctx.plugins)

And then check it in:

apisix/t/plugin/basic-auth.t

Line 192 in e66dd9a

=== TEST 8: verify

Add something like:

--- error_log find consumer foo

Ok, I think I've seen an example in the test cases for key-auth. I'll try to add test cases acceding for basic-auth.

spacewander · 2020-11-28T01:11:38Z

doc/plugin-develop.md

@@ -168,8 +168,8 @@ local _M = {
 Determine which phase to run, generally access or rewrite. If you don't know the [Openresty life cycle](https://openresty-reference.readthedocs.io/en/latest/Directives/), it's
 recommended to know it in advance. For example key-auth is an authentication plugin, thus the authentication should be completed
 before forwarding the request to any upstream service. Therefore, the plugin can be executed in the rewrite and access phases.
-In APISIX, the authentication logic is implemented in the rewrite phase. Generally, IP access and interface
-permission are completed in the access phase.
+In APISIX, the authentication logic is implemented in the access phase. Generally, IP access and interface


Remember to revert this change.

…un in the rewrite phase.

membphis · 2020-11-30T11:11:26Z

apisix/plugins/basic-auth.lua

@@ -125,7 +125,7 @@ do
    end
 end

-function _M.access(conf, ctx)
+function _M.rewrite(conf, ctx)


why do we need to change another plugin?

one PR for one thing, we should only update something about plugin authz-keycloak

please revert this code

membphis · 2020-11-30T11:11:48Z

apisix/plugins/openid-connect.lua

@@ -49,6 +49,7 @@ local schema = {
 local _M = {
    version = 0.1,
    priority = 2599,
+    type = 'auth',


ditto

you can create a new PR to fix this

I'm sorry but I currently can't afford the time to split this into separate PRs. I think the change set is reasonably small to be considered in one PR. If you prefer more fine-grained PRs, then please consider creating these yourself, e.g. based on the changes in this branch.

I found you have submitted new PR: https://github.com/apache/apisix/pull/2905/files

Many thanks for your nice job.

Small PR is important, it easy to find problems and we can merge it asap when it is small.

By the way, it is a clearer way to show what we are doing.

Many thx for nice contribution again @jenskeiner

Sure, you're very welcome. Thanks for the info. I'll try to put the remaining outstanding changes from this one into a separate new PR and then we take it from there.

Have created #2910 which should contain the outstanding changes from this PR.

jenskeiner · 2020-12-01T08:59:29Z

Closing this one as it has since been superseded by other PRs addressing the same issues.

jenskeiner added 2 commits November 26, 2020 12:27

fix: Make plugin run in access phase. (apache#2857)

cc3f2c0

docs: Reflect that authentication/authorization should be done in acc…

3ad3a20

…ess phase. (apache#2857)

spacewander reviewed Nov 27, 2020

View reviewed changes

Make all authentication plugins run in rewrite phase. Don't designate…

aea636e

… authz-keycloak as authentication plugin anymore, since it's only concerned with authorization.

spacewander reviewed Nov 28, 2020

View reviewed changes

Revert previous change to restore that authentication plugins should …

fa061ee

…un in the rewrite phase.

membphis requested changes Nov 30, 2020

View reviewed changes

jenskeiner added 5 commits November 30, 2020 14:11

Add debug code to log consumer.

1ee7cd4

Add missing config for test case.

cc46bc0

Wrap line.

e88f887

Change log level.

ac84f84

Use debug log level again.

cccaf75

jenskeiner changed the title ~~fix: authz keycloak should run in access phase~~ fix: authz-keycloak plugin should not be marked as auth-type and should run in access phase, basic-auth plugin should run in rewrite phase. Nov 30, 2020

jenskeiner mentioned this pull request Nov 30, 2020

fix: Basic auth plugin should run in rewrite phase. #2905

Merged

4 tasks

jenskeiner closed this Dec 1, 2020

jenskeiner deleted the 2857-authz-keycloak-should-run-in-access-phase branch December 1, 2020 08:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: authz-keycloak plugin should not be marked as auth-type and should run in access phase, basic-auth plugin should run in rewrite phase. #2859

fix: authz-keycloak plugin should not be marked as auth-type and should run in access phase, basic-auth plugin should run in rewrite phase. #2859

jenskeiner commented Nov 26, 2020

membphis commented Nov 26, 2020

jenskeiner commented Nov 26, 2020

membphis commented Nov 26, 2020

spacewander commented Nov 27, 2020

spacewander Nov 27, 2020

jenskeiner Nov 27, 2020

spacewander Nov 27, 2020

spacewander Nov 27, 2020

spacewander Nov 27, 2020

jenskeiner Nov 27, 2020

jenskeiner Nov 30, 2020

membphis commented Nov 27, 2020

spacewander Nov 28, 2020

jenskeiner Nov 30, 2020

spacewander Nov 30, 2020

jenskeiner Nov 30, 2020

spacewander Nov 28, 2020

jenskeiner Nov 30, 2020

membphis Nov 30, 2020

membphis Nov 30, 2020

jenskeiner Nov 30, 2020

membphis Dec 1, 2020

jenskeiner Dec 1, 2020

jenskeiner Dec 1, 2020

jenskeiner commented Dec 1, 2020

	run_plugin("rewrite", plugins, api_ctx)
	if api_ctx.consumer then
	local changed
	route, changed = plugin.merge_consumer_route(
	route,
	api_ctx.consumer,
	api_ctx
	)
	if changed then
	core.table.clear(api_ctx.plugins)
	api_ctx.plugins = plugin.filter(route, api_ctx.plugins)
	end
	end

fix: authz-keycloak plugin should not be marked as auth-type and should run in access phase, basic-auth plugin should run in rewrite phase. #2859

fix: authz-keycloak plugin should not be marked as auth-type and should run in access phase, basic-auth plugin should run in rewrite phase. #2859

Conversation

jenskeiner commented Nov 26, 2020

What this PR does / why we need it:

Pre-submission checklist:

membphis commented Nov 26, 2020

jenskeiner commented Nov 26, 2020

membphis commented Nov 26, 2020

spacewander commented Nov 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

membphis commented Nov 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenskeiner commented Dec 1, 2020