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/04/22 02:51:30 UTC
[GitHub] [apisix] spacewander opened a new pull request, #6906: fix: the client verify flag might not be set
spacewander opened a new pull request, #6906:
URL: https://github.com/apache/apisix/pull/6906
A more suitable way is to reject client TLS handshake directly, just
like what Go has done.
Signed-off-by: spacewander <sp...@gmail.com>
### Description
<!-- Please include a summary of the change and which issue is fixed. -->
<!-- Please also include relevant motivation and context. -->
Fixes 6896
### Checklist
- [ ] 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
- [ ] I have added tests corresponding to this change
- [ ] 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] spacewander merged pull request #6906: fix: the client verify flag might not be set
Posted by GitBox <gi...@apache.org>.
spacewander merged PR #6906:
URL: https://github.com/apache/apisix/pull/6906
--
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] membphis commented on a diff in pull request #6906: fix: the client verify flag might not be set
Posted by GitBox <gi...@apache.org>.
membphis commented on code in PR #6906:
URL: https://github.com/apache/apisix/pull/6906#discussion_r855894300
##########
apisix/init.lua:
##########
@@ -350,14 +357,14 @@ end
function _M.http_access_phase()
local ngx_ctx = ngx.ctx
- if not verify_tls_client(ngx_ctx.api_ctx) then
- return core.response.exit(400)
- end
-
-- always fetch table from the table pool, we don't need a reused api_ctx
local api_ctx = core.tablepool.fetch("api_ctx", 0, 32)
ngx_ctx.api_ctx = api_ctx
+ if not verify_tls_client(ngx_ctx.api_ctx) then
Review Comment:
we can use `api_ctx` 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] tokers commented on pull request #6906: fix: the client verify flag might not be set
Posted by GitBox <gi...@apache.org>.
tokers commented on PR #6906:
URL: https://github.com/apache/apisix/pull/6906#issuecomment-1106419057
> > Anyway to test the fix?
>
>
>
> Not so easy to do it.
What about writing the case in shell? And use curl to send two requests.
--
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 #6906: fix: the client verify flag might not be set
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #6906:
URL: https://github.com/apache/apisix/pull/6906#discussion_r855844274
##########
apisix/init.lua:
##########
@@ -313,7 +314,13 @@ end
local function verify_tls_client(ctx)
- if ctx and ctx.ssl_client_verified then
+ local matched = router.router_ssl.match_and_set(ctx, true)
+ if not matched then
+ return true
Review Comment:
Return true means the verification is successful when this function is called with non-TLS 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
[GitHub] [apisix] spacewander commented on pull request #6906: fix: the client verify flag might not be set
Posted by GitBox <gi...@apache.org>.
spacewander commented on PR #6906:
URL: https://github.com/apache/apisix/pull/6906#issuecomment-1106345443
> Anyway to test the fix?
Not so easy to do 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] membphis commented on a diff in pull request #6906: fix: the client verify flag might not be set
Posted by GitBox <gi...@apache.org>.
membphis commented on code in PR #6906:
URL: https://github.com/apache/apisix/pull/6906#discussion_r855866976
##########
apisix/init.lua:
##########
@@ -313,7 +314,13 @@ end
local function verify_tls_client(ctx)
- if ctx and ctx.ssl_client_verified then
+ local matched = router.router_ssl.match_and_set(ctx, true)
+ if not matched then
+ return true
Review Comment:
got it, many thx
--
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] membphis commented on a diff in pull request #6906: fix: the client verify flag might not be set
Posted by GitBox <gi...@apache.org>.
membphis commented on code in PR #6906:
URL: https://github.com/apache/apisix/pull/6906#discussion_r855895944
##########
apisix/init.lua:
##########
@@ -870,15 +877,15 @@ function _M.stream_preread_phase()
local ngx_ctx = ngx.ctx
local api_ctx = ngx_ctx.api_ctx
- if not verify_tls_client(ngx_ctx.api_ctx) then
- return ngx_exit(1)
- end
-
if not api_ctx then
api_ctx = core.tablepool.fetch("api_ctx", 0, 32)
ngx_ctx.api_ctx = api_ctx
end
+ if not verify_tls_client(ngx_ctx.api_ctx) then
Review Comment:
ditto
--
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 pull request #6906: fix: the client verify flag might not be set
Posted by GitBox <gi...@apache.org>.
tokers commented on PR #6906:
URL: https://github.com/apache/apisix/pull/6906#issuecomment-1106215081
Anyway to test the fix?
--
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] membphis commented on a diff in pull request #6906: fix: the client verify flag might not be set
Posted by GitBox <gi...@apache.org>.
membphis commented on code in PR #6906:
URL: https://github.com/apache/apisix/pull/6906#discussion_r855832832
##########
apisix/init.lua:
##########
@@ -313,7 +314,13 @@ end
local function verify_tls_client(ctx)
- if ctx and ctx.ssl_client_verified then
+ local matched = router.router_ssl.match_and_set(ctx, true)
Review Comment:
need to capture the `err` message
##########
apisix/init.lua:
##########
@@ -313,7 +314,13 @@ end
local function verify_tls_client(ctx)
- if ctx and ctx.ssl_client_verified then
+ local matched = router.router_ssl.match_and_set(ctx, true)
+ if not matched then
+ return true
Review Comment:
little confused, we should return false, all right?
--
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 #6906: fix: the client verify flag might not be set
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #6906:
URL: https://github.com/apache/apisix/pull/6906#discussion_r855843320
##########
apisix/init.lua:
##########
@@ -313,7 +314,13 @@ end
local function verify_tls_client(ctx)
- if ctx and ctx.ssl_client_verified then
+ local matched = router.router_ssl.match_and_set(ctx, true)
Review Comment:
match_and_set will return `err` when it fails to match the SSL certificate, but this is not considered an error here because both non-TLS and TLS will execute this function.
--
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