SNI-based cert selection during TLS handshake#22036
SNI-based cert selection during TLS handshake#22036ggreenway merged 1 commit intoenvoyproxy:mainfrom
Conversation
|
@ggreenway Do we need to add release notes for this? |
|
I've been busy and haven't had time to review this yet; thanks for your patience. |
It's ok. :) Thanks for your response. |
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
|
/retest |
|
Retrying Azure Pipelines: |
|
@ggreenway would you be able to take a look today? |
ggreenway
left a comment
There was a problem hiding this comment.
Please add more test coverage:
- Multiple certs with multiple SANs; matching works correctly
- CN is not used if SANs are present
- Wildcard in SANs is matched properly
- Wildcard does not match if an exact match is present
- Wildcard only matches 1 level, eg *.example.com does not match a.b.example.com
- Config fails to load with conflicting SANs in certs, both exact and wildcard
- Any other new behavior you've added in this PR that isn't tested.
/wait
|
/retest |
|
Retrying Azure Pipelines: |
|
Please fix DCO (you may need to squash the entire thing and force push). Then we can do a final pass. Thanks. /wait |
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. Signed-off-by: Luyao Zhong <luyao.zhong@intel.com>
c019fef to
e62edc4
Compare
|
/retest |
|
Retrying Azure Pipelines: |
Done |
ggreenway
left a comment
There was a problem hiding this comment.
This looks good. Thanks for all the work on this!
| } | ||
|
|
||
| for (auto& ctx : tls_contexts_) { | ||
| bssl::UniquePtr<EVP_PKEY> public_key(X509_get_pubkey(ctx.cert_chain_.get())); |
There was a problem hiding this comment.
This crashes for cases when cert_chain is nullptr. It is missing nullptr check present in ContextImpl::getCertChainInformation()
There was a problem hiding this comment.
It already gets checked in loading phase. If the cert_chain is nullptr, envoy will log and raise exception, you could check following code path
envoy/source/extensions/transport_sockets/tls/context_impl.cc
Lines 198 to 204 in 8433082
So I don't think this will cause crash, and we can see similar code in constructor of its parent class, it checks nothing since it gets checked earlier.
envoy/source/extensions/transport_sockets/tls/context_impl.cc
Lines 215 to 216 in 8433082
I saw this change was reverted by #24475 , but no enough context for me, could you elaborate the example config that it will crash?
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>
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>
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.
Risk Level: Medium
Testing: unit tests
Docs Changes: ssl
Release Notes: yes
Fixes #21739
Signed-off-by: Luyao Zhong luyao.zhong@intel.com