Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Make headers to add to request in openid-connect plugin configurable. #2903

Merged
merged 112 commits into from
Jan 5, 2021
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
112 commits
Select commit Hold shift + click to select a range
4af92db
Make headers to add to request configurable.
jenskeiner Nov 30, 2020
fc57f35
Fix lines that were too long.
jenskeiner Nov 30, 2020
49e904f
Import global string module.
jenskeiner Nov 30, 2020
4c033d9
Fix an issue when trying to call a boolean value.
jenskeiner Dec 1, 2020
e997baa
Addressing several pull request comments.
jenskeiner Dec 1, 2020
fbca5d8
Remove old way of setting defaults for added configuration options.
jenskeiner Dec 1, 2020
b842247
Merge branch 'master' into 2764-oidc-configurable-headers
jenskeiner Dec 1, 2020
8eabdef
Fix lines that are too long.
jenskeiner Dec 1, 2020
6328c68
Remove trailing whitespace.
jenskeiner Dec 1, 2020
bfa4aa7
Add auxilliary method to insert/update request header while updating …
jenskeiner Dec 1, 2020
1aeec0b
Small fixes.
jenskeiner Dec 1, 2020
eb73cee
Merge branch 'master_upstream' into 2764-oidc-configurable-headers
jenskeiner Dec 16, 2020
f13fc9d
CHange backend from 127.0.0.1:1980 to 127.0.0.1:8888 to use mendhak/h…
jenskeiner Dec 16, 2020
f088a88
Fix port number.
jenskeiner Dec 16, 2020
a4d503d
Revert local port back to 1980.
jenskeiner Dec 16, 2020
76c6b8b
Add endpoint that reflects OIDC-relevant request headers back in resp…
jenskeiner Dec 16, 2020
ade0bec
Add test cases to check if request bearer token in Authoriztation hea…
jenskeiner Dec 16, 2020
19c62a7
Fix issue when not all desired headers were reflected.
jenskeiner Dec 16, 2020
84c8684
Fix header verification tests.
jenskeiner Dec 16, 2020
4ce30ac
Fix test cases.
jenskeiner Dec 16, 2020
1b9cf28
Temporarily run only openid-connect plugin tests.
jenskeiner Dec 17, 2020
de7600e
Use different endpoint to reflect headers back in response body.
jenskeiner Dec 17, 2020
2e19611
Use direct GET request.
jenskeiner Dec 17, 2020
989f6b9
Fix unit tests.
jenskeiner Dec 17, 2020
d865b5d
Fix test case.
jenskeiner Dec 17, 2020
032b0e8
Remove obsolete helper function.
jenskeiner Dec 17, 2020
0ead6da
Improve documentation. Start new test case that simulates the authori…
jenskeiner Dec 17, 2020
593853b
Further process response from authorization endpoint and print to res…
jenskeiner Dec 17, 2020
a5ece5d
Small fixes.
jenskeiner Dec 17, 2020
f916b7e
Print out cookies.
jenskeiner Dec 17, 2020
8194baa
Concatenate extracted cookies into request header format.
jenskeiner Dec 17, 2020
12cf142
Fix cookie header format.
jenskeiner Dec 17, 2020
a9f4248
Extract raw cookie values.
jenskeiner Dec 18, 2020
728c750
Log out response of initial call to /hello endpoint redirect to ID pr…
jenskeiner Dec 18, 2020
3de1aff
Fix missing content_by_lua_block.
jenskeiner Dec 18, 2020
866fcbd
Fix more errors.
jenskeiner Dec 18, 2020
a04ca52
More fixes.
jenskeiner Dec 18, 2020
86589f2
Reorganise test order.
jenskeiner Dec 18, 2020
0a1ee78
More fixes.
jenskeiner Dec 18, 2020
009c6bc
More fixes.
jenskeiner Dec 18, 2020
f872a58
Even more fixes.
jenskeiner Dec 18, 2020
797c44a
Extract nonce, state and session cookie.
jenskeiner Dec 18, 2020
4a9a5a3
Fixes.
jenskeiner Dec 18, 2020
3249f59
Fixes.
jenskeiner Dec 18, 2020
385de27
Add back in some code.
jenskeiner Dec 18, 2020
58b8e55
More debugging.
jenskeiner Dec 18, 2020
071da28
Debugging.
jenskeiner Dec 18, 2020
f39187a
Add back in nonce and state extraction.
jenskeiner Dec 18, 2020
8f669dc
Add back in call to authorization endpoint.
jenskeiner Dec 18, 2020
70e2407
Fix syntax error.
jenskeiner Dec 18, 2020
cd3cb32
Debugging.
jenskeiner Dec 18, 2020
ad1a527
Debugging.
jenskeiner Dec 18, 2020
3100ff7
Debugging.
jenskeiner Dec 18, 2020
3f3e447
Add back assembling of form submission request.
jenskeiner Dec 18, 2020
3d5596f
Send actual request to simulate form submission.
jenskeiner Dec 18, 2020
ec55584
Pass some parameters in URL, not request body.
jenskeiner Dec 18, 2020
7fac749
Set status first.
jenskeiner Dec 18, 2020
fe3493f
Minor fix.
jenskeiner Dec 18, 2020
930fd2d
Only print out Location header.
jenskeiner Dec 18, 2020
3572499
Add /authenticated endpoint to route.
jenskeiner Dec 18, 2020
636b518
Revert previous change.
jenskeiner Dec 18, 2020
3a93ce2
Use catch-all route.
jenskeiner Dec 18, 2020
c348143
Fix redirect URL.
jenskeiner Dec 18, 2020
bf948e8
Actually call redirect_uri.
jenskeiner Dec 18, 2020
5fe5fc2
Add other cookies.
jenskeiner Dec 18, 2020
926729b
Use redirect URI from Location header directly.
jenskeiner Dec 18, 2020
730ef07
Final redirect.
jenskeiner Dec 18, 2020
ce4d794
Debugging.
jenskeiner Dec 18, 2020
c17b359
Fix final URI. Update cookie.
jenskeiner Dec 18, 2020
431a0fe
Use /uri endpoint.
jenskeiner Dec 19, 2020
1e77d12
Start clean up.
jenskeiner Dec 21, 2020
78eddff
Continue clean up.
jenskeiner Dec 21, 2020
b98d609
Continue clean up.
jenskeiner Dec 21, 2020
76c613e
Continue clean up.
jenskeiner Dec 21, 2020
f0ce405
Continue clean up.
jenskeiner Dec 21, 2020
ec57b24
Fix stray end.
jenskeiner Dec 21, 2020
159672d
Make config parametrizable through NGINX variables.
jenskeiner Dec 21, 2020
ec8addd
Clean up test cases.
jenskeiner Dec 22, 2020
61cce4d
Fix test cases.
jenskeiner Dec 22, 2020
2afae7b
Fix test cases.
jenskeiner Dec 22, 2020
0f8a53d
Fix unit tests.
jenskeiner Dec 22, 2020
40bd353
Fix test cases.
jenskeiner Dec 22, 2020
8fb5bd3
Debugging.
jenskeiner Dec 22, 2020
d9bdcef
Take out another test for debugging.
jenskeiner Dec 22, 2020
eb36145
Add back case.
jenskeiner Dec 22, 2020
239f506
Fix test case.
jenskeiner Dec 22, 2020
8a43122
Add back another case.
jenskeiner Dec 22, 2020
0a8b046
Debugging.
jenskeiner Dec 22, 2020
61f6008
Debugging.
jenskeiner Dec 22, 2020
5e2ea0a
Debugging.
jenskeiner Dec 22, 2020
95cd514
Debuging.
jenskeiner Dec 22, 2020
3583c3a
Debugging.
jenskeiner Dec 22, 2020
1e7031f
Merge branch 'master_upstream' into 2764-oidc-configurable-headers
jenskeiner Dec 22, 2020
d12aa16
Add defaults check.
jenskeiner Dec 22, 2020
ba4260f
Re-enable all tests. Revert server.lua.
jenskeiner Dec 22, 2020
aed18dc
Clean up.
jenskeiner Dec 22, 2020
700d26c
Fix linting errors.
jenskeiner Dec 22, 2020
8bf1498
Clean up.
jenskeiner Dec 23, 2020
ae34c7f
Minor fixes.
jenskeiner Dec 26, 2020
d9ab5ba
Minor fix.
jenskeiner Dec 26, 2020
8ca9c8e
Fix test.
jenskeiner Dec 26, 2020
0e924d7
Remove duplicate test case.
jenskeiner Dec 28, 2020
18a188b
Check Authorization heade parse result.
jenskeiner Dec 28, 2020
5354eca
Fix stray return value.
jenskeiner Dec 28, 2020
207cfa1
Always try to extract access token from header if bearer_only flag is…
jenskeiner Dec 28, 2020
61ae9f3
Fix test cases.
jenskeiner Dec 28, 2020
d3b046b
Fix expected response header.
jenskeiner Dec 28, 2020
29a4904
Trivial change to re-start failed checks on GitHub.
jenskeiner Dec 28, 2020
307e07b
Move check into auxilliary function.
jenskeiner Dec 29, 2020
f51baf7
Update plugin documentation.
jenskeiner Jan 4, 2021
4ec11c6
Fix stray quotation mark.
jenskeiner Jan 4, 2021
6f93a1e
Merge branch 'master_upstream' into 2764-oidc-configurable-headers
jenskeiner Jan 4, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
136 changes: 115 additions & 21 deletions apisix/plugins/openid-connect.lua
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
-- See the License for the specific language governing permissions and
-- limitations under the License.
--
local string = string
local core = require("apisix.core")
local ngx_re = require("ngx.re")
local openidc = require("resty.openidc")
Expand All @@ -40,7 +41,31 @@ 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 = {
description = "Whether the access token should be added as a header to the request " ..
"for downstream",
type = "boolean",
default = true
},
set_userinfo_token_header = {
description = "Whether the user info token should be added in the X-Userinfo " ..
"header to the request for downstream.",
type = "boolean",
default = true
},
set_id_token_header = {
description = "Whether the ID token should be added in the X-ID-Token header to " ..
"the request for downstream.",
type = "boolean",
default = true
},
access_token_in_authorization_header = {
description = "Whether the access token should be added in the Authorization " ..
"header as opposed to the X-Access-Token header.",
type = "boolean",
default = false
}
},
required = {"client_id", "client_secret", "discovery"}
}
Expand Down Expand Up @@ -93,59 +118,124 @@ function _M.check_schema(conf)
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 set_header(ctx, name, value)
-- Set a request header to the given value and update the cached headers in the context as well.

-- Set header in request.
ngx.req.set_header(name, value)

-- Set header in cache, maybe.
if ctx and ctx.headers then
ctx.headers[name] = value
end
end


local function add_user_header(ctx, user)
local userinfo = core.json.encode(user)
set_header(ctx, "X-Userinfo", ngx_encode_base64(userinfo))
end


local function add_access_token_header(ctx, conf, token)
jenskeiner marked this conversation as resolved.
Show resolved Hide resolved
-- 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.
set_header(ctx, "Authorization", "Bearer " .. token)
end
else
if not core.request.header(ctx, "X-Access-Token") then
-- Add X-Access-Token header.
set_header(ctx, "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 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Token is invalid if has_token is false but conf.bearer_only is true

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(ctx, res)
end

-- Add configured access token header, maybe.
add_access_token_header(ctx, conf, token)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

return res
end
end
if conf.bearer_only then
-- If we get here, the token could not be validated, but we always require a valid
-- token in the request.
ngx.header["WWW-Authenticate"] = 'Bearer realm="' .. conf.realm
.. '",error="' .. err .. '"'
return ngx.HTTP_UNAUTHORIZED, err
end
end

-- Return nil to indicate that a token could not be extracted or validated, but that we don't
-- want to fail quickly.
return nil
end


local function add_user_header(user)
local userinfo = core.json.encode(user)
ngx.req.set_header("X-Userinfo", ngx_encode_base64(userinfo))
end


function _M.rewrite(plugin_conf, ctx)
local conf = core.table.clone(plugin_conf)
if not conf.redirect_uri then
Expand All @@ -163,28 +253,32 @@ function _M.rewrite(plugin_conf, ctx)
core.log.error("failed to introspect in openidc: ", err)
return response
end
if response then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove this part?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason is that adding the user info header is now handled in the introspect method. The header will still be added, if so configured, and introspection is done via the actual endpoint as opposed to just checking against the public key. Let me know if you see any problem with this approach.

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_user_header(response.user)
-- Add X-Userinfo header, maybe.
if response.user and conf.set_userinfo_token_header then
add_user_header(ctx, response.user)
end

-- Add configured access token header, maybe.
if response.access_token then
ngx.req.set_header("X-Access-Token", response.access_token)
add_access_token_header(ctx, conf, response.access_token)
end
if response.id_token then

-- Add X-ID-Token header, maybe.
if response.id_token and conf.set_id_token_header then
local token = core.json.encode(response.id_token)
ngx.req.set_header("X-ID-Token", ngx.encode_base64(token))
set_header(ctx, "X-ID-Token", ngx.encode_base64(token))
end
end
end
Expand Down
22 changes: 11 additions & 11 deletions t/plugin/openid-connect.t
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ done



=== TEST 4: add plugin
=== TEST 4: add plugin to route
--- config
location /t {
content_by_lua_block {
Expand All @@ -112,7 +112,7 @@ done
},
"upstream": {
"nodes": {
"127.0.0.1:1980": 1
"127.0.0.1:8888": 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

8888, Is it a valid "openid" port?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, I'm trying to add tests to check if the plugin sets the headers correctly as configured. I don't have a local development setup yet, so have to run the checks on this PR on GitHub to see if it's working.

I switched the upstream port to 8888 since that's where a mendhak/http-https-echo container is running. This one returns in the response body the full content of the incoming request as a JSON string. So I think I can check if the plugin has set the headers correctly by checking the response body. If the tests still work with the new port, I will try to add a case to do some actual verification of the headers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thanks for the link. It's more that I have to set up a fresh Linux VM or similar since I want that separate from the rest.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood, seems to be the better option. Will adjust the code.

},
"type": "roundrobin"
},
Expand All @@ -134,7 +134,7 @@ done
},
"upstream": {
"nodes": {
"127.0.0.1:1980": 1
"127.0.0.1:88": 1
},
"type": "roundrobin"
},
Expand All @@ -161,7 +161,7 @@ passed



=== TEST 5: access
=== TEST 5: access route w/o bearer token
--- config
location /t {
content_by_lua_block {
Expand Down Expand Up @@ -213,7 +213,7 @@ true
},
"upstream": {
"nodes": {
"127.0.0.1:1980": 1
"127.0.0.1:8888": 1
},
"type": "roundrobin"
},
Expand All @@ -236,7 +236,7 @@ true
},
"upstream": {
"nodes": {
"127.0.0.1:1980": 1
"127.0.0.1:8888": 1
},
"type": "roundrobin"
},
Expand All @@ -263,7 +263,7 @@ passed



=== TEST 7: access
=== TEST 7: access route w/o bearer token
--- timeout: 10s
--- request
GET /hello
Expand Down Expand Up @@ -302,7 +302,7 @@ WWW-Authenticate: Bearer realm=apisix
},
"upstream": {
"nodes": {
"127.0.0.1:1980": 1
"127.0.0.1:8888": 1
},
"type": "roundrobin"
},
Expand All @@ -329,7 +329,7 @@ WWW-Authenticate: Bearer realm=apisix
},
"upstream": {
"nodes": {
"127.0.0.1:1980": 1
"127.0.0.1:8888": 1
},
"type": "roundrobin"
},
Expand Down Expand Up @@ -443,7 +443,7 @@ jwt signature verification failed
},
"upstream": {
"nodes": {
"127.0.0.1:1980": 1
"127.0.0.1:8888": 1
},
"type": "roundrobin"
},
Expand All @@ -468,7 +468,7 @@ jwt signature verification failed
},
"upstream": {
"nodes": {
"127.0.0.1:1980": 1
"127.0.0.1:8888": 1
},
"type": "roundrobin"
},
Expand Down