From 7913a15d3c1b225f7427e9240c63ab5075281845 Mon Sep 17 00:00:00 2001
From: Rob Best <robertbest89@gmail.com>
Date: Thu, 20 Aug 2020 13:40:14 +0100
Subject: [PATCH] prober/tls: fix probe_ssl_last_chain_expiry_timestamp_seconds
 (#681)

* prober/tls: fix probe_ssl_last_chain_expiry_timestamp_seconds

This metric should report the earliest expiry of the chain that expires
the latest out of all the verified chains. Presently, it reports the
earliest expiry of the chain that expires first.

The current test for this metric was using an expired root certificate which
is omitted from the verified chain, so the test was passing despite this
bug. I've changed it to use a root that is still valid but expires before a
root held by the client.

* prober/tls: improve verified cert test

Include the older root certificate in the chain presented by the server
as well as in the client root CAs. This ensures that the peer
certificate metric identifies the older root CA as the earliest expiry
while it is ignored by the verified metric in favour of the longer-lived
chain.

Signed-off-by: Rob Best <robertbest89@gmail.com>
---
 prober/tcp_test.go | 24 +++++++++++++-----------
 prober/tls.go      |  2 +-
 2 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/prober/tcp_test.go b/prober/tcp_test.go
index fd219f9..6b4fdb3 100644
--- a/prober/tcp_test.go
+++ b/prober/tcp_test.go
@@ -217,33 +217,34 @@ func TestTCPConnectionWithTLSAndVerifiedCertificateChain(t *testing.T) {
 	testCTX, cancel := context.WithTimeout(context.Background(), 10*time.Second)
 	defer cancel()
 
-	// From here prepare two certificate chains where one is expired
+	// From here prepare two certificate chains where one expires before the
+	// other
 
 	rootPrivatekey, err := rsa.GenerateKey(rand.Reader, 2048)
 	if err != nil {
 		panic(fmt.Sprintf("Error creating rsa key: %s", err))
 	}
 
-	rootCertExpiry := time.Now().AddDate(0, 0, 2)
+	rootCertExpiry := time.Now().AddDate(0, 0, 3)
 	rootCertTmpl := generateCertificateTemplate(rootCertExpiry, false)
 	rootCertTmpl.IsCA = true
 	_, rootCertPem := generateSelfSignedCertificateWithPrivateKey(rootCertTmpl, rootPrivatekey)
 
-	oldRootCertExpiry := time.Now().AddDate(0, 0, -1)
-	expiredRootCertTmpl := generateCertificateTemplate(oldRootCertExpiry, false)
-	expiredRootCertTmpl.IsCA = true
-	expiredRootCert, expiredRootCertPem := generateSelfSignedCertificateWithPrivateKey(expiredRootCertTmpl, rootPrivatekey)
+	olderRootCertExpiry := time.Now().AddDate(0, 0, 1)
+	olderRootCertTmpl := generateCertificateTemplate(olderRootCertExpiry, false)
+	olderRootCertTmpl.IsCA = true
+	olderRootCert, olderRootCertPem := generateSelfSignedCertificateWithPrivateKey(olderRootCertTmpl, rootPrivatekey)
 
-	serverCertExpiry := time.Now().AddDate(0, 0, 1)
+	serverCertExpiry := time.Now().AddDate(0, 0, 2)
 	serverCertTmpl := generateCertificateTemplate(serverCertExpiry, false)
-	_, serverCertPem, serverKey := generateSignedCertificate(serverCertTmpl, expiredRootCert, rootPrivatekey)
+	_, serverCertPem, serverKey := generateSignedCertificate(serverCertTmpl, olderRootCert, rootPrivatekey)
 
 	// CAFile must be passed via filesystem, use a tempfile.
 	tmpCaFile, err := ioutil.TempFile("", "cafile.pem")
 	if err != nil {
 		t.Fatalf(fmt.Sprintf("Error creating CA tempfile: %s", err))
 	}
-	if _, err := tmpCaFile.Write(bytes.Join([][]byte{rootCertPem, expiredRootCertPem}, []byte("\n"))); err != nil {
+	if _, err := tmpCaFile.Write(bytes.Join([][]byte{rootCertPem, olderRootCertPem}, []byte("\n"))); err != nil {
 		t.Fatalf(fmt.Sprintf("Error writing CA tempfile: %s", err))
 	}
 	if err := tmpCaFile.Close(); err != nil {
@@ -263,7 +264,8 @@ func TestTCPConnectionWithTLSAndVerifiedCertificateChain(t *testing.T) {
 
 		serverKeyPem := pem.EncodeToMemory(&pem.Block{Type: "RSA PRIVATE KEY", Bytes: x509.MarshalPKCS1PrivateKey(serverKey)})
 
-		keypair, err := tls.X509KeyPair(serverCertPem, serverKeyPem)
+		// Include the older root cert in the chain
+		keypair, err := tls.X509KeyPair(append(serverCertPem, olderRootCertPem...), serverKeyPem)
 		if err != nil {
 			panic(fmt.Sprintf("Failed to decode TLS testing keypair: %s\n", err))
 		}
@@ -316,7 +318,7 @@ func TestTCPConnectionWithTLSAndVerifiedCertificateChain(t *testing.T) {
 
 	// Check values
 	expectedResults := map[string]float64{
-		"probe_ssl_earliest_cert_expiry":                float64(serverCertExpiry.Unix()),
+		"probe_ssl_earliest_cert_expiry":                float64(olderRootCertExpiry.Unix()),
 		"probe_ssl_last_chain_expiry_timestamp_seconds": float64(serverCertExpiry.Unix()),
 		"probe_ssl_last_chain_info":                     1,
 		"probe_tls_version_info":                        1,
diff --git a/prober/tls.go b/prober/tls.go
index 8710797..38d5533 100644
--- a/prober/tls.go
+++ b/prober/tls.go
@@ -45,7 +45,7 @@ func getLastChainExpiry(state *tls.ConnectionState) time.Time {
 				earliestCertExpiry = cert.NotAfter
 			}
 		}
-		if lastChainExpiry.IsZero() || lastChainExpiry.After(earliestCertExpiry) {
+		if lastChainExpiry.IsZero() || lastChainExpiry.Before(earliestCertExpiry) {
 			lastChainExpiry = earliestCertExpiry
 		}
 
-- 
2.25.1