Include trusted issuer details in SSL diagnostics

[tvernum]

This commit changes the SSL Diagnostic warning to include additional
details about trusted certificate issuers when the provide certificate
chain does not match any trust anchors.

• If there are no trusted issuers, this is explicitly called out
• If there is one trusted issuer, it is listed by name (DN) and fingerprint
• If there are between 2 and 9 trusted issuers, then they are listed
  by name (DN)
• If there are 10 or more trusted issuers, the number of issuers is
  included in the message (but no other details).

[elasticmachine]

Pinging @elastic/es-security (:Security/Network)

[BigPandaToo]

Not part of yoour change, but is this a valid case at all:
if (trustedIssuers == null) {
return "";
}
And if yes, should we add any diagnostic message?
I also noticed resolveCertificateTrust relys on trustedIssuers not being null.

[ywangd]

LGTM.

Also echoing @BigPandaToo's comment. I am not sure why trustedIssuers is annotated with @Nullable. But based on the code flow, it cannot be null. Maybe an assert for non-null is better?

[tvernum]

You're right that none of the current usage of SslDiagnostics can ever pass in a null trustedIssuers.
The intent was that it should be possible (we didn't want to restrict the usage of diagnostics to only those places where there were issuers available).
However, we don't have any tests for a null (and, as you point out @BigPandaToo describeSelfIssuedCertificate takes a nullable argument and then passes it to resolveCertificateTrust which assumes it is non-null). We should either test that it can be null, or remove the nullable checks. I would prefer that to keep it @Nullable because there's a big difference between no trusted issuers (empty map) and unknown trusted issuers (null map), and I think it's worth maintaining support for null, but it needs tests.

[ywangd]

because there's a big difference between no trusted issuers (empty map) and unknown trusted issuers (null map)

I am having hard time to understand what would be a real use case for "unkown trusted issuers". Could you please help me out here?

[tvernum]

I am having hard time to understand what would be a real use case for "unknown trusted issuers". Could you please help me out here?

We currently run the diagnostics from inside a trust manager, which means we can find out the list of trusted issuers and report on it.
However, there may be cases (and at one point were) where we catch the SSLException outside the trust manager and still want diagnostics for it.
Those diagnostics aren't very useful - there's only so much we can report on if we don't know which issuers are trusted - but it would be a mistake to pass in an empty Map of issuers, because that would imply that the caller knew that this context has no trusted issuers, and the diagnostic would report accordingly, which would be wrong.

The intent is to make it clear that if the diagnostics are ever used in a context where we don't know the trusted issuers, then it should be passed in as null, not emptyMap

[ywangd]

Thanks for the explanation. It makes sense now. Given the subtlety, I wonder whether it would be worthwhile to have a separate method as the entry point of the null case, where the parameter can be removed entirely.