tls: SNI-based cert selection during TLS handshake (#22036)#24483
tls: SNI-based cert selection during TLS handshake (#22036)#24483ggreenway merged 3 commits intoenvoyproxy:mainfrom
Conversation
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
|
resubmit PR due to rollback by #24475, don't merge until https://github.com/envoyproxy/envoy/pull/22036/files#r1044579979 is addressed. |
Envoy supports selecting certs by selecting filter chain based on SNI. But it is possible that we access different services via one filter chain, which requires SNI-based cert selection in one single filter chain during handshake. This change is merged by envoyproxy#22036 and reverted by envoyproxy#24475. Signed-off-by: Luyao Zhong <luyao.zhong@intel.com>
e185630 to
22228c9
Compare
|
/wait |
|
@LuyaoZhong can you break this into two commits? One being the revert of the revert, which should be the original change you made, and the 2nd commit for the fix/change that is different from the original PR? You'll have to force-push to the PR branch, but that's fine in this case. |
Actually I think the root cause of crash is not missing nullptr check, since it gets checked in earlier place. I reply on https://github.com/envoyproxy/envoy/pull/22036/files#r1044579979. And I would like @yanavlasov and @KBaichoo can provide me more context about the crash case. And I'm OK with adding checking as a second safeguard. Will append a commit for that change. |
After dig the code, I got that when user has its own handshaker to provide certificates, it might trigger the bug. |
|
Thank you @LuyaoZhong! Yes, I think the problem was with using a custom handshaker, this looks good. Can you add a test we could add to capture this? |
@KBaichoo I think this should be tested in concrete handshaker extension test cases, but there is no any custom handshaker extension in envoy source code. The cert should not be null commonly, this check is just a safeguard to avoid crash. |
KBaichoo
left a comment
There was a problem hiding this comment.
That's a fair point, thank you for fixing. It LGTM.
@KBaichoo I think this should be tested in concrete handshaker extension test cases, but there is no any custom handshaker extension in envoy source code
cc @yanavlasov
|
There are at least some tests that involve custom handshakes here. Can this be modified to add a test case for this? |
When custom handshaker provides certificates, it's possible that no certificate is loaded correctly and become a null pointer. Skip the null cert when populate the map used for SNI-based certificate selection. Signed-off-by: Luyao Zhong <luyao.zhong@intel.com>
2d6a482 to
7bf2a8d
Compare
Thanks. SslHandshakerImplForTest can not help here, but I found HandshakerFactoryImplForTest can be a test base. I force push the new testcase to second commit, please take a look. |
|
/retest |
|
Retrying Azure Pipelines: |
ggreenway
left a comment
There was a problem hiding this comment.
This looks good; thanks for writing the test. There's a gcc error and a clang-tidy error to fix to make CI pass, but other than that I think this is ready to merge.
/wait
test/extensions/transport_sockets/tls/handshaker_factory_test.cc
Outdated
Show resolved
Hide resolved
source/extensions/transport_sockets/tls/context_manager_impl.cc
Outdated
Show resolved
Hide resolved
|
/wait confirming the test correctness |
Envoy supports selecting certs by selecting filter chain based on SNI. But it is possible that we access different services via one filter chain, which requires SNI-based cert selection in one single filter chain during handshake.
This change is merged by #22036 and reverted by #24475.
Signed-off-by: Luyao Zhong luyao.zhong@intel.com