Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@
import java.util.Map;
import java.util.Optional;
import java.util.stream.Collectors;
import java.util.Arrays;
import java.util.stream.IntStream;

public class SslDiagnostics {

Expand Down Expand Up @@ -151,6 +153,30 @@ boolean isSameCertificate() {
}
}

public enum ExtendedKeyUsage {
serverAuth ("1.3.6.1.5.5.7.3.1"),
clientAuth ("1.3.6.1.5.5.7.3.2"),
codeSigning ("1.3.6.1.5.5.7.3.3"),
emailProtection ("1.3.6.1.5.5.7.3.4"),
timeStamping ("1.3.6.1.5.5.7.3.8"),
ocspSigning ("1.3.6.1.5.5.7.3.9");

private String oid;

private ExtendedKeyUsage(String oid) {
this.oid = oid;
}

public static String decodeOid(String oid) {
for (ExtendedKeyUsage e : values()) {
if (e.oid != null && e.oid.equals(oid)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.oid can never be null, because this is an enum with fixed (non-null) values.
I think it's fine to skip the null check here.

If you'd prefer to be safe, you could use this.oid = Objects.requireNonNull(oid); in the constructor.

return e.name();
}
}
return oid;
}
}

/**
* @param contextName The descriptive name of this SSL context (e.g. "xpack.security.transport.ssl")
* @param trustedIssuers A Map of DN to Certificate, for the issuers that were trusted in the context in which this failure occurred
Expand All @@ -177,8 +203,14 @@ public static String getTrustDiagnosticFailure(X509Certificate[] chain, PeerType
.append(peerType.name().toLowerCase(Locale.ROOT))
.append(" provided a certificate with subject name [")
.append(peerCert.getSubjectX500Principal().getName())
.append("] and ")
.append(fingerprintDescription(peerCert));
.append("], ")
.append(fingerprintDescription(peerCert))
.append(", ")
.append(keyUsageDescription(peerCert))
.append(" and ")
.append(extendedKeyUsageDescription(peerCert));

addSessionDescription(session, message);

if (peerType == PeerType.SERVER) {
try {
Expand Down Expand Up @@ -406,4 +438,58 @@ private static boolean checkIssuer(X509Certificate certificate, X509Certificate
private static boolean isSelfIssued(X509Certificate certificate) {
return certificate.getIssuerX500Principal().equals(certificate.getSubjectX500Principal());
}

private static String keyUsageDescription(X509Certificate certificate) {
boolean[] keyUsage = certificate.getKeyUsage();
if (keyUsage == null || keyUsage.length == 0) {
return "no keyUsage";
}

final String[] keyUsageGlossary = {"digitalSignature", "nonRepudiation", "keyEncipherment",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I use an enum here too like I've done with ExtendedKeyUsage, for uniformity? I could define the index as the Enum field.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think either is fine.
I do like the consistency of having an enum for each, but it would require care so that the ordinals matched correctly.

I think my personal preference would be to just move this array to be a constant that is declared in the same part of the file as ExtendedKeyUsage

"dataEncipherment", "keyAgreement", "keyCertSign", "cRLSign", "encipherOnly",
"decipherOnly"};

List<String> keyUsageDescription = new ArrayList<>();
IntStream.range(0, keyUsage.length).forEach(i -> {
if (keyUsage[i]) {
keyUsageDescription.add(keyUsageGlossary[i]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For safety we should check that i < keyUsageGlossary.length

}
});
return keyUsageDescription.stream()
.reduce((a, b) -> a + ", " + b)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The style use have used here is fine, but I would have written it like this:

Suggested change
List<String> keyUsageDescription = new ArrayList<>();
IntStream.range(0, keyUsage.length).forEach(i -> {
if (keyUsage[i]) {
keyUsageDescription.add(keyUsageGlossary[i]);
}
});
return keyUsageDescription.stream()
.reduce((a, b) -> a + ", " + b)
String keyUsageDescription = IntStream.range(0, keyUsage.length)
.filter(i -> keyUsage[i])
.map(i -> i < keyUsageGlossary.length ? keyUsageGlossary[i] : "#" + i)
.collect(Collectors.joining(", "));

And then tested whether that was empty or not.

.map(str -> "keyUsage [" + str + "]")
.orElse("no keyUsage");
}

private static String extendedKeyUsageDescription(X509Certificate certificate) {
try {
return Optional.ofNullable(certificate.getExtendedKeyUsage())
.map(keyUsage -> generateExtendedKeyUsageDescription(keyUsage))
.orElse("no extendedKeyUsage");
} catch (CertificateParsingException e) {
return "invalid extendedKeyUsage [" + e.toString() + "]";
}
}

private static String generateExtendedKeyUsageDescription(List<String> oids) {
return oids.stream()
.map(ExtendedKeyUsage::decodeOid)
.reduce((x, y) -> x + ", " + y)
.map(str -> "extendedKeyUsage [" + str + "]")
.orElse("no extendedKeyUsage");
}

private static void addSessionDescription(SSLSession session, StringBuilder message) {
String cipherSuite = Optional.ofNullable(session)
.map(SSLSession::getCipherSuite)
.orElse("<unknown cipherSuite>");
String protocol = Optional.ofNullable(session)
.map(SSLSession::getProtocol)
.orElse("<unknown protocol>");
message.append("; the session supports cipher suite [")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
message.append("; the session supports cipher suite [")
message.append("; the session uses cipher suite [")

Strictly, by the time we get to a session being established it's not really "supports", it's "has selected for use"

.append(cipherSuite)
.append("] and protocol [")
.append(protocol)
.append("]");
}
}
Loading