You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2021/01/12 21:54:23 UTC
[GitHub] [apisix] jenskeiner opened a new pull request #3263: Uma discovery
jenskeiner opened a new pull request #3263:
URL: https://github.com/apache/apisix/pull/3263
### What this PR does / why we need it:
The `authz-keycloak` plugin needs to know the Keycloak token endpoint to use. A configuration option to point to the token endpoint is already there.
Additionally, Keycloak supports discovery of Authorization Services-related endpoints, like the token endpoint, through a discovery document served under a well-known path relative to the realm, e.g. `https://keycloak-host/auth/realms/foo/.well-known/uma2-configuration`.
It can be convenient to use the discovery document to get the token endpoint URL, instead of specifying the token endpoint explicitly, since the URL is typically shorter. More importantly, future build out of the `authz-keycloak` may require access to additional related endpoints. In this case, just providing the single path to the discovery document would be more convenient than specifying each endpoint URL explicitly.
This PR adds an attribute that allows to specify the discovery document URL and makes the token endpoint attribute optional. However, it is checked that at least one, discovery or token endpoint, is given.
The plugin now fetches the discovery document lazily and caches it in shared storage already used by the `openid-connect` plugin (better: the `openidc` module used therein) for similar purposes in the context of OIDC endpoint discovery.
If both, discovery and token endpoint are given, the plugin prefers the explicit token endpoint value over the one from discovery.
### Pre-submission checklist:
* [x] Did you explain what problem does this PR solve? Or what new features have been added?
* [x] Have you added corresponding test cases?
* [x] Have you modified the corresponding document?
* [x] Is this PR backward compatible? **If it is not backward compatible, please discuss on the [mailing list](https://github.com/apache/apisix/tree/master#community) first**
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] jenskeiner commented on a change in pull request #3263: feat: authz-keycloak plugin - Add Keycloak Authorization Services endpoint discovery.
Posted by GitBox <gi...@apache.org>.
jenskeiner commented on a change in pull request #3263:
URL: https://github.com/apache/apisix/pull/3263#discussion_r556539817
##########
File path: apisix/plugins/authz-keycloak.lua
##########
@@ -64,10 +67,169 @@ local _M = {
schema = schema,
}
+
function _M.check_schema(conf)
return core.schema.check(schema, conf)
end
+
+-- Some auxiliary functions below heavily inspired by the excellent
+-- lua-resty-openidc module; see https://github.com/zmartzone/lua-resty-openidc
+
+
+-- Retrieve value from server-wide cache, if available.
+local function authz_keycloak_cache_get(type, key)
+ local dict = ngx.shared[type]
+ local value
+ if dict then
+ value = dict:get(key)
+ if value then log.debug("cache hit: type=", type, " key=", key) end
+ end
+ return value
+end
+
+
+-- Set value in server-wide cache, if available.
+local function authz_keycloak_cache_set(type, key, value, exp)
+ local dict = ngx.shared[type]
+ if dict and (exp > 0) then
+ local success, err, forcible = dict:set(key, value, exp)
+ if err then
+ log.error("cache set: success=", success, " err=", err, " forcible=", forcible)
+ else
+ log.debug("cache set: success=", success, " err=", err, " forcible=", forcible)
+ end
+ end
+end
+
+
+-- Configure timeouts.
+local function authz_keycloak_configure_timeouts(httpc, timeout)
+ if timeout then
+ if type(timeout) == "table" then
+ httpc:set_timeouts(timeout.connect or 0, timeout.send or 0, timeout.read or 0)
+ else
+ httpc:set_timeout(timeout)
+ end
+ end
+end
+
+
+-- Set outgoing proxy options.
+local function authz_keycloak_configure_proxy(httpc, proxy_opts)
+ if httpc and proxy_opts and type(proxy_opts) == "table" then
+ log.debug("authz_keycloak_configure_proxy : use http proxy")
+ httpc:set_proxy_options(proxy_opts)
+ else
+ log.debug("authz_keycloak_configure_proxy : don't use http proxy")
+ end
+end
+
+
+-- Parse the JSON result from a call to the OP.
+local function authz_keycloak_parse_json_response(response, ignore_body_on_success)
Review comment:
I've taken some of these helper functions largely from the `openidc` module; see https://github.com/zmartzone/lua-resty-openidc
Agree that we don't need that parameter here. Will remove it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] jenskeiner commented on a change in pull request #3263: feat: authz-keycloak plugin - Keycloak Authorization Services endpoint discovery
Posted by GitBox <gi...@apache.org>.
jenskeiner commented on a change in pull request #3263:
URL: https://github.com/apache/apisix/pull/3263#discussion_r556412344
##########
File path: apisix/plugins/authz-keycloak.lua
##########
@@ -52,8 +55,7 @@ local schema = {
keepalive_timeout = {type = "integer", minimum = 1000, default = 60000},
keepalive_pool = {type = "integer", minimum = 1, default = 5},
ssl_verify = {type = "boolean", default = true},
- },
- required = {"token_endpoint"}
Review comment:
Hmm, I think we should actually use `anyOf` since the explicit endpoint URL will be preferred if both options are given. The full schema unit test already uses both options so would have to touch that again as well if we make this XOR.
I have future dev work in mind that would require using an additional endpoint. In that case, you may want to use discovery for one endpoint and an explicit value for the other. But, admittedly, that's a rather strange case. In any sane environment, you would always use only discovery or only explicit endpoints, I think.
Let me know your thoughts @spacewander and I will adjust accordingly.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #3263: feat: authz-keycloak plugin - Keycloak Authorization Services endpoint discovery
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #3263:
URL: https://github.com/apache/apisix/pull/3263#discussion_r556199767
##########
File path: apisix/plugins/authz-keycloak.lua
##########
@@ -64,10 +66,164 @@ local _M = {
schema = schema,
}
+
function _M.check_schema(conf)
+ if not conf.discovery and not conf.token_endpoint then
+ return false, 'Neither discovery nor token endpoint given.'
+ end
return core.schema.check(schema, conf)
end
+
+-- Retrieve value from server-wide cache, if available.
+local function authz_keycloak_cache_get(type, key)
+ local dict = ngx.shared[type]
+ local value
+ if dict then
+ value = dict:get(key)
+ if value then log.debug("cache hit: type=", type, " key=", key) end
+ end
+ return value
+end
+
+
+-- Set value in server-wide cache, if available.
+local function authz_keycloak_cache_set(type, key, value, exp)
+ local dict = ngx.shared[type]
+ if dict and (exp > 0) then
+ local success, err, forcible = dict:set(key, value, exp)
+ log.debug("cache set: success=", success, " err=", err, " forcible=", forcible)
+ end
+end
+
+
+-- Configure timeouts.
+local function authz_keycloak_configure_timeouts(httpc, timeout)
+ if timeout then
+ if type(timeout) == "table" then
+ httpc:set_timeouts(timeout.connect or 0, timeout.send or 0, timeout.read or 0)
+ else
+ httpc:set_timeout(timeout)
+ end
+ end
+end
+
+
+-- Set outgoing proxy options.
+local function authz_keycloak_configure_proxy(httpc, proxy_opts)
+ if httpc and proxy_opts and type(proxy_opts) == "table" then
+ log.debug("authz_keycloak_configure_proxy : use http proxy")
+ httpc:set_proxy_options(proxy_opts)
+ else
+ log.debug("authz_keycloak_configure_proxy : don't use http proxy")
+ end
+end
+
+
+-- Parse the JSON result from a call to the OP.
+local function authz_keycloak_parse_json_response(response, ignore_body_on_success)
+ local ignore_body_on_success = ignore_body_on_success or false
+
+ local err
+ local res
+
+ -- Check the response from the OP.
+ if response.status ~= 200 then
+ err = "response indicates failure, status=" .. response.status .. ", body=" .. response.body
+ else
+ if ignore_body_on_success then
+ return nil, nil
+ end
+
+ -- Decode the response and extract the JSON object.
+ res = cjson_s.decode(response.body)
+
+ if not res then
+ err = "JSON decoding failed"
Review comment:
Better to concat the original decode err returned by `decode`
##########
File path: apisix/plugins/authz-keycloak.lua
##########
@@ -64,10 +66,164 @@ local _M = {
schema = schema,
}
+
function _M.check_schema(conf)
+ if not conf.discovery and not conf.token_endpoint then
+ return false, 'Neither discovery nor token endpoint given.'
+ end
return core.schema.check(schema, conf)
end
+
+-- Retrieve value from server-wide cache, if available.
+local function authz_keycloak_cache_get(type, key)
+ local dict = ngx.shared[type]
+ local value
+ if dict then
+ value = dict:get(key)
+ if value then log.debug("cache hit: type=", type, " key=", key) end
+ end
+ return value
+end
+
+
+-- Set value in server-wide cache, if available.
+local function authz_keycloak_cache_set(type, key, value, exp)
+ local dict = ngx.shared[type]
+ if dict and (exp > 0) then
+ local success, err, forcible = dict:set(key, value, exp)
+ log.debug("cache set: success=", success, " err=", err, " forcible=", forcible)
+ end
+end
+
+
+-- Configure timeouts.
+local function authz_keycloak_configure_timeouts(httpc, timeout)
+ if timeout then
+ if type(timeout) == "table" then
+ httpc:set_timeouts(timeout.connect or 0, timeout.send or 0, timeout.read or 0)
+ else
+ httpc:set_timeout(timeout)
+ end
+ end
+end
+
+
+-- Set outgoing proxy options.
+local function authz_keycloak_configure_proxy(httpc, proxy_opts)
+ if httpc and proxy_opts and type(proxy_opts) == "table" then
+ log.debug("authz_keycloak_configure_proxy : use http proxy")
+ httpc:set_proxy_options(proxy_opts)
+ else
+ log.debug("authz_keycloak_configure_proxy : don't use http proxy")
+ end
+end
+
+
+-- Parse the JSON result from a call to the OP.
+local function authz_keycloak_parse_json_response(response, ignore_body_on_success)
+ local ignore_body_on_success = ignore_body_on_success or false
+
+ local err
+ local res
+
+ -- Check the response from the OP.
+ if response.status ~= 200 then
+ err = "response indicates failure, status=" .. response.status .. ", body=" .. response.body
+ else
+ if ignore_body_on_success then
+ return nil, nil
+ end
+
+ -- Decode the response and extract the JSON object.
+ res = cjson_s.decode(response.body)
Review comment:
Better to use `core.json.decode`.
##########
File path: apisix/plugins/authz-keycloak.lua
##########
@@ -52,8 +55,7 @@ local schema = {
keepalive_timeout = {type = "integer", minimum = 1000, default = 60000},
keepalive_pool = {type = "integer", minimum = 1, default = 5},
ssl_verify = {type = "boolean", default = true},
- },
- required = {"token_endpoint"}
Review comment:
We can do it in jsonschema: https://github.com/apache/apisix/blob/3e19b06293463f52c8d26f0958542fc475ff0fcd/apisix/plugins/echo.lua#L48-L51
And need to a test for it.
##########
File path: apisix/plugins/authz-keycloak.lua
##########
@@ -64,10 +66,164 @@ local _M = {
schema = schema,
}
+
function _M.check_schema(conf)
+ if not conf.discovery and not conf.token_endpoint then
+ return false, 'Neither discovery nor token endpoint given.'
+ end
return core.schema.check(schema, conf)
end
+
+-- Retrieve value from server-wide cache, if available.
+local function authz_keycloak_cache_get(type, key)
+ local dict = ngx.shared[type]
+ local value
+ if dict then
+ value = dict:get(key)
+ if value then log.debug("cache hit: type=", type, " key=", key) end
+ end
+ return value
+end
+
+
+-- Set value in server-wide cache, if available.
+local function authz_keycloak_cache_set(type, key, value, exp)
+ local dict = ngx.shared[type]
+ if dict and (exp > 0) then
+ local success, err, forcible = dict:set(key, value, exp)
+ log.debug("cache set: success=", success, " err=", err, " forcible=", forcible)
+ end
+end
+
+
+-- Configure timeouts.
+local function authz_keycloak_configure_timeouts(httpc, timeout)
+ if timeout then
+ if type(timeout) == "table" then
+ httpc:set_timeouts(timeout.connect or 0, timeout.send or 0, timeout.read or 0)
+ else
+ httpc:set_timeout(timeout)
+ end
+ end
+end
+
+
+-- Set outgoing proxy options.
+local function authz_keycloak_configure_proxy(httpc, proxy_opts)
+ if httpc and proxy_opts and type(proxy_opts) == "table" then
+ log.debug("authz_keycloak_configure_proxy : use http proxy")
+ httpc:set_proxy_options(proxy_opts)
+ else
+ log.debug("authz_keycloak_configure_proxy : don't use http proxy")
+ end
+end
+
+
+-- Parse the JSON result from a call to the OP.
+local function authz_keycloak_parse_json_response(response, ignore_body_on_success)
+ local ignore_body_on_success = ignore_body_on_success or false
+
+ local err
+ local res
+
+ -- Check the response from the OP.
+ if response.status ~= 200 then
+ err = "response indicates failure, status=" .. response.status .. ", body=" .. response.body
+ else
+ if ignore_body_on_success then
+ return nil, nil
+ end
+
+ -- Decode the response and extract the JSON object.
+ res = cjson_s.decode(response.body)
+
+ if not res then
+ err = "JSON decoding failed"
+ end
+ end
+
+ return res, err
+end
+
+
+local function decorate_request(http_request_decorator, req)
+ return http_request_decorator and http_request_decorator(req) or req
+end
+
+
+-- Get the Discovery metadata from the specified URL.
+local function authz_keycloak_discover(url, ssl_verify, keepalive, timeout,
+ exptime, proxy_opts, http_request_decorator)
+ log.debug("authz_keycloak_discover: URL is: " .. url)
+
+ local json, err
+ local v = authz_keycloak_cache_get("discovery", url)
+ if not v then
+
+ log.debug("Discovery data not in cache, making call to discovery endpoint.")
+ -- Make the call to the discovery endpoint.
+ local httpc = http.new()
+ authz_keycloak_configure_timeouts(httpc, timeout)
+ authz_keycloak_configure_proxy(httpc, proxy_opts)
+ local res, error = httpc:request_uri(url, decorate_request(http_request_decorator, {
+ ssl_verify = (ssl_verify ~= "no"),
+ keepalive = (keepalive ~= "no")
+ }))
+ if not res then
+ err = "accessing discovery url (" .. url .. ") failed: " .. error
+ log.error(err)
+ else
+ log.debug("response data: " .. res.body)
+ json, err = authz_keycloak_parse_json_response(res)
+ if json then
+ authz_keycloak_cache_set("discovery", url, cjson.encode(json), exptime or 24 * 60 * 60)
+ else
+ err = "could not decode JSON from Discovery data" .. (err and (": " .. err) or '')
+ log.error(err)
+ end
+ end
+
+ else
+ json = cjson.decode(v)
Review comment:
ditto
##########
File path: apisix/plugins/authz-keycloak.lua
##########
@@ -64,10 +66,164 @@ local _M = {
schema = schema,
}
+
function _M.check_schema(conf)
+ if not conf.discovery and not conf.token_endpoint then
+ return false, 'Neither discovery nor token endpoint given.'
+ end
return core.schema.check(schema, conf)
end
+
+-- Retrieve value from server-wide cache, if available.
+local function authz_keycloak_cache_get(type, key)
+ local dict = ngx.shared[type]
+ local value
+ if dict then
+ value = dict:get(key)
+ if value then log.debug("cache hit: type=", type, " key=", key) end
+ end
+ return value
+end
+
+
+-- Set value in server-wide cache, if available.
+local function authz_keycloak_cache_set(type, key, value, exp)
+ local dict = ngx.shared[type]
+ if dict and (exp > 0) then
+ local success, err, forcible = dict:set(key, value, exp)
+ log.debug("cache set: success=", success, " err=", err, " forcible=", forcible)
+ end
+end
+
+
+-- Configure timeouts.
+local function authz_keycloak_configure_timeouts(httpc, timeout)
+ if timeout then
+ if type(timeout) == "table" then
+ httpc:set_timeouts(timeout.connect or 0, timeout.send or 0, timeout.read or 0)
+ else
+ httpc:set_timeout(timeout)
+ end
+ end
+end
+
+
+-- Set outgoing proxy options.
+local function authz_keycloak_configure_proxy(httpc, proxy_opts)
+ if httpc and proxy_opts and type(proxy_opts) == "table" then
+ log.debug("authz_keycloak_configure_proxy : use http proxy")
+ httpc:set_proxy_options(proxy_opts)
+ else
+ log.debug("authz_keycloak_configure_proxy : don't use http proxy")
+ end
+end
+
+
+-- Parse the JSON result from a call to the OP.
+local function authz_keycloak_parse_json_response(response, ignore_body_on_success)
+ local ignore_body_on_success = ignore_body_on_success or false
+
+ local err
+ local res
+
+ -- Check the response from the OP.
+ if response.status ~= 200 then
+ err = "response indicates failure, status=" .. response.status .. ", body=" .. response.body
+ else
+ if ignore_body_on_success then
+ return nil, nil
+ end
+
+ -- Decode the response and extract the JSON object.
+ res = cjson_s.decode(response.body)
+
+ if not res then
+ err = "JSON decoding failed"
+ end
+ end
+
+ return res, err
+end
+
+
+local function decorate_request(http_request_decorator, req)
+ return http_request_decorator and http_request_decorator(req) or req
+end
+
+
+-- Get the Discovery metadata from the specified URL.
+local function authz_keycloak_discover(url, ssl_verify, keepalive, timeout,
+ exptime, proxy_opts, http_request_decorator)
+ log.debug("authz_keycloak_discover: URL is: " .. url)
+
+ local json, err
+ local v = authz_keycloak_cache_get("discovery", url)
+ if not v then
+
+ log.debug("Discovery data not in cache, making call to discovery endpoint.")
+ -- Make the call to the discovery endpoint.
+ local httpc = http.new()
+ authz_keycloak_configure_timeouts(httpc, timeout)
+ authz_keycloak_configure_proxy(httpc, proxy_opts)
+ local res, error = httpc:request_uri(url, decorate_request(http_request_decorator, {
+ ssl_verify = (ssl_verify ~= "no"),
+ keepalive = (keepalive ~= "no")
+ }))
+ if not res then
+ err = "accessing discovery url (" .. url .. ") failed: " .. error
+ log.error(err)
+ else
+ log.debug("response data: " .. res.body)
+ json, err = authz_keycloak_parse_json_response(res)
+ if json then
+ authz_keycloak_cache_set("discovery", url, cjson.encode(json), exptime or 24 * 60 * 60)
Review comment:
Better to use `core.json.encode`.
##########
File path: apisix/plugins/authz-keycloak.lua
##########
@@ -64,10 +66,164 @@ local _M = {
schema = schema,
}
+
function _M.check_schema(conf)
+ if not conf.discovery and not conf.token_endpoint then
+ return false, 'Neither discovery nor token endpoint given.'
+ end
return core.schema.check(schema, conf)
end
+
+-- Retrieve value from server-wide cache, if available.
+local function authz_keycloak_cache_get(type, key)
+ local dict = ngx.shared[type]
+ local value
+ if dict then
+ value = dict:get(key)
+ if value then log.debug("cache hit: type=", type, " key=", key) end
+ end
+ return value
+end
+
+
+-- Set value in server-wide cache, if available.
+local function authz_keycloak_cache_set(type, key, value, exp)
+ local dict = ngx.shared[type]
+ if dict and (exp > 0) then
+ local success, err, forcible = dict:set(key, value, exp)
Review comment:
Better to `log.error` if set dict failed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #3263: feat: authz-keycloak plugin - Keycloak Authorization Services endpoint discovery
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #3263:
URL: https://github.com/apache/apisix/pull/3263#discussion_r556354404
##########
File path: apisix/plugins/authz-keycloak.lua
##########
@@ -52,8 +55,7 @@ local schema = {
keepalive_timeout = {type = "integer", minimum = 1000, default = 60000},
keepalive_pool = {type = "integer", minimum = 1, default = 5},
ssl_verify = {type = "boolean", default = true},
- },
- required = {"token_endpoint"}
Review comment:
@jenskeiner
Sorry for my mistake! It should be `oneOf` instead of `anyOf`. 😢
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] jenskeiner commented on a change in pull request #3263: feat: authz-keycloak plugin - Keycloak Authorization Services endpoint discovery
Posted by GitBox <gi...@apache.org>.
jenskeiner commented on a change in pull request #3263:
URL: https://github.com/apache/apisix/pull/3263#discussion_r556356858
##########
File path: apisix/plugins/authz-keycloak.lua
##########
@@ -17,15 +17,18 @@
local core = require("apisix.core")
local http = require "resty.http"
local sub_str = string.sub
-local url = require "net.url"
-local tostring = tostring
+local type = type
local ngx = ngx
+local cjson = require("cjson")
+local cjson_s = require("cjson.safe")
Review comment:
Yep, using that one now. We're incurring a slight performance penalty by now using `cjson.safe.decode` through `apisix.core.json.decode` to decode values from the shared memory which are known to have been encoded correctly. But not sure how much of a hit that actually is. Just this line here: https://github.com/jenskeiner/apisix/blob/26277f1ea98366ac852ee6673667e1be1320001c/apisix/plugins/authz-keycloak.lua#L190
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] jenskeiner commented on a change in pull request #3263: feat: authz-keycloak plugin - Keycloak Authorization Services endpoint discovery
Posted by GitBox <gi...@apache.org>.
jenskeiner commented on a change in pull request #3263:
URL: https://github.com/apache/apisix/pull/3263#discussion_r556490057
##########
File path: t/plugin/authz-keycloak.t
##########
@@ -420,7 +587,7 @@ passed
-=== TEST 10: TEST with fake token and https endpoint
+=== TEST 14: TEST with fake token and https endpoint
Review comment:
done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] jenskeiner commented on a change in pull request #3263: feat: authz-keycloak plugin - Add Keycloak Authorization Services endpoint discovery.
Posted by GitBox <gi...@apache.org>.
jenskeiner commented on a change in pull request #3263:
URL: https://github.com/apache/apisix/pull/3263#discussion_r556558063
##########
File path: t/plugin/authz-keycloak.t
##########
@@ -49,12 +49,38 @@ done
-=== TEST 2: full schema check
+=== TEST 2: sanity (using discovery endpoint)
--- config
location /t {
content_by_lua_block {
local plugin = require("apisix.plugins.authz-keycloak")
- local ok, err = plugin.check_schema({token_endpoint = "https://efactory-security-portal.salzburgresearch.at/",
+ local ok, err = plugin.check_schema({
+ discovery = "https://efactory-security-portal.salzburgresearch.at/.well-known/uma2-configuration",
Review comment:
sure
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] jenskeiner commented on a change in pull request #3263: feat: authz-keycloak plugin - Keycloak Authorization Services endpoint discovery
Posted by GitBox <gi...@apache.org>.
jenskeiner commented on a change in pull request #3263:
URL: https://github.com/apache/apisix/pull/3263#discussion_r556412344
##########
File path: apisix/plugins/authz-keycloak.lua
##########
@@ -52,8 +55,7 @@ local schema = {
keepalive_timeout = {type = "integer", minimum = 1000, default = 60000},
keepalive_pool = {type = "integer", minimum = 1, default = 5},
ssl_verify = {type = "boolean", default = true},
- },
- required = {"token_endpoint"}
Review comment:
Hmm, I think we should actually use `anyOf` since the explicit endpoint URL will be preferred if both options are given. The full schema unit test already uses both options so would have to touch that again as well if we make this XOR.
I have future dev work in mind that would require using an additional endpoint. In that case, you may want to use discovery for one endpoint and an explicit value for the other. But, admittedly, that's a rather strange case. In any sane environment, you would always use only discovery or only explicit endpoints, I think.
Let me know your thoughts @spacewander.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] jenskeiner commented on a change in pull request #3263: feat: authz-keycloak plugin - Keycloak Authorization Services endpoint discovery
Posted by GitBox <gi...@apache.org>.
jenskeiner commented on a change in pull request #3263:
URL: https://github.com/apache/apisix/pull/3263#discussion_r556361613
##########
File path: apisix/plugins/authz-keycloak.lua
##########
@@ -64,10 +66,164 @@ local _M = {
schema = schema,
}
+
function _M.check_schema(conf)
+ if not conf.discovery and not conf.token_endpoint then
+ return false, 'Neither discovery nor token endpoint given.'
+ end
return core.schema.check(schema, conf)
end
+
+-- Retrieve value from server-wide cache, if available.
+local function authz_keycloak_cache_get(type, key)
+ local dict = ngx.shared[type]
+ local value
+ if dict then
+ value = dict:get(key)
+ if value then log.debug("cache hit: type=", type, " key=", key) end
+ end
+ return value
+end
+
+
+-- Set value in server-wide cache, if available.
+local function authz_keycloak_cache_set(type, key, value, exp)
+ local dict = ngx.shared[type]
+ if dict and (exp > 0) then
+ local success, err, forcible = dict:set(key, value, exp)
+ log.debug("cache set: success=", success, " err=", err, " forcible=", forcible)
+ end
+end
+
+
+-- Configure timeouts.
+local function authz_keycloak_configure_timeouts(httpc, timeout)
+ if timeout then
+ if type(timeout) == "table" then
+ httpc:set_timeouts(timeout.connect or 0, timeout.send or 0, timeout.read or 0)
+ else
+ httpc:set_timeout(timeout)
+ end
+ end
+end
+
+
+-- Set outgoing proxy options.
+local function authz_keycloak_configure_proxy(httpc, proxy_opts)
+ if httpc and proxy_opts and type(proxy_opts) == "table" then
+ log.debug("authz_keycloak_configure_proxy : use http proxy")
+ httpc:set_proxy_options(proxy_opts)
+ else
+ log.debug("authz_keycloak_configure_proxy : don't use http proxy")
+ end
+end
+
+
+-- Parse the JSON result from a call to the OP.
+local function authz_keycloak_parse_json_response(response, ignore_body_on_success)
+ local ignore_body_on_success = ignore_body_on_success or false
+
+ local err
+ local res
+
+ -- Check the response from the OP.
+ if response.status ~= 200 then
+ err = "response indicates failure, status=" .. response.status .. ", body=" .. response.body
+ else
+ if ignore_body_on_success then
+ return nil, nil
+ end
+
+ -- Decode the response and extract the JSON object.
+ res = cjson_s.decode(response.body)
Review comment:
Fixed.
##########
File path: apisix/plugins/authz-keycloak.lua
##########
@@ -64,10 +66,164 @@ local _M = {
schema = schema,
}
+
function _M.check_schema(conf)
+ if not conf.discovery and not conf.token_endpoint then
+ return false, 'Neither discovery nor token endpoint given.'
+ end
return core.schema.check(schema, conf)
end
+
+-- Retrieve value from server-wide cache, if available.
+local function authz_keycloak_cache_get(type, key)
+ local dict = ngx.shared[type]
+ local value
+ if dict then
+ value = dict:get(key)
+ if value then log.debug("cache hit: type=", type, " key=", key) end
+ end
+ return value
+end
+
+
+-- Set value in server-wide cache, if available.
+local function authz_keycloak_cache_set(type, key, value, exp)
+ local dict = ngx.shared[type]
+ if dict and (exp > 0) then
+ local success, err, forcible = dict:set(key, value, exp)
+ log.debug("cache set: success=", success, " err=", err, " forcible=", forcible)
+ end
+end
+
+
+-- Configure timeouts.
+local function authz_keycloak_configure_timeouts(httpc, timeout)
+ if timeout then
+ if type(timeout) == "table" then
+ httpc:set_timeouts(timeout.connect or 0, timeout.send or 0, timeout.read or 0)
+ else
+ httpc:set_timeout(timeout)
+ end
+ end
+end
+
+
+-- Set outgoing proxy options.
+local function authz_keycloak_configure_proxy(httpc, proxy_opts)
+ if httpc and proxy_opts and type(proxy_opts) == "table" then
+ log.debug("authz_keycloak_configure_proxy : use http proxy")
+ httpc:set_proxy_options(proxy_opts)
+ else
+ log.debug("authz_keycloak_configure_proxy : don't use http proxy")
+ end
+end
+
+
+-- Parse the JSON result from a call to the OP.
+local function authz_keycloak_parse_json_response(response, ignore_body_on_success)
+ local ignore_body_on_success = ignore_body_on_success or false
+
+ local err
+ local res
+
+ -- Check the response from the OP.
+ if response.status ~= 200 then
+ err = "response indicates failure, status=" .. response.status .. ", body=" .. response.body
+ else
+ if ignore_body_on_success then
+ return nil, nil
+ end
+
+ -- Decode the response and extract the JSON object.
+ res = cjson_s.decode(response.body)
+
+ if not res then
+ err = "JSON decoding failed"
Review comment:
Fixed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] jenskeiner commented on a change in pull request #3263: feat: authz-keycloak plugin - Keycloak Authorization Services endpoint discovery
Posted by GitBox <gi...@apache.org>.
jenskeiner commented on a change in pull request #3263:
URL: https://github.com/apache/apisix/pull/3263#discussion_r556368970
##########
File path: apisix/plugins/authz-keycloak.lua
##########
@@ -52,8 +55,7 @@ local schema = {
keepalive_timeout = {type = "integer", minimum = 1000, default = 60000},
keepalive_pool = {type = "integer", minimum = 1, default = 5},
ssl_verify = {type = "boolean", default = true},
- },
- required = {"token_endpoint"}
Review comment:
Right, true. Will change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #3263: feat: authz-keycloak plugin - Keycloak Authorization Services endpoint discovery
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #3263:
URL: https://github.com/apache/apisix/pull/3263#discussion_r556467540
##########
File path: t/plugin/authz-keycloak.t
##########
@@ -420,7 +587,7 @@ passed
-=== TEST 10: TEST with fake token and https endpoint
+=== TEST 14: TEST with fake token and https endpoint
Review comment:
Should update the expected message to
```
error while sending authz request to https://127.0.0.1:8443/auth/realms/University/protocol/openid-connect/token: 18: self signed certificate
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] jenskeiner commented on a change in pull request #3263: feat: authz-keycloak plugin - Keycloak Authorization Services endpoint discovery
Posted by GitBox <gi...@apache.org>.
jenskeiner commented on a change in pull request #3263:
URL: https://github.com/apache/apisix/pull/3263#discussion_r556353778
##########
File path: apisix/plugins/authz-keycloak.lua
##########
@@ -64,10 +66,164 @@ local _M = {
schema = schema,
}
+
function _M.check_schema(conf)
+ if not conf.discovery and not conf.token_endpoint then
+ return false, 'Neither discovery nor token endpoint given.'
+ end
return core.schema.check(schema, conf)
end
+
+-- Retrieve value from server-wide cache, if available.
+local function authz_keycloak_cache_get(type, key)
+ local dict = ngx.shared[type]
+ local value
+ if dict then
+ value = dict:get(key)
+ if value then log.debug("cache hit: type=", type, " key=", key) end
+ end
+ return value
+end
+
+
+-- Set value in server-wide cache, if available.
+local function authz_keycloak_cache_set(type, key, value, exp)
+ local dict = ngx.shared[type]
+ if dict and (exp > 0) then
+ local success, err, forcible = dict:set(key, value, exp)
+ log.debug("cache set: success=", success, " err=", err, " forcible=", forcible)
+ end
+end
+
+
+-- Configure timeouts.
+local function authz_keycloak_configure_timeouts(httpc, timeout)
+ if timeout then
+ if type(timeout) == "table" then
+ httpc:set_timeouts(timeout.connect or 0, timeout.send or 0, timeout.read or 0)
+ else
+ httpc:set_timeout(timeout)
+ end
+ end
+end
+
+
+-- Set outgoing proxy options.
+local function authz_keycloak_configure_proxy(httpc, proxy_opts)
+ if httpc and proxy_opts and type(proxy_opts) == "table" then
+ log.debug("authz_keycloak_configure_proxy : use http proxy")
+ httpc:set_proxy_options(proxy_opts)
+ else
+ log.debug("authz_keycloak_configure_proxy : don't use http proxy")
+ end
+end
+
+
+-- Parse the JSON result from a call to the OP.
+local function authz_keycloak_parse_json_response(response, ignore_body_on_success)
+ local ignore_body_on_success = ignore_body_on_success or false
+
+ local err
+ local res
+
+ -- Check the response from the OP.
+ if response.status ~= 200 then
+ err = "response indicates failure, status=" .. response.status .. ", body=" .. response.body
+ else
+ if ignore_body_on_success then
+ return nil, nil
+ end
+
+ -- Decode the response and extract the JSON object.
+ res = cjson_s.decode(response.body)
+
+ if not res then
+ err = "JSON decoding failed"
+ end
+ end
+
+ return res, err
+end
+
+
+local function decorate_request(http_request_decorator, req)
+ return http_request_decorator and http_request_decorator(req) or req
+end
+
+
+-- Get the Discovery metadata from the specified URL.
+local function authz_keycloak_discover(url, ssl_verify, keepalive, timeout,
+ exptime, proxy_opts, http_request_decorator)
+ log.debug("authz_keycloak_discover: URL is: " .. url)
+
+ local json, err
+ local v = authz_keycloak_cache_get("discovery", url)
+ if not v then
+
+ log.debug("Discovery data not in cache, making call to discovery endpoint.")
+ -- Make the call to the discovery endpoint.
+ local httpc = http.new()
+ authz_keycloak_configure_timeouts(httpc, timeout)
+ authz_keycloak_configure_proxy(httpc, proxy_opts)
+ local res, error = httpc:request_uri(url, decorate_request(http_request_decorator, {
+ ssl_verify = (ssl_verify ~= "no"),
+ keepalive = (keepalive ~= "no")
+ }))
+ if not res then
+ err = "accessing discovery url (" .. url .. ") failed: " .. error
+ log.error(err)
+ else
+ log.debug("response data: " .. res.body)
+ json, err = authz_keycloak_parse_json_response(res)
+ if json then
+ authz_keycloak_cache_set("discovery", url, cjson.encode(json), exptime or 24 * 60 * 60)
Review comment:
Fixed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] jenskeiner commented on a change in pull request #3263: feat: authz-keycloak plugin - Keycloak Authorization Services endpoint discovery
Posted by GitBox <gi...@apache.org>.
jenskeiner commented on a change in pull request #3263:
URL: https://github.com/apache/apisix/pull/3263#discussion_r556338967
##########
File path: apisix/plugins/authz-keycloak.lua
##########
@@ -52,8 +55,7 @@ local schema = {
keepalive_timeout = {type = "integer", minimum = 1000, default = 60000},
keepalive_pool = {type = "integer", minimum = 1, default = 5},
ssl_verify = {type = "boolean", default = true},
- },
- required = {"token_endpoint"}
Review comment:
Right, I'm not yet a JSON schema pro.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Firstsawyou commented on a change in pull request #3263: feat: authz-keycloak plugin - Keycloak Authorization Services endpoint discovery
Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #3263:
URL: https://github.com/apache/apisix/pull/3263#discussion_r556301713
##########
File path: apisix/plugins/authz-keycloak.lua
##########
@@ -17,15 +17,18 @@
local core = require("apisix.core")
local http = require "resty.http"
local sub_str = string.sub
-local url = require "net.url"
-local tostring = tostring
+local type = type
local ngx = ngx
+local cjson = require("cjson")
+local cjson_s = require("cjson.safe")
Review comment:
It is better to use the public methods provided here.[https://github.com/apache/apisix/blob/master/apisix/core/json.lua ](https://github.com/apache/apisix/blob/master/apisix/core/json.lua)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #3263: feat: authz-keycloak plugin - Add Keycloak Authorization Services endpoint discovery.
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #3263:
URL: https://github.com/apache/apisix/pull/3263#discussion_r556505949
##########
File path: apisix/plugins/authz-keycloak.lua
##########
@@ -64,10 +67,169 @@ local _M = {
schema = schema,
}
+
function _M.check_schema(conf)
return core.schema.check(schema, conf)
end
+
+-- Some auxiliary functions below heavily inspired by the excellent
+-- lua-resty-openidc module; see https://github.com/zmartzone/lua-resty-openidc
+
+
+-- Retrieve value from server-wide cache, if available.
+local function authz_keycloak_cache_get(type, key)
+ local dict = ngx.shared[type]
+ local value
+ if dict then
+ value = dict:get(key)
+ if value then log.debug("cache hit: type=", type, " key=", key) end
+ end
+ return value
+end
+
+
+-- Set value in server-wide cache, if available.
+local function authz_keycloak_cache_set(type, key, value, exp)
+ local dict = ngx.shared[type]
+ if dict and (exp > 0) then
+ local success, err, forcible = dict:set(key, value, exp)
+ if err then
+ log.error("cache set: success=", success, " err=", err, " forcible=", forcible)
+ else
+ log.debug("cache set: success=", success, " err=", err, " forcible=", forcible)
+ end
+ end
+end
+
+
+-- Configure timeouts.
+local function authz_keycloak_configure_timeouts(httpc, timeout)
+ if timeout then
+ if type(timeout) == "table" then
+ httpc:set_timeouts(timeout.connect or 0, timeout.send or 0, timeout.read or 0)
+ else
+ httpc:set_timeout(timeout)
+ end
+ end
+end
+
+
+-- Set outgoing proxy options.
+local function authz_keycloak_configure_proxy(httpc, proxy_opts)
+ if httpc and proxy_opts and type(proxy_opts) == "table" then
+ log.debug("authz_keycloak_configure_proxy : use http proxy")
+ httpc:set_proxy_options(proxy_opts)
+ else
+ log.debug("authz_keycloak_configure_proxy : don't use http proxy")
+ end
+end
+
+
+-- Parse the JSON result from a call to the OP.
+local function authz_keycloak_parse_json_response(response, ignore_body_on_success)
Review comment:
What is the `ignore_body_on_success` for? It seems there is nowhere to pass it.
##########
File path: apisix/plugins/authz-keycloak.lua
##########
@@ -52,8 +55,7 @@ local schema = {
keepalive_timeout = {type = "integer", minimum = 1000, default = 60000},
keepalive_pool = {type = "integer", minimum = 1, default = 5},
ssl_verify = {type = "boolean", default = true},
- },
- required = {"token_endpoint"}
Review comment:
OK
##########
File path: doc/plugins/authz-keycloak.md
##########
@@ -40,14 +40,23 @@ For more information on Keycloak, refer to [Keycloak Authorization Docs](https:/
| Name | Type | Requirement | Default | Valid | Description |
| ----------------------- | ------------- | ----------- | --------------------------------------------- | ----------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------- |
-| token_endpoint | string | required | | [1, 4096] | A OAuth2-compliant Token Endpoint that supports the `urn:ietf:params:oauth:grant-type:uma-ticket` grant type. |
+| discovery | string | optional | | [1, 4096] | URL to discovery document for Keycloak Authorization Services. |
Review comment:
We should change the `[1, 4096]` to a valid example. It is the problem of original doc.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] jenskeiner commented on a change in pull request #3263: feat: authz-keycloak plugin - Keycloak Authorization Services endpoint discovery
Posted by GitBox <gi...@apache.org>.
jenskeiner commented on a change in pull request #3263:
URL: https://github.com/apache/apisix/pull/3263#discussion_r556338437
##########
File path: apisix/plugins/authz-keycloak.lua
##########
@@ -64,10 +66,164 @@ local _M = {
schema = schema,
}
+
function _M.check_schema(conf)
+ if not conf.discovery and not conf.token_endpoint then
+ return false, 'Neither discovery nor token endpoint given.'
+ end
return core.schema.check(schema, conf)
end
+
+-- Retrieve value from server-wide cache, if available.
+local function authz_keycloak_cache_get(type, key)
+ local dict = ngx.shared[type]
+ local value
+ if dict then
+ value = dict:get(key)
+ if value then log.debug("cache hit: type=", type, " key=", key) end
+ end
+ return value
+end
+
+
+-- Set value in server-wide cache, if available.
+local function authz_keycloak_cache_set(type, key, value, exp)
+ local dict = ngx.shared[type]
+ if dict and (exp > 0) then
+ local success, err, forcible = dict:set(key, value, exp)
+ log.debug("cache set: success=", success, " err=", err, " forcible=", forcible)
+ end
+end
+
+
+-- Configure timeouts.
+local function authz_keycloak_configure_timeouts(httpc, timeout)
+ if timeout then
+ if type(timeout) == "table" then
+ httpc:set_timeouts(timeout.connect or 0, timeout.send or 0, timeout.read or 0)
+ else
+ httpc:set_timeout(timeout)
+ end
+ end
+end
+
+
+-- Set outgoing proxy options.
+local function authz_keycloak_configure_proxy(httpc, proxy_opts)
+ if httpc and proxy_opts and type(proxy_opts) == "table" then
+ log.debug("authz_keycloak_configure_proxy : use http proxy")
+ httpc:set_proxy_options(proxy_opts)
+ else
+ log.debug("authz_keycloak_configure_proxy : don't use http proxy")
+ end
+end
+
+
+-- Parse the JSON result from a call to the OP.
+local function authz_keycloak_parse_json_response(response, ignore_body_on_success)
+ local ignore_body_on_success = ignore_body_on_success or false
+
+ local err
+ local res
+
+ -- Check the response from the OP.
+ if response.status ~= 200 then
+ err = "response indicates failure, status=" .. response.status .. ", body=" .. response.body
+ else
+ if ignore_body_on_success then
+ return nil, nil
+ end
+
+ -- Decode the response and extract the JSON object.
+ res = cjson_s.decode(response.body)
Review comment:
Agreed. Will fix.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] jenskeiner commented on a change in pull request #3263: feat: authz-keycloak plugin - Keycloak Authorization Services endpoint discovery
Posted by GitBox <gi...@apache.org>.
jenskeiner commented on a change in pull request #3263:
URL: https://github.com/apache/apisix/pull/3263#discussion_r556338586
##########
File path: apisix/plugins/authz-keycloak.lua
##########
@@ -64,10 +66,164 @@ local _M = {
schema = schema,
}
+
function _M.check_schema(conf)
+ if not conf.discovery and not conf.token_endpoint then
+ return false, 'Neither discovery nor token endpoint given.'
+ end
return core.schema.check(schema, conf)
end
+
+-- Retrieve value from server-wide cache, if available.
+local function authz_keycloak_cache_get(type, key)
+ local dict = ngx.shared[type]
+ local value
+ if dict then
+ value = dict:get(key)
+ if value then log.debug("cache hit: type=", type, " key=", key) end
+ end
+ return value
+end
+
+
+-- Set value in server-wide cache, if available.
+local function authz_keycloak_cache_set(type, key, value, exp)
+ local dict = ngx.shared[type]
+ if dict and (exp > 0) then
+ local success, err, forcible = dict:set(key, value, exp)
+ log.debug("cache set: success=", success, " err=", err, " forcible=", forcible)
+ end
+end
+
+
+-- Configure timeouts.
+local function authz_keycloak_configure_timeouts(httpc, timeout)
+ if timeout then
+ if type(timeout) == "table" then
+ httpc:set_timeouts(timeout.connect or 0, timeout.send or 0, timeout.read or 0)
+ else
+ httpc:set_timeout(timeout)
+ end
+ end
+end
+
+
+-- Set outgoing proxy options.
+local function authz_keycloak_configure_proxy(httpc, proxy_opts)
+ if httpc and proxy_opts and type(proxy_opts) == "table" then
+ log.debug("authz_keycloak_configure_proxy : use http proxy")
+ httpc:set_proxy_options(proxy_opts)
+ else
+ log.debug("authz_keycloak_configure_proxy : don't use http proxy")
+ end
+end
+
+
+-- Parse the JSON result from a call to the OP.
+local function authz_keycloak_parse_json_response(response, ignore_body_on_success)
+ local ignore_body_on_success = ignore_body_on_success or false
+
+ local err
+ local res
+
+ -- Check the response from the OP.
+ if response.status ~= 200 then
+ err = "response indicates failure, status=" .. response.status .. ", body=" .. response.body
+ else
+ if ignore_body_on_success then
+ return nil, nil
+ end
+
+ -- Decode the response and extract the JSON object.
+ res = cjson_s.decode(response.body)
+
+ if not res then
+ err = "JSON decoding failed"
Review comment:
Ok.
##########
File path: apisix/plugins/authz-keycloak.lua
##########
@@ -64,10 +66,164 @@ local _M = {
schema = schema,
}
+
function _M.check_schema(conf)
+ if not conf.discovery and not conf.token_endpoint then
+ return false, 'Neither discovery nor token endpoint given.'
+ end
return core.schema.check(schema, conf)
end
+
+-- Retrieve value from server-wide cache, if available.
+local function authz_keycloak_cache_get(type, key)
+ local dict = ngx.shared[type]
+ local value
+ if dict then
+ value = dict:get(key)
+ if value then log.debug("cache hit: type=", type, " key=", key) end
+ end
+ return value
+end
+
+
+-- Set value in server-wide cache, if available.
+local function authz_keycloak_cache_set(type, key, value, exp)
+ local dict = ngx.shared[type]
+ if dict and (exp > 0) then
+ local success, err, forcible = dict:set(key, value, exp)
+ log.debug("cache set: success=", success, " err=", err, " forcible=", forcible)
+ end
+end
+
+
+-- Configure timeouts.
+local function authz_keycloak_configure_timeouts(httpc, timeout)
+ if timeout then
+ if type(timeout) == "table" then
+ httpc:set_timeouts(timeout.connect or 0, timeout.send or 0, timeout.read or 0)
+ else
+ httpc:set_timeout(timeout)
+ end
+ end
+end
+
+
+-- Set outgoing proxy options.
+local function authz_keycloak_configure_proxy(httpc, proxy_opts)
+ if httpc and proxy_opts and type(proxy_opts) == "table" then
+ log.debug("authz_keycloak_configure_proxy : use http proxy")
+ httpc:set_proxy_options(proxy_opts)
+ else
+ log.debug("authz_keycloak_configure_proxy : don't use http proxy")
+ end
+end
+
+
+-- Parse the JSON result from a call to the OP.
+local function authz_keycloak_parse_json_response(response, ignore_body_on_success)
+ local ignore_body_on_success = ignore_body_on_success or false
+
+ local err
+ local res
+
+ -- Check the response from the OP.
+ if response.status ~= 200 then
+ err = "response indicates failure, status=" .. response.status .. ", body=" .. response.body
+ else
+ if ignore_body_on_success then
+ return nil, nil
+ end
+
+ -- Decode the response and extract the JSON object.
+ res = cjson_s.decode(response.body)
+
+ if not res then
+ err = "JSON decoding failed"
+ end
+ end
+
+ return res, err
+end
+
+
+local function decorate_request(http_request_decorator, req)
+ return http_request_decorator and http_request_decorator(req) or req
+end
+
+
+-- Get the Discovery metadata from the specified URL.
+local function authz_keycloak_discover(url, ssl_verify, keepalive, timeout,
+ exptime, proxy_opts, http_request_decorator)
+ log.debug("authz_keycloak_discover: URL is: " .. url)
+
+ local json, err
+ local v = authz_keycloak_cache_get("discovery", url)
+ if not v then
+
+ log.debug("Discovery data not in cache, making call to discovery endpoint.")
+ -- Make the call to the discovery endpoint.
+ local httpc = http.new()
+ authz_keycloak_configure_timeouts(httpc, timeout)
+ authz_keycloak_configure_proxy(httpc, proxy_opts)
+ local res, error = httpc:request_uri(url, decorate_request(http_request_decorator, {
+ ssl_verify = (ssl_verify ~= "no"),
+ keepalive = (keepalive ~= "no")
+ }))
+ if not res then
+ err = "accessing discovery url (" .. url .. ") failed: " .. error
+ log.error(err)
+ else
+ log.debug("response data: " .. res.body)
+ json, err = authz_keycloak_parse_json_response(res)
+ if json then
+ authz_keycloak_cache_set("discovery", url, cjson.encode(json), exptime or 24 * 60 * 60)
Review comment:
Yep.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] jenskeiner commented on a change in pull request #3263: feat: authz-keycloak plugin - Keycloak Authorization Services endpoint discovery
Posted by GitBox <gi...@apache.org>.
jenskeiner commented on a change in pull request #3263:
URL: https://github.com/apache/apisix/pull/3263#discussion_r556353907
##########
File path: apisix/plugins/authz-keycloak.lua
##########
@@ -64,10 +66,164 @@ local _M = {
schema = schema,
}
+
function _M.check_schema(conf)
+ if not conf.discovery and not conf.token_endpoint then
+ return false, 'Neither discovery nor token endpoint given.'
+ end
return core.schema.check(schema, conf)
end
+
+-- Retrieve value from server-wide cache, if available.
+local function authz_keycloak_cache_get(type, key)
+ local dict = ngx.shared[type]
+ local value
+ if dict then
+ value = dict:get(key)
+ if value then log.debug("cache hit: type=", type, " key=", key) end
+ end
+ return value
+end
+
+
+-- Set value in server-wide cache, if available.
+local function authz_keycloak_cache_set(type, key, value, exp)
+ local dict = ngx.shared[type]
+ if dict and (exp > 0) then
+ local success, err, forcible = dict:set(key, value, exp)
+ log.debug("cache set: success=", success, " err=", err, " forcible=", forcible)
+ end
+end
+
+
+-- Configure timeouts.
+local function authz_keycloak_configure_timeouts(httpc, timeout)
+ if timeout then
+ if type(timeout) == "table" then
+ httpc:set_timeouts(timeout.connect or 0, timeout.send or 0, timeout.read or 0)
+ else
+ httpc:set_timeout(timeout)
+ end
+ end
+end
+
+
+-- Set outgoing proxy options.
+local function authz_keycloak_configure_proxy(httpc, proxy_opts)
+ if httpc and proxy_opts and type(proxy_opts) == "table" then
+ log.debug("authz_keycloak_configure_proxy : use http proxy")
+ httpc:set_proxy_options(proxy_opts)
+ else
+ log.debug("authz_keycloak_configure_proxy : don't use http proxy")
+ end
+end
+
+
+-- Parse the JSON result from a call to the OP.
+local function authz_keycloak_parse_json_response(response, ignore_body_on_success)
+ local ignore_body_on_success = ignore_body_on_success or false
+
+ local err
+ local res
+
+ -- Check the response from the OP.
+ if response.status ~= 200 then
+ err = "response indicates failure, status=" .. response.status .. ", body=" .. response.body
+ else
+ if ignore_body_on_success then
+ return nil, nil
+ end
+
+ -- Decode the response and extract the JSON object.
+ res = cjson_s.decode(response.body)
+
+ if not res then
+ err = "JSON decoding failed"
+ end
+ end
+
+ return res, err
+end
+
+
+local function decorate_request(http_request_decorator, req)
+ return http_request_decorator and http_request_decorator(req) or req
+end
+
+
+-- Get the Discovery metadata from the specified URL.
+local function authz_keycloak_discover(url, ssl_verify, keepalive, timeout,
+ exptime, proxy_opts, http_request_decorator)
+ log.debug("authz_keycloak_discover: URL is: " .. url)
+
+ local json, err
+ local v = authz_keycloak_cache_get("discovery", url)
+ if not v then
+
+ log.debug("Discovery data not in cache, making call to discovery endpoint.")
+ -- Make the call to the discovery endpoint.
+ local httpc = http.new()
+ authz_keycloak_configure_timeouts(httpc, timeout)
+ authz_keycloak_configure_proxy(httpc, proxy_opts)
+ local res, error = httpc:request_uri(url, decorate_request(http_request_decorator, {
+ ssl_verify = (ssl_verify ~= "no"),
+ keepalive = (keepalive ~= "no")
+ }))
+ if not res then
+ err = "accessing discovery url (" .. url .. ") failed: " .. error
+ log.error(err)
+ else
+ log.debug("response data: " .. res.body)
+ json, err = authz_keycloak_parse_json_response(res)
+ if json then
+ authz_keycloak_cache_set("discovery", url, cjson.encode(json), exptime or 24 * 60 * 60)
+ else
+ err = "could not decode JSON from Discovery data" .. (err and (": " .. err) or '')
+ log.error(err)
+ end
+ end
+
+ else
+ json = cjson.decode(v)
Review comment:
Fixed.
##########
File path: apisix/plugins/authz-keycloak.lua
##########
@@ -64,10 +66,164 @@ local _M = {
schema = schema,
}
+
function _M.check_schema(conf)
+ if not conf.discovery and not conf.token_endpoint then
+ return false, 'Neither discovery nor token endpoint given.'
+ end
return core.schema.check(schema, conf)
end
+
+-- Retrieve value from server-wide cache, if available.
+local function authz_keycloak_cache_get(type, key)
+ local dict = ngx.shared[type]
+ local value
+ if dict then
+ value = dict:get(key)
+ if value then log.debug("cache hit: type=", type, " key=", key) end
+ end
+ return value
+end
+
+
+-- Set value in server-wide cache, if available.
+local function authz_keycloak_cache_set(type, key, value, exp)
+ local dict = ngx.shared[type]
+ if dict and (exp > 0) then
+ local success, err, forcible = dict:set(key, value, exp)
Review comment:
Fixed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] jenskeiner edited a comment on pull request #3263: feat: authz-keycloak plugin - Keycloak Authorization Services endpoint discovery
Posted by GitBox <gi...@apache.org>.
jenskeiner edited a comment on pull request #3263:
URL: https://github.com/apache/apisix/pull/3263#issuecomment-759061451
@sshniro: Penny for your thoughts. I have a POC in a separate branch that dynamically resolves URIs to resources defined in Keycloak, so this is just a first change in that direction.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] jenskeiner commented on a change in pull request #3263: feat: authz-keycloak plugin - Keycloak Authorization Services endpoint discovery
Posted by GitBox <gi...@apache.org>.
jenskeiner commented on a change in pull request #3263:
URL: https://github.com/apache/apisix/pull/3263#discussion_r556373701
##########
File path: apisix/plugins/authz-keycloak.lua
##########
@@ -64,10 +68,165 @@ local _M = {
schema = schema,
}
+
function _M.check_schema(conf)
return core.schema.check(schema, conf)
end
+
+-- Retrieve value from server-wide cache, if available.
+local function authz_keycloak_cache_get(type, key)
+ local dict = ngx.shared[type]
+ local value
+ if dict then
+ value = dict:get(key)
+ if value then log.debug("cache hit: type=", type, " key=", key) end
+ end
+ return value
+end
+
+
+-- Set value in server-wide cache, if available.
+local function authz_keycloak_cache_set(type, key, value, exp)
+ local dict = ngx.shared[type]
+ if dict and (exp > 0) then
+ local success, err, forcible = dict:set(key, value, exp)
+ if err then
+ log.error("cache set: success=", success, " err=", err, " forcible=", forcible)
+ else
+ log.debug("cache set: success=", success, " err=", err, " forcible=", forcible)
+ end
+ end
+end
+
+
+-- Configure timeouts.
+local function authz_keycloak_configure_timeouts(httpc, timeout)
+ if timeout then
+ if type(timeout) == "table" then
+ httpc:set_timeouts(timeout.connect or 0, timeout.send or 0, timeout.read or 0)
+ else
+ httpc:set_timeout(timeout)
+ end
+ end
+end
+
+
+-- Set outgoing proxy options.
+local function authz_keycloak_configure_proxy(httpc, proxy_opts)
+ if httpc and proxy_opts and type(proxy_opts) == "table" then
+ log.debug("authz_keycloak_configure_proxy : use http proxy")
+ httpc:set_proxy_options(proxy_opts)
+ else
+ log.debug("authz_keycloak_configure_proxy : don't use http proxy")
+ end
+end
+
+
+-- Parse the JSON result from a call to the OP.
+local function authz_keycloak_parse_json_response(response, ignore_body_on_success)
+ local ignore_body_on_success = ignore_body_on_success or false
+
+ local err
+ local res
+
+ -- Check the response from the OP.
+ if response.status ~= 200 then
+ err = "response indicates failure, status=" .. response.status .. ", body=" .. response.body
+ else
+ if ignore_body_on_success then
+ return nil, nil
+ end
+
+ -- Decode the response and extract the JSON object.
+ res, err = json.decode(response.body)
+
+ if not res then
+ err = "JSON decoding failed: " .. err
+ end
+ end
+
+ return res, err
+end
+
+
+local function decorate_request(http_request_decorator, req)
+ return http_request_decorator and http_request_decorator(req) or req
+end
+
+
+-- Get the Discovery metadata from the specified URL.
+local function authz_keycloak_discover(url, ssl_verify, keepalive, timeout,
+ exptime, proxy_opts, http_request_decorator)
+ log.debug("authz_keycloak_discover: URL is: " .. url)
+
+ local json, err
Review comment:
Ok. I'm using e.g. `core.json.decode` now, then `json` can be used as a variable name w/o any confusion.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] jenskeiner commented on a change in pull request #3263: feat: authz-keycloak plugin - Keycloak Authorization Services endpoint discovery
Posted by GitBox <gi...@apache.org>.
jenskeiner commented on a change in pull request #3263:
URL: https://github.com/apache/apisix/pull/3263#discussion_r556354122
##########
File path: apisix/plugins/authz-keycloak.lua
##########
@@ -52,8 +55,7 @@ local schema = {
keepalive_timeout = {type = "integer", minimum = 1000, default = 60000},
keepalive_pool = {type = "integer", minimum = 1, default = 5},
ssl_verify = {type = "boolean", default = true},
- },
- required = {"token_endpoint"}
Review comment:
Fixed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #3263: feat: authz-keycloak plugin - Keycloak Authorization Services endpoint discovery
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #3263:
URL: https://github.com/apache/apisix/pull/3263#discussion_r556369659
##########
File path: apisix/plugins/authz-keycloak.lua
##########
@@ -64,10 +68,165 @@ local _M = {
schema = schema,
}
+
function _M.check_schema(conf)
return core.schema.check(schema, conf)
end
+
+-- Retrieve value from server-wide cache, if available.
+local function authz_keycloak_cache_get(type, key)
+ local dict = ngx.shared[type]
+ local value
+ if dict then
+ value = dict:get(key)
+ if value then log.debug("cache hit: type=", type, " key=", key) end
+ end
+ return value
+end
+
+
+-- Set value in server-wide cache, if available.
+local function authz_keycloak_cache_set(type, key, value, exp)
+ local dict = ngx.shared[type]
+ if dict and (exp > 0) then
+ local success, err, forcible = dict:set(key, value, exp)
+ if err then
+ log.error("cache set: success=", success, " err=", err, " forcible=", forcible)
+ else
+ log.debug("cache set: success=", success, " err=", err, " forcible=", forcible)
+ end
+ end
+end
+
+
+-- Configure timeouts.
+local function authz_keycloak_configure_timeouts(httpc, timeout)
+ if timeout then
+ if type(timeout) == "table" then
+ httpc:set_timeouts(timeout.connect or 0, timeout.send or 0, timeout.read or 0)
+ else
+ httpc:set_timeout(timeout)
+ end
+ end
+end
+
+
+-- Set outgoing proxy options.
+local function authz_keycloak_configure_proxy(httpc, proxy_opts)
+ if httpc and proxy_opts and type(proxy_opts) == "table" then
+ log.debug("authz_keycloak_configure_proxy : use http proxy")
+ httpc:set_proxy_options(proxy_opts)
+ else
+ log.debug("authz_keycloak_configure_proxy : don't use http proxy")
+ end
+end
+
+
+-- Parse the JSON result from a call to the OP.
+local function authz_keycloak_parse_json_response(response, ignore_body_on_success)
+ local ignore_body_on_success = ignore_body_on_success or false
+
+ local err
+ local res
+
+ -- Check the response from the OP.
+ if response.status ~= 200 then
+ err = "response indicates failure, status=" .. response.status .. ", body=" .. response.body
+ else
+ if ignore_body_on_success then
+ return nil, nil
+ end
+
+ -- Decode the response and extract the JSON object.
+ res, err = json.decode(response.body)
+
+ if not res then
+ err = "JSON decoding failed: " .. err
+ end
+ end
+
+ return res, err
+end
+
+
+local function decorate_request(http_request_decorator, req)
+ return http_request_decorator and http_request_decorator(req) or req
+end
+
+
+-- Get the Discovery metadata from the specified URL.
+local function authz_keycloak_discover(url, ssl_verify, keepalive, timeout,
+ exptime, proxy_opts, http_request_decorator)
+ log.debug("authz_keycloak_discover: URL is: " .. url)
+
+ local json, err
Review comment:
Better to use different name than `json`, since we already have `json` defined as a module.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] jenskeiner commented on a change in pull request #3263: feat: authz-keycloak plugin - Keycloak Authorization Services endpoint discovery
Posted by GitBox <gi...@apache.org>.
jenskeiner commented on a change in pull request #3263:
URL: https://github.com/apache/apisix/pull/3263#discussion_r556412344
##########
File path: apisix/plugins/authz-keycloak.lua
##########
@@ -52,8 +55,7 @@ local schema = {
keepalive_timeout = {type = "integer", minimum = 1000, default = 60000},
keepalive_pool = {type = "integer", minimum = 1, default = 5},
ssl_verify = {type = "boolean", default = true},
- },
- required = {"token_endpoint"}
Review comment:
Hmm, I think we should actually use `anyOf` since the explicit endpoint URL will be preferred if both options are given. The full schema unit tests already uses both options so would have to touch that again as well if we make this XOR.
I have future dev work in mind that would require using an additional endpoint. In that case, you may want to use discovery for one endpoint and an explicit value for the other. But, admittedly, that's a rather strange case. In any sane environment, you would always use only discovery or only explicit endpoints, I think.
Let me know your thoughts @spacewander.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] jenskeiner commented on pull request #3263: Uma discovery
Posted by GitBox <gi...@apache.org>.
jenskeiner commented on pull request #3263:
URL: https://github.com/apache/apisix/pull/3263#issuecomment-759061451
@sshniro: Penny for your in your thoughts. I have a POC in a separate branch that dynamically resolves URIs to resources defined in Keycloak, so this is just a first change in that direction.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] jenskeiner commented on a change in pull request #3263: feat: authz-keycloak plugin - Add Keycloak Authorization Services endpoint discovery.
Posted by GitBox <gi...@apache.org>.
jenskeiner commented on a change in pull request #3263:
URL: https://github.com/apache/apisix/pull/3263#discussion_r556544459
##########
File path: doc/plugins/authz-keycloak.md
##########
@@ -40,14 +40,23 @@ For more information on Keycloak, refer to [Keycloak Authorization Docs](https:/
| Name | Type | Requirement | Default | Valid | Description |
| ----------------------- | ------------- | ----------- | --------------------------------------------- | ----------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------- |
-| token_endpoint | string | required | | [1, 4096] | A OAuth2-compliant Token Endpoint that supports the `urn:ietf:params:oauth:grant-type:uma-ticket` grant type. |
+| discovery | string | optional | | [1, 4096] | URL to discovery document for Keycloak Authorization Services. |
Review comment:
done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] sshniro commented on a change in pull request #3263: feat: authz-keycloak plugin - Add Keycloak Authorization Services endpoint discovery.
Posted by GitBox <gi...@apache.org>.
sshniro commented on a change in pull request #3263:
URL: https://github.com/apache/apisix/pull/3263#discussion_r556546204
##########
File path: t/plugin/authz-keycloak.t
##########
@@ -49,12 +49,38 @@ done
-=== TEST 2: full schema check
+=== TEST 2: sanity (using discovery endpoint)
--- config
location /t {
content_by_lua_block {
local plugin = require("apisix.plugins.authz-keycloak")
- local ok, err = plugin.check_schema({token_endpoint = "https://efactory-security-portal.salzburgresearch.at/",
+ local ok, err = plugin.check_schema({
+ discovery = "https://efactory-security-portal.salzburgresearch.at/.well-known/uma2-configuration",
Review comment:
Aw, bummer, it seems I have used my previous dev configurations, please do change it to a generic URL if possible.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander merged pull request #3263: feat(authz-keycloak): Add Keycloak Authorization Services endpoint discovery.
Posted by GitBox <gi...@apache.org>.
spacewander merged pull request #3263:
URL: https://github.com/apache/apisix/pull/3263
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org