From 796507216c284c4c366fd3bbd683465eaf5c4ad7 Mon Sep 17 00:00:00 2001 From: Todd Short Date: Wed, 17 Jul 2024 17:40:34 -0400 Subject: [PATCH 1/4] Add support for CA/certificate rotation Mounted secrets are automatically updated into pods, but... * It doesn't work with `subPath` mountings * When `subPath` is not used, then a bunch of directories are mounted * And one of those directories is a symlink, so `IsDir()` returns false * And a watch is needed to notice the change So, update the certificate volume patch, which requires a change in how we look for certificates in the CA cert directory. Add a watch, so when the certs do change, we update the cert pool. Also look at validity dates of certificates, and error on expired certs. The default cert-manager certificates have 90 days validities. Signed-off-by: Todd Short --- .github/workflows/e2e.yaml | 12 +++ Makefile | 9 ++ cmd/manager/main.go | 45 +++++++--- .../tls/patches/manager_deployment_cert.yaml | 2 +- go.mod | 2 +- hack/test/cert-e2e.sh | 31 +++++++ internal/catalogmetadata/cache/cache.go | 12 ++- internal/catalogmetadata/cache/cache_test.go | 4 +- .../clusterextension_controller.go | 1 + internal/httputil/certutil.go | 88 +++++++++++++++++-- internal/httputil/certutil_test.go | 7 +- internal/rukpak/source/image_registry.go | 10 ++- testdata/certs/expired/expired.pem | 31 +++++++ 13 files changed, 225 insertions(+), 29 deletions(-) create mode 100755 hack/test/cert-e2e.sh create mode 100644 testdata/certs/expired/expired.pem diff --git a/.github/workflows/e2e.yaml b/.github/workflows/e2e.yaml index 06a2e9d31a..7c69f11fa5 100644 --- a/.github/workflows/e2e.yaml +++ b/.github/workflows/e2e.yaml @@ -59,3 +59,15 @@ jobs: - name: Run the upgrade e2e test run: make test-upgrade-e2e + + cert-e2e: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-go@v5 + with: + go-version-file: go.mod + + - name: Run the cert e2e test + run: make test-cert-e2e diff --git a/Makefile b/Makefile index 587502ff92..cfea3f9109 100644 --- a/Makefile +++ b/Makefile @@ -188,6 +188,15 @@ test-upgrade-e2e: export TEST_CLUSTER_CATALOG_NAME := test-catalog test-upgrade-e2e: export TEST_CLUSTER_EXTENSION_NAME := test-package test-upgrade-e2e: kind-cluster run-latest-release image-registry build-push-e2e-catalog registry-load-bundles pre-upgrade-setup docker-build kind-load kind-deploy post-upgrade-checks kind-clean #HELP Run upgrade e2e tests on a local kind cluster +.PHONY: run-cert-e2e +run-cert-e2e: + ./hack/test/cert-e2e.sh + +.PHONY: test-cert-e2e +test-cert-e2e: KIND_CLUSTER_NAME := operator-controller-cert-e2e +test-cert-e2e: KUSTOMIZE_BUILD_DIR := config/overlays/cert-manager +test-cert-e2e: run run-cert-e2e kind-clean #HELP Run certificate e2e tests on a local kind cluster + .PHONY: e2e-coverage e2e-coverage: COVERAGE_OUTPUT=./coverage/e2e.out ./hack/test/e2e-coverage.sh diff --git a/cmd/manager/main.go b/cmd/manager/main.go index a7d4bcda73..0c26cf46f3 100644 --- a/cmd/manager/main.go +++ b/cmd/manager/main.go @@ -18,8 +18,10 @@ package main import ( "context" + "crypto/x509" "flag" "fmt" + "net/http" "os" "path/filepath" @@ -176,16 +178,23 @@ func main() { os.Exit(1) } - certPool, err := httputil.NewCertPool(caCertDir) + certPool, err := httputil.NewCertPool(caCertDir, ctrl.Log.WithName("cert-pool")) if err != nil { setupLog.Error(err, "unable to create CA certificate pool") os.Exit(1) } + + ceRecon := &controllers.ClusterExtensionReconciler{} + unpacker := &source.ImageRegistry{ BaseCachePath: filepath.Join(cachePath, "unpack"), // TODO: This needs to be derived per extension via ext.Spec.InstallNamespace AuthNamespace: systemNamespace, - CaCertPool: certPool, + GetCaCertPool: func() (*x509.CertPool, error) { + ceRecon.CaMutex.RLock() + defer ceRecon.CaMutex.RUnlock() + return ceRecon.CaCertPool.Clone(), nil + }, } clusterExtensionFinalizers := crfinalizer.NewFinalizers() @@ -210,18 +219,17 @@ func main() { } cl := mgr.GetClient() - httpClient, err := httputil.BuildHTTPClient(certPool) - if err != nil { - setupLog.Error(err, "unable to create catalogd http client") - os.Exit(1) - } catalogsCachePath := filepath.Join(cachePath, "catalogs") if err := os.MkdirAll(catalogsCachePath, 0700); err != nil { setupLog.Error(err, "unable to create catalogs cache directory") os.Exit(1) } - catalogClient := catalogclient.New(cache.NewFilesystemCache(catalogsCachePath, httpClient)) + catalogClient := catalogclient.New(cache.NewFilesystemCache(catalogsCachePath, func() (*http.Client, error) { + ceRecon.CaMutex.RLock() + defer ceRecon.CaMutex.RUnlock() + return httputil.BuildHTTPClient(ceRecon.CaCertPool.Clone()) + })) resolver := &resolve.CatalogResolver{ WalkCatalogsFunc: resolve.CatalogWalker( @@ -235,8 +243,7 @@ func main() { catalogClient.GetPackage, ), } - - if err = (&controllers.ClusterExtensionReconciler{ + ceRecon = &controllers.ClusterExtensionReconciler{ Client: cl, Resolver: resolver, ActionClientGetter: acg, @@ -245,7 +252,8 @@ func main() { Finalizers: clusterExtensionFinalizers, CaCertPool: certPool, Preflights: preflights, - }).SetupWithManager(mgr); err != nil { + } + if err = ceRecon.SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "ClusterExtension") os.Exit(1) } @@ -259,6 +267,21 @@ func main() { setupLog.Error(err, "unable to set up ready check") os.Exit(1) } + setupLog.Info("adding certificate watch", "directory", caCertDir) + if err := httputil.AddCertWatch(caCertDir, func() error { + ctrl.Log.WithName("cert-pool").Info("reloading certificate pool") + certPool, err := httputil.NewCertPool(caCertDir, ctrl.Log.WithName("cert-pool")) + if err != nil { + return err + } + ceRecon.CaMutex.Lock() + defer ceRecon.CaMutex.Unlock() + ceRecon.CaCertPool = certPool + return nil + }); err != nil { + setupLog.Error(err, "unable to add CA certificate watch") + os.Exit(1) + } setupLog.Info("starting manager") ctx := ctrl.SetupSignalHandler() diff --git a/config/components/tls/patches/manager_deployment_cert.yaml b/config/components/tls/patches/manager_deployment_cert.yaml index 9a1cf1b7a3..747979321b 100644 --- a/config/components/tls/patches/manager_deployment_cert.yaml +++ b/config/components/tls/patches/manager_deployment_cert.yaml @@ -3,7 +3,7 @@ value: {"name":"olmv1-certificate", "secret":{"secretName":"olmv1-cert", "optional": false, "items": [{"key": "ca.crt", "path": "olm-ca.crt"}]}} - op: add path: /spec/template/spec/containers/0/volumeMounts/- - value: {"name":"olmv1-certificate", "readOnly": true, "mountPath":"/var/certs/olm-ca.crt", "subPath":"olm-ca.crt"} + value: {"name":"olmv1-certificate", "readOnly": true, "mountPath":"/var/certs/"} - op: add path: /spec/template/spec/containers/0/args/- value: "--ca-certs-dir=/var/certs" diff --git a/go.mod b/go.mod index f47c51bb1f..cad5ebec95 100644 --- a/go.mod +++ b/go.mod @@ -7,6 +7,7 @@ require ( github.com/Masterminds/semver/v3 v3.2.1 github.com/blang/semver/v4 v4.0.0 github.com/containerd/containerd v1.7.20 + github.com/fsnotify/fsnotify v1.7.0 github.com/go-logr/logr v1.4.2 github.com/google/go-cmp v0.6.0 github.com/google/go-containerregistry v0.20.1 @@ -112,7 +113,6 @@ require ( github.com/exponent-io/jsonpath v0.0.0-20210407135951-1de76d718b3f // indirect github.com/fatih/color v1.15.0 // indirect github.com/felixge/httpsnoop v1.0.4 // indirect - github.com/fsnotify/fsnotify v1.7.0 // indirect github.com/go-errors/errors v1.4.2 // indirect github.com/go-git/gcfg v1.5.1-0.20230307220236-3a3c6141e376 // indirect github.com/go-git/go-billy/v5 v5.5.0 // indirect diff --git a/hack/test/cert-e2e.sh b/hack/test/cert-e2e.sh new file mode 100755 index 0000000000..37fb9e06c0 --- /dev/null +++ b/hack/test/cert-e2e.sh @@ -0,0 +1,31 @@ +#!/bin/bash + +set -e + +# patch the certificates to renew every minute +# (if set to 2 minutes, then the big timeout wait is about 5 minutes) +kubectl patch certificate.cert-manager.io -n cert-manager olmv1-ca --type='merge' -p '{"spec":{"duration":"1h", "renewBefore":"59m"}}' +kubectl patch certificate.cert-manager.io -n olmv1-system olmv1-cert --type='merge' -p '{"spec":{"duration":"1h", "renewBefore":"59m"}}' + +# delete the old secrets, so new secrets are generated +kubectl delete secret -n cert-manager olmv1-ca +kubectl delete secret -n olmv1-system olmv1-cert + +# delete the existing operator-controller, to force it to get a new secret +# (deleting the secret itself isn't enough) +kubectl delete pod -n olmv1-system -l control-plane=controller-manager +kubectl wait --for=condition=Available --namespace=olmv1-system "deployment/operator-controller-controller-manager" --timeout="60s" + +# and then search through the previous logs +function check_logs() { + kubectl logs -c manager -n olmv1-system -l control-plane=controller-manager | grep "reloading certificate pool" >& /dev/null +} + +WAIT_TIME=0 +MAX_TIME=300 +until (( NEXT_WAIT_TIME == MAX_TIME )) || check_logs; do + echo -n "." + sleep "$(( WAIT_TIME += 10 ))" +done +(( WAIT_TIME < MAX_TIME )) +echo "#" diff --git a/internal/catalogmetadata/cache/cache.go b/internal/catalogmetadata/cache/cache.go index acb0c89a8d..419eef0964 100644 --- a/internal/catalogmetadata/cache/cache.go +++ b/internal/catalogmetadata/cache/cache.go @@ -25,11 +25,11 @@ var _ client.Fetcher = &filesystemCache{} // - IF cached it will verify the cache is up to date. If it is up to date it will return // the cached contents, if not it will fetch the new contents from the catalogd HTTP // server and update the cached contents. -func NewFilesystemCache(cachePath string, client *http.Client) client.Fetcher { +func NewFilesystemCache(cachePath string, clientFunc func() (*http.Client, error)) client.Fetcher { return &filesystemCache{ cachePath: cachePath, mutex: sync.RWMutex{}, - client: client, + getClient: clientFunc, cacheDataByCatalogName: map[string]cacheData{}, } } @@ -50,7 +50,7 @@ type cacheData struct { type filesystemCache struct { mutex sync.RWMutex cachePath string - client *http.Client + getClient func() (*http.Client, error) cacheDataByCatalogName map[string]cacheData } @@ -95,7 +95,11 @@ func (fsc *filesystemCache) FetchCatalogContents(ctx context.Context, catalog *c return nil, fmt.Errorf("error forming request: %v", err) } - resp, err := fsc.client.Do(req) + client, err := fsc.getClient() + if err != nil { + return nil, fmt.Errorf("error getting HTTP client: %w", err) + } + resp, err := client.Do(req) if err != nil { return nil, fmt.Errorf("error performing request: %v", err) } diff --git a/internal/catalogmetadata/cache/cache_test.go b/internal/catalogmetadata/cache/cache_test.go index 168b32581a..05ea28ec59 100644 --- a/internal/catalogmetadata/cache/cache_test.go +++ b/internal/catalogmetadata/cache/cache_test.go @@ -214,7 +214,9 @@ func TestFilesystemCache(t *testing.T) { maps.Copy(tt.tripper.content, tt.contents) httpClient := http.DefaultClient httpClient.Transport = tt.tripper - c := cache.NewFilesystemCache(cacheDir, httpClient) + c := cache.NewFilesystemCache(cacheDir, func() (*http.Client, error) { + return httpClient, nil + }) actualFS, err := c.FetchCatalogContents(ctx, tt.catalog) if !tt.wantErr { diff --git a/internal/controllers/clusterextension_controller.go b/internal/controllers/clusterextension_controller.go index 1fb63037b0..1b46e531df 100644 --- a/internal/controllers/clusterextension_controller.go +++ b/internal/controllers/clusterextension_controller.go @@ -89,6 +89,7 @@ type ClusterExtensionReconciler struct { cache cache.Cache InstalledBundleGetter InstalledBundleGetter Finalizers crfinalizer.Finalizers + CaMutex sync.RWMutex CaCertPool *x509.CertPool Preflights []Preflight } diff --git a/internal/httputil/certutil.go b/internal/httputil/certutil.go index 23e1321641..d0820d433a 100644 --- a/internal/httputil/certutil.go +++ b/internal/httputil/certutil.go @@ -2,12 +2,17 @@ package httputil import ( "bytes" + "context" "crypto/x509" "encoding/base64" "encoding/pem" "fmt" "os" "path/filepath" + "time" + + "github.com/fsnotify/fsnotify" + "github.com/go-logr/logr" ) var pemStart = []byte("\n-----BEGIN ") @@ -157,7 +162,7 @@ func pemDecode(data []byte) (*pem.Block, []byte) { } // This version of (*x509.CertPool).AppendCertsFromPEM() will error out if parsing fails -func appendCertsFromPEM(s *x509.CertPool, pemCerts []byte) error { +func appendCertsFromPEM(s *x509.CertPool, pemCerts []byte, firstExpiration *time.Time) error { n := 1 for len(pemCerts) > 0 { var block *pem.Block @@ -179,15 +184,25 @@ func appendCertsFromPEM(s *x509.CertPool, pemCerts []byte) error { if err != nil { return fmt.Errorf("unable to parse cert %d: %w", n, err) } - // no return values - panics or always succeeds - s.AddCert(cert) + if firstExpiration.IsZero() || firstExpiration.After(cert.NotAfter) { + *firstExpiration = cert.NotAfter + } + now := time.Now() + if now.Before(cert.NotBefore) { + return fmt.Errorf("not yet valid cert %d: %q", n, cert.NotBefore.Format(time.RFC3339)) + } else if now.After(cert.NotAfter) { + return fmt.Errorf("expired cert %d: %q", n, cert.NotAfter.Format(time.RFC3339)) + } else { + // no return values - panics or always succeeds + s.AddCert(cert) + } n++ } return nil } -func NewCertPool(caDir string) (*x509.CertPool, error) { +func NewCertPool(caDir string, log logr.Logger) (*x509.CertPool, error) { caCertPool, err := x509.SystemCertPool() if err != nil { return nil, err @@ -200,20 +215,79 @@ func NewCertPool(caDir string) (*x509.CertPool, error) { if err != nil { return nil, err } + count := 0 + firstExpiration := time.Time{} + for _, e := range dirEntries { - if e.IsDir() { + file := filepath.Join(caDir, e.Name()) + // These might be symlinks pointing to directories, so use Stat() to resolve + fi, err := os.Stat(file) + if err != nil { + return nil, err + } + if fi.IsDir() { + log.Info("skip directory", "name", e.Name()) continue } - file := filepath.Join(caDir, e.Name()) + log.Info("load certificate", "name", e.Name()) data, err := os.ReadFile(file) if err != nil { return nil, fmt.Errorf("error reading cert file %q: %w", file, err) } - err = appendCertsFromPEM(caCertPool, data) + err = appendCertsFromPEM(caCertPool, data, &firstExpiration) if err != nil { return nil, fmt.Errorf("error adding cert file %q: %w", file, err) } + count++ } + // Found no certs! + if count == 0 { + return nil, fmt.Errorf("no certificates found in %q", caDir) + } + + log.Info("first expiration", "time", firstExpiration.Format(time.RFC3339)) return caCertPool, nil } + +func AddCertWatch(caDir string, update func() error) error { + watcher, err := fsnotify.NewWatcher() + if err != nil { + return err + } + if err = watcher.Add(caDir); err != nil { + return err + } + go func() { + for { + select { + case <-watcher.Events: + // drain as many events as possible before doing anything + func() { + for { + time.Sleep(time.Millisecond * 100) + select { + case <-watcher.Events: + default: + return + } + } + }() + if err := update(); err != nil { + log, err2 := logr.FromContext(context.Background()) + if err2 == nil { + log.WithName("cert-pool").Error(err, "error updating cert-pool") + } + os.Exit(1) + } + case err = <-watcher.Errors: + log, err2 := logr.FromContext(context.Background()) + if err2 == nil { + log.WithName("cert-pool").Error(err, "error watching certificate dir") + } + os.Exit(1) + } + } + }() + return nil +} diff --git a/internal/httputil/certutil_test.go b/internal/httputil/certutil_test.go index 9f72b51a89..a8a158ff37 100644 --- a/internal/httputil/certutil_test.go +++ b/internal/httputil/certutil_test.go @@ -1,8 +1,10 @@ package httputil_test import ( + "context" "testing" + "github.com/go-logr/logr" "github.com/stretchr/testify/require" "github.com/operator-framework/operator-controller/internal/httputil" @@ -21,17 +23,20 @@ func TestNewCertPool(t *testing.T) { dir string msg string }{ + {"../../testdata/certs/", `no certificates found in "../../testdata/certs/"`}, {"../../testdata/certs/good", ""}, {"../../testdata/certs/bad", `error adding cert file "../../testdata/certs/bad/Amazon_Root_CA_2.pem": unable to PEM decode cert 1`}, {"../../testdata/certs/ugly", `error adding cert file "../../testdata/certs/ugly/Amazon_Root_CA.pem": unable to PEM decode cert 2`}, {"../../testdata/certs/ugly2", `error adding cert file "../../testdata/certs/ugly2/Amazon_Root_CA_1.pem": unable to PEM decode cert 1`}, {"../../testdata/certs/ugly3", `error adding cert file "../../testdata/certs/ugly3/not_a_cert.pem": unable to PEM decode cert 1`}, {"../../testdata/certs/empty", `error adding cert file "../../testdata/certs/empty/empty.pem": unable to parse cert 1: x509: malformed certificate`}, + {"../../testdata/certs/expired", `error adding cert file "../../testdata/certs/expired/expired.pem": expired cert 1: "2024-01-02T15:00:00Z"`}, } + log, _ := logr.FromContext(context.Background()) for _, caDir := range caDirs { t.Logf("Loading certs from %q", caDir.dir) - pool, err := httputil.NewCertPool(caDir.dir) + pool, err := httputil.NewCertPool(caDir.dir, log) if caDir.msg == "" { require.NoError(t, err) require.NotNil(t, pool) diff --git a/internal/rukpak/source/image_registry.go b/internal/rukpak/source/image_registry.go index 775d7d47e6..ef898b9f20 100644 --- a/internal/rukpak/source/image_registry.go +++ b/internal/rukpak/source/image_registry.go @@ -53,7 +53,7 @@ func NewUnrecoverable(err error) *Unrecoverable { type ImageRegistry struct { BaseCachePath string AuthNamespace string - CaCertPool *x509.CertPool + GetCaCertPool func() (*x509.CertPool, error) } func (i *ImageRegistry) Unpack(ctx context.Context, bundle *BundleSource) (*Result, error) { @@ -99,8 +99,12 @@ func (i *ImageRegistry) Unpack(ctx context.Context, bundle *BundleSource) (*Resu if bundle.Image.InsecureSkipTLSVerify { transport.TLSClientConfig.InsecureSkipVerify = true // nolint:gosec } - if i.CaCertPool != nil { - transport.TLSClientConfig.RootCAs = i.CaCertPool + if i.GetCaCertPool != nil { + pool, err := i.GetCaCertPool() + if err != nil { + return nil, err + } + transport.TLSClientConfig.RootCAs = pool } remoteOpts = append(remoteOpts, remote.WithTransport(transport)) diff --git a/testdata/certs/expired/expired.pem b/testdata/certs/expired/expired.pem new file mode 100644 index 0000000000..e8912ba61e --- /dev/null +++ b/testdata/certs/expired/expired.pem @@ -0,0 +1,31 @@ +-----BEGIN CERTIFICATE----- +MIIFXzCCA0egAwIBAgIUN5r8l1RrpH53+9e6pfj6CXoyqP0wDQYJKoZIhvcNAQEL +BQAwPzELMAkGA1UEBhMCVVMxEDAOBgNVBAoMB1JlZCBIYXQxDDAKBgNVBAsMA09M +TTEQMA4GA1UEAwwHZXhwaXJlZDAeFw0yNDAxMDExNTAwMDBaFw0yNDAxMDIxNTAw +MDBaMD8xCzAJBgNVBAYTAlVTMRAwDgYDVQQKDAdSZWQgSGF0MQwwCgYDVQQLDANP +TE0xEDAOBgNVBAMMB2V4cGlyZWQwggIiMA0GCSqGSIb3DQEBAQUAA4ICDwAwggIK +AoICAQDFalyjEXz0cMGs3pt360Cz0uD0CDnnAqQFHxXPchfCMZnW/VRGrJQq29rZ +UgU5PnxPgqadrw20BodfR2RS9xIacMP+092GY7Ep96xWokwXcsGPj2e5VMlEYVM1 +0MGqIbEv52ZnoEaZDHl4yprYeTs+b/7NGvdG1+N/YNAjkpk8cCBKUXo4ZhkgAZoW +jbv3DkAdkpQHipUYkQZNRws1ebyfTbKaEPxw7abEh9TJrHD1EI9hbmYOGJWLfe1e +zeBQjFioQA31FcQR3/v+aNEDX390+qi3p0LXe7GMabgcoFYcGXO7XvX0DdUBvdZZ +dyHA7cJvyfWfcbucI7xQ9xvAnu/4Ih4D8mHnJXjZK5ReQn06FPM/ZCgZ5LrHAKcZ +0mrOts/8noY9dMmBreSJmLCP8EqzY7yKJFFHVCeKo+bU6/KOyNhJGGSCHVJ/pZGK +ZpOQcNwVvHciLH+MfpW12xJXPEs8Wv24KufDdBCDliSFnVTYH3kZaq4Ozb7+3A5j +wUQ2aDg8nrq4oNORMSCafvia8MYH3NXbpUq1SAyD5DTKtMcWY3gcVnJgrBai1hPn +TPhrMb2NMDFnMnj7/l8jdu9xHrsgOmOrv7Zj0ytmpT6ITJgWNGXsiq7Dp+HH1c6N +ggG6g0zqoyoaxcPVN7PMrWTvfKUD3LHfIsesPc4+lT+TSlBQYwIDAQABo1MwUTAd +BgNVHQ4EFgQU8mBHR/00anEl8Io/A2c0LQlGF5MwHwYDVR0jBBgwFoAU8mBHR/00 +anEl8Io/A2c0LQlGF5MwDwYDVR0TAQH/BAUwAwEB/zANBgkqhkiG9w0BAQsFAAOC +AgEABZYGEeJY2dgyi4W0LNVgN8mKuZuapIcisQ66foe46WuWGAjVONIHlb0Ciy75 +aaClLC8fiiIh+FUFZ5aIZkfhKH97QvehFO5O7mqCjM7ipvtEm+Vs1IVtXWDONUxo +SfgbjEPBV8+eflgvKQ6jJqiSqs8EnqdbGAfhxVG/3RN1b5xSFtKz6kzHQE+Gy6QT +DGCVhYvDq8j6G2LCePsqE8piOnSaXuRwD4/YEOaYhx4jjgOnaM0m/dM/Cx9wy2xg +LMRBjBwxFf6palgiFUvyqvturIPONQICkM/lZkpmHbeM4FCat/CD5VW+JgpYiEtW +2oFslTEbawUjmEYnzdo9iw9KPLJQqtasFEWzkWWnJrfm7AVGxcgAHVqGZhUMgq0k +MccM2zYZN2fCSZUUueDB7VCxFq5jK2oLzE14ngXdR7ZbxT3qai/zvGg1kl9y1bIF +WVTK0WZnHqZwVnHQVBH0Duv0uyRUzb6yRRziuLN5aBGQpy/Jm7MS0jLidCbqoCXC +dYqGMFlImzU+6CwPyTJo+X+v6L+FATIxZRpBBeEhHqEU6wz51ms68Sjx4bpW33b+ +WFt0JKEmIxB1puJK1qQvKu/MxJyy52GNqiRg7HXkJH9MMYWoAkF2jKMLFoerUPun +7GaV8SIUTFO/5pbnpxZ97a2FuB2RvDKs7GSdspEmC3wbAPU= +-----END CERTIFICATE----- From e52b5b7f3eec9b4d80d55464216639952712cdb0 Mon Sep 17 00:00:00 2001 From: Todd Short Date: Thu, 18 Jul 2024 17:30:18 -0400 Subject: [PATCH 2/4] fixup! Add support for CA/certificate rotation --- cmd/manager/main.go | 40 ++-------- hack/test/cert-e2e.sh | 2 +- .../clusterextension_controller.go | 3 - internal/httputil/certpoolwatcher.go | 80 +++++++++++++++++++ internal/httputil/certutil.go | 44 ---------- internal/httputil/httputil.go | 10 ++- internal/rukpak/source/image_registry.go | 13 +-- 7 files changed, 102 insertions(+), 90 deletions(-) create mode 100644 internal/httputil/certpoolwatcher.go diff --git a/cmd/manager/main.go b/cmd/manager/main.go index 0c26cf46f3..2905eb3cff 100644 --- a/cmd/manager/main.go +++ b/cmd/manager/main.go @@ -18,7 +18,6 @@ package main import ( "context" - "crypto/x509" "flag" "fmt" "net/http" @@ -178,23 +177,16 @@ func main() { os.Exit(1) } - certPool, err := httputil.NewCertPool(caCertDir, ctrl.Log.WithName("cert-pool")) + certPoolWatcher, err := httputil.NewCertPoolWatcher(caCertDir, ctrl.Log.WithName("cert-pool")) if err != nil { setupLog.Error(err, "unable to create CA certificate pool") os.Exit(1) } - - ceRecon := &controllers.ClusterExtensionReconciler{} - unpacker := &source.ImageRegistry{ BaseCachePath: filepath.Join(cachePath, "unpack"), // TODO: This needs to be derived per extension via ext.Spec.InstallNamespace - AuthNamespace: systemNamespace, - GetCaCertPool: func() (*x509.CertPool, error) { - ceRecon.CaMutex.RLock() - defer ceRecon.CaMutex.RUnlock() - return ceRecon.CaCertPool.Clone(), nil - }, + AuthNamespace: systemNamespace, + CertPoolWatcher: certPoolWatcher, } clusterExtensionFinalizers := crfinalizer.NewFinalizers() @@ -226,9 +218,7 @@ func main() { os.Exit(1) } catalogClient := catalogclient.New(cache.NewFilesystemCache(catalogsCachePath, func() (*http.Client, error) { - ceRecon.CaMutex.RLock() - defer ceRecon.CaMutex.RUnlock() - return httputil.BuildHTTPClient(ceRecon.CaCertPool.Clone()) + return httputil.BuildHTTPClient(certPoolWatcher) })) resolver := &resolve.CatalogResolver{ @@ -243,17 +233,16 @@ func main() { catalogClient.GetPackage, ), } - ceRecon = &controllers.ClusterExtensionReconciler{ + + if err = (&controllers.ClusterExtensionReconciler{ Client: cl, Resolver: resolver, ActionClientGetter: acg, Unpacker: unpacker, InstalledBundleGetter: &controllers.DefaultInstalledBundleGetter{ActionClientGetter: acg}, Finalizers: clusterExtensionFinalizers, - CaCertPool: certPool, Preflights: preflights, - } - if err = ceRecon.SetupWithManager(mgr); err != nil { + }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "ClusterExtension") os.Exit(1) } @@ -267,21 +256,6 @@ func main() { setupLog.Error(err, "unable to set up ready check") os.Exit(1) } - setupLog.Info("adding certificate watch", "directory", caCertDir) - if err := httputil.AddCertWatch(caCertDir, func() error { - ctrl.Log.WithName("cert-pool").Info("reloading certificate pool") - certPool, err := httputil.NewCertPool(caCertDir, ctrl.Log.WithName("cert-pool")) - if err != nil { - return err - } - ceRecon.CaMutex.Lock() - defer ceRecon.CaMutex.Unlock() - ceRecon.CaCertPool = certPool - return nil - }); err != nil { - setupLog.Error(err, "unable to add CA certificate watch") - os.Exit(1) - } setupLog.Info("starting manager") ctx := ctrl.SetupSignalHandler() diff --git a/hack/test/cert-e2e.sh b/hack/test/cert-e2e.sh index 37fb9e06c0..57b9e6f298 100755 --- a/hack/test/cert-e2e.sh +++ b/hack/test/cert-e2e.sh @@ -18,7 +18,7 @@ kubectl wait --for=condition=Available --namespace=olmv1-system "deployment/oper # and then search through the previous logs function check_logs() { - kubectl logs -c manager -n olmv1-system -l control-plane=controller-manager | grep "reloading certificate pool" >& /dev/null + kubectl logs -c manager -n olmv1-system -l control-plane=controller-manager | grep "updating certificate pool" >& /dev/null } WAIT_TIME=0 diff --git a/internal/controllers/clusterextension_controller.go b/internal/controllers/clusterextension_controller.go index 1b46e531df..49dcea8322 100644 --- a/internal/controllers/clusterextension_controller.go +++ b/internal/controllers/clusterextension_controller.go @@ -19,7 +19,6 @@ package controllers import ( "bytes" "context" - "crypto/x509" "errors" "fmt" "io" @@ -89,8 +88,6 @@ type ClusterExtensionReconciler struct { cache cache.Cache InstalledBundleGetter InstalledBundleGetter Finalizers crfinalizer.Finalizers - CaMutex sync.RWMutex - CaCertPool *x509.CertPool Preflights []Preflight } diff --git a/internal/httputil/certpoolwatcher.go b/internal/httputil/certpoolwatcher.go new file mode 100644 index 0000000000..cae293b7c9 --- /dev/null +++ b/internal/httputil/certpoolwatcher.go @@ -0,0 +1,80 @@ +package httputil + +import ( + "crypto/x509" + "fmt" + "os" + "sync" + "time" + + "github.com/fsnotify/fsnotify" + "github.com/go-logr/logr" +) + +type CertPoolWatcher struct { + dir string + mx sync.RWMutex + pool *x509.CertPool + watcher *fsnotify.Watcher +} + +func (cpw *CertPoolWatcher) Get() (*x509.CertPool, error) { + if cpw.pool == nil { + return nil, fmt.Errorf("no certificate pool available") + } + cpw.mx.RLock() + defer cpw.mx.RUnlock() + return cpw.pool.Clone(), nil +} + +func NewCertPoolWatcher(caDir string, log logr.Logger) (*CertPoolWatcher, error) { + pool, err := NewCertPool(caDir, log) + if err != nil { + return nil, err + } + watcher, err := fsnotify.NewWatcher() + if err != nil { + return nil, err + } + if err = watcher.Add(caDir); err != nil { + return nil, err + } + cpw := &CertPoolWatcher{ + dir: caDir, + pool: pool, + watcher: watcher, + } + go func() { + for { + select { + case <-watcher.Events: + func() { + // drain as many events as possible before doing anything + func() { + for { + time.Sleep(time.Millisecond * 100) + select { + case <-watcher.Events: + default: + return + } + } + }() + log.Info("updating certificate pool") + pool, err := NewCertPool(caDir, log) + if err != nil { + log.Error(err, "error updating certificate pool") + os.Exit(1) + } + cpw.mx.Lock() + defer cpw.mx.Unlock() + cpw.pool = pool + }() + case err = <-watcher.Errors: + log.Error(err, "error watching certificate dir") + os.Exit(1) + } + } + }() + return cpw, nil +} diff --git a/internal/httputil/certutil.go b/internal/httputil/certutil.go index d0820d433a..ee619563e5 100644 --- a/internal/httputil/certutil.go +++ b/internal/httputil/certutil.go @@ -2,7 +2,6 @@ package httputil import ( "bytes" - "context" "crypto/x509" "encoding/base64" "encoding/pem" @@ -11,7 +10,6 @@ import ( "path/filepath" "time" - "github.com/fsnotify/fsnotify" "github.com/go-logr/logr" ) @@ -249,45 +247,3 @@ func NewCertPool(caDir string, log logr.Logger) (*x509.CertPool, error) { log.Info("first expiration", "time", firstExpiration.Format(time.RFC3339)) return caCertPool, nil } - -func AddCertWatch(caDir string, update func() error) error { - watcher, err := fsnotify.NewWatcher() - if err != nil { - return err - } - if err = watcher.Add(caDir); err != nil { - return err - } - go func() { - for { - select { - case <-watcher.Events: - // drain as many events as possible before doing anything - func() { - for { - time.Sleep(time.Millisecond * 100) - select { - case <-watcher.Events: - default: - return - } - } - }() - if err := update(); err != nil { - log, err2 := logr.FromContext(context.Background()) - if err2 == nil { - log.WithName("cert-pool").Error(err, "error updating cert-pool") - } - os.Exit(1) - } - case err = <-watcher.Errors: - log, err2 := logr.FromContext(context.Background()) - if err2 == nil { - log.WithName("cert-pool").Error(err, "error watching certificate dir") - } - os.Exit(1) - } - } - }() - return nil -} diff --git a/internal/httputil/httputil.go b/internal/httputil/httputil.go index 2f15bfaf16..9aedf2c9e7 100644 --- a/internal/httputil/httputil.go +++ b/internal/httputil/httputil.go @@ -2,16 +2,20 @@ package httputil import ( "crypto/tls" - "crypto/x509" "net/http" "time" ) -func BuildHTTPClient(caCertPool *x509.CertPool) (*http.Client, error) { +func BuildHTTPClient(cpw *CertPoolWatcher) (*http.Client, error) { httpClient := &http.Client{Timeout: 10 * time.Second} + pool, err := cpw.Get() + if err != nil { + return nil, err + } + tlsConfig := &tls.Config{ - RootCAs: caCertPool, + RootCAs: pool, MinVersion: tls.VersionTLS12, } tlsTransport := &http.Transport{ diff --git a/internal/rukpak/source/image_registry.go b/internal/rukpak/source/image_registry.go index ef898b9f20..4ce10792b0 100644 --- a/internal/rukpak/source/image_registry.go +++ b/internal/rukpak/source/image_registry.go @@ -4,7 +4,6 @@ import ( "archive/tar" "context" "crypto/tls" - "crypto/x509" "errors" "fmt" "io/fs" @@ -20,6 +19,8 @@ import ( "github.com/google/go-containerregistry/pkg/v1/remote" apimacherrors "k8s.io/apimachinery/pkg/util/errors" "sigs.k8s.io/controller-runtime/pkg/log" + + "github.com/operator-framework/operator-controller/internal/httputil" ) // SourceTypeImage is the identifier for image-type bundle sources @@ -51,9 +52,9 @@ func NewUnrecoverable(err error) *Unrecoverable { // TODO: Make asynchronous type ImageRegistry struct { - BaseCachePath string - AuthNamespace string - GetCaCertPool func() (*x509.CertPool, error) + BaseCachePath string + AuthNamespace string + CertPoolWatcher *httputil.CertPoolWatcher } func (i *ImageRegistry) Unpack(ctx context.Context, bundle *BundleSource) (*Result, error) { @@ -99,8 +100,8 @@ func (i *ImageRegistry) Unpack(ctx context.Context, bundle *BundleSource) (*Resu if bundle.Image.InsecureSkipTLSVerify { transport.TLSClientConfig.InsecureSkipVerify = true // nolint:gosec } - if i.GetCaCertPool != nil { - pool, err := i.GetCaCertPool() + if i.CertPoolWatcher != nil { + pool, err := i.CertPoolWatcher.Get() if err != nil { return nil, err } From 5e7beeec8bc7e3f23a12fd318c81c798abbd4eef Mon Sep 17 00:00:00 2001 From: Todd Short Date: Fri, 19 Jul 2024 09:57:23 -0400 Subject: [PATCH 3/4] fixup! Add support for CA/certificate rotation Signed-off-by: Todd Short --- Makefile | 1 - hack/test/cert-e2e.sh | 7 +--- internal/httputil/certpoolwatcher.go | 53 ++++++++++++++++------------ 3 files changed, 32 insertions(+), 29 deletions(-) diff --git a/Makefile b/Makefile index cfea3f9109..966020e458 100644 --- a/Makefile +++ b/Makefile @@ -194,7 +194,6 @@ run-cert-e2e: .PHONY: test-cert-e2e test-cert-e2e: KIND_CLUSTER_NAME := operator-controller-cert-e2e -test-cert-e2e: KUSTOMIZE_BUILD_DIR := config/overlays/cert-manager test-cert-e2e: run run-cert-e2e kind-clean #HELP Run certificate e2e tests on a local kind cluster .PHONY: e2e-coverage diff --git a/hack/test/cert-e2e.sh b/hack/test/cert-e2e.sh index 57b9e6f298..ac4edf2d43 100755 --- a/hack/test/cert-e2e.sh +++ b/hack/test/cert-e2e.sh @@ -11,11 +11,6 @@ kubectl patch certificate.cert-manager.io -n olmv1-system olmv1-cert --type='mer kubectl delete secret -n cert-manager olmv1-ca kubectl delete secret -n olmv1-system olmv1-cert -# delete the existing operator-controller, to force it to get a new secret -# (deleting the secret itself isn't enough) -kubectl delete pod -n olmv1-system -l control-plane=controller-manager -kubectl wait --for=condition=Available --namespace=olmv1-system "deployment/operator-controller-controller-manager" --timeout="60s" - # and then search through the previous logs function check_logs() { kubectl logs -c manager -n olmv1-system -l control-plane=controller-manager | grep "updating certificate pool" >& /dev/null @@ -23,7 +18,7 @@ function check_logs() { WAIT_TIME=0 MAX_TIME=300 -until (( NEXT_WAIT_TIME == MAX_TIME )) || check_logs; do +until (( WAIT_TIME == MAX_TIME )) || check_logs; do echo -n "." sleep "$(( WAIT_TIME += 10 ))" done diff --git a/internal/httputil/certpoolwatcher.go b/internal/httputil/certpoolwatcher.go index cae293b7c9..6eeecad26b 100644 --- a/internal/httputil/certpoolwatcher.go +++ b/internal/httputil/certpoolwatcher.go @@ -15,6 +15,7 @@ type CertPoolWatcher struct { dir string mx sync.RWMutex pool *x509.CertPool + log logr.Logger watcher *fsnotify.Watcher } @@ -27,6 +28,33 @@ func (cpw *CertPoolWatcher) Get() (*x509.CertPool, error) { return cpw.pool.Clone(), nil } +func (cpw *CertPoolWatcher) update() { + cpw.log.Info("updating certificate pool") + pool, err := NewCertPool(cpw.dir, cpw.log) + if err != nil { + cpw.log.Error(err, "error updating certificate pool") + os.Exit(1) + } + cpw.mx.Lock() + defer cpw.mx.Unlock() + cpw.pool = pool +} + +// 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 +func (cpw *CertPoolWatcher) drainEvents() { + for { + // sleep to let events accumulate + time.Sleep(time.Millisecond * 50) + select { + case <-cpw.watcher.Events: + default: + return + } + } +} + func NewCertPoolWatcher(caDir string, log logr.Logger) (*CertPoolWatcher, error) { pool, err := NewCertPool(caDir, log) if err != nil { @@ -42,34 +70,15 @@ func NewCertPoolWatcher(caDir string, log logr.Logger) (*CertPoolWatcher, error) cpw := &CertPoolWatcher{ dir: caDir, pool: pool, + log: log, watcher: watcher, } go func() { for { select { case <-watcher.Events: - func() { - // drain as many events as possible before doing anything - func() { - for { - time.Sleep(time.Millisecond * 100) - select { - case <-watcher.Events: - default: - return - } - } - }() - log.Info("updating certificate pool") - pool, err := NewCertPool(caDir, log) - if err != nil { - log.Error(err, "error updating certificate pool") - os.Exit(1) - } - cpw.mx.Lock() - defer cpw.mx.Unlock() - cpw.pool = pool - }() + cpw.drainEvents() + cpw.update() case err = <-watcher.Errors: log.Error(err, "error watching certificate dir") os.Exit(1) From 5eea0725abebc3df0430037d7a384a65be7762b2 Mon Sep 17 00:00:00 2001 From: Todd Short Date: Fri, 19 Jul 2024 15:43:05 -0400 Subject: [PATCH 4/4] fixup! Add support for CA/certificate rotation Signed-off-by: Todd Short --- .github/workflows/e2e.yaml | 12 --- Makefile | 8 -- hack/test/cert-e2e.sh | 26 ------ internal/httputil/certpoolwatcher.go | 97 ++++++++++++++--------- internal/httputil/certpoolwatcher_test.go | 97 +++++++++++++++++++++++ internal/httputil/certutil.go | 5 +- internal/httputil/httputil.go | 2 +- internal/rukpak/source/image_registry.go | 2 +- 8 files changed, 159 insertions(+), 90 deletions(-) delete mode 100755 hack/test/cert-e2e.sh create mode 100644 internal/httputil/certpoolwatcher_test.go diff --git a/.github/workflows/e2e.yaml b/.github/workflows/e2e.yaml index 7c69f11fa5..06a2e9d31a 100644 --- a/.github/workflows/e2e.yaml +++ b/.github/workflows/e2e.yaml @@ -59,15 +59,3 @@ jobs: - name: Run the upgrade e2e test run: make test-upgrade-e2e - - cert-e2e: - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v4 - - - uses: actions/setup-go@v5 - with: - go-version-file: go.mod - - - name: Run the cert e2e test - run: make test-cert-e2e diff --git a/Makefile b/Makefile index 966020e458..587502ff92 100644 --- a/Makefile +++ b/Makefile @@ -188,14 +188,6 @@ test-upgrade-e2e: export TEST_CLUSTER_CATALOG_NAME := test-catalog test-upgrade-e2e: export TEST_CLUSTER_EXTENSION_NAME := test-package test-upgrade-e2e: kind-cluster run-latest-release image-registry build-push-e2e-catalog registry-load-bundles pre-upgrade-setup docker-build kind-load kind-deploy post-upgrade-checks kind-clean #HELP Run upgrade e2e tests on a local kind cluster -.PHONY: run-cert-e2e -run-cert-e2e: - ./hack/test/cert-e2e.sh - -.PHONY: test-cert-e2e -test-cert-e2e: KIND_CLUSTER_NAME := operator-controller-cert-e2e -test-cert-e2e: run run-cert-e2e kind-clean #HELP Run certificate e2e tests on a local kind cluster - .PHONY: e2e-coverage e2e-coverage: COVERAGE_OUTPUT=./coverage/e2e.out ./hack/test/e2e-coverage.sh diff --git a/hack/test/cert-e2e.sh b/hack/test/cert-e2e.sh deleted file mode 100755 index ac4edf2d43..0000000000 --- a/hack/test/cert-e2e.sh +++ /dev/null @@ -1,26 +0,0 @@ -#!/bin/bash - -set -e - -# patch the certificates to renew every minute -# (if set to 2 minutes, then the big timeout wait is about 5 minutes) -kubectl patch certificate.cert-manager.io -n cert-manager olmv1-ca --type='merge' -p '{"spec":{"duration":"1h", "renewBefore":"59m"}}' -kubectl patch certificate.cert-manager.io -n olmv1-system olmv1-cert --type='merge' -p '{"spec":{"duration":"1h", "renewBefore":"59m"}}' - -# delete the old secrets, so new secrets are generated -kubectl delete secret -n cert-manager olmv1-ca -kubectl delete secret -n olmv1-system olmv1-cert - -# and then search through the previous logs -function check_logs() { - kubectl logs -c manager -n olmv1-system -l control-plane=controller-manager | grep "updating certificate pool" >& /dev/null -} - -WAIT_TIME=0 -MAX_TIME=300 -until (( WAIT_TIME == MAX_TIME )) || check_logs; do - echo -n "." - sleep "$(( WAIT_TIME += 10 ))" -done -(( WAIT_TIME < MAX_TIME )) -echo "#" diff --git a/internal/httputil/certpoolwatcher.go b/internal/httputil/certpoolwatcher.go index 6eeecad26b..2a250d0695 100644 --- a/internal/httputil/certpoolwatcher.go +++ b/internal/httputil/certpoolwatcher.go @@ -12,47 +12,27 @@ import ( ) type CertPoolWatcher struct { - dir string - mx sync.RWMutex - pool *x509.CertPool - log logr.Logger - watcher *fsnotify.Watcher + generation int + dir string + mx sync.RWMutex + pool *x509.CertPool + log logr.Logger + watcher *fsnotify.Watcher + done chan bool } -func (cpw *CertPoolWatcher) Get() (*x509.CertPool, error) { - if cpw.pool == nil { - return nil, fmt.Errorf("no certificate pool available") - } +// Returns the current CertPool and the generation number +func (cpw *CertPoolWatcher) Get() (*x509.CertPool, int, error) { cpw.mx.RLock() defer cpw.mx.RUnlock() - return cpw.pool.Clone(), nil -} - -func (cpw *CertPoolWatcher) update() { - cpw.log.Info("updating certificate pool") - pool, err := NewCertPool(cpw.dir, cpw.log) - if err != nil { - cpw.log.Error(err, "error updating certificate pool") - os.Exit(1) + if cpw.pool == nil { + return nil, 0, fmt.Errorf("no certificate pool available") } - cpw.mx.Lock() - defer cpw.mx.Unlock() - cpw.pool = pool + return cpw.pool.Clone(), cpw.generation, nil } -// 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 -func (cpw *CertPoolWatcher) drainEvents() { - for { - // sleep to let events accumulate - time.Sleep(time.Millisecond * 50) - select { - case <-cpw.watcher.Events: - default: - return - } - } +func (cpw *CertPoolWatcher) Done() { + cpw.done <- true } func NewCertPoolWatcher(caDir string, log logr.Logger) (*CertPoolWatcher, error) { @@ -67,11 +47,14 @@ func NewCertPoolWatcher(caDir string, log logr.Logger) (*CertPoolWatcher, error) if err = watcher.Add(caDir); err != nil { return nil, err } + cpw := &CertPoolWatcher{ - dir: caDir, - pool: pool, - log: log, - watcher: watcher, + generation: 1, + dir: caDir, + pool: pool, + log: log, + watcher: watcher, + done: make(chan bool), } go func() { for { @@ -79,11 +62,47 @@ func NewCertPoolWatcher(caDir string, log logr.Logger) (*CertPoolWatcher, error) case <-watcher.Events: cpw.drainEvents() cpw.update() - case err = <-watcher.Errors: + case err := <-watcher.Errors: log.Error(err, "error watching certificate dir") os.Exit(1) + case <-cpw.done: + err := watcher.Close() + if err != nil { + log.Error(err, "error closing watcher") + } + return } } }() return cpw, nil } + +func (cpw *CertPoolWatcher) update() { + cpw.log.Info("updating certificate pool") + pool, err := NewCertPool(cpw.dir, cpw.log) + if err != nil { + cpw.log.Error(err, "error updating certificate pool") + os.Exit(1) + } + cpw.mx.Lock() + defer cpw.mx.Unlock() + cpw.pool = pool + cpw.generation++ +} + +// 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 +func (cpw *CertPoolWatcher) drainEvents() { + for { + drainTimer := time.NewTimer(time.Millisecond * 50) + select { + case <-drainTimer.C: + return + case <-cpw.watcher.Events: + } + if !drainTimer.Stop() { + <-drainTimer.C + } + } +} diff --git a/internal/httputil/certpoolwatcher_test.go b/internal/httputil/certpoolwatcher_test.go new file mode 100644 index 0000000000..bfebebd288 --- /dev/null +++ b/internal/httputil/certpoolwatcher_test.go @@ -0,0 +1,97 @@ +package httputil_test + +import ( + "context" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/x509" + "crypto/x509/pkix" + "encoding/pem" + "math/big" + "os" + "path/filepath" + "testing" + "time" + + "github.com/stretchr/testify/require" + "sigs.k8s.io/controller-runtime/pkg/log" + + "github.com/operator-framework/operator-controller/internal/httputil" +) + +func createCert(t *testing.T, name string) { + priv, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + + notBefore := time.Now() + notAfter := notBefore.Add(time.Hour) + serialNumberLimit := new(big.Int).Lsh(big.NewInt(1), 128) + serialNumber, err := rand.Int(rand.Reader, serialNumberLimit) + require.NoError(t, err) + + template := x509.Certificate{ + SerialNumber: serialNumber, + Subject: pkix.Name{ + Organization: []string{name}, + }, + NotBefore: notBefore, + NotAfter: notAfter, + + IsCA: true, + + KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, + + BasicConstraintsValid: true, + } + + derBytes, err := x509.CreateCertificate(rand.Reader, &template, &template, &priv.PublicKey, priv) + require.NoError(t, err) + + certOut, err := os.Create(name) + require.NoError(t, err) + + err = pem.Encode(certOut, &pem.Block{Type: "CERTIFICATE", Bytes: derBytes}) + require.NoError(t, err) + + err = certOut.Close() + require.NoError(t, err) + + // ignore the key +} + +func TestCertPoolWatcher(t *testing.T) { + // create a temporary directory + tmpDir, err := os.MkdirTemp("", "cert-pool") + require.NoError(t, err) + defer os.RemoveAll(tmpDir) + + // create the first cert + certName := filepath.Join(tmpDir, "test1.pem") + t.Logf("Create cert file at %q\n", certName) + createCert(t, certName) + + // Create the cert pool watcher + cpw, err := httputil.NewCertPoolWatcher(tmpDir, log.FromContext(context.Background())) + require.NoError(t, err) + defer cpw.Done() + + // Get the original pool + firstPool, firstGen, err := cpw.Get() + require.NoError(t, err) + require.NotNil(t, firstPool) + + // Create a second cert + certName = filepath.Join(tmpDir, "test2.pem") + t.Logf("Create cert file at %q\n", certName) + createCert(t, certName) + + require.Eventually(t, func() bool { + secondPool, secondGen, err := cpw.Get() + if err != nil { + return false + } + return secondGen != firstGen && !firstPool.Equal(secondPool) + }, 30*time.Second, time.Second) +} diff --git a/internal/httputil/certutil.go b/internal/httputil/certutil.go index ee619563e5..767fd57a64 100644 --- a/internal/httputil/certutil.go +++ b/internal/httputil/certutil.go @@ -190,10 +190,9 @@ func appendCertsFromPEM(s *x509.CertPool, pemCerts []byte, firstExpiration *time return fmt.Errorf("not yet valid cert %d: %q", n, cert.NotBefore.Format(time.RFC3339)) } else if now.After(cert.NotAfter) { return fmt.Errorf("expired cert %d: %q", n, cert.NotAfter.Format(time.RFC3339)) - } else { - // no return values - panics or always succeeds - s.AddCert(cert) } + // no return values - panics or always succeeds + s.AddCert(cert) n++ } diff --git a/internal/httputil/httputil.go b/internal/httputil/httputil.go index 9aedf2c9e7..d620866e4d 100644 --- a/internal/httputil/httputil.go +++ b/internal/httputil/httputil.go @@ -9,7 +9,7 @@ import ( func BuildHTTPClient(cpw *CertPoolWatcher) (*http.Client, error) { httpClient := &http.Client{Timeout: 10 * time.Second} - pool, err := cpw.Get() + pool, _, err := cpw.Get() if err != nil { return nil, err } diff --git a/internal/rukpak/source/image_registry.go b/internal/rukpak/source/image_registry.go index 4ce10792b0..a6d6640d42 100644 --- a/internal/rukpak/source/image_registry.go +++ b/internal/rukpak/source/image_registry.go @@ -101,7 +101,7 @@ func (i *ImageRegistry) Unpack(ctx context.Context, bundle *BundleSource) (*Resu transport.TLSClientConfig.InsecureSkipVerify = true // nolint:gosec } if i.CertPoolWatcher != nil { - pool, err := i.CertPoolWatcher.Get() + pool, _, err := i.CertPoolWatcher.Get() if err != nil { return nil, err }