tls: fix ssl.no_certificate doc and add ssl.no_certificate_for_upstream for upstream/cluster TLS handshakes#43669
tls: fix ssl.no_certificate doc and add ssl.no_certificate_for_upstream for upstream/cluster TLS handshakes#43669botengyao wants to merge 11 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Boteng Yao <boteng@google.com>
Signed-off-by: Boteng Yao <boteng@google.com>
|
/retest |
|
I am not sure it's a good idea to change the stats like this. Stats are actually an API, so changes should be done carefully. I suggest adding a new stats on whether client responded with to server requesting a client cert, only available on the upstream |
Signed-off-by: Boteng Yao <boteng@google.com>
Signed-off-by: Boteng Yao <boteng@google.com>
Signed-off-by: Boteng Yao <boteng@google.com>
Signed-off-by: Boteng Yao <boteng@google.com>
Signed-off-by: Boteng Yao <boteng@google.com>
botengyao
changed the title
Signed-off-by: Boteng Yao <boteng@google.com>
yes, agreed this is better, thanks for the feedback. |
|
@kyessenov @ggreenway, this is ready, PTAL, thanks! |
| :header: Name, Type, Description | ||
| :widths: 1, 1, 2 | ||
|
|
||
| no_certificate_for_upstream, Counter, Total successful upstream TLS connections with no client certificate provided |
There was a problem hiding this comment.
I think I'll defer to @ggreenway here, but I think it'd make more sense to do it the other, e.g. have client_certificate stat that's incremented when client presents a cert. mTLS is less common than TLS, so it'd make sense for the default to be zeroes.
There was a problem hiding this comment.
yea, good suggestion, and I also don't have a strong opinion. changes done.
Signed-off-by: Boteng Yao <boteng@google.com>
|
|
||
| const std::string server_ctx_yaml = R"EOF( | ||
| common_tls_context: | ||
| tls_certificates: |
There was a problem hiding this comment.
This is actually not an mTLS setup, or am I missing something? There should be request-client-cert option enabled.
There was a problem hiding this comment.
yes, this is intentional, but I think the real way to say it is mTLS from client side is to add a callback when server asks certificate from client, so the stats is only fired when there is a cert from client side and the server is asking it.
I updated a new version of this support, but more code, PTAL for which one do you prefer.
Signed-off-by: Boteng Yao <boteng@google.com>
botengyao
force-pushed
the
fix-tls-no-cert-stats
branch
from
4225e1a to
739f353
Compare
| } | ||
|
|
||
| // Delegate to the cert selector if one exists. | ||
| if (context_impl->tls_certificate_selector_ != nullptr) { |
There was a problem hiding this comment.
I think if you have to do this - a default cert selector would be cleaner, and then you can track the state in the handshaker directly. This would be the same as the server side, which always uses a cert selector.
But it's worth double checking whether bssl already records this information somewhere.
3 participants
https://www.envoyproxy.io/docs/envoy/latest/configuration/upstream/cluster_manager/cluster_stats.html#tls-statistics
ssl.no_certificatestat was never incremented on cluster (client-side) TLS contexts and update the doc to use peer.ssl.client_certificate_presented, which is useful for cluster metrics to detect mtlsRisk Level: low
Testing: unit