[Heartbeat] Handle TLS certs missing notBefore/notAfter

CHANGELOG.asciidoc

@@ -76,6 +76,9 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d
 
 *Heartbeat*
 
+- Fixed rare issue where TLS connections to endpoints with x509 certificates missing either notBefore or notAfter would cause the check to fail with a stacktrace.  {pull}9566[9566]
+
+
 *Journalbeat*
 
 *Metricbeat*

heartbeat/monitors/active/dialchain/tls.go

@@ -19,12 +19,14 @@ package dialchain
 
 import (
 	cryptoTLS "crypto/tls"
+	"crypto/x509"
 	"fmt"
 	"net"
 	"time"
 
 	"github.com/elastic/beats/heartbeat/look"
 	"github.com/elastic/beats/libbeat/beat"
+	"github.com/elastic/beats/libbeat/common"
 	"github.com/elastic/beats/libbeat/outputs/transport"
 )
 
@@ -60,30 +62,53 @@ func TLSLayer(cfg *transport.TLSConfig, to time.Duration) Layer {
 			timer.stop()
 			event.PutValue("tls.rtt.handshake", look.RTT(timer.duration()))
 
-			// Pointers because we need a nil value
-			var chainNotValidBefore *time.Time
-			var chainNotValidAfter *time.Time
-
-			// Here we compute the minimal bounds during which this certificate chain is valid
-			// 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 tlsConn.ConnectionState().VerifiedChains {
-				for _, cert := range chain {
-					if chainNotValidBefore == nil || chainNotValidBefore.Before(cert.NotBefore) {
-						chainNotValidBefore = &cert.NotBefore
-					}
-
-					if chainNotValidAfter == nil || chainNotValidAfter.After(cert.NotAfter) {
-						chainNotValidAfter = &cert.NotAfter
-					}
-				}
-			}
-
-			event.PutValue("tls.certificate_not_valid_before", *chainNotValidBefore)
-			event.PutValue("tls.certificate_not_valid_after", *chainNotValidAfter)
+			addCertMetdata(event.Fields, tlsConn.ConnectionState().VerifiedChains)
 
 			return conn, nil
 		}), nil
 	}
 }
+
+func addCertMetdata(fields common.MapStr, chains [][]*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
+	// so we have no way to know if the user actually set these fields. The x509 RFC says that only one of the
+	// two fields must be set. Most tools (including openssl and go's certgen) always set both. BECAUSE WHY NOT
+	//
+	// In the wild, however, there are certs missing one of these two fields.
+	// So, what's the correct behavior here? We cannot know if a field was omitted due to the lack of nullability.
+	// So, in this case, we try to do what people will want 99.99999999999999999% of the time.
+	// People might set notBefore to go's zero date intentionally when creating certs. So, we always set that
+	// field, even if we find a zero value.
+	// However, it would be weird to set notAfter to the zero value. That could invalidate a cert that was intended
+	// to be valid forever. So, in that case, we treat the zero value as non-existent.
+	// This is why notBefore is a time.Time and notAfter is a *time.Time
+	var chainNotValidBefore time.Time
+	var chainNotValidAfter *time.Time
+
+	// We need the zero date later
+	var zeroTime time.Time
+
+	// Here we compute the minimal bounds during which this certificate chain is valid
+	// 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
+			}
+
+			if cert.NotAfter != zeroTime && (chainNotValidAfter == nil || chainNotValidAfter.After(cert.NotAfter)) {
+				chainNotValidAfter = &cert.NotAfter
+			}
+		}
+	}
+
+	fields.Put("tls.certificate_not_valid_before", chainNotValidBefore)
+
+	if chainNotValidAfter != nil {
+		fields.Put("tls.certificate_not_valid_after", *chainNotValidAfter)
+	}
+}

heartbeat/monitors/active/dialchain/tls_test.go

@@ -0,0 +1,123 @@
+// Licensed to Elasticsearch B.V. under one or more contributor
+// license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright
+// ownership. Elasticsearch B.V. licenses this file to you under
+// the Apache License, Version 2.0 (the "License"); you may
+// not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package dialchain
+
+import (
+	"crypto/x509"
+	"crypto/x509/pkix"
+	"math/big"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+
+	"github.com/elastic/beats/libbeat/common"
+)
+
+func Test_addCertMetdata(t *testing.T) {
+	goodNotBefore := time.Now().Add(-time.Hour)
+	goodNotAfter := time.Now().Add(time.Hour)
+	goodCert := x509.Certificate{
+		SerialNumber: big.NewInt(1),
+		Subject: pkix.Name{
+			Organization: []string{"Acme Co"},
+		},
+		NotBefore:             goodNotBefore,
+		NotAfter:              goodNotAfter,
+		KeyUsage:              x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature,
+		ExtKeyUsage:           []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth},
+		BasicConstraintsValid: true,
+	}
+
+	missingNotBeforeCert := x509.Certificate{
+		SerialNumber: big.NewInt(1),
+		Subject: pkix.Name{
+			Organization: []string{"Acme Co"},
+		},
+		NotAfter:              goodNotAfter,
+		KeyUsage:              x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature,
+		ExtKeyUsage:           []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth},
+		BasicConstraintsValid: true,
+	}
+
+	missingNotAfterCert := x509.Certificate{
+		SerialNumber: big.NewInt(1),
+		Subject: pkix.Name{
+			Organization: []string{"Acme Co"},
+		},
+		NotBefore:             goodNotBefore,
+		KeyUsage:              x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature,
+		ExtKeyUsage:           []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth},
+		BasicConstraintsValid: true,
+	}
+
+	// notBefore is intentionally not a pointer type because go certificates don't have nullable time types
+	// we cheat a bit and make not after nullable because there's no valid reason to create a cert with go's zero
+	// time.
+	// see the addCertMetadata function for more info on this.
+	type expected struct {
+		notBefore time.Time
+		notAfter  *time.Time
+	}
+	tests := []struct {
+		name     string
+		chains   [][]*x509.Certificate
+		expected expected
+	}{
+		{
+			"Valid cert",
+			[][]*x509.Certificate{{&goodCert}},
+			expected{
+				notBefore: goodNotBefore,
+				notAfter:  &goodNotAfter,
+			},
+		},
+		{
+			"Missing not before",
+			[][]*x509.Certificate{{&missingNotBeforeCert}},
+			expected{
+				notAfter: &goodNotAfter,
+			},
+		},
+		{
+			"Missing not after",
+			[][]*x509.Certificate{{&missingNotAfterCert}},
+			expected{
+				notBefore: goodNotBefore,
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			event := common.MapStr{}
+			addCertMetdata(event, tt.chains)
+			v, err := event.GetValue("tls.certificate_not_valid_before")
+			assert.NoError(t, err)
+			assert.Equal(t, tt.expected.notBefore, v)
+
+			if tt.expected.notAfter != nil {
+				v, err := event.GetValue("tls.certificate_not_valid_after")
+				assert.NoError(t, err)
+				assert.Equal(t, *tt.expected.notAfter, v)
+			} else {
+				ok, _ := event.HasKey("tls.certificate_not_valid_after")
+				assert.False(t, ok, "event should not have not after %v", event)
+			}
+		})
+	}
+}