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

jenskeiner · 2020-11-30T20:55:23Z

What this PR does / why we need it:

See #2880. The current implementation may lead to very large headers, thus bloating the request size to downstream substantially. This can be an issue by itself but is also not desired if the different tokens (access, ID, user info) largely contain the same information. Also, instead of using the X-Access-Token header it may be more appropriate to use the Authorization header for the access token, so that downstream plugins or services can extract it correctly; see e.g. authz-keycloak plugin. Finally, the plugin was so far only expecting an incoming access token in the Authorization header, but not the X-Access-Token header, which seemed somewhat inconsistent.

I've added options that can be configured for the openid-connect plugin that allow to control which headers get added and which header specifically shall be used for the access token. The defaults reproduce the current behavior. The plugin will also look in the X-Access-Token header, in addition to the Authorization header, for an access token in an incoming request.

Pre-submission checklist:

Did you explain what problem does this PR solve? Or what new features have been added?
Have you added corresponding test cases?
Haven't added any new cases for now. The headers set by the plugin were not tested, as far as I can tell. It may be a good idea to think about some new tests around here. That would likely require a bit of legwork to set up a dummy OIDC Identity Provider with all needed endpoints to go through all possible code paths though.
Have you modified the corresponding document?
I understand that the plugin documentation should be updated, but I want to get some feedback on the code changes first. Happy to adjust the documentation if the code changes get signed off.
Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

apisix/plugins/openid-connect.lua

spacewander · 2020-12-01T01:38:09Z

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


Why remove this part?

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.

apisix/plugins/openid-connect.lua

spacewander · 2020-12-01T01:43:15Z

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


Let's use the new way to set the default value:

apisix/apisix/plugins/cors.lua

Line 37 in 9dfe697

default = "*"

Sure, hadn't seen it before. Will change.

I've changed the scheme for the options that I've added. But have left the existing options untouched to not make this PR any larger.

spacewander · 2020-12-01T01:44:02Z

Please write new tests to cover your change, and make sure they are passed.

jenskeiner · 2020-12-01T10:15:25Z

Please write new tests to cover your change, and make sure they are passed.

I'm on it. I had a look at the existing structure and I'm relatively confident I can add more test cases to verify the setting of the respective headers.

However, one thing is still unclear to me: There are cases already where token introspection is performed against a configured identity provider endpoint. The cases were added by @sshniro here: 14f979e

Can anyone point me to the place where this introspection endpoint is set up for the tests? I can't find the corresponding code inside the project.

I may need to add more dummy endpoints to verify the authorization flow in case a bearer token is not present in the incoming request and I want to make that consistent with the existing dummy introspection endpoint.

…the header cache in the context as well.

spacewander · 2020-12-02T01:26:47Z

apisix/plugins/openid-connect.lua

+                -- Token is valid.
+
+                -- Add configured access token header, maybe.
+                add_access_token_header(ctx, conf, token)


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

spacewander · 2020-12-02T01:26:56Z

apisix/plugins/openid-connect.lua

+                end
+
+                -- Add configured access token header, maybe.
+                add_access_token_header(ctx, conf, token)


sshniro · 2020-12-02T09:41:14Z

Hi @jenskeiner for your question on the introspection endpoint test case, I have used the Keycloak to test this scenario.

The code link to setup Keycloak:

apisix/.travis/linux_openresty_runner.sh

Line 27 in f84a44e

    
           docker run --rm -itd -e KEYCLOAK_USER=admin -e KEYCLOAK_PASSWORD=123456 -p 8090:8080 -p 8443:8443 sshniro/keycloak-apisix

Test cases written for the introspection with Keyclaok:

apisix/t/plugin/openid-connect.t

Line 424 in f84a44e

=== TEST 11: Update route with keycloak config for introspection

@membphis the docker image is using my personal docker repo, if needed I can migrate it to an official APISIX docker repository.

membphis · 2020-12-02T14:53:13Z

@membphis the docker image is using my personal docker repo, if needed I can migrate it to an official APISIX docker repository.

seems good for me.

what do you think? @moonming

moonming · 2020-12-03T06:02:04Z

@membphis the docker image is using my personal docker repo, if needed I can migrate it to an official APISIX docker repository.

seems good for me.

what do you think? @moonming

we can add keycloak-apisix docker to CI, and I am not sure if it is appropriate to migrate it to an official APISIX docker repository, most of APISIX users don't need keycloak.

…ttp-https-echo container which returns all headers in request.

membphis · 2020-12-16T11:24:19Z

t/plugin/openid-connect.t

@@ -112,7 +112,7 @@ done
                        },
                        "upstream": {
                            "nodes": {
-                                "127.0.0.1:1980": 1
+                                "127.0.0.1:8888": 1


8888, Is it a valid "openid" port?

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.

@jenskeiner
You can setup the test environment via https://github.com/apache/apisix/blob/master/.travis/linux_openresty_runner.sh

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.

And you can write your echo-back here: https://github.com/apache/apisix/blob/master/t/lib/server.lua

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

…onse.

…der gets also sent to downstream in X-Access-Token header (default), or not, if so configured.

jenskeiner · 2020-12-26T15:40:14Z

@spacewander @membphis @moonming Sorry for the noise, but took a while to set up test cases that go through the full OIDC Relying Party flow which can be used when the incoming request doesn't contain a bearer token yet.

Please give this another good review and let me know any issues. Thanks.

I know, I still need to update the documentation, but will wait on feedback regarding code changes first.

jenskeiner · 2020-12-26T15:52:37Z

The code changes in this PR are basically just to solve some issues I was seeing when combining this plugin with the auth-keycloak one. I wanted to make sure that the latter can extract the access token from the request, even if it is contained in a session cookie or potentially some other server-side storage as supported by the openidc module. The changes make sure that the token can always be added in a header where authz-keycloak expects it. I also wanted to reduce some noise from the ID token and the user info that were getting added in separate headers because you probably don't need those always and they make the HTTP header even larger.

Generally, I think the interplay between this plugin and auth-keycloak should be re-considered. Messing with the request headers could be avoided completely if auth-keycloak could leverage the openidc module to extract the access token from the request (from a header or cookie or...) itself using a method that is currently internal to that module. Then, it wouldn't need to rely on the token being stored in a header and code to extract the token could be removed in favor of using the openidc module's code for that.

I'm just wondering what anyone's thoughts on this are.

The current solution with the headers works for me and I'll look at authz-keycloak next to start the work on lazy path loading from Keycloak. But longer term, would be better to clean up the code some more and not rely on the openid-connect plugin to fix up the headers for the auth-keycloak plugin. Thoughts and feedback welcome.

@sshniro Maybe you have thoughts on this as well...

t/plugin/openid-connect.t

apisix/plugins/openid-connect.lua

… set.

spacewander · 2020-12-30T01:17:25Z

Fantasy! Before we can merge it, @jenskeiner please write some English docs.

jenskeiner · 2020-12-30T18:54:48Z

Fantasy! Before we can merge it, @jenskeiner please write some English docs.

Yep, will do, may take until next Monday though. In the meantime, let me know any worries.

Bitte geben Sie eine Commit-Beschreibung für Ihre Änderungen ein. Zeilen,

jenskeiner · 2021-01-04T10:37:10Z

@spacewander Updated the documentation now as well. Can you give it another good review to ensure it's consistent and clear enough?

spacewander · 2021-01-05T02:14:52Z

@jenskeiner
Merged. Thanks!

jenskeiner added 3 commits November 30, 2020 21:44

Make headers to add to request configurable.

4af92db

Fix lines that were too long.

fc57f35

Import global string module.

49e904f

jenskeiner changed the title ~~feat: Make headers to add to request configurable.~~ feat: Make headers to add to request in openid-connect plugin configurable. Nov 30, 2020

spacewander requested changes Dec 1, 2020

View reviewed changes

jenskeiner added 3 commits December 1, 2020 09:13

Fix an issue when trying to call a boolean value.

4c033d9

Addressing several pull request comments.

e997baa

Remove old way of setting defaults for added configuration options.

fbca5d8

jenskeiner added 5 commits December 1, 2020 11:18

Merge branch 'master' into 2764-oidc-configurable-headers

b842247

Fix lines that are too long.

8eabdef

Remove trailing whitespace.

6328c68

Add auxilliary method to insert/update request header while updating …

bfa4aa7

…the header cache in the context as well.

Small fixes.

1aeec0b

spacewander requested changes Dec 2, 2020

View reviewed changes

spacewander mentioned this pull request Dec 10, 2020

request help: About OIDC plugin #3014

Closed

jenskeiner added 2 commits December 16, 2020 11:22

Merge branch 'master_upstream' into 2764-oidc-configurable-headers

eb73cee

CHange backend from 127.0.0.1:1980 to 127.0.0.1:8888 to use mendhak/h…

f13fc9d

…ttp-https-echo container which returns all headers in request.

membphis reviewed Dec 16, 2020

View reviewed changes

jenskeiner added 7 commits December 16, 2020 12:24

Fix port number.

f088a88

Revert local port back to 1980.

a4d503d

Add endpoint that reflects OIDC-relevant request headers back in resp…

76c6b8b

…onse.

Add test cases to check if request bearer token in Authoriztation hea…

ade0bec

…der gets also sent to downstream in X-Access-Token header (default), or not, if so configured.

Fix issue when not all desired headers were reflected.

19c62a7

Fix header verification tests.

84c8684

Fix test cases.

4ce30ac

jenskeiner added 6 commits December 22, 2020 23:11

Clean up.

aed18dc

Fix linting errors.

700d26c

Clean up.

8bf1498

Minor fixes.

ae34c7f

Minor fix.

d9ab5ba

Fix test.

8ca9c8e

spacewander requested changes Dec 27, 2020

View reviewed changes

t/plugin/openid-connect.t Outdated Show resolved Hide resolved

apisix/plugins/openid-connect.lua Show resolved Hide resolved

apisix/plugins/openid-connect.lua Outdated Show resolved Hide resolved

spacewander added this to the 2.3 milestone Dec 27, 2020

jenskeiner added 7 commits December 28, 2020 10:53

Remove duplicate test case.

0e924d7

Check Authorization heade parse result.

18a188b

Fix stray return value.

5354eca

Always try to extract access token from header if bearer_only flag is…

207cfa1

… set.

Fix test cases.

61ae9f3

Fix expected response header.

d3b046b

Trivial change to re-start failed checks on GitHub.

29a4904

sshniro approved these changes Dec 28, 2020

View reviewed changes

Move check into auxilliary function.

307e07b

jenskeiner requested a review from spacewander December 29, 2020 14:52

spacewander approved these changes Dec 30, 2020

View reviewed changes

jenskeiner added 2 commits January 4, 2021 11:33

Update plugin documentation.

f51baf7

Fix stray quotation mark.

4ec11c6

Bitte geben Sie eine Commit-Beschreibung für Ihre Änderungen ein. Zeilen,

Merge branch 'master_upstream' into 2764-oidc-configurable-headers

6f93a1e

spacewander merged commit 67df073 into apache:master Jan 5, 2021

spacewander mentioned this pull request Jan 5, 2021

feat: Make HTTP headers set by openid-connect plugin more configurable. #2880

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jenskeiner commented Nov 30, 2020 •

edited

Loading

spacewander Dec 1, 2020

jenskeiner Dec 1, 2020

spacewander Dec 1, 2020

jenskeiner Dec 1, 2020

jenskeiner Dec 1, 2020

spacewander commented Dec 1, 2020

jenskeiner commented Dec 1, 2020

spacewander Dec 2, 2020

spacewander Dec 2, 2020

sshniro commented Dec 2, 2020

membphis commented Dec 2, 2020

moonming commented Dec 3, 2020 •

edited

Loading

membphis Dec 16, 2020

jenskeiner Dec 16, 2020

spacewander Dec 16, 2020

jenskeiner Dec 16, 2020

spacewander Dec 16, 2020

jenskeiner Dec 16, 2020

jenskeiner commented Dec 26, 2020

jenskeiner commented Dec 26, 2020

spacewander commented Dec 30, 2020

jenskeiner commented Dec 30, 2020

jenskeiner commented Jan 4, 2021

spacewander commented Jan 5, 2021

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

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

Conversation

jenskeiner commented Nov 30, 2020 • edited Loading

What this PR does / why we need it:

Pre-submission checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spacewander commented Dec 1, 2020

jenskeiner commented Dec 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sshniro commented Dec 2, 2020

membphis commented Dec 2, 2020

moonming commented Dec 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenskeiner commented Dec 26, 2020

jenskeiner commented Dec 26, 2020

spacewander commented Dec 30, 2020

jenskeiner commented Dec 30, 2020

jenskeiner commented Jan 4, 2021

spacewander commented Jan 5, 2021

jenskeiner commented Nov 30, 2020 •

edited

Loading

moonming commented Dec 3, 2020 •

edited

Loading