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/12/23 14:32:20 UTC
[GitHub] [apisix] ilteriseroglu-ty opened a new pull request, #8564: fix(proxy-rewrite): fix url normalization bypass
ilteriseroglu-ty opened a new pull request, #8564:
URL: https://github.com/apache/apisix/pull/8564
### Description
This PR fixes a bug where ctx.var.upstream_uri is not set when use_real_request_uri_unsafe is enabled.
It seems that somewhere in the runtime chain (LuaJIT?) a memory jump occurs and ctx.var.upstream_uri _does_ get set even though it definitely **shouldn't**. Hence why this got unnoticed in the CI tests of #7401.
### 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
- [x] I have added tests corresponding to this change
- [x] I have updated the documentation to reflect this change
- [x] 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 #8564: fix(proxy-rewrite): fix url normalization bypass
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #8564: fix(proxy-rewrite): fix url normalization bypass
URL: https://github.com/apache/apisix/pull/8564
--
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 #8564: fix(proxy-rewrite): fix url normalization bypass
Posted by GitBox <gi...@apache.org>.
tokers commented on code in PR #8564:
URL: https://github.com/apache/apisix/pull/8564#discussion_r1057135332
##########
apisix/plugins/proxy-rewrite.lua:
##########
@@ -297,6 +297,8 @@ do
else
ctx.var.upstream_uri = upstream_uri
end
+ else
+ ctx.var.upstream_uri = upstream_uri
Review Comment:
Do you have reproducible steps for 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
[GitHub] [apisix] spacewander commented on a diff in pull request #8564: fix(proxy-rewrite): fix url normalization bypass
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8564:
URL: https://github.com/apache/apisix/pull/8564#discussion_r1057087432
##########
apisix/plugins/proxy-rewrite.lua:
##########
@@ -297,6 +297,8 @@ do
else
ctx.var.upstream_uri = upstream_uri
end
+ else
+ ctx.var.upstream_uri = upstream_uri
Review Comment:
If `ctx.var.upstream_uri` is empty, Nginx will proxy the request with the client's path. Why do we need to set the uri here?
--
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 #8564: fix(proxy-rewrite): fix url normalization bypass
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8564:
URL: https://github.com/apache/apisix/pull/8564#discussion_r1057461475
##########
apisix/plugins/proxy-rewrite.lua:
##########
@@ -297,6 +297,8 @@ do
else
ctx.var.upstream_uri = upstream_uri
end
+ else
+ ctx.var.upstream_uri = upstream_uri
Review Comment:
> To be quite honest, I don't know. _But_, a route to a GitLab API backend running at couple thousand requests/s has almost 50 percent error rate without this change because the request uri gets normalized _somewhere_ in the execution chain, even though it shouldn't be because `use_real_request_uri_unsafe` is set.
>
> For instance `/api/v4/projects/12345/repository/files/technology%2Ftest.yaml?ref=stage` will be normalized down to `/api/v4/projects/12345/repository/files/technology/test.yaml?ref=stage` without this change, which shouldn't happen as this route has `use_real_request_uri_unsafe` enabled (which we did confirm).
Interesting. AFAIK, the "missing" assignment here is intended.
Because `upstream_uri` here is equal to `ctx.var.uri`:
https://github.com/ilteriseroglu-ty/apisix/blob/f6d05266852e8a46b8ef060e249c05c577a926f0/apisix/plugins/proxy-rewrite.lua#L263
While `ctx.var.uri` is normalized (see http://nginx.org/en/docs/http/ngx_http_core_module.html#var_uri), assigning it to `ctx.var.upstream_uri` will break the `use_real_request_uri_unsafe`.
Before use_real_request_uri_unsafe is added, we always assign `upstream_uri` to `ctx.var.upstream_uri`:
https://github.com/apache/apisix/blob/964a92ee773264999eb67d0cd5539baf8961001b/apisix/plugins/proxy-rewrite.lua#L171-L179
That's why we don't assign it anymore when use_real_request_uri_unsafe is set.
--
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] ilteriseroglu-ty commented on a diff in pull request #8564: fix(proxy-rewrite): fix url normalization bypass
Posted by GitBox <gi...@apache.org>.
ilteriseroglu-ty commented on code in PR #8564:
URL: https://github.com/apache/apisix/pull/8564#discussion_r1057691403
##########
apisix/plugins/proxy-rewrite.lua:
##########
@@ -297,6 +297,8 @@ do
else
ctx.var.upstream_uri = upstream_uri
end
+ else
+ ctx.var.upstream_uri = upstream_uri
Review Comment:
If `use_real_request_uri_unsafe` gets set, the local `upstream_uri` becomes `ctx.var.real_request_uri`:
https://github.com/apache/apisix/blob/0f1391693bf98a38c7716b1091f40181de8690bb/apisix/plugins/proxy-rewrite.lua#L263-L265
`ctx.var.real_request_uri` is used instead of `ctx.var.request_uri` as the nginx native `request_uri` is rewritten in init.lua to use the normalized version instead:
https://github.com/apache/apisix/blob/5822eca9a28ff7b92dc7fc0431281eb62dd60ba8/apisix/init.lua#L509-L513
Normally, everything should work *without* this PR (as I authored the original `use_real_request_uri_unsafe` change) but, after examining an issue where this didn't work half the time when the system was under load; I had to set `ctx.var.upstream_uri` to `upstream_uri`, which is set to `ctx.var.real_request_uri` because `use_real_request_uri_unsafe` is set.
I am not sure whether this is an issue under LuaJIT or something else.
--
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] ilteriseroglu-ty commented on a diff in pull request #8564: fix(proxy-rewrite): fix url normalization bypass
Posted by GitBox <gi...@apache.org>.
ilteriseroglu-ty commented on code in PR #8564:
URL: https://github.com/apache/apisix/pull/8564#discussion_r1057091242
##########
apisix/plugins/proxy-rewrite.lua:
##########
@@ -297,6 +297,8 @@ do
else
ctx.var.upstream_uri = upstream_uri
end
+ else
+ ctx.var.upstream_uri = upstream_uri
Review Comment:
To be quite honest, I don't know. _But_, a route to a GitLab API backend running at couple thousand requests/s has almost 50 percent error rate without this change because the request uri gets normalized _somewhere_ in the execution chain, even though it shouldn't be because `use_real_request_uri_unsafe` is set.
For instance `/api/v4/projects/12345/repository/files/technology%2Ftest.yaml?ref=stage` will be normalized down to `/api/v4/projects/12345/repository/files/technology/test.yaml?ref=stage` without this change, which shouldn't happen as this route has `use_real_request_uri_unsafe` enabled (which we did confirm).
--
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 #8564: fix(proxy-rewrite): fix url normalization bypass
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #8564:
URL: https://github.com/apache/apisix/pull/8564#issuecomment-1531208128
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
[GitHub] [apisix] github-actions[bot] commented on pull request #8564: fix(proxy-rewrite): fix url normalization bypass
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #8564:
URL: https://github.com/apache/apisix/pull/8564#issuecomment-1569890161
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
[GitHub] [apisix] spacewander commented on a diff in pull request #8564: fix(proxy-rewrite): fix url normalization bypass
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8564:
URL: https://github.com/apache/apisix/pull/8564#discussion_r1058071044
##########
apisix/plugins/proxy-rewrite.lua:
##########
@@ -297,6 +297,8 @@ do
else
ctx.var.upstream_uri = upstream_uri
end
+ else
+ ctx.var.upstream_uri = upstream_uri
Review Comment:
I see. Can you add a test case to cover this? You can learn how to write test by reading https://github.com/apache/apisix/blob/master/docs/en/latest/internal/testing-framework.md
--
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