Repository: prometheus/blackbox_exporter
Pull request #681 (2020-08-19 19:58:40)

prober/tls: fix probe_ssl_last_chain_expiry_timestamp_seconds
Apologies if I've misunderstood the purpose of this metric but I believe that it should be reporting the earliest expiry date of the chain that expires last out of all the verified chains. Presently it's reporting the earliest expiry of the chain that expires first.

This wasn't identified by the test for this metric because the test is using an expired root certificate which is not included in the `VerifiedChains`, so there was only one chain for the method to iterate over and therefore `getLastChainExpiry` returned the same regardless of the logic.

brian-brazil: Your change seems correct, @itkq did you miss this in your testing?

The unittest still doesn't seem right, we should have a test with different values for the two metrics.
ribbybibby: @brian-brazil I've tweaked the test. I think this captures the situation this metric was put in place to address now: the server presents a root cert that expires before the server cert, despite the client holding another, valid, longer-lived root.
itkq: This change seems right for me. I'm sorry, I should have noticed that `VerifiedChains` didn't have multiple chains in testing.

Patch:
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
 		}
 


Test patch:
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,