You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "Revolyssup (via GitHub)" <gi...@apache.org> on 2023/08/21 09:02:40 UTC
[GitHub] [apisix] Revolyssup opened a new pull request, #10066: fix: error on https requests due to nil matched_ssl
Revolyssup opened a new pull request, #10066:
URL: https://github.com/apache/apisix/pull/10066
### Description
<!-- Please include a summary of the change and which issue is fixed. -->
<!-- Please also include relevant motivation and context. -->
Fixes # (issue)
### 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
[GitHub] [apisix] kingluo commented on a diff in pull request #10066: fix: error on https requests due to nil matched_ssl
Posted by "kingluo (via GitHub)" <gi...@apache.org>.
kingluo commented on code in PR #10066:
URL: https://github.com/apache/apisix/pull/10066#discussion_r1299959083
##########
apisix/init.lua:
##########
@@ -358,33 +358,20 @@ local function verify_https_client(ctx)
return true
end
- local matched_ssl = ngx.ctx.matched_ssl
- if matched_ssl.value.client
- and matched_ssl.value.client.skip_mtls_uri_regex
- and apisix_ssl.support_client_verification()
- and (not uri_matches_skip_mtls_route_patterns(matched_ssl, ngx.var.uri)) then
- local res = ctx.var.ssl_client_verify
- if res ~= "SUCCESS" then
- if res == "NONE" then
- core.log.error("client certificate was not present")
- else
- core.log.error("client certificate verification is not passed: ", res)
- end
-
- return false
- end
- end
-
local host = ctx.var.host
- local matched = router.router_ssl.match_and_set(ctx, true, host)
- if not matched then
- return true
+ if not ctx.matched_ssl then
+ local matched = router.router_ssl.match_and_set(ctx, true, host)
+ if not matched then
+ return true
+ end
end
local matched_ssl = ctx.matched_ssl
if matched_ssl.value.client and apisix_ssl.support_client_verification() then
local verified = apisix_base_flags.client_cert_verified_in_handshake
- if not verified then
+ local uri_in_whitelist = matched_ssl.value.client.skip_mtls_uri_regex
Review Comment:
Move skip-mtls logic here, because we need to check all requests within the same SSL connection.
--
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] monkeyDluffy6017 commented on pull request #10066: fix: error on https requests due to nil matched_ssl
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on PR #10066:
URL: https://github.com/apache/apisix/pull/10066#issuecomment-1687493534
And maybe you shoud also explain the reason in https://github.com/apache/apisix/issues/9610, https://github.com/apache/apisix/issues/9931, https://github.com/apache/apisix/issues/9801
--
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] AlinsRan commented on a diff in pull request #10066: fix: error on https requests due to nil matched_ssl
Posted by "AlinsRan (via GitHub)" <gi...@apache.org>.
AlinsRan commented on code in PR #10066:
URL: https://github.com/apache/apisix/pull/10066#discussion_r1301083231
##########
apisix/init.lua:
##########
@@ -358,7 +358,15 @@ local function verify_https_client(ctx)
return true
end
- local matched_ssl = ngx.ctx.matched_ssl
+ local host = ctx.var.host
+ if not ctx.matched_ssl then
Review Comment:
Before that, `api_ctx.matched_ssl` was not initialized and the `api_ctx` will be destroyed after SSL handshake.
You need to check `ngx.ctx.matched_ssl`, not `api_ctx.matched_ssl`.
--
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] Revolyssup commented on pull request #10066: fix: error on https requests due to nil matched_ssl
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on PR #10066:
URL: https://github.com/apache/apisix/pull/10066#issuecomment-1689443305
@kingluo I will write the test for it. But to confirm the situation that I need to simulate. The client must create an SSL connection with APISIX and send multiple HTTP requests on the same connection, right? And also new TCP connection using the existing SSL Connection?
--
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] Revolyssup commented on a diff in pull request #10066: fix: error on https requests due to nil matched_ssl
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #10066:
URL: https://github.com/apache/apisix/pull/10066#discussion_r1303481201
##########
apisix/init.lua:
##########
@@ -358,7 +358,15 @@ local function verify_https_client(ctx)
return true
end
- local matched_ssl = ngx.ctx.matched_ssl
+ local host = ctx.var.host
+ if not ctx.matched_ssl then
Review Comment:
Also, when changing on just this line https://github.com/apache/apisix/pull/10066/files#diff-7439ffe94cc4bcad12066d5d83f6c1004b8e9b3ff5097b645fa7c700eb3b3743R363, we get an error because after the if block we still try to access `matched_ssl` from `api_ctx` so I made changes below as well to extract from `ngx.ctx`
--
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] monkeyDluffy6017 commented on a diff in pull request #10066: fix: error on https requests due to nil matched_ssl
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #10066:
URL: https://github.com/apache/apisix/pull/10066#discussion_r1301915053
##########
apisix/init.lua:
##########
@@ -358,7 +358,15 @@ local function verify_https_client(ctx)
return true
end
- local matched_ssl = ngx.ctx.matched_ssl
+ local host = ctx.var.host
+ if not ctx.matched_ssl then
Review Comment:
Agreed with @AlinsRan
--
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] Revolyssup commented on a diff in pull request #10066: fix: error on https requests due to nil matched_ssl
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #10066:
URL: https://github.com/apache/apisix/pull/10066#discussion_r1301181047
##########
apisix/init.lua:
##########
@@ -358,7 +358,15 @@ local function verify_https_client(ctx)
return true
end
- local matched_ssl = ngx.ctx.matched_ssl
+ local host = ctx.var.host
+ if not ctx.matched_ssl then
Review Comment:
cc: @kingluo might explain better
--
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] kingluo closed pull request #10066: fix: error on https requests due to nil matched_ssl
Posted by "kingluo (via GitHub)" <gi...@apache.org>.
kingluo closed pull request #10066: fix: error on https requests due to nil matched_ssl
URL: https://github.com/apache/apisix/pull/10066
--
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] kingluo commented on pull request #10066: fix: error on https requests due to nil matched_ssl
Posted by "kingluo (via GitHub)" <gi...@apache.org>.
kingluo commented on PR #10066:
URL: https://github.com/apache/apisix/pull/10066#issuecomment-1687488440
> Could you explain the reason for the bug? why can't it be reproduced in the old version?
Because the current test cases do not cover these bugs:
1. matched_ssl is nil.
2. The MTLS whitelist does not work, which allows subsequent requests to bypass the whitelist check.
Only fires when one of the following conditions is met:
1. Second and subsequent requests (HTTP keepalive) within the same SSL connection.
2. All requests in all subsequent new connections using the previously successful SSL session.
--
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] monkeyDluffy6017 commented on pull request #10066: fix: error on https requests due to nil matched_ssl
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on PR #10066:
URL: https://github.com/apache/apisix/pull/10066#issuecomment-1687490621
@Revolyssup Could you add test cases to cover what @kingluo metioned?
--
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] Revolyssup commented on a diff in pull request #10066: fix: error on https requests due to nil matched_ssl
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #10066:
URL: https://github.com/apache/apisix/pull/10066#discussion_r1303277470
##########
apisix/init.lua:
##########
@@ -358,7 +358,15 @@ local function verify_https_client(ctx)
return true
end
- local matched_ssl = ngx.ctx.matched_ssl
+ local host = ctx.var.host
+ if not ctx.matched_ssl then
Review Comment:
tests fail with ngx.ctx for some reason
--
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] monkeyDluffy6017 commented on pull request #10066: fix: error on https requests due to nil matched_ssl
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on PR #10066:
URL: https://github.com/apache/apisix/pull/10066#issuecomment-1687406959
Could you explain the reason for the bug? why can't it be reproduced in the old version?
And test cases are needed
--
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] kingluo commented on a diff in pull request #10066: fix: error on https requests due to nil matched_ssl
Posted by "kingluo (via GitHub)" <gi...@apache.org>.
kingluo commented on code in PR #10066:
URL: https://github.com/apache/apisix/pull/10066#discussion_r1299959083
##########
apisix/init.lua:
##########
@@ -358,33 +358,20 @@ local function verify_https_client(ctx)
return true
end
- local matched_ssl = ngx.ctx.matched_ssl
- if matched_ssl.value.client
- and matched_ssl.value.client.skip_mtls_uri_regex
- and apisix_ssl.support_client_verification()
- and (not uri_matches_skip_mtls_route_patterns(matched_ssl, ngx.var.uri)) then
- local res = ctx.var.ssl_client_verify
- if res ~= "SUCCESS" then
- if res == "NONE" then
- core.log.error("client certificate was not present")
- else
- core.log.error("client certificate verification is not passed: ", res)
- end
-
- return false
- end
- end
-
local host = ctx.var.host
- local matched = router.router_ssl.match_and_set(ctx, true, host)
- if not matched then
- return true
+ if not ctx.matched_ssl then
+ local matched = router.router_ssl.match_and_set(ctx, true, host)
+ if not matched then
+ return true
+ end
end
local matched_ssl = ctx.matched_ssl
if matched_ssl.value.client and apisix_ssl.support_client_verification() then
local verified = apisix_base_flags.client_cert_verified_in_handshake
- if not verified then
+ local uri_in_whitelist = matched_ssl.value.client.skip_mtls_uri_regex
Review Comment:
Move skip-mtls logic here, because we need to check all requests within the same SSL connection or session.
--
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] Sn0rt commented on pull request #10066: fix: error on https requests due to nil matched_ssl
Posted by "Sn0rt (via GitHub)" <gi...@apache.org>.
Sn0rt commented on PR #10066:
URL: https://github.com/apache/apisix/pull/10066#issuecomment-1698441724
@Revolyssup need test case .
--
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] kingluo commented on a diff in pull request #10066: fix: error on https requests due to nil matched_ssl
Posted by "kingluo (via GitHub)" <gi...@apache.org>.
kingluo commented on code in PR #10066:
URL: https://github.com/apache/apisix/pull/10066#discussion_r1301248085
##########
apisix/init.lua:
##########
@@ -358,7 +358,15 @@ local function verify_https_client(ctx)
return true
end
- local matched_ssl = ngx.ctx.matched_ssl
+ local host = ctx.var.host
+ if not ctx.matched_ssl then
Review Comment:
However, it's a trivial stuff, because it only costs one more matching step.
--
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] Revolyssup commented on a diff in pull request #10066: fix: error on https requests due to nil matched_ssl
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #10066:
URL: https://github.com/apache/apisix/pull/10066#discussion_r1302831910
##########
apisix/init.lua:
##########
@@ -358,7 +358,15 @@ local function verify_https_client(ctx)
return true
end
- local matched_ssl = ngx.ctx.matched_ssl
+ local host = ctx.var.host
+ if not ctx.matched_ssl then
Review Comment:
done
--
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] hansedong commented on pull request #10066: fix: error on https requests due to nil matched_ssl
Posted by "hansedong (via GitHub)" <gi...@apache.org>.
hansedong commented on PR #10066:
URL: https://github.com/apache/apisix/pull/10066#issuecomment-1711321844
Is there any new progress? It seems that this test case is not easy to supplement...
--
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] kingluo commented on pull request #10066: fix: error on https requests due to nil matched_ssl
Posted by "kingluo (via GitHub)" <gi...@apache.org>.
kingluo commented on PR #10066:
URL: https://github.com/apache/apisix/pull/10066#issuecomment-1715632277
We can not reproduce the issue on the master branch anymore, because the commit https://github.com/apache/apisix/pull/9903 after 3.4.1: adds ssl_client_hello_by_lua_block. This phase used by apisix always constructs ngx.ctx.matched_ssl:
https://github.com/apache/apisix/blob/f47c2d79f7cf2d8618189924fc12a226c8420564/apisix/init.lua#L205-L207
--
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] Revolyssup commented on pull request #10066: fix: error on https requests due to nil matched_ssl
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on PR #10066:
URL: https://github.com/apache/apisix/pull/10066#issuecomment-1711327266
> Is there any new progress? It seems that this test case is not easy to supplement...
Yeah I'm on it. The test case is little tricky so it's taking a little 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] Revolyssup commented on pull request #10066: fix: error on https requests due to nil matched_ssl
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on PR #10066:
URL: https://github.com/apache/apisix/pull/10066#issuecomment-1687412221
> Could you explain the reason for the bug? why can't it be reproduced in the old version? And test cases are needed
@monkeyDluffy6017
Here[1] on version 3.3, `match_and_set` is called during verification and if matched is nil then it will skip verification and the nil error will never happen.
Here[2] on version 3.4, we try to extract `matched_ssl` from `ngx.ctx` and directly use it without nil check. This will be non nil only at the time of ssl handshake and after that it will be nil causing error.
1. https://github.com/apache/apisix/blob/d4e9b36dccec249e81733ebe6ee6df228a6e9a85/apisix/init.lua#L290
2. https://github.com/apache/apisix/blob/04b8bca4b60667aae45c463b7122b2dc1f4e70f2/apisix/init.lua#L331
--
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] Revolyssup commented on pull request #10066: fix: error on https requests due to nil matched_ssl
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on PR #10066:
URL: https://github.com/apache/apisix/pull/10066#issuecomment-1687491305
> 2\. e MTLS whitelist d
sure
--
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] kingluo commented on pull request #10066: fix: error on https requests due to nil matched_ssl
Posted by "kingluo (via GitHub)" <gi...@apache.org>.
kingluo commented on PR #10066:
URL: https://github.com/apache/apisix/pull/10066#issuecomment-1687508906
> And maybe you shoud also explain the reason in #9610, #9931, #9801
They're all due to the first bug, and the second is a side bug.
The test cases are a bit more difficult as they require HTTP keepalive or SSL session reuse tools, unfortunately, test::nginx and curl are not of any help. Maybe we need to write a simple golang program to simulate the second bug situation.
--
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] kingluo commented on a diff in pull request #10066: fix: error on https requests due to nil matched_ssl
Posted by "kingluo (via GitHub)" <gi...@apache.org>.
kingluo commented on code in PR #10066:
URL: https://github.com/apache/apisix/pull/10066#discussion_r1301246851
##########
apisix/init.lua:
##########
@@ -358,7 +358,15 @@ local function verify_https_client(ctx)
return true
end
- local matched_ssl = ngx.ctx.matched_ssl
+ local host = ctx.var.host
+ if not ctx.matched_ssl then
Review Comment:
Yes, should use ngx.ctx.matched_ssl instead. It's my typo.
--
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