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 2020/12/01 01:43:35 UTC
[GitHub] [apisix] spacewander commented on a change in pull request #2903: feat: Make headers to add to request in openid-connect plugin configurable.
spacewander commented on a change in pull request #2903:
URL: https://github.com/apache/apisix/pull/2903#discussion_r533015815
##########
File path: apisix/plugins/openid-connect.lua
##########
@@ -82,63 +87,131 @@ function _M.check_schema(conf)
conf.logout_path = '/logout'
end
+ if not conf.set_access_token_header then
+ conf.set_access_token_header = true
+ end
+
+ if not conf.set_userinfo_token_header then
+ conf.set_userinfo_token_header = true
+ end
+
+ if not conf.set_id_token_header then
+ conf.set_id_token_header = true
+ end
+
+ if not conf.access_token_in_authorization_header then
+ conf.access_token_in_authorization_header = false
+ end
+
return true
end
-local function has_bearer_access_token(ctx)
+local function check_bearer_access_token(ctx)
+ -- Get Authorization header, maybe.
local auth_header = core.request.header(ctx, "Authorization")
if not auth_header then
- return false
+ -- No Authorization header, get X-Access-Token header, maybe.
+ local access_token_header = core.request.header(ctx, "X-Access-Token")
+ if not access_token_header then
+ -- No X-Access-Token header neither.
+ return false, nil, nil
+ end
+
+ -- Return extracted header value.
+ return true, access_token_header, nil
end
+ -- Check format of Authorization header.
local res, err = ngx_re.split(auth_header, " ", nil, nil, 2)
if not res then
- return false, err
+ return false, nil, err
end
- if res[1] == "bearer" then
- return true
+ if string.lower(res[1]) == "bearer" then
+ -- Return extracted token.
+ return true, res[2], nil
end
return false
end
+local function add_user_header(user)
+ local userinfo = core.json.encode(user)
+ ngx.req.set_header("X-Userinfo", ngx_encode_base64(userinfo))
+end
+
+
+local function add_access_token_header(ctx, conf, token)
+ -- Add Authorization or X-Access-Token header, respectively, if not already set.
+ if conf.set_access_token_header then
+ if conf.access_token_in_authorization_header then
+ if not core.request.header(ctx, "Authorization") then
+ -- Add Authorization header.
+ ngx.req.set_header("Authorization", "Bearer " .. token)
+ end
+ else
+ if not core.request.header(ctx, "X-Access-Token") then
+ -- Add X-Access-Token header.
+ ngx.req.set_header("X-Access-Token", token)
+ end
+ end
+ end
+end
+
+
local function introspect(ctx, conf)
- if has_bearer_access_token(ctx) or conf.bearer_only then
+ -- Extract token, maybe. Ignore errors.
+ local has_token, token, _ = check_bearer_access_token(ctx)
+
+ -- Check if token was extracted or if we always require a token in the request.
+ if has_token(ctx) or conf.bearer_only then
Review comment:
Why you call the boolean value `has_token`?
##########
File path: apisix/plugins/openid-connect.lua
##########
@@ -82,63 +87,131 @@ function _M.check_schema(conf)
conf.logout_path = '/logout'
end
+ if not conf.set_access_token_header then
+ conf.set_access_token_header = true
+ end
+
+ if not conf.set_userinfo_token_header then
+ conf.set_userinfo_token_header = true
+ end
+
+ if not conf.set_id_token_header then
+ conf.set_id_token_header = true
+ end
+
+ if not conf.access_token_in_authorization_header then
+ conf.access_token_in_authorization_header = false
+ end
+
return true
end
-local function has_bearer_access_token(ctx)
+local function check_bearer_access_token(ctx)
+ -- Get Authorization header, maybe.
local auth_header = core.request.header(ctx, "Authorization")
if not auth_header then
- return false
+ -- No Authorization header, get X-Access-Token header, maybe.
+ local access_token_header = core.request.header(ctx, "X-Access-Token")
+ if not access_token_header then
+ -- No X-Access-Token header neither.
+ return false, nil, nil
+ end
+
+ -- Return extracted header value.
+ return true, access_token_header, nil
end
+ -- Check format of Authorization header.
local res, err = ngx_re.split(auth_header, " ", nil, nil, 2)
if not res then
- return false, err
+ return false, nil, err
end
- if res[1] == "bearer" then
- return true
+ if string.lower(res[1]) == "bearer" then
+ -- Return extracted token.
+ return true, res[2], nil
end
return false
end
+local function add_user_header(user)
+ local userinfo = core.json.encode(user)
+ ngx.req.set_header("X-Userinfo", ngx_encode_base64(userinfo))
+end
+
+
+local function add_access_token_header(ctx, conf, token)
+ -- Add Authorization or X-Access-Token header, respectively, if not already set.
+ if conf.set_access_token_header then
+ if conf.access_token_in_authorization_header then
+ if not core.request.header(ctx, "Authorization") then
+ -- Add Authorization header.
+ ngx.req.set_header("Authorization", "Bearer " .. token)
+ end
+ else
+ if not core.request.header(ctx, "X-Access-Token") then
+ -- Add X-Access-Token header.
+ ngx.req.set_header("X-Access-Token", token)
+ end
+ end
+ end
+end
+
+
local function introspect(ctx, conf)
- if has_bearer_access_token(ctx) or conf.bearer_only then
+ -- Extract token, maybe. Ignore errors.
+ local has_token, token, _ = check_bearer_access_token(ctx)
+
+ -- Check if token was extracted or if we always require a token in the request.
+ if has_token(ctx) or conf.bearer_only then
local res, err
if conf.public_key then
+ -- Validate token against public key.
res, err = openidc.bearer_jwt_verify(conf)
if res then
+ -- Token is valid.
+
+ -- Add configured access token header, maybe.
+ add_access_token_header(ctx, conf, token)
return res
end
else
+ -- Validate token against introspection endpoint.
res, err = openidc.introspect(conf)
if err then
return ngx.HTTP_UNAUTHORIZED, err
else
+ -- Token is valid and res contains the response from the introspection endpoint.
+
+ if conf.set_userinfo_token_header then
+ -- Set X-Userinfo header to introspection endpoint response.
+ add_user_header(res)
+ end
+
+ -- Add configured access token header, maybe.
+ add_access_token_header(token, conf)
Review comment:
`add_access_token_header(ctx, conf, token)`
Argument mismatch.
##########
File path: apisix/plugins/openid-connect.lua
##########
@@ -14,6 +14,7 @@
-- See the License for the specific language governing permissions and
-- limitations under the License.
--
+local string = require("string")
Review comment:
`local string = string`
is enough.
##########
File path: apisix/plugins/openid-connect.lua
##########
@@ -152,26 +225,28 @@ function _M.access(plugin_conf, ctx)
core.log.error("failed to introspect in openidc: ", err)
return response
end
- if response then
- add_user_header(response)
- end
end
if not response then
+ -- A valid token was not in the request. Try to obtain one by authenticatin against the
+ -- configured identity provider.
local response, err = openidc.authenticate(conf)
if err then
core.log.error("failed to authenticate in openidc: ", err)
return 500
end
if response then
- if response.user then
+ -- Add X-Userinfo header, maybe.
+ if conf.set_userinfo_token_header and response.user then
add_user_header(response.user)
end
- if response.access_token then
- ngx.req.set_header("X-Access-Token", response.access_token)
- end
- if response.id_token then
+
+ -- Add configured access token header, maybe.
+ add_access_token_header(ctx, conf, response.access_token)
Review comment:
Should check if `response.access_token` is nil
##########
File path: apisix/plugins/openid-connect.lua
##########
@@ -152,26 +225,28 @@ function _M.access(plugin_conf, ctx)
core.log.error("failed to introspect in openidc: ", err)
return response
end
- if response then
Review comment:
Why remove this part?
##########
File path: apisix/plugins/openid-connect.lua
##########
@@ -40,7 +41,11 @@ local schema = {
logout_path = {type = "string"}, -- default is /logout
redirect_uri = {type = "string"}, -- default is ngx.var.request_uri
public_key = {type = "string"},
- token_signing_alg_values_expected = {type = "string"}
+ token_signing_alg_values_expected = {type = "string"},
+ set_access_token_header = {type = "boolean"}, -- default is true
Review comment:
Let's use the new way to set the default value:
https://github.com/apache/apisix/blob/9dfe697b644691509b55cd7ec58188b33dd3e792/apisix/plugins/cors.lua#L37
----------------------------------------------------------------
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