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 2022/08/16 04:00:27 UTC
[GitHub] [apisix] liweitianux opened a new pull request, #7690: fix(openid-connect): Fix the default 'redirect_uri' (#2426)
liweitianux opened a new pull request, #7690:
URL: https://github.com/apache/apisix/pull/7690
### 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
- [x] I have explained the need for this PR and the problem it solves
- [x] I have explained the changes or the new features added to this PR
- [ ] I have added tests corresponding to this change
- [x] 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](https://github.com/apache/apisix/tree/master#community) first)
<!--
Note
1. Mark the PR as draft until it's ready to be reviewed.
2. Always add/update tests for any changes unless you have a good reason.
3. Always update the documentation to reflect the changes made in the PR.
4. Make a new commit to resolve conversations instead of `push -f`.
5. To resolve merge conflicts, merge master instead of rebasing.
6. Use "request review" to notify the reviewer after making changes.
7. Only a reviewer can mark a conversation as resolved.
-->
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] github-actions[bot] closed pull request #7690: fix(openid-connect): Fix the default 'redirect_uri' (#2426)
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #7690: fix(openid-connect): Fix the default 'redirect_uri' (#2426)
URL: https://github.com/apache/apisix/pull/7690
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(openid-connect): the default 'redirect_uri' (#2426) [apisix]
Posted by "moonming (via GitHub)" <gi...@apache.org>.
moonming merged PR #7690:
URL: https://github.com/apache/apisix/pull/7690
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers commented on a diff in pull request #7690: fix(openid-connect): Fix the default 'redirect_uri' (#2426)
Posted by GitBox <gi...@apache.org>.
tokers commented on code in PR #7690:
URL: https://github.com/apache/apisix/pull/7690#discussion_r947686734
##########
apisix/plugins/openid-connect.lua:
##########
@@ -65,7 +65,7 @@ local schema = {
},
redirect_uri = {
type = "string",
- description = "use ngx.var.request_uri if not configured"
+ description = "auto append '.apisix/redirect' if not configured"
Review Comment:
@liweitianux Could you write some hints about this in the OpenID Connect 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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] liweitianux commented on a diff in pull request #7690: fix(openid-connect): Fix the default 'redirect_uri' (#2426)
Posted by GitBox <gi...@apache.org>.
liweitianux commented on code in PR #7690:
URL: https://github.com/apache/apisix/pull/7690#discussion_r946545804
##########
apisix/plugins/openid-connect.lua:
##########
@@ -65,7 +65,7 @@ local schema = {
},
redirect_uri = {
type = "string",
- description = "use ngx.var.request_uri if not configured"
+ description = "auto append '.apisix/redirect' if not configured"
Review Comment:
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.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(openid-connect): the default 'redirect_uri' (#2426) [apisix]
Posted by "luoluoyuyu (via GitHub)" <gi...@apache.org>.
luoluoyuyu commented on code in PR #7690:
URL: https://github.com/apache/apisix/pull/7690#discussion_r1433631810
##########
apisix/plugins/openid-connect.lua:
##########
@@ -86,7 +86,7 @@ local schema = {
},
redirect_uri = {
type = "string",
- description = "use ngx.var.request_uri if not configured"
+ description = "auto append '.apisix/redirect' if not configured"
Review Comment:
> 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`.
I think this explanation is correct
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(openid-connect): the default 'redirect_uri' (#2426) [apisix]
Posted by "luoluoyuyu (via GitHub)" <gi...@apache.org>.
luoluoyuyu commented on code in PR #7690:
URL: https://github.com/apache/apisix/pull/7690#discussion_r1433631810
##########
apisix/plugins/openid-connect.lua:
##########
@@ -86,7 +86,7 @@ local schema = {
},
redirect_uri = {
type = "string",
- description = "use ngx.var.request_uri if not configured"
+ description = "auto append '.apisix/redirect' if not configured"
Review Comment:
> 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`.
I think this explanation is correct
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] github-actions[bot] commented on pull request #7690: fix(openid-connect): Fix the default 'redirect_uri' (#2426)
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #7690:
URL: https://github.com/apache/apisix/pull/7690#issuecomment-1319788361
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.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(openid-connect): the default 'redirect_uri' (#2426) [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on PR #7690:
URL: https://github.com/apache/apisix/pull/7690#issuecomment-1837394080
@luoluoyuyu 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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(openid-connect): Fix the default 'redirect_uri' (#2426) [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on code in PR #7690:
URL: https://github.com/apache/apisix/pull/7690#discussion_r1396149966
##########
apisix/plugins/openid-connect.lua:
##########
@@ -263,7 +263,22 @@ function _M.rewrite(plugin_conf, ctx)
end
if not conf.redirect_uri then
- conf.redirect_uri = ctx.var.request_uri
+ -- 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)
Review Comment:
@liweitianux any thoughts about this?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(openid-connect): Fix the default 'redirect_uri' (#2426) [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on code in PR #7690:
URL: https://github.com/apache/apisix/pull/7690#discussion_r1396966773
##########
apisix/plugins/openid-connect.lua:
##########
@@ -263,7 +263,22 @@ function _M.rewrite(plugin_conf, ctx)
end
if not conf.redirect_uri then
- conf.redirect_uri = ctx.var.request_uri
+ -- 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)
Review Comment:
We need a test case to show that not providing `redirect_uri` works fine.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(openid-connect): Fix the default 'redirect_uri' (#2426) [apisix]
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on PR #7690:
URL: https://github.com/apache/apisix/pull/7690#issuecomment-1820164142
Please resolve the conflicts and make the ci pass
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(openid-connect): the default 'redirect_uri' (#2426) [apisix]
Posted by "liweitianux (via GitHub)" <gi...@apache.org>.
liweitianux commented on PR #7690:
URL: https://github.com/apache/apisix/pull/7690#issuecomment-1837678235
> @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.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(openid-connect): the default 'redirect_uri' (#2426) [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on PR #7690:
URL: https://github.com/apache/apisix/pull/7690#issuecomment-1837514105
@liweitianux could you invite @luoluoyuyu with access to your repo?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(openid-connect): Fix the default 'redirect_uri' (#2426) [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on code in PR #7690:
URL: https://github.com/apache/apisix/pull/7690#discussion_r1396965449
##########
apisix/plugins/openid-connect.lua:
##########
@@ -263,7 +263,22 @@ function _M.rewrite(plugin_conf, ctx)
end
if not conf.redirect_uri then
- conf.redirect_uri = ctx.var.request_uri
+ -- 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)
Review Comment:
I agree with you.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(openid-connect): Fix the default 'redirect_uri' (#2426) [apisix]
Posted by "kayx23 (via GitHub)" <gi...@apache.org>.
kayx23 commented on code in PR #7690:
URL: https://github.com/apache/apisix/pull/7690#discussion_r1408700480
##########
docs/en/latest/plugins/openid-connect.md:
##########
@@ -198,3 +198,4 @@ In this example, the Plugin can enforce that the access token, the ID token, and
- `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)
Review Comment:
```suggestion
- `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.
```
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(openid-connect): the default 'redirect_uri' (#2426) [apisix]
Posted by "luoluoyuyu (via GitHub)" <gi...@apache.org>.
luoluoyuyu commented on PR #7690:
URL: https://github.com/apache/apisix/pull/7690#issuecomment-1837390405
Hi @shreemaan-abhishek @monkeyDluffy6017
Can I perfect this PR work?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers commented on a diff in pull request #7690: fix(openid-connect): Fix the default 'redirect_uri' (#2426)
Posted by GitBox <gi...@apache.org>.
tokers commented on code in PR #7690:
URL: https://github.com/apache/apisix/pull/7690#discussion_r946525140
##########
apisix/plugins/openid-connect.lua:
##########
@@ -65,7 +65,7 @@ local schema = {
},
redirect_uri = {
type = "string",
- description = "use ngx.var.request_uri if not configured"
+ description = "auto append '.apisix/redirect' if not configured"
Review Comment:
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.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] github-actions[bot] commented on pull request #7690: fix(openid-connect): Fix the default 'redirect_uri' (#2426)
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #7690:
URL: https://github.com/apache/apisix/pull/7690#issuecomment-1280690335
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 dev@apisix.apache.org list. Thank you for your contributions.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(openid-connect): Fix the default 'redirect_uri' (#2426) [apisix]
Posted by "liweitianux (via GitHub)" <gi...@apache.org>.
liweitianux commented on code in PR #7690:
URL: https://github.com/apache/apisix/pull/7690#discussion_r1396863414
##########
apisix/plugins/openid-connect.lua:
##########
@@ -263,7 +263,22 @@ function _M.rewrite(plugin_conf, ctx)
end
if not conf.redirect_uri then
- conf.redirect_uri = ctx.var.request_uri
+ -- 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)
Review Comment:
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.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(openid-connect): the default 'redirect_uri' (#2426) [apisix]
Posted by "Vacant2333 (via GitHub)" <gi...@apache.org>.
Vacant2333 commented on code in PR #7690:
URL: https://github.com/apache/apisix/pull/7690#discussion_r1413298230
##########
t/plugin/openid-connect2.t:
##########
@@ -334,3 +334,58 @@ passed
--- timeout: 10s
--- response_body
true
+
+
+=== TEST 9: Set up route with plugin matching URI `/hello` with redirect_uri use default value.
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "plugins": {
+ "openid-connect": {
+ "client_id": "kbyuFDidLLm280LIwVFiazOqjO3ty8KH",
+ "client_secret": "60Op4HFM0I8ajz0WdiStAbziZ-VFQttXuxixHHs2R7r7-CW8GR79l-mmLqMhc-Sa",
+ "discovery": "http://127.0.0.1:1980/.well-known/openid-configuration",
+ "scope": "openid profile",
+ "bearer_only": false
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]]
+ )
+ if code == 200 then
+ ngx.say(true)
+ end
+ }
+ }
+--- response_body
+true
+
+
+
+=== TEST 10: OpenID server does not have a corresponding redirect_uri, should return 400.
+--- config
+ location /t {
+ content_by_lua_block {
+ local http = require "resty.http"
+ 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)
Review Comment:
Redundant code
##########
t/plugin/openid-connect2.t:
##########
@@ -334,3 +334,58 @@ passed
--- timeout: 10s
--- response_body
true
+
+
+=== TEST 9: Set up route with plugin matching URI `/hello` with redirect_uri use default value.
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "plugins": {
+ "openid-connect": {
+ "client_id": "kbyuFDidLLm280LIwVFiazOqjO3ty8KH",
+ "client_secret": "60Op4HFM0I8ajz0WdiStAbziZ-VFQttXuxixHHs2R7r7-CW8GR79l-mmLqMhc-Sa",
+ "discovery": "http://127.0.0.1:1980/.well-known/openid-configuration",
+ "scope": "openid profile",
+ "bearer_only": false
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]]
+ )
+ if code == 200 then
+ ngx.say(true)
+ end
+ }
+ }
+--- response_body
+true
+
+
+
+=== TEST 10: OpenID server does not have a corresponding redirect_uri, should return 400.
+--- config
+ location /t {
+ content_by_lua_block {
+ local http = require "resty.http"
+ 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)
+ if res.status == 400 then
+ ngx.say(res.status)
Review Comment:
the response_body was `true`
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] liweitianux commented on a diff in pull request #7690: fix(openid-connect): Fix the default 'redirect_uri' (#2426)
Posted by GitBox <gi...@apache.org>.
liweitianux commented on code in PR #7690:
URL: https://github.com/apache/apisix/pull/7690#discussion_r946545804
##########
apisix/plugins/openid-connect.lua:
##########
@@ -65,7 +65,7 @@ local schema = {
},
redirect_uri = {
type = "string",
- description = "use ngx.var.request_uri if not configured"
+ description = "auto append '.apisix/redirect' if not configured"
Review Comment:
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`.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(openid-connect): the default 'redirect_uri' (#2426) [apisix]
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #7690:
URL: https://github.com/apache/apisix/pull/7690#discussion_r1433616858
##########
apisix/plugins/openid-connect.lua:
##########
@@ -86,7 +86,7 @@ local schema = {
},
redirect_uri = {
type = "string",
- description = "use ngx.var.request_uri if not configured"
+ description = "auto append '.apisix/redirect' if not configured"
Review Comment:
Could you clarify what you will append `'.apisix/redirect` to ? `ngx.var.uri`?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(openid-connect): the default 'redirect_uri' (#2426) [apisix]
Posted by "luoluoyuyu (via GitHub)" <gi...@apache.org>.
luoluoyuyu commented on PR #7690:
URL: https://github.com/apache/apisix/pull/7690#issuecomment-1837482781
@shreemaan-abhishek
It seems I don't have permission to add commits. i may need to submit a new PR.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(openid-connect): Fix the default 'redirect_uri' (#2426) [apisix]
Posted by "liweitianux (via GitHub)" <gi...@apache.org>.
liweitianux commented on code in PR #7690:
URL: https://github.com/apache/apisix/pull/7690#discussion_r1396994373
##########
apisix/plugins/openid-connect.lua:
##########
@@ -263,7 +263,22 @@ function _M.rewrite(plugin_conf, ctx)
end
if not conf.redirect_uri then
- conf.redirect_uri = ctx.var.request_uri
+ -- 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)
Review Comment:
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.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #7690: fix(openid-connect): Fix the default 'redirect_uri' (#2426)
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7690:
URL: https://github.com/apache/apisix/pull/7690#discussion_r946371484
##########
docs/en/latest/plugins/openid-connect.md:
##########
@@ -198,3 +198,4 @@ In this example, the Plugin can enforce that the access token, the ID token, and
- `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)
Review Comment:
Could you write the conclusion in a short sentence, instead of multiple reference links?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on a diff in pull request #7690: fix(openid-connect): Fix the default 'redirect_uri' (#2426)
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #7690:
URL: https://github.com/apache/apisix/pull/7690#discussion_r947408785
##########
apisix/plugins/openid-connect.lua:
##########
@@ -263,7 +263,22 @@ function _M.rewrite(plugin_conf, ctx)
end
if not conf.redirect_uri then
- conf.redirect_uri = ctx.var.request_uri
+ -- 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)
Review Comment:
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:
1. 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.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] liweitianux commented on a diff in pull request #7690: fix(openid-connect): Fix the default 'redirect_uri' (#2426)
Posted by GitBox <gi...@apache.org>.
liweitianux commented on code in PR #7690:
URL: https://github.com/apache/apisix/pull/7690#discussion_r948639130
##########
apisix/plugins/openid-connect.lua:
##########
@@ -65,7 +65,7 @@ local schema = {
},
redirect_uri = {
type = "string",
- description = "use ngx.var.request_uri if not configured"
+ description = "auto append '.apisix/redirect' if not configured"
Review Comment:
Sure. I'll update the doc later.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(openid-connect): the default 'redirect_uri' (#2426) [apisix]
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #7690:
URL: https://github.com/apache/apisix/pull/7690#discussion_r1433616858
##########
apisix/plugins/openid-connect.lua:
##########
@@ -86,7 +86,7 @@ local schema = {
},
redirect_uri = {
type = "string",
- description = "use ngx.var.request_uri if not configured"
+ description = "auto append '.apisix/redirect' if not configured"
Review Comment:
Could you clarify what you will append `'.apisix/redirect` to ?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(openid-connect): Fix the default 'redirect_uri' (#2426) [apisix]
Posted by "kayx23 (via GitHub)" <gi...@apache.org>.
kayx23 commented on code in PR #7690:
URL: https://github.com/apache/apisix/pull/7690#discussion_r1408493712
##########
docs/zh/latest/plugins/openid-connect.md:
##########
@@ -208,3 +208,4 @@ curl http://127.0.0.1:9180/apisix/admin/routes/1 \
- `redirect_uri` 需要能被当前 APISIX 所在路由捕获，比如当前路由的 `uri` 是 `/api/v1/*`, `redirect_uri` 可以填写为 `/api/v1/callback`；
- `redirect_uri`（`scheme:host`）的 `scheme` 和 `host` 是身份认证服务视角下访问 APISIX 所需的值。
+- 另请参考[此 GitHub 问题](https://github.com/apache/apisix/issues/2426), 尤其是下述评论: [@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)
Review Comment:
```suggestion
- 另请参考[此 GitHub 问题](https://github.com/apache/apisix/issues/2426), 尤其是下述评论：[@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)
```
fix for CI
##########
docs/zh/latest/plugins/openid-connect.md:
##########
@@ -208,3 +208,4 @@ curl http://127.0.0.1:9180/apisix/admin/routes/1 \
- `redirect_uri` 需要能被当前 APISIX 所在路由捕获，比如当前路由的 `uri` 是 `/api/v1/*`, `redirect_uri` 可以填写为 `/api/v1/callback`；
- `redirect_uri`（`scheme:host`）的 `scheme` 和 `host` 是身份认证服务视角下访问 APISIX 所需的值。
+- 另请参考[此 GitHub 问题](https://github.com/apache/apisix/issues/2426), 尤其是下述评论: [@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)
Review Comment:
```suggestion
- 另请参考[此 GitHub 问题](https://github.com/apache/apisix/issues/2426), 尤其是下述评论：[@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)
```
fix for CI
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org