Restart when SystemCertPool should change

This fixes a downstream bug

There was a problem downstream where the OpenShift servivce-ca was not yet available,
and due to the way the manifests were set up, the service-ca was considered to be
part of the SystemCertPool. The problem is that the SystemCertPool, once initialized,
will never reload itself.

We can get into this situation when we use SSL_CERT_DIR and SSL_CERT_FILE to provide
OpenShift CAs to be used by containers/image for pulling. These environment variables
change the source of the SystemCertPool. The CertPoolWatcher then watches these
locations, and tries to update the pool it provides to the HTTPS client connecting to
catalogd. But the SystemCertPool is never updated. (It did not help that there was
no explicit CertPoolWatcher for the pull CAs.)

I tried to fix this downstream by removing SSL_CERT_DIR, and specifying the
`--pull-cas-dir` option. This means that containers/image would directly use
certificates that we specify, rather than the default location.

But this breaks the use of custom CAs for local image registries.

The containers/image package does not provide a way to manipulate the certificate
locations beyond a simple directory setting, and we need to leave that directory
setting as the default in downstream because it (i.e. /etc/docker/certs.d) is a host-
mounted directory that contains certificates for local image registries. And it is
possible to configure a custom CA for a local image registry, so that directory
must be included, ALONG with the OpenShift provided CAs and service-ca, which is
defined by SSL_CERT_DIR.

But because of the use of SSL_CERT_DIR to include the OpenShift service-ca, if the
service-ca was not available at startup, but became available later, it was not
possible to reload the SystemCertPool. Which could cause problems in operator-controller
when it tried to connect to catalogd.

The fundamental problem is that there's no way to refresh the SystemCertPool,
which will become more and more of an issue as certificate lifetimes decrease.

Using SSL_CERT_DIR allows us to use the CertPoolWatcher to notice changes to the
SystemCertPool. This will allow us to restart the process when certificates
change (e.g. OpenShift service-ca becomes available).

Changes:
* Update CertPoolWatcher to restart on changes to SSL_CERT_DIR and SSL_CERT_FILE
* Update CertPoolWatcher to use a Runnable interface, so that it can be added to
the manager, and started later, which may improve the changes that the service-ca
is ready.
* Update CertPoolWatcher to not be created when there's nothing to watch.
* Add CertPoolWatcher to catalogd for pull CAs
* Add CertPoolWatcher to operator-controller for pull CAs
* Improve logging

With this, my downstream manifest change should be reverted.

Signed-off-by: Todd Short <[email protected]>
Assisted-by: Claude Code (alternatives)

Author: tmshort

cmd/catalogd/main.go

@@ -60,6 +60,7 @@ import (
 	"github.com/operator-framework/operator-controller/internal/catalogd/webhook"
 	sharedcontrollers "github.com/operator-framework/operator-controller/internal/shared/controllers"
 	fsutil "github.com/operator-framework/operator-controller/internal/shared/util/fs"
+	httputil "github.com/operator-framework/operator-controller/internal/shared/util/http"
 	imageutil "github.com/operator-framework/operator-controller/internal/shared/util/image"
 	"github.com/operator-framework/operator-controller/internal/shared/util/pullsecretcache"
 	sautil "github.com/operator-framework/operator-controller/internal/shared/util/sa"
@@ -291,6 +292,18 @@ func run(ctx context.Context) error {
 		return err
 	}
 
+	// This watches the pullCasDir and the SSL_CERT_DIR, and SSL_CERT_FILE for changes
+	cpwPull, err := httputil.NewCertPoolWatcher(cfg.pullCasDir, ctrl.Log.WithName("pull-ca-pool"))
+	if err != nil {
+		setupLog.Error(err, "unable to create pull-ca-pool watcher")
+		return err
+	}
+	cpwPull.Restart(os.Exit)
+	if err = mgr.Add(cpwPull); err != nil {
+		setupLog.Error(err, "unable to add pull-ca-pool watcher to manager")
+		return err
+	}
+
 	if cfg.systemNamespace == "" {
 		cfg.systemNamespace = podNamespace()
 	}

cmd/operator-controller/main.go

@@ -336,9 +336,26 @@ func run() error {
 		return err
 	}
 
-	certPoolWatcher, err := httputil.NewCertPoolWatcher(cfg.catalogdCasDir, ctrl.Log.WithName("cert-pool"))
+	cpwCatalogd, err := httputil.NewCertPoolWatcher(cfg.catalogdCasDir, ctrl.Log.WithName("catalogd-ca-pool"))
 	if err != nil {
-		setupLog.Error(err, "unable to create CA certificate pool")
+		setupLog.Error(err, "unable to create catalogd-ca-pool watcher")
+		return err
+	}
+	cpwCatalogd.Restart(os.Exit)
+	if err = mgr.Add(cpwCatalogd); err != nil {
+		setupLog.Error(err, "unable to add catalogd-ca-pool watcher to manager")
+		return err
+	}
+
+	// This watches the pullCasDir and the SSL_CERT_DIR, and SSL_CERT_FILE for changes
+	cpwPull, err := httputil.NewCertPoolWatcher(cfg.pullCasDir, ctrl.Log.WithName("pull-ca-pool"))
+	if err != nil {
+		setupLog.Error(err, "unable to create pull-ca-pool watcher")
+		return err
+	}
+	cpwPull.Restart(os.Exit)
+	if err = mgr.Add(cpwPull); err != nil {
+		setupLog.Error(err, "unable to add pull-ca-pool watcher to manager")
 		return err
 	}
 
@@ -392,7 +409,7 @@ func run() error {
 	}
 	catalogClientBackend := cache.NewFilesystemCache(catalogsCachePath)
 	catalogClient := catalogclient.New(catalogClientBackend, func() (*http.Client, error) {
-		return httputil.BuildHTTPClient(certPoolWatcher)
+		return httputil.BuildHTTPClient(cpwCatalogd)
 	})
 
 	resolver := &resolve.CatalogResolver{

internal/shared/util/http/certlog.go

@@ -122,7 +122,16 @@ func logFile(filename, path, action string, log logr.Logger) {
 		log.Error(err, "error in os.ReadFile()", "file", filepath)
 		return
 	}
-	logPem(data, filename, path, action, log)
+	if len(data) > 0 {
+		logPem(data, filename, path, action, log)
+		return
+	}
+	// Indicate that the file is empty
+	args := []any{"file", filename, "empty", "true"}
+	if path != "" {
+		args = append(args, "directory", path)
+	}
+	log.V(defaultLogLevel).Info(action, args...)
 }
 
 func logPem(data []byte, filename, path, action string, log logr.Logger) {

internal/shared/util/http/certpoolwatcher.go

@@ -1,6 +1,7 @@
 package http
 
 import (
+	"context"
 	"crypto/x509"
 	"fmt"
 	"os"
@@ -14,13 +15,15 @@ import (
 )
 
 type CertPoolWatcher struct {
-	generation int
-	dir        string
-	mx         sync.RWMutex
-	pool       *x509.CertPool
-	log        logr.Logger
-	watcher    *fsnotify.Watcher
-	done       chan bool
+	generation   int
+	dir          string
+	sslCertPaths []string
+	mx           sync.RWMutex
+	pool         *x509.CertPool
+	log          logr.Logger
+	watcher      *fsnotify.Watcher
+	done         chan bool
+	restart      func(int)
 }
 
 // Returns the current CertPool and the generation number
@@ -33,77 +36,111 @@ func (cpw *CertPoolWatcher) Get() (*x509.CertPool, int, error) {
 	return cpw.pool.Clone(), cpw.generation, nil
 }
 
-func (cpw *CertPoolWatcher) Done() {
-	cpw.done <- true
+// Change the restart behavior
+func (cpw *CertPoolWatcher) Restart(f func(int)) {
+	cpw.restart = f
 }
 
-func NewCertPoolWatcher(caDir string, log logr.Logger) (*CertPoolWatcher, error) {
-	pool, err := NewCertPool(caDir, log)
-	if err != nil {
-		return nil, err
+// Indicate that you're done with the CertPoolWatcher so it can terminate
+// the watcher go func
+func (cpw *CertPoolWatcher) Done() {
+	if cpw.watcher != nil {
+		cpw.done <- true
 	}
-	watcher, err := fsnotify.NewWatcher()
+}
+
+func (cpw *CertPoolWatcher) Start(ctx context.Context) error {
+	var err error
+	cpw.pool, err = NewCertPool(cpw.dir, cpw.log)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
-	// If the SSL_CERT_DIR or SSL_CERT_FILE environment variables are
-	// specified, this means that we have some control over the system root
-	// location, thus they may change, thus we should watch those locations.
-	sslCertDir := os.Getenv("SSL_CERT_DIR")
-	sslCertFile := os.Getenv("SSL_CERT_FILE")
-	log.V(defaultLogLevel).Info("SSL environment", "SSL_CERT_DIR", sslCertDir, "SSL_CERT_FILE", sslCertFile)
+	watchPaths := append(cpw.sslCertPaths, cpw.dir)
+	watchPaths = slices.DeleteFunc(watchPaths, deleteEmptyStrings)
 
-	watchPaths := strings.Split(sslCertDir, ":")
-	watchPaths = append(watchPaths, caDir, sslCertFile)
-	watchPaths = slices.DeleteFunc(watchPaths, func(p string) bool {
-		if p == "" {
-			return true
-		}
-		if _, err := os.Stat(p); err != nil {
-			return true
-		}
-		return false
-	})
+	// Nothing was configured to be watched, which means this is
+	// using the SystemCertPool, so we still need to no error out
+	if len(watchPaths) == 0 {
+		cpw.log.Info("No paths to watch")
+		return nil
+	}
+
+	cpw.watcher, err = fsnotify.NewWatcher()
+	if err != nil {
+		return err
+	}
 
 	for _, p := range watchPaths {
-		if err := watcher.Add(p); err != nil {
-			return nil, err
+		if err := cpw.watcher.Add(p); err != nil {
+			cpw.watcher.Close()
+			cpw.watcher = nil
+			return err
 		}
-		logPath(p, "watching certificate", log)
+		logPath(p, "watching certificate", cpw.log)
 	}
 
-	cpw := &CertPoolWatcher{
-		generation: 1,
-		dir:        caDir,
-		pool:       pool,
-		log:        log,
-		watcher:    watcher,
-		done:       make(chan bool),
-	}
 	go func() {
 		for {
 			select {
-			case <-watcher.Events:
+			case e := <-cpw.watcher.Events:
+				cpw.checkForRestart(e.Name)
 				cpw.drainEvents()
-				cpw.update()
-			case err := <-watcher.Errors:
-				log.Error(err, "error watching certificate dir")
+				cpw.update(e.Name)
+			case err := <-cpw.watcher.Errors:
+				cpw.log.Error(err, "error watching certificate dir")
 				os.Exit(1)
+			case <-ctx.Done():
+				cpw.Done()
 			case <-cpw.done:
-				err := watcher.Close()
+				err := cpw.watcher.Close()
 				if err != nil {
-					log.Error(err, "error closing watcher")
+					cpw.log.Error(err, "error closing watcher")
 				}
 				return
 			}
 		}
 	}()
+	return nil
+}
+
+func NewCertPoolWatcher(caDir string, log logr.Logger) (*CertPoolWatcher, error) {
+	// If the SSL_CERT_DIR or SSL_CERT_FILE environment variables are
+	// specified, this means that we have some control over the system root
+	// location, thus they may change, thus we should watch those locations.
+	//
+	// BECAUSE THE SYSTEM POOL WILL NOT UPDATE, WE HAVE TO RESTART IF THERE
+	// CHANGES TO EITHER OF THESE LOCATIONS: SSL_CERT_DIR, SSL_CERT_FILE
+	//
+	sslCertDir := os.Getenv("SSL_CERT_DIR")
+	sslCertFile := os.Getenv("SSL_CERT_FILE")
+	log.V(defaultLogLevel).Info("SSL environment", "SSL_CERT_DIR", sslCertDir, "SSL_CERT_FILE", sslCertFile)
+
+	sslCertPaths := append(strings.Split(sslCertDir, ":"), sslCertFile)
+	sslCertPaths = slices.DeleteFunc(sslCertPaths, deleteEmptyStrings)
+
+	cpw := &CertPoolWatcher{
+		generation:   1,
+		dir:          caDir,
+		sslCertPaths: sslCertPaths,
+		log:          log,
+		done:         make(chan bool),
+	}
 	return cpw, nil
 }
 
-func (cpw *CertPoolWatcher) update() {
-	cpw.log.Info("updating certificate pool")
+func deleteEmptyStrings(p string) bool {
+	if p == "" {
+		return true
+	}
+	if _, err := os.Stat(p); err != nil {
+		return true
+	}
+	return false
+}
+
+func (cpw *CertPoolWatcher) update(name string) {
+	cpw.log.Info("updating certificate pool", "file", name)
 	pool, err := NewCertPool(cpw.dir, cpw.log)
 	if err != nil {
 		cpw.log.Error(err, "error updating certificate pool")
@@ -115,6 +152,17 @@ func (cpw *CertPoolWatcher) update() {
 	cpw.generation++
 }
 
+func (cpw *CertPoolWatcher) checkForRestart(name string) {
+	for _, p := range cpw.sslCertPaths {
+		if strings.Contains(name, p) {
+			cpw.log.Info("restarting due to file change", "file", name)
+			if cpw.restart != nil {
+				cpw.restart(0)
+			}
+		}
+	}
+}
+
 // Drain as many events as possible before doing anything
 // Otherwise, we will be hit with an event for _every_ entry in the
 // directory, and end up doing an update for each one
@@ -124,7 +172,8 @@ func (cpw *CertPoolWatcher) drainEvents() {
 		select {
 		case <-drainTimer.C:
 			return
-		case <-cpw.watcher.Events:
+		case e := <-cpw.watcher.Events:
+			cpw.checkForRestart(e.Name)
 		}
 		if !drainTimer.Stop() {
 			<-drainTimer.C