From 33e3fc381d1ca8b38473cdbcde7505edf63fb5ae Mon Sep 17 00:00:00 2001 From: Nico Schieder Date: Mon, 9 Sep 2024 09:58:24 +0200 Subject: [PATCH] Ensure docker registry CA is trusted in e2e tests --- Makefile | 4 +- cmd/manager/main.go | 59 ++++++++++++++- cmd/manager/main_test.go | 73 +++++++++++++++++++ config/base/manager/manager.yaml | 1 + .../tls/patches/manager_deployment_certs.yaml | 3 + internal/source/image_registry_client.go | 14 ++++ internal/source/unpacker.go | 4 +- test/e2e/unpack_test.go | 3 +- test/tools/imageregistry/imgreg.yaml | 12 +-- 9 files changed, 158 insertions(+), 15 deletions(-) create mode 100644 cmd/manager/main_test.go diff --git a/Makefile b/Makefile index 543064af..43da8b35 100644 --- a/Makefile +++ b/Makefile @@ -113,10 +113,12 @@ verify: tidy fmt vet generate ## Verify the current code generation and lint lint: $(GOLANGCI_LINT) ## Run golangci linter. $(GOLANGCI_LINT) run $(GOLANGCI_LINT_ARGS) +## image-registry target has to come after run-latest-release, +## because the image-registry depends on the olm-ca issuer. .PHONY: test-upgrade-e2e test-upgrade-e2e: export TEST_CLUSTER_CATALOG_NAME := test-catalog test-upgrade-e2e: export TEST_CLUSTER_CATALOG_IMAGE := docker-registry.catalogd-e2e.svc:5000/test-catalog:e2e -test-upgrade-e2e: kind-cluster cert-manager build-container kind-load image-registry run-latest-release pre-upgrade-setup only-deploy-manifest wait post-upgrade-checks kind-cluster-cleanup ## Run upgrade e2e tests on a local kind cluster +test-upgrade-e2e: kind-cluster cert-manager build-container kind-load run-latest-release image-registry pre-upgrade-setup only-deploy-manifest wait post-upgrade-checks kind-cluster-cleanup ## Run upgrade e2e tests on a local kind cluster pre-upgrade-setup: ./test/tools/imageregistry/pre-upgrade-setup.sh ${TEST_CLUSTER_CATALOG_IMAGE} ${TEST_CLUSTER_CATALOG_NAME} diff --git a/cmd/manager/main.go b/cmd/manager/main.go index ff7c102f..6145570e 100644 --- a/cmd/manager/main.go +++ b/cmd/manager/main.go @@ -18,6 +18,7 @@ package main import ( "crypto/tls" + "crypto/x509" "flag" "fmt" "log" @@ -26,6 +27,7 @@ import ( "path/filepath" "time" + "github.com/go-logr/logr" "github.com/spf13/pflag" "k8s.io/apimachinery/pkg/runtime" utilruntime "k8s.io/apimachinery/pkg/util/runtime" @@ -81,6 +83,7 @@ func main() { certFile string keyFile string webhookPort int + caCertDir string ) flag.StringVar(&metricsAddr, "metrics-bind-address", ":8080", "The address the metric endpoint binds to.") flag.StringVar(&probeAddr, "health-probe-bind-address", ":8081", "The address the probe endpoint binds to.") @@ -97,6 +100,7 @@ func main() { flag.StringVar(&certFile, "tls-cert", "", "The certificate file used for serving catalog contents over HTTPS. Requires tls-key.") flag.StringVar(&keyFile, "tls-key", "", "The key file used for serving catalog contents over HTTPS. Requires tls-cert.") flag.IntVar(&webhookPort, "webhook-server-port", 9443, "The port that the mutating webhook server serves at.") + flag.StringVar(&caCertDir, "ca-certs-dir", "", "The directory of TLS certificate to use for verifying HTTPS connections to the Catalogd and docker-registry web servers.") opts := zap.Options{ Development: true, } @@ -175,7 +179,13 @@ func main() { os.Exit(1) } - unpacker, err := source.NewDefaultUnpacker(systemNamespace, cacheDir) + certPool, err := newCertPool(caCertDir, ctrl.Log.WithName("cert-pool")) + if err != nil { + setupLog.Error(err, "unable to create CA certificate pool") + os.Exit(1) + } + + unpacker, err := source.NewDefaultUnpacker(systemNamespace, cacheDir, certPool) if err != nil { setupLog.Error(err, "unable to create unpacker") os.Exit(1) @@ -269,3 +279,50 @@ func podNamespace() string { } return string(namespace) } + +// Should share code from operator-controller. +// see: https://issues.redhat.com/browse/OPRUN-3535 +func newCertPool(caDir string, log logr.Logger) (*x509.CertPool, error) { + caCertPool, err := x509.SystemCertPool() + if err != nil { + return nil, err + } + if caDir == "" { + return caCertPool, nil + } + + dirEntries, err := os.ReadDir(caDir) + if err != nil { + return nil, err + } + count := 0 + + for _, e := range dirEntries { + 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 + } + 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) + } + + if ok := caCertPool.AppendCertsFromPEM(data); ok { + count++ + } + } + + // Found no certs! + if count == 0 { + return nil, fmt.Errorf("no certificates found in %q", caDir) + } + + return caCertPool, nil +} diff --git a/cmd/manager/main_test.go b/cmd/manager/main_test.go new file mode 100644 index 00000000..b17319a2 --- /dev/null +++ b/cmd/manager/main_test.go @@ -0,0 +1,73 @@ +package main + +import ( + "crypto/rand" + "crypto/rsa" + "crypto/x509" + "crypto/x509/pkix" + "encoding/pem" + "math/big" + "os" + "testing" + "time" + + "github.com/go-logr/logr/testr" + "github.com/stretchr/testify/require" +) + +func Test_newCertPool(t *testing.T) { + t.Parallel() + + // set up our CA certificate + ca := &x509.Certificate{ + SerialNumber: big.NewInt(2019), + Subject: pkix.Name{ + Organization: []string{"Company, INC."}, + Country: []string{"US"}, + Province: []string{""}, + Locality: []string{"San Francisco"}, + StreetAddress: []string{"Golden Gate Bridge"}, + PostalCode: []string{"94016"}, + }, + NotBefore: time.Now(), + NotAfter: time.Now().AddDate(10, 0, 0), + IsCA: true, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth, x509.ExtKeyUsageServerAuth}, + KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign, + BasicConstraintsValid: true, + } + + // create our private and public key + caPrivKey, err := rsa.GenerateKey(rand.Reader, 4096) + require.NoError(t, err) + + // create the CA + caBytes, err := x509.CreateCertificate(rand.Reader, ca, ca, &caPrivKey.PublicKey, caPrivKey) + require.NoError(t, err) + + // pem encode + err = os.MkdirAll("testdata/newCertPool/subfolder", 0700) + require.NoError(t, err) + t.Cleanup(func() { + require.NoError(t, os.RemoveAll("testdata/newCertPool")) + }) + + caPEM, err := os.Create("testdata/newCertPool/my.pem") + require.NoError(t, err) + err = pem.Encode(caPEM, &pem.Block{ + Type: "CERTIFICATE", + Bytes: caBytes, + }) + require.NoError(t, err) + + _, err = newCertPool("testdata/newCertPool", testr.New(t)) + require.NoError(t, err) +} + +func Test_newCertPool_empty(t *testing.T) { + err := os.MkdirAll("testdata/newCertPoolEmpty", 0700) + require.NoError(t, err) + + _, err = newCertPool("testdata/newCertPoolEmpty", testr.New(t)) + require.EqualError(t, err, `no certificates found in "testdata/newCertPoolEmpty"`) +} diff --git a/config/base/manager/manager.yaml b/config/base/manager/manager.yaml index 4fdb642c..f1ff8189 100644 --- a/config/base/manager/manager.yaml +++ b/config/base/manager/manager.yaml @@ -20,6 +20,7 @@ spec: matchLabels: control-plane: catalogd-controller-manager replicas: 1 + minReadySeconds: 5 template: metadata: annotations: diff --git a/config/components/tls/patches/manager_deployment_certs.yaml b/config/components/tls/patches/manager_deployment_certs.yaml index b0005f1c..91f8f8c0 100644 --- a/config/components/tls/patches/manager_deployment_certs.yaml +++ b/config/components/tls/patches/manager_deployment_certs.yaml @@ -10,3 +10,6 @@ - op: add path: /spec/template/spec/containers/1/args/- value: "--tls-key=/var/certs/tls.key" +- op: add + path: /spec/template/spec/containers/1/args/- + value: "--ca-certs-dir=/var/certs" diff --git a/internal/source/image_registry_client.go b/internal/source/image_registry_client.go index 60fd3f77..c1cfd338 100644 --- a/internal/source/image_registry_client.go +++ b/internal/source/image_registry_client.go @@ -4,6 +4,7 @@ import ( "archive/tar" "context" "crypto/tls" + "crypto/x509" "errors" "fmt" "io/fs" @@ -31,6 +32,7 @@ import ( type ImageRegistry struct { BaseCachePath string AuthNamespace string + CertPool *x509.CertPool } const ConfigDirLabel = "operators.operatorframework.io.index.configs.v1" @@ -51,6 +53,18 @@ func (i *ImageRegistry) Unpack(ctx context.Context, catalog *catalogdv1alpha1.Cl } remoteOpts := []remote.Option{} + tlsTransport := remote.DefaultTransport.(*http.Transport).Clone() + if tlsTransport.TLSClientConfig == nil { + tlsTransport.TLSClientConfig = &tls.Config{ + InsecureSkipVerify: false, + MinVersion: tls.VersionTLS12, + } // nolint:gosec + } + if i.CertPool != nil { + tlsTransport.TLSClientConfig.RootCAs = i.CertPool + } + remoteOpts = append(remoteOpts, remote.WithTransport(tlsTransport)) + if catalog.Spec.Source.Image.PullSecret != "" { chainOpts := k8schain.Options{ ImagePullSecrets: []string{catalog.Spec.Source.Image.PullSecret}, diff --git a/internal/source/unpacker.go b/internal/source/unpacker.go index f2ca09a2..7a9b90be 100644 --- a/internal/source/unpacker.go +++ b/internal/source/unpacker.go @@ -2,6 +2,7 @@ package source import ( "context" + "crypto/x509" "fmt" "io/fs" "os" @@ -110,7 +111,7 @@ const UnpackCacheDir = "unpack" // source types. // // TODO: refactor NewDefaultUnpacker due to growing parameter list -func NewDefaultUnpacker(namespace, cacheDir string) (Unpacker, error) { +func NewDefaultUnpacker(namespace, cacheDir string, certPool *x509.CertPool) (Unpacker, error) { unpackPath := path.Join(cacheDir, UnpackCacheDir) if err := os.MkdirAll(unpackPath, 0700); err != nil { return nil, fmt.Errorf("creating unpack cache directory: %w", err) @@ -120,6 +121,7 @@ func NewDefaultUnpacker(namespace, cacheDir string) (Unpacker, error) { catalogdv1alpha1.SourceTypeImage: &ImageRegistry{ BaseCachePath: unpackPath, AuthNamespace: namespace, + CertPool: certPool, }, }), nil } diff --git a/test/e2e/unpack_test.go b/test/e2e/unpack_test.go index d0f0d201..337d390f 100644 --- a/test/e2e/unpack_test.go +++ b/test/e2e/unpack_test.go @@ -54,8 +54,7 @@ var _ = Describe("ClusterCatalog Unpacking", func() { Source: catalogd.CatalogSource{ Type: catalogd.SourceTypeImage, Image: &catalogd.ImageSource{ - Ref: catalogImageRef(), - InsecureSkipTLSVerify: true, + Ref: catalogImageRef(), }, }, }, diff --git a/test/tools/imageregistry/imgreg.yaml b/test/tools/imageregistry/imgreg.yaml index af63a23f..7d86bc2a 100644 --- a/test/tools/imageregistry/imgreg.yaml +++ b/test/tools/imageregistry/imgreg.yaml @@ -4,14 +4,6 @@ metadata: name: catalogd-e2e --- apiVersion: cert-manager.io/v1 -kind: Issuer -metadata: - name: selfsigned-issuer - namespace: catalogd-e2e -spec: - selfSigned: {} ---- -apiVersion: cert-manager.io/v1 kind: Certificate metadata: name: catalogd-e2e-registry @@ -25,8 +17,8 @@ spec: algorithm: ECDSA size: 256 issuerRef: - name: selfsigned-issuer - kind: Issuer + name: olmv1-ca + kind: ClusterIssuer group: cert-manager.io --- apiVersion: apps/v1