feat(authz-keycloak): dynamic scope and resource mapping. #3308

jenskeiner · 2021-01-15T11:44:20Z

What this PR does / why we need it:

Add code to authz-keycloak plugin to optionally resolve requests URI to resource(s) and map HTTP method to scope; fixes #3274.

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

Open this PR to get some feedback on the changes. Still need to add tests to cover the new functionality. May still need to polish the JSON schema slightly as well. However, already tested changes in a real deployment to confirm this is actually working.

Also, strictly speaking, this plugin is not backwards compatible. E.g. we now require static permissions to be set when not using the new dynamic resource resolution. We also require client_id to be set because it will be used in any sensible configuration scenario. This mostly affects corner cases, so I don't think it represents a real problem.

In any case, will likely polish everything a bit more, but would appreciate any feedback already.

@sshniro You should definitely have a look.

…s even when not using TLS for the server itself.

…e-new access token.

… them.

jenskeiner · 2021-01-15T11:46:26Z

apisix/cli/ngx_tpl.lua

@@ -371,16 +374,16 @@ http {
        {% end %}
        {% end %} {% -- if enable_ipv6 %}

+        {% if ssl.ssl_trusted_certificate ~= nil then %}


Moved this here since it was previously only used when APISIX itself was accepting HTTPS from the outside. But even if TLS is disabled, internally, plugins that send requests may still need to be able to use TLS and may have custom CA certs configured.

jenskeiner · 2021-01-15T11:47:07Z

apisix/plugins/authz-keycloak.lua

@@ -42,7 +43,6 @@ local schema = {
            enum = {"urn:ietf:params:oauth:grant-type:uma-ticket"},
            minLength = 1, maxLength = 100
        },
-        audience = {type = "string", minLength = 1, maxLength = 100},


This is now called client_id which is more consistent with the opendid-connect plugin as well.

Maybe we can keep the old field and add a deprecated check?

apisix/apisix/init.lua

Lines 463 to 470 in bbbdf58

if upstream.value.enable_websocket then

core.log.warn("DEPRECATE: enable websocket in upstream will be removed soon. ",

"Please enable it in route/service level.")

enable_websocket = true

end

if upstream.value.pass_host then

api_ctx.pass_host = upstream.value.pass_host

@spacewander Possible, it's just a name. So we keep client_id as well and prefer it, if given?

Yes. Maybe we can add a description on them:

apisix/apisix/schema_def.lua

Line 75 in bbbdf58

description = "client IP",

Ok, will change accordingly. Will also add/fix more test cases soon.

spacewander · 2021-01-21T01:46:36Z

doc/plugins/authz-keycloak.md

+| policy_enforcement_mode        | string        | optional    | "ENFORCING"                                   | ["ENFORCING", "PERMISSIVE"]                                        |                                                                                                                                                             |
+| permissions                    | array[string] | optional    |                                               |                                                                    | Static permission to request, an array of strings each representing a resources and optionally one or more scopes the client is seeking access.             |
+| lazy_load_paths                | boolean       | optional    | false                                         |                                                                    | Dynamically resolve the request URI to resource(s) using the resource registration endpoint instead of using the static permission.                         |
+| http_method_as_scope           | boolean       | optional    | false                                         |                                                                    | Map HTTP request type to scope of same name and add to all permissions requested.                                                                           |


Missing doc for cache_ttl_seconds?

Correct. Will add.

spacewander · 2021-01-21T01:54:23Z

apisix/plugins/authz-keycloak.lua

@@ -171,13 +222,13 @@ local function authz_keycloak_discover(url, ssl_verify, keepalive, timeout,
      keepalive = (keepalive ~= "no")


It seems a bug in the previous code: the keepalive and ssl_verify are boolean.

My bad, was copied over from openidc module. Fixed it and consolidated the HTTP client setup -- which is done in various places -- to apply options like timeout, keepalive, ssl_verifyand others consistently.

spacewander · 2021-01-21T02:03:39Z

doc/plugins/authz-keycloak.md

+| token_endpoint                 | string        | optional    |                                               | https://host.domain/auth/realms/foo/protocol/openid-connect/token  | A OAuth2-compliant Token Endpoint that supports the `urn:ietf:params:oauth:grant-type:uma-ticket` grant type. Overrides value from discovery, if given.     |
+| resource_registration_endpoint | string        | optional    |                                               | https://host.domain/auth/realms/foo/authz/protection/resource_set  | A Keycloak Protection API-compliant resource registration endpoint. Overrides value from discovery, if given.                                               |
+| grant_type                     | string        | optional    | "urn:ietf:params:oauth:grant-type:uma-ticket" | ["urn:ietf:params:oauth:grant-type:uma-ticket"]                    |                                                                                                                                                             |
+| client_id                      | string        | required    |                                               |                                                                    | The client identifier of the resource server to which the client is seeking access. <br>This parameter is mandatory when parameter permission is defined.   |


This parameter is mandatory when parameter permission is defined.

Look like it should be when parameter lazy_load_paths is defined? Should require it in the schema and add a test for it.

Ok, so I'm now again requiring one of client_id or audience, the latter for better backwards compatibility. Rationale: The plugin always uses the token endpoint to get a decision from Keycloak on the requested permissions. As per https://www.keycloak.org/docs/latest/authorization_services/#_service_obtaining_permissions the audience parameter (what is either in client_id or audience for the plugin) is required when the permission parameter is set (which the plugin always does).

If lazy_load_paths is true, the plugin also needs to obtain an access token for itself from Keycloak. To that end, it most likely also needs client_secret to be set, but that depends on how Keycloak is configured. Hence, this is still optional.

Also added test cases to check if one of client_id or audience is correctly required and schema verification fails if both are absent.

spacewander · 2021-01-21T02:04:48Z

apisix/plugins/authz-keycloak.lua

+    end
+
+    local session = authz_keycloak_cache_get("access_tokens", token_endpoint .. ":"
+                                             .. client_id)


The client_id could be nil if you don't force when lazy_load_paths is used.

See above, we're now requiring client_id (or audience).

spacewander · 2021-01-21T02:43:12Z

t/plugin/authz-keycloak.t

@@ -137,7 +224,7 @@ done
                            "authz-keycloak": {
                                "token_endpoint": "http://127.0.0.1:8090/auth/realms/University/protocol/openid-connect/token",
                                "permissions": ["course_resource#view"],
-                                "audience": "course_management",


Maybe we can leave audience unchanged in one or two cases so that we know the old configuration is still compatible?

Sure, I can add some cases that use it.

spacewander · 2021-01-21T02:45:41Z

BTW, you can split the test into two files, like the https://github.com/apache/apisix/blob/master/t/plugin/limit-conn2.t. Since it is too big now.

jenskeiner · 2021-01-21T12:50:39Z

BTW, you can split the test into two files, like the https://github.com/apache/apisix/blob/master/t/plugin/limit-conn2.t. Since it is too big now.

done

jenskeiner · 2021-01-21T14:37:09Z

@spacewander I think I've addressed all of the feedback so far. Thanks for reviewing. Let me know any further issues.

spacewander · 2021-01-24T12:17:13Z

apisix/plugins/authz-keycloak.lua

+-- Return access_token expires_in value (in seconds).
+local function authz_keycloak_access_token_expires_in(conf, expires_in)
+    return (expires_in or conf.access_token_expires_in or 300)
+           - 1 - (conf.access_token_expires_leeway or 0)


Will you add this configurations in this PR, or in the future one?

spacewander · 2021-01-24T12:18:57Z

apisix/plugins/authz-keycloak.lua

+-- Return refresh_token expires_in value (in seconds).
+local function authz_keycloak_refresh_token_expires_in(conf, expires_in)
+    return (expires_in or conf.refresh_token_expires_in or 3600)
+           - 1 - (conf.refresh_token_expires_leeway or 0)


spacewander

We can merge this PR first and solve the remain in the next one.

jenskeiner · 2021-01-24T16:19:59Z

@spacewander Thanks, I will surely continue the work on this plugin and I can add the config options in a future PR.

membphis · 2021-01-26T15:32:37Z

I have been busy recently, I will review this PR later.

Welcome everyone to review. many thx

jenskeiner added 26 commits January 11, 2021 16:42

Add our own mod of authz-keycloak plugin.

b0b6165

Fix session id parameter name.

fd96cb6

Adjust Nginx config template to allow setting trusted TLS certificate…

8aedd89

…s even when not using TLS for the server itself.

Fix plugin name.

4336446

Debugging.

2d90fb4

Query matching resources from server.

1c09d30

Continue build out.

6e36faf

More build out.

fc47492

Add UMA discovery.

51be98c

Remove audience parameter in favour of client_id.

d16dd16

Add request decorator.

0deede0

Make token endpoint optional.

67d4823

Small fixes.

5ddfe3d

Add debug output.

c032bfb

Polishing.

1ca808d

Merge branch 'master_upstream' into dynamic-scope-and-resource-mapping

2515ad5

Add service account access token retrieval.

1d58f48

Smaller fixes.

b5c6626

Add complete session management, including use of refresh tokens to r…

6d7749f

…e-new access token.

Add lazy_load_paths and http_method_as_scope parameters and implement…

a73a702

… them.

Several fixes.

b9e9c80

Several fixes.

65d491f

Polishing.

be6cde9

Return Keycloak-style message when unable to resolve permission.

fb4e0ad

Update documentation.

6a9f12c

Remove temporary plugin version.

1ec673c

jenskeiner commented Jan 15, 2021

View reviewed changes

jenskeiner added 2 commits January 15, 2021 12:51

Breake some long lines.

6bcf69e

Break some long lines and general polishing.

be03eeb

spacewander reviewed Jan 21, 2021

View reviewed changes

jenskeiner added 12 commits January 21, 2021 09:19

Add documentation for cache_ttl_seconds attribute.

ab55569

Fix incorrect usage of boolean value.

59dccc9

Temporarily disable some unit tests to speed up checks.

5ca93a5

Cleanup documentation, JSON schema, and HTTP handling.

450ce05

Fix syntax error.

e08224b

Merge branch 'master_upstream' into dynamic-scope-and-resource-mapping

e97d00d

Fix syntax error.

f2eabee

Cleanup.

ce2eb72

Fix syntax error.

caffeee

Fix test case.

1f658d2

Fix stray conf.http_request_decorator.

67a7fe7

Split test into two files.

977759f

jenskeiner added 2 commits January 21, 2021 13:52

Re-enable all tests.

c4c4449

Fix test case numbering scheme.

7a8460c

jenskeiner requested a review from spacewander January 21, 2021 14:36

spacewander reviewed Jan 24, 2021

View reviewed changes

spacewander approved these changes Jan 24, 2021

View reviewed changes

juzhiyuan requested a review from membphis January 25, 2021 01:13

moonming approved these changes Jan 27, 2021

View reviewed changes

spacewander changed the title ~~feat: authz-keycloak plugin - dynamic scope and resource mapping.~~ feat(authz-keycloak): dynamic scope and resource mapping. Jan 27, 2021

spacewander merged commit b5374bb into apache:master Jan 27, 2021

jenskeiner deleted the dynamic-scope-and-resource-mapping branch January 29, 2021 08:57

spacewander mentioned this pull request Feb 2, 2021

authz-keycloak: support configurating token refresh/expire time #3479

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(authz-keycloak): dynamic scope and resource mapping. #3308

feat(authz-keycloak): dynamic scope and resource mapping. #3308

jenskeiner commented Jan 15, 2021 •

edited

Loading

jenskeiner Jan 15, 2021

jenskeiner Jan 15, 2021

spacewander Jan 15, 2021

jenskeiner Jan 15, 2021

spacewander Jan 15, 2021

jenskeiner Jan 15, 2021

jenskeiner Jan 18, 2021

spacewander Jan 21, 2021

jenskeiner Jan 21, 2021

jenskeiner Jan 21, 2021

spacewander Jan 21, 2021

jenskeiner Jan 21, 2021

spacewander Jan 21, 2021

jenskeiner Jan 21, 2021 •

edited

Loading

spacewander Jan 21, 2021

spacewander Jan 21, 2021

jenskeiner Jan 21, 2021

spacewander Jan 21, 2021

jenskeiner Jan 21, 2021

spacewander commented Jan 21, 2021

jenskeiner commented Jan 21, 2021

jenskeiner commented Jan 21, 2021

spacewander Jan 24, 2021

spacewander Jan 24, 2021

spacewander left a comment

jenskeiner commented Jan 24, 2021

membphis commented Jan 26, 2021

	if upstream.value.enable_websocket then
	core.log.warn("DEPRECATE: enable websocket in upstream will be removed soon. ",
	"Please enable it in route/service level.")
	enable_websocket = true
	end

	if upstream.value.pass_host then
	api_ctx.pass_host = upstream.value.pass_host

		@@ -171,13 +222,13 @@ local function authz_keycloak_discover(url, ssl_verify, keepalive, timeout,
		keepalive = (keepalive ~= "no")

feat(authz-keycloak): dynamic scope and resource mapping. #3308

feat(authz-keycloak): dynamic scope and resource mapping. #3308

Conversation

jenskeiner commented Jan 15, 2021 • edited Loading

What this PR does / why we need it:

Pre-submission checklist:

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

Choose a reason for hiding this comment

jenskeiner Jan 21, 2021 • edited Loading

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

spacewander commented Jan 21, 2021

jenskeiner commented Jan 21, 2021

jenskeiner commented Jan 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spacewander left a comment

Choose a reason for hiding this comment

jenskeiner commented Jan 24, 2021

membphis commented Jan 26, 2021

jenskeiner commented Jan 15, 2021 •

edited

Loading

jenskeiner Jan 21, 2021 •

edited

Loading