You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "suryaprabhakark (via GitHub)" <gi...@apache.org> on 2024/03/09 15:50:05 UTC
[PR] Adding post request header only if authservice request method is post [apisix]
suryaprabhakark opened a new pull request, #11021:
URL: https://github.com/apache/apisix/pull/11021
### Description
forward-auth plugin doesn't work if the request is POST and authservice api is GET. Its happening due to forwading the POST method request headers like content-type and expect. Moved these headers only if authserver api is also POST.
Fixes # (issue)
Fixes the issue with forward-auth plugin with POST headers.
https://github.com/apache/apisix/issues/10927
https://github.com/apache/apisix/issues/11020
### 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](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
Re: [PR] fix: forward-auth Adding post request headers only if authservice request method is post [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on PR #11021:
URL: https://github.com/apache/apisix/pull/11021#issuecomment-1987605017
Hi could you also handle this case?
https://github.com/apache/apisix/issues/10927#issuecomment-1975557939
and add test cases?
--
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: forward-auth Adding post request headers only if authservice request method is post [apisix]
Posted by "membphis (via GitHub)" <gi...@apache.org>.
membphis commented on code in PR #11021:
URL: https://github.com/apache/apisix/pull/11021#discussion_r1537163320
##########
apisix/plugins/forward-auth.lua:
##########
@@ -89,11 +89,15 @@ function _M.access(conf, ctx)
["X-Forwarded-Host"] = core.request.get_host(ctx),
["X-Forwarded-Uri"] = ctx.var.request_uri,
["X-Forwarded-For"] = core.request.get_remote_client_ip(ctx),
- ["Expect"] = core.request.header(ctx, "expect"),
- ["Content-Length"] = core.request.header(ctx, "content-length"),
- ["Transfer-Encoding"] = core.request.header(ctx, "transfer-encoding")
}
+ if conf.request_method == "POST" then
+ auth_headers["Content-Length"] = core.request.header(ctx, "content-length")
Review Comment:
For HTTP requests, if the request header contains the ["Content-Length"] field, the request body should be included.
So I think this request header should not be included.
--
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: forward-auth Adding post request headers only if authservice request method is post [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on code in PR #11021:
URL: https://github.com/apache/apisix/pull/11021#discussion_r1533672473
##########
t/plugin/forward-auth.t:
##########
@@ -143,6 +143,24 @@ property "request_method" validation failed: matches none of the enum values
end
end
end
+ end]],
+ [[return function(conf, ctx)
Review Comment:
please write the test cases in a new file `t/plugin/forward-auth2.t` . This file already contains too many serverless functions and routes which can cause conflict.
--
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: forward-auth Adding post request headers only if authservice request method is post [apisix]
Posted by "suryaprabhakark (via GitHub)" <gi...@apache.org>.
suryaprabhakark commented on PR #11021:
URL: https://github.com/apache/apisix/pull/11021#issuecomment-2010636201
@shreemaan-abhishek @BrandonArp Added the testcases, please review
--
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: forward-auth Adding post request headers only if authservice request method is post [apisix]
Posted by "suryaprabhakark (via GitHub)" <gi...@apache.org>.
suryaprabhakark commented on code in PR #11021:
URL: https://github.com/apache/apisix/pull/11021#discussion_r1533865018
##########
t/plugin/forward-auth.t:
##########
@@ -143,6 +143,24 @@ property "request_method" validation failed: matches none of the enum values
end
end
end
+ end]],
+ [[return function(conf, ctx)
Review Comment:
@shreemaan-abhishek Had split the testcases into new file.
--
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: forward-auth Adding post request headers only if authservice request method is post [apisix]
Posted by "motongxue (via GitHub)" <gi...@apache.org>.
motongxue commented on PR #11021:
URL: https://github.com/apache/apisix/pull/11021#issuecomment-1987928642
> Hi could you also handle this case?  [#10927 (comment)](https://github.com/apache/apisix/issues/10927#issuecomment-1975557939)
>
> and add test cases?
Hello abhishek. I have submitted a [PR](https://github.com/apache/apisix/pull/11023) regarding your issue, and I hope it can be of help to you. At the same time, I would greatly appreciate your assistance in reviewing and finalizing the test cases.
--
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: forward-auth Adding post request headers only if authservice request method is post [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on PR #11021:
URL: https://github.com/apache/apisix/pull/11021#issuecomment-1993814627
the content-encoding header should also be passed. Also, test cases should be added to support this 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: forward-auth Adding post request headers only if authservice request method is post [apisix]
Posted by "suryaprabhakark (via GitHub)" <gi...@apache.org>.
suryaprabhakark commented on PR #11021:
URL: https://github.com/apache/apisix/pull/11021#issuecomment-1991140750
Thanks @BrandonArp. Let me know one of the contributor.
@shreemaan-abhishek Can you help with the content-encoding header, I think its required by the authserver if its present in the request.
--
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: forward-auth Adding post request headers only if authservice request method is post [apisix]
Posted by "BrandonArp (via GitHub)" <gi...@apache.org>.
BrandonArp commented on PR #11021:
URL: https://github.com/apache/apisix/pull/11021#issuecomment-1988694950
> Do you think we need to add content-encoding header as well, authserver should also know if the content is encoded.
Content-Encoding should be added at some point. I'm not very familiar with the APISIX architecture, so I don't know if it would be added somewhere else. If this is the only place we're adding headers, then yes, it should be added. Otherwise the auth server won't know how to interpret the body.
--
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: forward-auth Adding post request headers only if authservice request method is post [apisix]
Posted by "suryaprabhakark (via GitHub)" <gi...@apache.org>.
suryaprabhakark commented on code in PR #11021:
URL: https://github.com/apache/apisix/pull/11021#discussion_r1537518599
##########
apisix/plugins/forward-auth.lua:
##########
@@ -89,11 +89,15 @@ function _M.access(conf, ctx)
["X-Forwarded-Host"] = core.request.get_host(ctx),
["X-Forwarded-Uri"] = ctx.var.request_uri,
["X-Forwarded-For"] = core.request.get_remote_client_ip(ctx),
- ["Expect"] = core.request.header(ctx, "expect"),
- ["Content-Length"] = core.request.header(ctx, "content-length"),
- ["Transfer-Encoding"] = core.request.header(ctx, "transfer-encoding")
}
+ if conf.request_method == "POST" then
+ auth_headers["Content-Length"] = core.request.header(ctx, "content-length")
Review Comment:
@membphis If the auth server `request_method` is POST, the same upstream body is being forwarded to auth server as well, thats why we need this header incase its present. Below code reference is where body is being added to auth api request.
https://github.com/apache/apisix/pull/11021/files/48112cc9057b16611ba86d0761a84dd635ac0e8b#diff-4b85e7666119852676bcd1c5fd15889e752f061a9ae891e10a0cbbefd30c2e0dL115-L124
--
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: forward-auth Adding post request headers only if authservice request method is post [apisix]
Posted by "suryaprabhakark (via GitHub)" <gi...@apache.org>.
suryaprabhakark commented on PR #11021:
URL: https://github.com/apache/apisix/pull/11021#issuecomment-2017616551
@BrandonArp @shreemaan-abhishek Got these following errros in the tests, could you please help.
`Error: nginx: [error] open() "/home/runner/work/apisix/apisix/logs/nginx.pid" failed (2: No such file or directory)
Error: 2024/03/24 06:39:06 [error] 94305#94305: *155 stream [lua] radixtree_sni.lua:178: match_and_set(): failed to find any SSL certificate by SNI: test.com, context: ssl_certificate_by_lua*, client: 127.0.0.1, server: 0.0.0.0:9100
`
--
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: forward-auth Adding post request headers only if authservice request method is post [apisix]
Posted by "suryaprabhakark (via GitHub)" <gi...@apache.org>.
suryaprabhakark commented on PR #11021:
URL: https://github.com/apache/apisix/pull/11021#issuecomment-2016559509
@shreemaan-abhishek @BrandonArp Can you please give approval for workflows to run.
--
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: add post request headers only if auth request method is post [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek merged PR #11021:
URL: https://github.com/apache/apisix/pull/11021
--
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: forward-auth Adding post request headers only if authservice request method is post [apisix]
Posted by "suryaprabhakark (via GitHub)" <gi...@apache.org>.
suryaprabhakark commented on PR #11021:
URL: https://github.com/apache/apisix/pull/11021#issuecomment-1987849092
> Should the Transfer-Encoding header be removed? If APISIX is trying to just pass through the bytes of the request body, we'll need to send the Transfer-Encoding header to make sure the auth server knows how to interpret the bytes.
@BrandonArp My initial reading about the transfer encoding was that its set in the responses rather than request, further research suggests it can be used in both request and responses as well. Ill add back the header. Do you think we need to add content-encoding header as well, authserver should also know if the content is encoded.
--
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: forward-auth Adding post request headers only if authservice request method is post [apisix]
Posted by "suryaprabhakark (via GitHub)" <gi...@apache.org>.
suryaprabhakark commented on PR #11021:
URL: https://github.com/apache/apisix/pull/11021#issuecomment-2004677039
Apologies for the delay, will add the testcases over the weekend(24th March).
--
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: forward-auth Adding post request headers only if authservice request method is post [apisix]
Posted by "BrandonArp (via GitHub)" <gi...@apache.org>.
BrandonArp commented on PR #11021:
URL: https://github.com/apache/apisix/pull/11021#issuecomment-1986955146
Should the Transfer-Encoding header be removed? If APISIX is trying to just pass through the bytes of the request body, we'll need to send the Transfer-Encoding header to make sure the auth server knows how to interpret the bytes.
--
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: forward-auth Adding post request headers only if authservice request method is post [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on PR #11021:
URL: https://github.com/apache/apisix/pull/11021#issuecomment-2027452904
@suryaprabhakark sorry for the delayed response, the CI failure was very likely a flaky error I have re-ran the 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