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/09/09 02:04:53 UTC
[GitHub] [apisix] cj2a7t opened a new pull request, #7891: fix(http-logger): explicitly read the req body in rewrite phase
cj2a7t opened a new pull request, #7891:
URL: https://github.com/apache/apisix/pull/7891
### Description
Explicitly read the req body:
When the pre plugins returns, the following collection request will be empty
Such as the pre jwt plugin in rewrite phase:
`return 401, {message = "Missing JWT token in request"}`
@see log-util line 179 or line 183
`local body = req_get_body_data() or local body_file = ngx.req.get_body_file()`
body or body_file is empty
如果使用了前置jwt-plugin或者其他自定义的鉴权插件，使用如下的退出
`return 401, {message = "Missing JWT token in request"}`
会导致在log阶段调用 log-util 179行 或者 183行读取请求体为空
`local body = req_get_body_data() or local body_file = ngx.req.get_body_file()`
Fixes # 7890
### 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)
--
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 #7891: fix(http-logger): explicitly read the req body in rewrite phase
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #7891: fix(http-logger): explicitly read the req body in rewrite phase
URL: https://github.com/apache/apisix/pull/7891
--
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 pull request #7891: fix(http-logger): explicitly read the req body in rewrite phase
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on PR #7891:
URL: https://github.com/apache/apisix/pull/7891#issuecomment-1246119253
> Copy that. I'll try. Is there any better plan @tzssangglass
No more idea for now, let's hear from others.
--
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] cj2a7t commented on pull request #7891: fix(http-logger): explicitly read the req body in rewrite phase
Posted by GitBox <gi...@apache.org>.
cj2a7t commented on PR #7891:
URL: https://github.com/apache/apisix/pull/7891#issuecomment-1241554679
@spacewander
I see what you mean.
However, in most of our scenarios, the authentication plugin will go ahead. In the rewrite phase, the logger's rewrite is post-existing and does not exist untrusted client.
Whether to turn it on as a schema configuration，give the user the read_body option to enable configuration.
<img width="525" alt="image" src="https://user-images.githubusercontent.com/70355872/189285865-4dbeb9fa-51b6-4e39-9f7b-045e27291057.png">
--
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] cj2a7t commented on pull request #7891: fix(http-logger): explicitly read the req body in rewrite phase
Posted by GitBox <gi...@apache.org>.
cj2a7t commented on PR #7891:
URL: https://github.com/apache/apisix/pull/7891#issuecomment-1245288015
Copy that. I'll try. Is there any better plan @tzssangglass
--
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] cj2a7t commented on pull request #7891: fix(http-logger): explicitly read the req body in rewrite phase
Posted by GitBox <gi...@apache.org>.
cj2a7t commented on PR #7891:
URL: https://github.com/apache/apisix/pull/7891#issuecomment-1244906938
@tzssangglass Is it more unfriendly to control through lua_need_request_body command
--
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 #7891: fix(http-logger): explicitly read the req body in rewrite phase
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #7891:
URL: https://github.com/apache/apisix/pull/7891#issuecomment-1328211289
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] tzssangglass commented on pull request #7891: fix(http-logger): explicitly read the req body in rewrite phase
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on PR #7891:
URL: https://github.com/apache/apisix/pull/7891#issuecomment-1245244725
> From the perspective of the logger plugin only, include_req_body = true. However, the request body cannot be obtained, which is ambiguous for users.
This is from your point of view. Would this problem still occur without `jwt-auth` plugin?
I was thinking maybe we could try `ngx.req.read_body()` when get body is nil, now this fix is not reasonable.
--
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 pull request #7891: fix(http-logger): explicitly read the req body in rewrite phase
Posted by GitBox <gi...@apache.org>.
spacewander commented on PR #7891:
URL: https://github.com/apache/apisix/pull/7891#issuecomment-1243639583
If the authentication plugin goes ahead and Nginx reads the body, it should be available in the logger, unless the body is too big or proxy_request_buffering is disabled.
It is not a good idea to force reading an unused body just because the logger needs it.
--
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 pull request #7891: fix(http-logger): explicitly read the req body in rewrite phase
Posted by GitBox <gi...@apache.org>.
spacewander commented on PR #7891:
URL: https://github.com/apache/apisix/pull/7891#issuecomment-1246202297
> > From the perspective of the logger plugin only, include_req_body = true. However, the request body cannot be obtained, which is ambiguous for users.
> > This is from your point of view. Would this problem still occur without `jwt-auth` plugin?
>
> I was thinking maybe we could try `ngx.req.read_body()` when get body is nil, now this fix is not reasonable.
There are many situations that `get body is nil`, for example:
1. Nginx doesn't read the body (this case)
2. the body is too large so Nginx stores it in a temp file
3. the request doesn't have a body
4. Nginx is configured to pass the request body streamingly (which will be broken if we always force Nginx to read the body just because someone wants to log the body)
5. Nginx is configured to always buffer the request body in a temp file
Not every situation needs to be "fixed"
--
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 #7891: fix(http-logger): explicitly read the req body in rewrite phase
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #7891:
URL: https://github.com/apache/apisix/pull/7891#issuecomment-1364656197
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] cj2a7t commented on pull request #7891: fix(http-logger): explicitly read the req body in rewrite phase
Posted by GitBox <gi...@apache.org>.
cj2a7t commented on PR #7891:
URL: https://github.com/apache/apisix/pull/7891#issuecomment-1244909302
From the perspective of the plug-in only, include_req_body = true.
However, the request body cannot be obtained, which seems difficult for the user to understand.
--
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] cj2a7t commented on pull request #7891: fix(http-logger): explicitly read the req body in rewrite phase
Posted by GitBox <gi...@apache.org>.
cj2a7t commented on PR #7891:
URL: https://github.com/apache/apisix/pull/7891#issuecomment-1244801776
hi , @spacewander oh yes, I see what you mean.
This is not the case that body is too big or proxy_request_buffering is disabled.
You can try the combination of http-logger plugin and jwt-auth plugin.
<img width="1013" alt="image" src="https://user-images.githubusercontent.com/70355872/189789934-520a4864-ccf9-433d-94e8-4b043fe3d60a.png">
In this case, I want to get the request body from the http-logger(jwt-auth plugin goes ahead return). Is there any other way?
--
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 pull request #7891: fix(http-logger): explicitly read the req body in rewrite phase
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on PR #7891:
URL: https://github.com/apache/apisix/pull/7891#issuecomment-1244876618
> However, in most of our scenarios, the authentication plugin will go ahead.
If you always need to read the body in your scenario, you can try the `lua_need_request_body` directive, but it is not recommended.
--
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