Cherry-pick elastic#9566 to 6.6: [Heartbeat] Handle TLS certs missing…

… notBefore/notAfter (elastic#9759)

* Add heartbeat test for TLS client cert auth (elastic#8984) (elastic#9676)

* Add heartbeat test for TLS client cert auth

We were missing a test for this specific case. I wrote this hoping to confirm elastic#8979, but actually wound up disproving it.

That said, this is still a good test to have, so we should merge it.

* [Heartbeat] Handle TLS certs missing notBefore/notAfter (elastic#9566)

Some certs in the wild don't set these standard fields and can cause an NPE

Fixes elastic#9556

CHANGELOG.asciidoc

@@ -63,6 +63,9 @@ https://github.com/elastic/beats/compare/1035569addc4a3b29ffa14f8a08c27c1ace16ef
 
 *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,6 +19,7 @@ package dialchain
 
 import (
 	cryptoTLS "crypto/tls"
+	"crypto/x509"
 	"fmt"
 	"net"
 	"time"
@@ -60,30 +61,53 @@ func TLSLayer(cfg *transport.TLSConfig, to time.Duration) Layer {
 			timer.stop()
 			event.Put("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.Put("tls.certificate_not_valid_before", *chainNotValidBefore)
-			event.Put("tls.certificate_not_valid_after", *chainNotValidAfter)
+			addCertMetdata(event, 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)
+			}
+		})
+	}
+}

heartbeat/monitors/active/http/http_test.go

@@ -18,13 +18,18 @@
 package http
 
 import (
+	"crypto/tls"
 	"crypto/x509"
 	"fmt"
+	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
 	"os"
+	"path"
 	"testing"
 
+	"github.com/elastic/beats/libbeat/common/file"
+
 	"github.com/stretchr/testify/require"
 
 	"github.com/elastic/beats/heartbeat/hbtest"
@@ -36,19 +41,21 @@ import (
 )
 
 func testRequest(t *testing.T, testURL string) beat.Event {
-	return testTLSRequest(t, testURL, "")
+	return testTLSRequest(t, testURL, nil)
 }
 
 // testTLSRequest tests the given request. certPath is optional, if given
 // an empty string no cert will be set.
-func testTLSRequest(t *testing.T, testURL string, certPath string) beat.Event {
+func testTLSRequest(t *testing.T, testURL string, extraConfig map[string]interface{}) beat.Event {
 	configSrc := map[string]interface{}{
 		"urls":    testURL,
 		"timeout": "1s",
 	}
 
-	if certPath != "" {
-		configSrc["ssl.certificate_authorities"] = certPath
+	if extraConfig != nil {
+		for k, v := range extraConfig {
+			configSrc[k] = v
+		}
 	}
 
 	config, err := common.NewConfigFrom(configSrc)
@@ -247,8 +254,10 @@ func TestLargeResponse(t *testing.T) {
 	)
 }
 
-func TestHTTPSServer(t *testing.T) {
-	server := httptest.NewTLSServer(hbtest.HelloWorldHandler(http.StatusOK))
+func runHTTPSServerCheck(
+	t *testing.T,
+	server *httptest.Server,
+	reqExtraConfig map[string]interface{}) {
 	port, err := hbtest.ServerPort(server)
 	require.NoError(t, err)
 
@@ -261,7 +270,12 @@ func TestHTTPSServer(t *testing.T) {
 	require.NoError(t, certFile.Close())
 	defer os.Remove(certFile.Name())
 
-	event := testTLSRequest(t, server.URL, certFile.Name())
+	mergedExtraConfig := map[string]interface{}{"ssl.certificate_authorities": certFile.Name()}
+	for k, v := range reqExtraConfig {
+		mergedExtraConfig[k] = v
+	}
+
+	event := testTLSRequest(t, server.URL, mergedExtraConfig)
 
 	mapvaltest.Test(
 		t,
@@ -275,6 +289,50 @@ func TestHTTPSServer(t *testing.T) {
 	)
 }
 
+func TestHTTPSServer(t *testing.T) {
+	server := httptest.NewTLSServer(hbtest.HelloWorldHandler(http.StatusOK))
+
+	runHTTPSServerCheck(t, server, nil)
+}
+
+func TestHTTPSx509Auth(t *testing.T) {
+	wd, err := os.Getwd()
+	require.NoError(t, err)
+	clientKeyPath := path.Join(wd, "testdata", "client_key.pem")
+	clientCertPath := path.Join(wd, "testdata", "client_cert.pem")
+
+	certReader, err := file.ReadOpen(clientCertPath)
+	require.NoError(t, err)
+
+	clientCertBytes, err := ioutil.ReadAll(certReader)
+	require.NoError(t, err)
+
+	clientCerts := x509.NewCertPool()
+	certAdded := clientCerts.AppendCertsFromPEM(clientCertBytes)
+	require.True(t, certAdded)
+
+	tlsConf := &tls.Config{
+		ClientAuth: tls.RequireAndVerifyClientCert,
+		ClientCAs:  clientCerts,
+		MinVersion: tls.VersionTLS12,
+	}
+	tlsConf.BuildNameToCertificate()
+
+	server := httptest.NewUnstartedServer(hbtest.HelloWorldHandler(http.StatusOK))
+	server.TLS = tlsConf
+	server.StartTLS()
+	defer server.Close()
+
+	runHTTPSServerCheck(
+		t,
+		server,
+		map[string]interface{}{
+			"ssl.certificate": clientCertPath,
+			"ssl.key":         clientKeyPath,
+		},
+	)
+}
+
 func TestConnRefusedJob(t *testing.T) {
 	ip := "127.0.0.1"
 	port, err := btesting.AvailableTCP4Port()

heartbeat/monitors/active/http/testdata/client_cert.pem

@@ -0,0 +1,14 @@
+-----BEGIN CERTIFICATE-----
+MIICGzCCAXygAwIBAgIRANIf32fETNS13JB39JYszmwwCgYIKoZIzj0EAwQwEjEQ
+MA4GA1UEChMHQWNtZSBDbzAeFw0xODExMDgwMzIxNDlaFw0xOTExMDgwMzIxNDla
+MBIxEDAOBgNVBAoTB0FjbWUgQ28wgZswEAYHKoZIzj0CAQYFK4EEACMDgYYABAE+
+n/OJoo7jvetm8zR4lAX2s99fxWF/LiOR1/qTPQgLmLYVUZq1yTZB027GtJGWAqph
+kY/n0oNdxS4N9d2JPoaXMgHMGZAXl0A85Q3D5k0xKG/jwaEasTIbTe6UKHed2Zgk
+CtEqutG9KwmnqAHCtlia14mgcERpO1eT0A7NRcdtNlcjlKNwMG4wDgYDVR0PAQH/
+BAQDAgKkMB0GA1UdJQQWMBQGCCsGAQUFBwMBBggrBgEFBQcDAjAPBgNVHRMBAf8E
+BTADAQH/MCwGA1UdEQQlMCOCCWxvY2FsaG9zdIEWbm9zdWNoYWRkckBleGFtcGxl
+Lm5ldDAKBggqhkjOPQQDBAOBjAAwgYgCQgDvHj4Xt5TMqhR4Uavmfa0uOio0FZxL
+vGnk3aLj5koJyrQNynntHBcCZ+sPb14J08FWk0j4GPOGroMVud/XTX1BZgJCAc3k
+0p+X1r+lt1hkSGrumTY5NRWIGIvJ0gy1AhuZJzXYoPRRdPgnM04vBWniOLHDhmsX
+ExbWSt0EY2IiOJc/1GNO
+-----END CERTIFICATE-----

heartbeat/monitors/active/http/testdata/client_key.pem

@@ -0,0 +1,7 @@
+-----BEGIN EC PRIVATE KEY-----
+MIHcAgEBBEIB1YnGgQ42OFGz1rOFlmT97JB52b9/2h1dj85QaBLxX6isSNgnS7yC
+VQKAQCudJz+UpqiTNZBQK0goqbD/O47lswagBwYFK4EEACOhgYkDgYYABAE+n/OJ
+oo7jvetm8zR4lAX2s99fxWF/LiOR1/qTPQgLmLYVUZq1yTZB027GtJGWAqphkY/n
+0oNdxS4N9d2JPoaXMgHMGZAXl0A85Q3D5k0xKG/jwaEasTIbTe6UKHed2ZgkCtEq
+utG9KwmnqAHCtlia14mgcERpO1eT0A7NRcdtNlcjlA==
+-----END EC PRIVATE KEY-----