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.

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

fix(forward-auth): client body can not send to upstream after forwarding it to auth. service #11350

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

zhoujiexiong
Copy link
Contributor

@zhoujiexiong zhoujiexiong commented Jun 12, 2024

fix(forward-auth): client body can not send to upstream after forwarding it to auth. service

Description

Fixes #11050
Fixes #11200

  1. sync. with Nginx using ngx.req.<init|append|finish>_body APIs while forwarding/streaming to auth. svc
  2. consider the case of conf.allow_degradation == true
  3. delay invoking "get_client_body_reader() -> ngx.req.socket()"
  -- once invoking ngx.req.socket() would stop NGX from reading client body causing 504 issue
  -- why there is such a performance needs further clarification
  -- delay the invoking in order to follow the normal proxy_pass flow rather than buffering
  -- the client body by the plugin in case of client body has not started to read,
  -- e.g. auth svc. is unable to connect and allow_degradation is true
  1. suggest adding the following config field

-- suggest adding this switch
forward_client_body = {type = "boolean", default = true},

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)

@zhoujiexiong
Copy link
Contributor Author

@shreemaan-abhishek @monkeyDluffy6017 @Gallardot Would you please help me review this PR? thx.

@zhoujiexiong
Copy link
Contributor Author

@Gallardot @moonming
It seems that other test case unrelated fail the check, please help confirm.

image

@bzp2010
Copy link
Contributor

bzp2010 commented Jun 16, 2024

Hi, @zhoujiexiong. Thank you for your contribution.

First the clarification part, read_body shouldn't be used at the same time as ngx.req.socket. The socket call puts the client connection into streaming read mode, which is a different mode than blocking reads.
The documentation (https://github.com/openresty/lua-nginx-module?tab=readme-ov-file#ngxreqsocket) explicitly mentions that ngx.req.read_body and ngx.req.socket should not be used at the same time, and ngx.req.read_body is used in core.request.read_body.

This would be a systematic problem, and read_body and sockets are hard to avoid being used together, so perhaps we could consider modifying the core.request.read_body behavior so that it can read the request body as a stream.


But I have some questions about forward-auth. I am the original author of the plugin.

I don't think it's normal to send huge request bodies to an authentication service, or even to send any request bodies at all. This may defeat the purpose of the plugin as an extremely lightweight authentication middle layer in the first place. In contrast, the original design option of sending only request headers is probably better. (Yes, at first it didn't send request bodies at all, some other contributor added that feature.)

Additionally, attempting to read huge request bodies into LuaVM side memory may break APISIX's excellent performance. It may be more efficient to let NGINX handle the sending of request bodies directly. If you do need to read the body into memory, it is best to control the body size.

Can you comment on the necessity of this feature existing in forwath-auth itself? 🤔

@zhoujiexiong
Copy link
Contributor Author

Hi @bzp2010 ,

Thank you for replying.


I also noticed the doc. you attached.
It means that once we invoke ngx.req.socket(), it become ours responsibility to manage the client_body.

See also:
https://github.com/openresty/lua-nginx-module?tab=readme-ov-file#ngxreqinit_body

Code snippet from ngx_lua/ngx_http_lua_socket_tcp.c:

static int
ngx_http_lua_req_socket(lua_State *L)
{
...

  /* prevent other request body reader from running */

  rb = ngx_pcalloc(r->pool, sizeof(ngx_http_request_body_t));
  if (rb == NULL) {
      return luaL_error(L, "no memory");
   }

  rb->rest = r->headers_in.content_length_n;

  r->request_body = rb;

New empty request_body(control block) for the request but not touch the original content_length of the request.


In my recent applicaion(using APISIX v3.9/forward_auth):

  1. [POST]_client_req -> [POST]_fw_auth_svc
  2. Just need Authentication HTTP header for AUTH, but client_body is forwarded also
  3. Encounter the issue

My workaround:
[POST]_fw_auth_svc -> [GET]_fw_auth_svc

It's work fine for my scenario so far.

But I believe that there are scenarios that need to forward client_body
to auth_svc for some biz logic, e.g. Calc. digest from client_body and
verify request signature.

Envoy supports more control options for the similer feature,
such as "with_request_body/max_request_bytes".
See also:
https://www.envoyproxy.io/docs/envoy/v1.28.0/api-v3/extensions/filters/http/ext_authz/v3/ext_authz.proto#envoy-v3-api-msg-extensions-filters-http-ext-authz-v3-extauthz

So I proposed this PR.

@zhoujiexiong
Copy link
Contributor Author

@bzp2010 review required :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants