From a4e631c66ef9a94723a336fb4a3a8e6e845b6614 Mon Sep 17 00:00:00 2001
From: Andrew Cholakian <andrew@andrewvc.com>
Date: Fri, 22 Nov 2019 15:06:31 -0600
Subject: [PATCH] [Heartbeat] Record TLS Metadata for expired/invalid certs
 (#14588) (#14621)

This patch fixes https://github.com/elastic/beats/issues/13687 .

Previously heartbeat would only traverse valid x509 cert chains, with
this PR it now traverses all certs provided by the server.

(cherry picked from commit eff54c3069a63fe5c91b78c4edf15a6505c56759)
---
 CHANGELOG.next.asciidoc                       |  1 +
 heartbeat/monitors/active/dialchain/tls.go    | 18 +++++------
 .../monitors/active/dialchain/tls_test.go     | 31 ++++++++++++++++---
 3 files changed, 35 insertions(+), 15 deletions(-)

diff --git a/CHANGELOG.next.asciidoc b/CHANGELOG.next.asciidoc
index cbe47de2116..d21a1fb7d40 100644
--- a/CHANGELOG.next.asciidoc
+++ b/CHANGELOG.next.asciidoc
@@ -52,6 +52,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d
 
 *Heartbeat*
 
+- Fix recording of SSL cert metadata for Expired/Unvalidated x509 certs. {pull}13687[13687]
 
 *Journalbeat*
 
diff --git a/heartbeat/monitors/active/dialchain/tls.go b/heartbeat/monitors/active/dialchain/tls.go
index c613311fe9d..f7f6de720d3 100644
--- a/heartbeat/monitors/active/dialchain/tls.go
+++ b/heartbeat/monitors/active/dialchain/tls.go
@@ -62,14 +62,14 @@ func TLSLayer(cfg *transport.TLSConfig, to time.Duration) Layer {
 			timer.stop()
 			event.PutValue("tls.rtt.handshake", look.RTT(timer.duration()))
 
-			addCertMetdata(event.Fields, tlsConn.ConnectionState().VerifiedChains)
+			addCertMetdata(event.Fields, tlsConn.ConnectionState().PeerCertificates)
 
 			return conn, nil
 		}), nil
 	}
 }
 
-func addCertMetdata(fields common.MapStr, chains [][]*x509.Certificate) {
+func addCertMetdata(fields common.MapStr, certs []*x509.Certificate) {
 	// The behavior here might seem strange. We *always* set a notBefore, but only optionally set a notAfter.
 	// Why might we do this?
 	// The root cause is that the x509.Certificate type uses time.Time for these fields instead of *time.Time
@@ -94,15 +94,13 @@ func addCertMetdata(fields common.MapStr, chains [][]*x509.Certificate) {
 	// To do this correctly, we take the maximum NotBefore and the minimum NotAfter.
 	// This *should* always wind up being the terminal cert in the chain, but we should
 	// compute this correctly.
-	for _, chain := range chains {
-		for _, cert := range chain {
-			if chainNotValidBefore.Before(cert.NotBefore) {
-				chainNotValidBefore = cert.NotBefore
-			}
+	for _, cert := range certs {
+		if chainNotValidBefore.Before(cert.NotBefore) {
+			chainNotValidBefore = cert.NotBefore
+		}
 
-			if cert.NotAfter != zeroTime && (chainNotValidAfter == nil || chainNotValidAfter.After(cert.NotAfter)) {
-				chainNotValidAfter = &cert.NotAfter
-			}
+		if cert.NotAfter != zeroTime && (chainNotValidAfter == nil || chainNotValidAfter.After(cert.NotAfter)) {
+			chainNotValidAfter = &cert.NotAfter
 		}
 	}
 
diff --git a/heartbeat/monitors/active/dialchain/tls_test.go b/heartbeat/monitors/active/dialchain/tls_test.go
index 043b86dc899..b281bf602e9 100644
--- a/heartbeat/monitors/active/dialchain/tls_test.go
+++ b/heartbeat/monitors/active/dialchain/tls_test.go
@@ -44,6 +44,19 @@ func Test_addCertMetdata(t *testing.T) {
 		BasicConstraintsValid: true,
 	}
 
+	expiredNotAfter := time.Now().Add(-time.Hour)
+	expiredCert := x509.Certificate{
+		SerialNumber: big.NewInt(1),
+		Subject: pkix.Name{
+			Organization: []string{"Acme Co"},
+		},
+		NotBefore:             goodNotBefore,
+		NotAfter:              expiredNotAfter,
+		KeyUsage:              x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature,
+		ExtKeyUsage:           []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth},
+		BasicConstraintsValid: true,
+	}
+
 	missingNotBeforeCert := x509.Certificate{
 		SerialNumber: big.NewInt(1),
 		Subject: pkix.Name{
@@ -76,27 +89,35 @@ func Test_addCertMetdata(t *testing.T) {
 	}
 	tests := []struct {
 		name     string
-		chains   [][]*x509.Certificate
+		certs    []*x509.Certificate
 		expected expected
 	}{
 		{
 			"Valid cert",
-			[][]*x509.Certificate{{&goodCert}},
+			[]*x509.Certificate{&goodCert},
 			expected{
 				notBefore: goodNotBefore,
 				notAfter:  &goodNotAfter,
 			},
 		},
+		{
+			"Expired cert",
+			[]*x509.Certificate{&expiredCert},
+			expected{
+				notBefore: goodNotBefore,
+				notAfter:  &expiredNotAfter,
+			},
+		},
 		{
 			"Missing not before",
-			[][]*x509.Certificate{{&missingNotBeforeCert}},
+			[]*x509.Certificate{&missingNotBeforeCert},
 			expected{
 				notAfter: &goodNotAfter,
 			},
 		},
 		{
 			"Missing not after",
-			[][]*x509.Certificate{{&missingNotAfterCert}},
+			[]*x509.Certificate{&missingNotAfterCert},
 			expected{
 				notBefore: goodNotBefore,
 			},
@@ -105,7 +126,7 @@ func Test_addCertMetdata(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			event := common.MapStr{}
-			addCertMetdata(event, tt.chains)
+			addCertMetdata(event, tt.certs)
 			v, err := event.GetValue("tls.certificate_not_valid_before")
 			assert.NoError(t, err)
 			assert.Equal(t, tt.expected.notBefore, v)