fix(openid-connect): the default 'redirect_uri' (#2426) by liweitianux · Pull Request #7690 · apache/apisix

liweitianux · 2022-08-16T04:00:25Z

Description

Previously the redirect_uri was set to ngx.var.request_uri if not
configured. However, it caused the underlying lua-resty-openidc
module to raise this error:

request to the redirect_uri path but there's no session state found

because lua-resty-openidc would think it was the redirection response
from OP when the redirect_uri equals ngx.var.request_uri.

Although the OAuth 2.0 Security Best Current Practice [1] recommends
that the redirect_uri should be explicitly specified to prevent
malicious redirection attacks, it would also be handy for APISIX to
properly determine a default one if redirect_uri not given.

Therefore, append the .apisix/redirect suffix to the current request
URI to determine the default redirect_uri. It makes
lua-resty-openidc happy and it's almost unlikely to conflict with
user's URIs.

Also note that the OP should be properly configured to accept such
auto-determined redirect URIs.

Update the documentation accordingly.

Fix #2426.

[1] https://datatracker.ietf.org/doc/draft-ietf-oauth-security-topics/

Checklist

I have explained the need for this PR and the problem it solves
I have explained the changes or the new features added to this PR
I have added tests corresponding to this change
I have updated the documentation to reflect this change
I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

Previously the `redirect_uri` was set to `ngx.var.request_uri` if not configured. However, it caused the underlying `lua-resty-openidc` module to raise this error: ``` request to the redirect_uri path but there's no session state found ``` because `lua-resty-openidc` would think it was the redirection response from OP when the `redirect_uri` equals `ngx.var.request_uri`. Although the OAuth 2.0 Security Best Current Practice [1] recommends that the `redirect_uri` should be explicitly specified to prevent malicious redirection attacks, it would also be handy for APISIX to properly determine a default one if `redirect_uri` not given. Therefore, append the `.apisix/redirect` suffix to the current request URI to determine the default `redirect_uri`. It makes `lua-resty-openidc` happy and it's almost unlikely to conflict with user's URIs. Also note that the OP should be properly configured to accept such auto-determined redirect URIs. Update the documentation accordingly. Fix apache#2426. [1] https://datatracker.ietf.org/doc/draft-ietf-oauth-security-topics/

spacewander · 2022-08-16T06:11:32Z


 - `redirect_uri` needs to be captured by the route where the current APISIX is located. For example, the `uri` of the current route is `/api/v1/*`, `redirect_uri` can be filled in as `/api/v1/callback`;
 - `scheme` and `host` of `redirect_uri` (`scheme:host`) are the values required to access APISIX from the perspective of the identity provider.
+- See also [this GitHub issue](https://github.com/apache/apisix/issues/2426), especially these comments: [@starsz](https://github.com/apache/apisix/issues/2426#issuecomment-1091021687), [@david-woelfle](https://github.com/apache/apisix/issues/2426#issuecomment-1090675455), [@liweitianux (1)](https://github.com/apache/apisix/issues/2426#issuecomment-1206107085), [@liweitianux (2)](https://github.com/apache/apisix/issues/2426#issuecomment-1207423283)


Could you write the conclusion in a short sentence, instead of multiple reference links?

Suggested change

- See also [this GitHub issue](https://github.com/apache/apisix/issues/2426), especially these comments: [@starsz](https://github.com/apache/apisix/issues/2426#issuecomment-1091021687), [@david-woelfle](https://github.com/apache/apisix/issues/2426#issuecomment-1090675455), [@liweitianux (1)](https://github.com/apache/apisix/issues/2426#issuecomment-1206107085), [@liweitianux (2)](https://github.com/apache/apisix/issues/2426#issuecomment-1207423283)

- `redirect_uri` should not be the same as the URI of the route. This is because when a user initiates a request to visit the protected resource, the request directly hits the redirection URI with no session cookie in the request, which leads to the `no session state found` error.

tokers · 2022-08-16T09:10:39Z

        redirect_uri = {
            type = "string",
-            description = "use ngx.var.request_uri if not configured"
+            description = "auto append '.apisix/redirect' if not configured"


But it cannot make sure the new URI will be matched by the same route, it actually adds an implicit condition that the route match type is prefix instead of exact.

Well. If the route is of type exact, then it cannot actually use this openid-connect plugin, because the redirect_uri must be different from the request URI, as required by lua-resty-openidc.

@liweitianux Could you write some hints about this in the OpenID Connect doc?

Sure. I'll update the doc later.

starsz

Hi @liweitianux .Can you add a test case to cover it ?

tzssangglass · 2022-08-17T03:07:19Z

+        -- NOTE: 'lua-resty-openidc' requires that 'redirect_uri' be
+        --       different from 'uri'.  So default to append the
+        --       '.apisix/redirect' suffix if not configured.
+        local suffix = "/.apisix/redirect"
+        local uri = ctx.var.uri
+        if core.string.has_suffix(uri, suffix) then
+            -- This is the redirection response from the OIDC provider.
+            conf.redirect_uri = uri
+        else
+            if string.sub(uri, -1, -1) == "/" then
+                conf.redirect_uri = string.sub(uri, 1, -2) .. suffix
+            else
+                conf.redirect_uri = uri .. suffix
+            end
+        end
+        core.log.debug("auto set redirect_uri: ", conf.redirect_uri)


When there is no conf.redirect_uri, it looks like we generate redirect_uri for each request(except for the real redirect from IdP).

Can we inject a default value for redirect_uri when we add the openid-connect plugin.

For example:

we require that the match condition for a road has to be a prefix match, like /openid/*, then redirect_uri defaults to /openid/.apisix/redirect.

Listen to what others say.

@liweitianux any thoughts about this?

I don't think we can set a proper default value for redirect_uri, because a plugin can be configured globally, per-route, and per-service. An exception is that the openid-connect plugin is configured for a specific route, so we can determine a redirect_uri that's covered by the route.

Please correct me if I'm mistaken.

I agree with you.

We need a test case to show that not providing redirect_uri works fine.

I changed the job this year, and I haven't been working with APISIX for more than one year now. I'm unfamiliar with the test framework, and that was the main reason that this PR was lacking a test case. It would be great if you can rejuvenate this PR. Thanks.

github-actions · 2022-10-17T11:10:09Z

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the [email protected] list. Thank you for your contributions.

github-actions · 2022-11-18T10:04:44Z

This pull request/issue has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

juststillthinking · 2023-11-21T03:40:51Z

Please resolve the conflicts and make the ci pass

luoluoyuyu · 2023-12-03T07:01:53Z

Hi @liweitianux @shreemaan-abhishek @monkeyDluffy6017
Can I perfect this PR work?

shreemaan-abhishek · 2023-12-03T07:20:27Z

@luoluoyuyu sure.

luoluoyuyu · 2023-12-03T13:35:40Z

@shreemaan-abhishek
It seems I don't have permission to add commits. @liweitianux can you open the permission。

shreemaan-abhishek · 2023-12-03T15:20:34Z

@liweitianux could you invite @luoluoyuyu with access to your repo?

liweitianux · 2023-12-04T01:00:48Z

@liweitianux could you invite @luoluoyuyu with access to your repo?

Hi, @luoluoyuyu and @shreemaan-abhishek, I've invited both of you to my repo. Thank you.

Vacant2333 · 2023-12-04T02:14:55Z

+            local httpc = http.new()
+            local uri = "http://127.0.0.1:" .. ngx.var.server_port .. "/hello"
+            local res, err = httpc:request_uri(uri, {method = "GET"})
+            ngx.say(res.status)


Redundant code

Vacant2333 · 2023-12-04T02:15:26Z

+            local res, err = httpc:request_uri(uri, {method = "GET"})
+            ngx.say(res.status)
+            if res.status == 400 then
+                ngx.say(res.status)


the response_body was true

spacewander reviewed Aug 16, 2022

View reviewed changes

spacewander requested a review from starsz August 16, 2022 06:14

tokers reviewed Aug 16, 2022

View reviewed changes

starsz reviewed Aug 17, 2022

View reviewed changes

tzssangglass reviewed Aug 17, 2022

View reviewed changes

github-actions Bot added the stale label Oct 17, 2022

github-actions Bot closed this Nov 18, 2022

shreemaan-abhishek reopened this Nov 14, 2023

github-actions Bot removed the stale label Nov 14, 2023

shreemaan-abhishek added the need test cases label Nov 17, 2023

Sn0rt approved these changes Nov 21, 2023

View reviewed changes

Merge branch 'master' into openid-connect/redirect-uri

4af8303

kayx23 reviewed Nov 28, 2023

View reviewed changes

Comment thread docs/zh/latest/plugins/openid-connect.md Outdated

fix for doc CI

75abdd0

kayx23 requested a review from juststillthinking November 29, 2023 00:20

juststillthinking changed the title ~~fix(openid-connect): Fix the default 'redirect_uri' (#2426)~~ fix(openid-connect): the default 'redirect_uri' (#2426) Nov 29, 2023

shreemaan-abhishek self-assigned this Dec 1, 2023

shreemaan-abhishek assigned luoluoyuyu and unassigned shreemaan-abhishek Dec 3, 2023

github-actions Bot added the user responded label Dec 4, 2023

add test cases

02b0707

Vacant2333 suggested changes Dec 4, 2023

View reviewed changes

luoluoyuyu added 2 commits December 4, 2023 14:14

fix test case

d4f158a

fix test style

9d11f55

luoluoyuyu mentioned this pull request Dec 20, 2023

bug：enable openid-connect plugin without redirect_uri got 500 error #2426

Closed

5 tasks

luoluoyuyu added 2 commits December 20, 2023 16:18

re-add test cases

46ecafd

add error_code test

af8d34b

juststillthinking reviewed Dec 21, 2023

View reviewed changes

Comment thread apisix/plugins/openid-connect.lua Outdated

add redirect_uri description information

b375f9c

juststillthinking approved these changes Dec 21, 2023

View reviewed changes

juststillthinking added approved and removed need test cases user responded labels Dec 21, 2023

juststillthinking requested a review from moonming December 21, 2023 12:29

moonming approved these changes Dec 22, 2023

View reviewed changes

moonming merged commit c7d406d into apache:master Dec 22, 2023

Uh oh!

Conversation

liweitianux commented Aug 16, 2022

Description

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liweitianux Aug 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

starsz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Oct 17, 2022

Uh oh!

github-actions Bot commented Nov 18, 2022

Uh oh!

juststillthinking commented Nov 21, 2023

Uh oh!

Uh oh!

luoluoyuyu commented Dec 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shreemaan-abhishek commented Dec 3, 2023

Uh oh!

luoluoyuyu commented Dec 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shreemaan-abhishek commented Dec 3, 2023

Uh oh!

liweitianux commented Dec 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

liweitianux Aug 16, 2022 •

edited

Loading

luoluoyuyu commented Dec 3, 2023 •

edited

Loading

luoluoyuyu commented Dec 3, 2023 •

edited

Loading

liweitianux commented Dec 4, 2023 •

edited

Loading