Add KeyUsage, ExtendedKeyUsage, CipherSuite & Protocol to SSL diagnos… #65634

sindhusp · 2020-11-30T19:55:19Z

cla-checker-service · 2020-11-30T19:55:22Z

💚 CLA has been signed

sindhusp · 2020-11-30T20:24:04Z

Hi @tvernum , I signed the Contributor Agreement successfully, but that check is failing on my PR. Any thoughts on what I could do to verify the signing?

tvernum · 2020-12-01T05:01:44Z

@sindhusp The issue seems to be that the email address you used in your git commit is not the same email address as you used to sign the CLA.
and your commit doesn't link back to your GitHub account so we can't verify it based on your github userid.

Specifically your CLA has a gmail address, but your commit has a zeta.tech address.
And, it doesn't look like GitHub knows about your zeta.tech address either, so it doesn't know that this: sindhusp@3d85623 is you - the author on that commit is just a name, but if you compare it to one of my commits GitHub knows that it's me, and links it to my GitHub account.

I think what you need to do is make sure your GitHub profile has both your zeta.tech email address and your GMail address.

elasticmachine · 2020-12-01T05:04:07Z

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

tvernum

Thanks for your contribution, I have some suggested changes.

tvernum · 2020-12-01T05:51:12Z

libs/ssl-config/src/main/java/org/elasticsearch/common/ssl/SslDiagnostics.java

+
+    private static String keyUsageDescription(X509Certificate certificate) {
+        return Optional.ofNullable(certificate.getKeyUsage())
+            .map(keyUsage -> "keyUsage [" + Arrays.toString(keyUsage) + "]")


getKeyUsage() returns boolean[], but we need to decode it into the actual usage types (see the Javadoc for getKeyUsage for a description of each index)
Output like

keyUsage [true,true,false,false,true,false,true,false,false]

isn't explanatory enough for this diagnostic message.

tvernum · 2020-12-01T05:51:49Z

libs/ssl-config/src/main/java/org/elasticsearch/common/ssl/SslDiagnostics.java

@@ -178,7 +179,13 @@ public static String getTrustDiagnosticFailure(X509Certificate[] chain, PeerType
            .append(" provided a certificate with subject name [")
            .append(peerCert.getSubjectX500Principal().getName())
            .append("] and ")


Suggested change

.append("] and ")

.append("], ")

tvernum · 2020-12-01T05:51:58Z

libs/ssl-config/src/main/java/org/elasticsearch/common/ssl/SslDiagnostics.java

@@ -178,7 +179,13 @@ public static String getTrustDiagnosticFailure(X509Certificate[] chain, PeerType
            .append(" provided a certificate with subject name [")
            .append(peerCert.getSubjectX500Principal().getName())
            .append("] and ")
-            .append(fingerprintDescription(peerCert));
+            .append(fingerprintDescription(peerCert))
+            .append(" and ")


Suggested change

.append(" and ")

.append(", ")

tvernum · 2020-12-01T05:59:32Z

libs/ssl-config/src/main/java/org/elasticsearch/common/ssl/SslDiagnostics.java

+    }
+
+    private static String generateExtendedKeyUsageDescription(List<String> list) {
+        return list.stream()


getExtendedKeyUsage() returns a List of OIDs.
For the standard OIDs (defined here https://tools.ietf.org/html/rfc5280#page-44), we need to decode those OIDs into useful names.
Non standard OIDs should be left in their dotted notation.

tvernum · 2020-12-01T06:00:15Z

libs/ssl-config/src/main/java/org/elasticsearch/common/ssl/SslDiagnostics.java

+    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 the cipher suite [")


For brevity we can omit articles from the diagnostic

Suggested change

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

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

tvernum · 2020-12-01T06:00:39Z

libs/ssl-config/src/main/java/org/elasticsearch/common/ssl/SslDiagnostics.java

+            .append("] and ")
+            .append("the protocol [")


Suggested change

.append("] and ")

.append("the protocol [")

.append("] and protocol [")

tvernum · 2020-12-01T06:02:26Z

libs/ssl-config/src/test/java/org/elasticsearch/common/ssl/SslDiagnosticsTests.java

@@ -63,7 +63,8 @@ public void testDiagnosticMessageWhenServerProvidesAFullCertChainThatIsTrusted()
        final String message = SslDiagnostics.getTrustDiagnosticFailure(chain, SslDiagnostics.PeerType.SERVER, session,
            "xpack.http.ssl", trustIssuers);
        assertThat(message, Matchers.equalTo("failed to establish trust with server at [192.168.1.1];" +
-            " the server provided a certificate with subject name [CN=cert1] and fingerprint [3bebe388a66362784afd6c51a9000961a4e10050];" +
+            " the server provided a certificate with subject name [CN=cert1] and fingerprint [3bebe388a66362784afd6c51a9000961a4e10050] and no keyUsage and no extendedKeyUsage;" +
+            " the session supports the cipher suite [<unknown cipherSuite>] and the protocol [<unknown protocol>];" +


We need some of the tests to have real values for all of these.
As it is we're only testing the "no value" cases, which is the least important case.

That will require regenerating some of the certs (and updating the fingerprints) to have key usage properties.
(see libs/ssl-config/src/test/resources/certs/README.txt)
and/or changing mockCertificateWithIssuer and session to provide mock values.

I've edited the mockCertificateWithIssuer and session methods to provide mock values. I attempted to regenerate the cert yesterday without much success, so went with the mocking. Let me know if this works.

…tics

sindhusp · 2020-12-01T07:11:32Z

@tvernum thanks, my CLA problem is sorted, I've amended my commit to point to my gmail ID.
Thanks for the quick review, I'm addressing your comments.

* Fix formatting * Add descriptive keyUsage, extendedKeyUsage msgs * Mock cert's keyUsage, extendedKeyUsage * Mock session's cipherSuite, protocol

sindhusp · 2020-12-01T17:48:02Z

@tvernum, I have addressed your comments. Please let me know if you have more suggestions.

sindhusp · 2020-12-01T17:56:40Z

libs/ssl-config/src/main/java/org/elasticsearch/common/ssl/SslDiagnostics.java

+            return "no keyUsage";
+        }
+
+        final String[] keyUsageGlossary = {"digitalSignature", "nonRepudiation", "keyEncipherment",


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

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

sindhusp · 2020-12-04T10:12:32Z

A little nudge for review @tvernum :)

sindhusp · 2020-12-16T08:57:25Z

@tvernum bump for review.. Is it okay to remind this way? Is there someone else I can loop in for this PR?

tvernum · 2020-12-23T06:24:04Z

He @sindhusp, Pinging here is fine but we're just a bit short of time at the moment and this PR has had to take a back seat. I hope to have time next week.

tvernum · 2021-02-09T03:53:12Z

Hi @sindhusp Sorry for taking so long on this - can you let me know if you're still interested in working on this?

If you don't have time, then someone from Elastic can finish it.

tvernum · 2021-02-09T03:55:06Z

libs/ssl-config/src/main/java/org/elasticsearch/common/ssl/SslDiagnostics.java

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


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.

tvernum · 2021-02-09T03:57:24Z

libs/ssl-config/src/main/java/org/elasticsearch/common/ssl/SslDiagnostics.java

+            return "no keyUsage";
+        }
+
+        final String[] keyUsageGlossary = {"digitalSignature", "nonRepudiation", "keyEncipherment",


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

tvernum · 2021-02-09T04:01:35Z

libs/ssl-config/src/main/java/org/elasticsearch/common/ssl/SslDiagnostics.java

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


For safety we should check that i < keyUsageGlossary.length

tvernum · 2021-02-09T04:08:36Z

libs/ssl-config/src/main/java/org/elasticsearch/common/ssl/SslDiagnostics.java

+        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)


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.

libs/ssl-config/src/main/java/org/elasticsearch/common/ssl/SslDiagnostics.java

tvernum · 2021-02-09T04:11:48Z

libs/ssl-config/src/main/java/org/elasticsearch/common/ssl/SslDiagnostics.java

+        String protocol = Optional.ofNullable(session)
+            .map(SSLSession::getProtocol)
+            .orElse("<unknown protocol>");
+        message.append("; the session supports cipher suite [")


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"

sindhusp · 2021-03-05T10:10:51Z

@tvernum thanks for the comments, I didn't see it earlier. I shall address them today.

tvernum · 2021-08-13T04:04:47Z

@elasticmachine test this please 🙏

tvernum · 2021-08-13T07:12:50Z

@elasticmachine test this please 🙏

tvernum · 2021-08-13T07:14:21Z

@jkakavas I made some tweaks to this PR - can you give it a quick look?

jkakavas

LGTM, double checked key usage and extended key usage in relevant RFCs and messaging looks fine to me

…stics This commit extends the SSL diagnostics message to include descriptions of the - The KeyUsage and ExtendedKeyUsage of the peer's certificate - The CipherSuite & Protocol (TLS/SSL version) of the current session These can be helpful in diagnosing SSL errors. Co-authored-by: Tim Vernum <tim@adjective.org> Backport of: elastic#65634

* master: (868 commits) Query API key - Rest spec and yaml tests (elastic#76238) Delay shard reassignment from nodes which are known to be restarting (elastic#75606) Reenable bwc tests for elastic#76475 (elastic#76576) Set version to 7.15 in BWC code (elastic#76577) Don't remove warning headers on all failure (elastic#76434) Disable bwc tests for elastic#76475 (elastic#76541) Re-enable bwc tests (elastic#76567) Keep track of data recovered from snapshots in RecoveryState (elastic#76499) [Transform] Align transform checkpoint range with date_histogram interval for better performance (elastic#74004) EQL: Remove "wildcard" function (elastic#76099) Fix 'accept' and 'content_type' fields for search_mvt API Add persistent licensed feature tracking (elastic#76476) Add system data streams to feature state snapshots (elastic#75902) fix the error message for instance methods that don't exist (elastic#76512) ILM: Add validation of the number_of_shards parameter in Shrink Action of ILM (elastic#74219) remove dashboard only reserved role (elastic#76507) Fix Stack Overflow in UnassignedInfo in Corner Case (elastic#76480) Add (Extended)KeyUsage KeyUsage, CipherSuite & Protocol to SSL diagnostics (elastic#65634) Add recovery from snapshot to tests (elastic#76535) Reenable BwC Tests after elastic#76532 (elastic#76534) ...

tvernum self-requested a review December 1, 2020 04:48

tvernum added :Security/TLS SSL/TLS, Certificates >enhancement v7.11.0 v8.0.0 labels Dec 1, 2020

elasticmachine added the Team:Security Meta label for security team label Dec 1, 2020

tvernum reviewed Dec 1, 2020

View reviewed changes

Add KeyUsage, ExtendedKeyUsage, CipherSuite & Protocol to SSL diagnos…

88ecbd0

…tics

sindhusp force-pushed the sslDiagnostics branch from 3d85623 to 88ecbd0 Compare December 1, 2020 07:08

Review comments

f6af049

* Fix formatting * Add descriptive keyUsage, extendedKeyUsage msgs * Mock cert's keyUsage, extendedKeyUsage * Mock session's cipherSuite, protocol

sindhusp force-pushed the sslDiagnostics branch from d9f814e to f6af049 Compare December 1, 2020 17:46

sindhusp commented Dec 1, 2020

View reviewed changes

sindhusp requested a review from tvernum December 1, 2020 18:07

pugnascotia added v7.12.0 and removed v7.11.0 labels Jan 5, 2021

tvernum reviewed Feb 9, 2021

View reviewed changes

williamrandolph added v7.13.0 and removed v7.12.0 labels Feb 18, 2021

pugnascotia removed the v7.13.0 label Apr 21, 2021

pugnascotia added the v7.14.0 label Apr 21, 2021

mark-vieira added v7.15.0 and removed v7.14.0 labels Jun 30, 2021

tvernum added 2 commits August 13, 2021 13:06

Merge branch 'master' into sslDiagnostics

a7c6be0

Tweak implementation and add more tests

3752caf

tvernum self-assigned this Aug 13, 2021

Fix broken test

e23a504

tvernum requested a review from jkakavas August 13, 2021 07:13

jkakavas approved these changes Aug 13, 2021

View reviewed changes

tvernum merged commit f4e3f33 into elastic:master Aug 16, 2021

jakelandis added v8.0.0-alpha2 and removed v8.0.0 labels Sep 15, 2021

sindhusp deleted the sslDiagnostics branch February 19, 2023 10:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add KeyUsage, ExtendedKeyUsage, CipherSuite & Protocol to SSL diagnos… #65634

Add KeyUsage, ExtendedKeyUsage, CipherSuite & Protocol to SSL diagnos… #65634

sindhusp commented Nov 30, 2020 •

edited

Loading

cla-checker-service bot commented Nov 30, 2020 •

edited

Loading

sindhusp commented Nov 30, 2020

tvernum commented Dec 1, 2020

elasticmachine commented Dec 1, 2020

tvernum left a comment

tvernum Dec 1, 2020

tvernum Dec 1, 2020

tvernum Dec 1, 2020

tvernum Dec 1, 2020

tvernum Dec 1, 2020

tvernum Dec 1, 2020

tvernum Dec 1, 2020

sindhusp Dec 1, 2020

sindhusp commented Dec 1, 2020

sindhusp commented Dec 1, 2020

sindhusp Dec 1, 2020

tvernum Feb 9, 2021

sindhusp commented Dec 4, 2020

sindhusp commented Dec 16, 2020

tvernum commented Dec 23, 2020

tvernum commented Feb 9, 2021 •

edited

Loading

tvernum Feb 9, 2021

tvernum Feb 9, 2021

tvernum Feb 9, 2021

tvernum Feb 9, 2021

tvernum Feb 9, 2021

sindhusp commented Mar 5, 2021

tvernum commented Aug 13, 2021

tvernum commented Aug 13, 2021

tvernum commented Aug 13, 2021

jkakavas left a comment

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

	.append("] and ")
	.append("the protocol [")
	.append("] and protocol [")

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

Add KeyUsage, ExtendedKeyUsage, CipherSuite & Protocol to SSL diagnos… #65634

Add KeyUsage, ExtendedKeyUsage, CipherSuite & Protocol to SSL diagnos… #65634

Conversation

sindhusp commented Nov 30, 2020 • edited Loading

cla-checker-service bot commented Nov 30, 2020 • edited Loading

sindhusp commented Nov 30, 2020

tvernum commented Dec 1, 2020

elasticmachine commented Dec 1, 2020

tvernum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindhusp commented Dec 1, 2020

sindhusp commented Dec 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindhusp commented Dec 4, 2020

sindhusp commented Dec 16, 2020

tvernum commented Dec 23, 2020

tvernum commented Feb 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindhusp commented Mar 5, 2021

tvernum commented Aug 13, 2021

tvernum commented Aug 13, 2021

tvernum commented Aug 13, 2021

jkakavas left a comment

Choose a reason for hiding this comment

sindhusp commented Nov 30, 2020 •

edited

Loading

cla-checker-service bot commented Nov 30, 2020 •

edited

Loading

tvernum commented Feb 9, 2021 •

edited

Loading