fix: Make certificate reload handling more robust

Author: arnecls

httpserver/filebasedcert.go

@@ -1,7 +1,9 @@
 package httpserver
 
 import (
+	"bytes"
 	"crypto/tls"
+	"fmt"
 	"os"
 	"sync"
 	"time"
@@ -35,43 +37,76 @@ func newFileBasedCert(certFile, keyFile string, certCacheDuration time.Duration)
 // GetCertificate returns a certificate from the cache, or loads it from disk if
 // it is not cached yet or certCacheDuration has passed.
 func (c *fileBasedCert) GetCertificate() (*tls.Certificate, error) {
-	now := time.Now()
-
 	c.mutex.Lock()
 	defer c.mutex.Unlock()
+	now := time.Now()
 
-	// Make sure we force a refresh when the certificate has expired
-	if c.cert != nil && c.cert.Leaf != nil && now.After(c.cert.Leaf.NotAfter) {
-		log.Warn().Msg("TLS certificate has expired, reloading.")
-		c.cert = nil
-	}
+	reload := func() (*tls.Certificate, error) {
+		c.cert = nil // make sure we don't return the old certificate.
+		cert, err := tls.LoadX509KeyPair(c.certFile, c.keyFile)
 
-	if c.cert != nil {
-		// Check if the certificate file has been changed by comparing the last
-		// modification time with the time we last refreshed the certificate.
-		if fileInfo, err := os.Stat(c.certFile); err == nil && fileInfo.ModTime().After(c.lastRefresh) {
-			log.Warn().Msg("TLS certificate file has been changed, reloading.")
-			c.cert = nil
+		switch {
+		case err != nil:
+			return nil, err
+		case cert.Leaf == nil:
+			return nil, fmt.Errorf("certificate leaf is nil")
+		case now.After(cert.Leaf.NotAfter):
+			// This is a warning on purpose, as we don't want to fail the
+			// server startup if the certificate is expired. We will just keep
+			// using the expired certificate, which will be result in an error
+			// for the client.
+			log.Warn().Msg("reloaded TLS certificate has already expired.")
+
+			// When certCacheDuration is set to a value higher than one minute,
+			// we will retry within the next minute. This is to make sure that
+			// we don't keep using an expired certificate for too long.
+			if c.certCacheDuration > time.Minute {
+				now = now.Add(time.Minute - c.certCacheDuration)
+			}
 		}
+
+		c.cert = &cert
+		c.lastRefresh = now
+		return &cert, nil
 	}
 
-	// Load the certificate from disk if it is not cached yet or certCacheDuration
-	// has passed.
-	if c.cert == nil || now.Sub(c.lastRefresh) > c.certCacheDuration {
-		if c.cert != nil {
-			log.Warn().Msg("TLS cache duration has expired, reloading certificate from disk.")
+	switch {
+	// Load the certificate from disk if we don't have one cached yet.
+	case c.cert == nil:
+		log.Info().Msg("No TLS certificate cached, loading.")
+		return reload()
+
+	// Reload the certificate if it has expired.
+	case now.After(c.cert.Leaf.NotAfter):
+		log.Warn().Msg("TLS certificate has expired, reloading.")
+		return reload()
+
+	// Check for a new certificate in regular intervals.
+	// We only change the loaded certificate if the signature has changed.
+	case now.Sub(c.lastRefresh) > c.certCacheDuration:
+		log.Info().Msg("TLS certificate cache duration has passed.")
+
+		// Check if the certificate file has been changed since the last
+		// refresh. This is a simple check that only compares the modification
+		// time of the file.
+		if fileInfo, err := os.Stat(c.certFile); err == nil && fileInfo.ModTime().After(c.lastRefresh) {
+			log.Warn().Msg("TLS certificate file has been modifed since last refresh.")
+			return reload()
 		}
+
+		// Check if the certificate signature has changed since the last
+		// refresh. This is a more expensive check that compares the signature
+		// of the certificate.
 		cert, err := tls.LoadX509KeyPair(c.certFile, c.keyFile)
-		if err != nil {
-			return nil, err
+		if err != nil || cert.Leaf == nil {
+			log.Error().Err(err).Msg("Failed to load TLS certificate for comparison, keeping cached certificate.")
+			return c.cert, nil
 		}
 
-		if cert.Leaf != nil && now.After(cert.Leaf.NotAfter) {
-			log.Error().Msg("Reloaded TLS certificate has already expired.")
+		if !bytes.Equal(cert.Leaf.Signature, c.cert.Leaf.Signature) {
+			log.Warn().Msg("Detected certificate signature change, reloading.")
+			return reload()
 		}
-
-		c.cert = &cert
-		c.lastRefresh = time.Now()
 	}
 
 	return c.cert, nil