Improved diagnostics for TLS trust failures #48911
Conversation
- Improves HTTP client hostname verification failure messages - Adds "DiagnosticTrustManager" which logs certificate information when trust cannot be established (hostname failure, CA path failure, etc) These diagnostic messages are designed so that many common TLS problems can be diagnosed based solely (or primarily) on the elasticsearch logs.
|
Pinging @elastic/es-security (:Security/Network) |
libs/ssl-config/src/main/java/org/elasticsearch/common/ssl/SslUtil.java
Outdated
Show resolved
Hide resolved
libs/ssl-config/src/main/java/org/elasticsearch/common/ssl/SslDiagnostics.java
Outdated
Show resolved
Hide resolved
libs/ssl-config/src/main/java/org/elasticsearch/common/ssl/SslDiagnostics.java
Outdated
Show resolved
Hide resolved
| } | ||
| final List<String> description = new ArrayList<>(names.size()); | ||
| for (List<?> pair : names) { | ||
| if (pair == null || pair.size() != 2) { |
There was a problem hiding this comment.
could this be changed to something like :
if ( pair != null && pair.size() == 2 && pair.get(0) instanceof Integer && pair.get(1) instanceof String) {
final int type = ((Integer) pair.get(0)).intValue();
final String name = (String) pair.get(1);
if ( type == 2 ) {
description.add("DNS:" + name);
else if ( type == 7 ) {
description.add("IP:" + name);
}
}it seems more concise, but just a suggestion
There was a problem hiding this comment.
I replaced the inner-switch with if/else but I found the merged outer-if to be too complex to reason about, so I left the existing version.
| final StringBuilder message = new StringBuilder(); | ||
| final CertificateTrust trust = isTrusted(trustedIssuers, certificate); | ||
| message.append("self-issued [").append(certificate.getIssuerX500Principal().getName()).append("] and is ") | ||
| .append(trust.isTrusted() ? "trusted" : "not trusted") |
There was a problem hiding this comment.
I find the
the certificate is signed by (subject [CN=Test CA 1] fingerprint [A] {trusted issuer}) which is self-issued [CN=Test CA 1] and is trusted in this ssl context ([xpack.security.authc.realms.saml.saml1.ssl]) using certificates with fingerprint [X,Y,Z] but the same public key
somewhat hard to unpack. Could we add a sentence to the effect of "We trust the public key but not the certificate" ? I don't have a good suggestion atm.. :/
There was a problem hiding this comment.
I've changed it to
the certificate is signed by (subject [CN=Test CA 1] fingerprint [2b7b0416391bdf86502505c23149022d2213dadc] {trusted issuer}) which is self-issued; the [CN=Test CA 1] certificate is trusted in this ssl context ([xpack.security.authc.realms.saml.saml1.ssl]) because we trust a certificate with fingerprint [1f8ac10f23c5b5bc1167bda84b833e5c057a77d2] for the same public key
x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ssl/CertParsingUtils.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ssl/SSLService.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ssl/SSLService.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ssl/SSLService.java
Show resolved
Hide resolved
| final Supplier<String> contextName = () -> { | ||
| final List<String> names = sslConfigurations.entrySet().stream() | ||
| .filter(e -> e.getValue().equals(configuration)) | ||
| .limit(2) |
There was a problem hiding this comment.
is this because we don't care if we have more than 2 here as the name would be the same ?
There was a problem hiding this comment.
Yes, a (possibly silly) micro-optimisation because the only cases we care about are 0/1/many
libs/ssl-config/src/main/java/org/elasticsearch/common/ssl/SslDiagnostics.java
Outdated
Show resolved
Hide resolved
Co-Authored-By: Ioannis Kakavas <[email protected]>
|
I realised I forgot to document the new setting. I will add that before merging. |
|
@jkakavas Can you review the docs change please? |
- Improves HTTP client hostname verification failure messages
- Adds "DiagnosticTrustManager" which logs certificate information
when trust cannot be established (hostname failure, CA path failure,
etc)
These diagnostic messages are designed so that many common TLS
problems can be diagnosed based solely (or primarily) on the
elasticsearch logs.
These diagnostics can be disabled by setting
xpack.security.ssl.diagnose.trust: false
Backport of: elastic#48911
- Improves HTTP client hostname verification failure messages
- Adds "DiagnosticTrustManager" which logs certificate information
when trust cannot be established (hostname failure, CA path failure,
etc)
These diagnostic messages are designed so that many common TLS
problems can be diagnosed based solely (or primarily) on the
elasticsearch logs.
These diagnostics can be disabled by setting
xpack.security.ssl.diagnose.trust: false
Backport of: #48911
when trust cannot be established (hostname failure, CA path failure,
etc)
These diagnostic messages are designed so that many common TLS
problems can be diagnosed based solely (or primarily) on the
elasticsearch logs.