Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: add post request headers only if auth request method is post #11021

Merged
merged 4 commits into from
Apr 8, 2024
Merged

fix: add post request headers only if auth request method is post #11021

merged 4 commits into from
Apr 8, 2024

Conversation

suryaprabhakark
Copy link
Contributor

@suryaprabhakark suryaprabhakark commented Mar 9, 2024 •
edited

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.
#10927
#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 first)

@suryaprabhakark suryaprabhakark mentioned this pull request Mar 9, 2024
Open
@suryaprabhakark suryaprabhakark changed the title Adding post request header only if authservice request method is post fix: forward-auth Adding post request headers only if authservice request method is post Mar 9, 2024
@BrandonArp
Copy link

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.

@shreemaan-abhishek
Copy link
Contributor

Hi could you also handle this case?
image
#10927 (comment)

and add test cases?

@suryaprabhakark
Copy link
Contributor Author

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.

@motongxue
Copy link
Contributor

Hi could you also handle this case? image #10927 (comment)

and add test cases?

Hello abhishek. I have submitted a PR 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.

@BrandonArp
Copy link

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.

@suryaprabhakark
Copy link
Contributor Author

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.

@shreemaan-abhishek
Copy link
Contributor

the content-encoding header should also be passed. Also, test cases should be added to support this PR.

@suryaprabhakark
Copy link
Contributor Author

Apologies for the delay, will add the testcases over the weekend(24th March).

@suryaprabhakark
Copy link
Contributor Author

@shreemaan-abhishek @BrandonArp Added the testcases, please review

@suryaprabhakark
Copy link
Contributor Author

@shreemaan-abhishek @BrandonArp Can you please give approval for workflows to run.

membphis
membphis reviewed Mar 25, 2024
View reviewed changes
apisix/plugins/forward-auth.lua
}

if conf.request_method == "POST" then
auth_headers["Content-Length"] = core.request.header(ctx, "content-length")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

@suryaprabhakark
Copy link
Contributor Author

@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

@shreemaan-abhishek
Copy link
Contributor

@suryaprabhakark sorry for the delayed response, the CI failure was very likely a flaky error I have re-ran the CI.

@shreemaan-abhishek shreemaan-abhishek changed the title fix: forward-auth Adding post request headers only if authservice request method is post fix: add post request headers only if auth request method is post Apr 8, 2024
@shreemaan-abhishek shreemaan-abhishek merged commit 4df549c into apache:master Apr 8, 2024
57 checks passed
membphis added a commit to membphis/apisix that referenced this pull request Apr 20, 2024
@membphis
fix: add post request headers only if auth request method is post (ap…
1320a89 
…ache#11021)

author: suryaprabhakark
This was referenced Apr 20, 2024
Merged
Merged
membphis added a commit to membphis/apisix that referenced this pull request Apr 23, 2024
@membphis
fix: add post request headers only if auth request method is post
aeb1b22 
author: suryaprabhakark

sync apache#11021
@membphis membphis mentioned this pull request Apr 23, 2024
Merged
membphis added a commit that referenced this pull request Apr 23, 2024
@membphis
fix: add post request headers only if auth request method is post (#1…
c1560a8 
…1184)

author: suryaprabhakark

sync #11021
@OnGoingLzy OnGoingLzy mentioned this pull request Apr 25, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@membphis membphis membphis left review comments

@starsz starsz starsz approved these changes

@shreemaan-abhishek shreemaan-abhishek shreemaan-abhishek approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@suryaprabhakark @BrandonArp @shreemaan-abhishek @motongxue @membphis @starsz @Revolyssup