Use x509.CertPools instead of PEM strings

Signed-off-by: Todd Short <tshort@redhat.com>

cmd/manager/main.go

@@ -152,7 +152,13 @@ func main() {
 		os.Exit(1)
 	}
 
-	httpClient, err := httputil.BuildHTTPClient(caCertDir)
+	certPool, err := httputil.NewCertPool(caCertDir)
+	if err != nil {
+		setupLog.Error(err, "unable to create CA certificate pool")
+		os.Exit(1)
+	}
+
+	httpClient, err := httputil.BuildHTTPClient(certPool)
 	if err != nil {
 		setupLog.Error(err, "unable to create catalogd http client")
 	}
@@ -220,7 +226,7 @@ func main() {
 		Unpacker:              unpacker,
 		InstalledBundleGetter: &controllers.DefaultInstalledBundleGetter{ActionClientGetter: acg},
 		Finalizers:            clusterExtensionFinalizers,
-		CaCertDir:             caCertDir,
+		CaCertPool:            certPool,
 		Preflights:            preflights,
 	}).SetupWithManager(mgr); err != nil {
 		setupLog.Error(err, "unable to create controller", "controller", "ClusterExtension")

internal/controllers/clusterextension_controller.go

@@ -19,6 +19,7 @@ package controllers
 import (
 	"bytes"
 	"context"
+	"crypto/x509"
 	"errors"
 	"fmt"
 	"io"
@@ -69,7 +70,6 @@ import (
 	catalogfilter "github.com/operator-framework/operator-controller/internal/catalogmetadata/filter"
 	catalogsort "github.com/operator-framework/operator-controller/internal/catalogmetadata/sort"
 	"github.com/operator-framework/operator-controller/internal/conditionsets"
-	"github.com/operator-framework/operator-controller/internal/httputil"
 	"github.com/operator-framework/operator-controller/internal/labels"
 	"github.com/operator-framework/operator-controller/internal/rukpak/bundledeployment"
 	"github.com/operator-framework/operator-controller/internal/rukpak/convert"
@@ -94,7 +94,7 @@ type ClusterExtensionReconciler struct {
 	cache                 cache.Cache
 	InstalledBundleGetter InstalledBundleGetter
 	Finalizers            crfinalizer.Finalizers
-	CaCertDir             string
+	CaCertPool            *x509.CertPool
 	Preflights            []Preflight
 }
 
@@ -273,9 +273,9 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
 	// Generate a BundleDeployment from the ClusterExtension to Unpack.
 	// Note: The BundleDeployment here is not a k8s API, its a simple Go struct which
 	// necessary embedded values.
-	bd := r.generateBundleDeploymentForUnpack(ctx, bundle.Image, ext)
+	bd := r.generateBundleDeploymentForUnpack(bundle.Image, ext)
 	l.V(1).Info("unpacking resolved bundle")
-	unpackResult, err := r.Unpacker.Unpack(ctx, bd)
+	unpackResult, err := r.Unpacker.Unpack(ctx, bd, r.CaCertPool)
 	if err != nil {
 		setStatusUnpackFailed(ext, err.Error())
 		return ctrl.Result{}, err
@@ -577,20 +577,15 @@ func SetDeprecationStatus(ext *ocv1alpha1.ClusterExtension, bundle *catalogmetad
 	}
 }
 
-func (r *ClusterExtensionReconciler) generateBundleDeploymentForUnpack(ctx context.Context, bundlePath string, ce *ocv1alpha1.ClusterExtension) *bundledeployment.BundleDeployment {
-	certData, err := httputil.LoadCerts(r.CaCertDir)
-	if err != nil {
-		log.FromContext(ctx).WithName("operator-controller").WithValues("cluster-extension", ce.GetName()).Error(err, "unable to get TLS certificate")
-	}
+func (r *ClusterExtensionReconciler) generateBundleDeploymentForUnpack(bundlePath string, ce *ocv1alpha1.ClusterExtension) *bundledeployment.BundleDeployment {
 	return &bundledeployment.BundleDeployment{
 		Name: ce.Name,
 		Spec: bundledeployment.BundleDeploymentSpec{
 			InstallNamespace: ce.Spec.InstallNamespace,
 			Source: bundledeployment.BundleSource{
 				Type: bundledeployment.SourceTypeImage,
 				Image: &bundledeployment.ImageSource{
-					Ref:             bundlePath,
-					CertificateData: certData,
+					Ref: bundlePath,
 				},
 			},
 		},

internal/controllers/clusterextension_controller_test.go

@@ -140,7 +140,7 @@ func TestClusterExtensionChannelVersionExists(t *testing.T) {
 	cl, reconciler := newClientAndReconciler(t, nil)
 	mockUnpacker := unpacker.(*MockUnpacker)
 	// Set up the Unpack method to return a result with StateUnpacked
-	mockUnpacker.On("Unpack", mock.Anything, mock.AnythingOfType("*bundledeployment.BundleDeployment")).Return(&source.Result{
+	mockUnpacker.On("Unpack", mock.Anything, mock.AnythingOfType("*bundledeployment.BundleDeployment"), mock.AnythingOfType("*x509.CertPool")).Return(&source.Result{
 		State: source.StatePending,
 	}, nil)
 

internal/controllers/suite_test.go

@@ -18,6 +18,7 @@ package controllers_test
 
 import (
 	"context"
+	"crypto/x509"
 	"log"
 	"os"
 	"path/filepath"
@@ -48,8 +49,8 @@ type MockUnpacker struct {
 }
 
 // Unpack mocks the Unpack method
-func (m *MockUnpacker) Unpack(ctx context.Context, bd *bd.BundleDeployment) (*source.Result, error) {
-	args := m.Called(ctx, bd)
+func (m *MockUnpacker) Unpack(ctx context.Context, bd *bd.BundleDeployment, caCertPool *x509.CertPool) (*source.Result, error) {
+	args := m.Called(ctx, bd, caCertPool)
 	return args.Get(0).(*source.Result), args.Error(1)
 }
 

internal/httputil/httputil.go

@@ -6,48 +6,39 @@ import (
 	"net/http"
 	"os"
 	"path/filepath"
-	"strings"
 	"time"
 )
 
-func LoadCerts(caDir string) (string, error) {
+func NewCertPool(caDir string) (*x509.CertPool, error) {
+	caCertPool, err := x509.SystemCertPool()
+	if err != nil {
+		return nil, err
+	}
 	if caDir == "" {
-		return "", nil
+		return caCertPool, nil
 	}
 
-	certs := []string{}
 	dirEntries, err := os.ReadDir(caDir)
 	if err != nil {
-		return "", err
+		return nil, err
 	}
 	for _, e := range dirEntries {
 		if e.IsDir() {
 			continue
 		}
 		data, err := os.ReadFile(filepath.Join(caDir, e.Name()))
 		if err != nil {
-			return "", err
+			return nil, err
 		}
-		certs = append(certs, string(data))
+		// ignore ok
+		caCertPool.AppendCertsFromPEM(data)
 	}
-	return strings.Join(certs, "\n"), nil
+	return caCertPool, nil
 }
 
-func BuildHTTPClient(caDir string) (*http.Client, error) {
+func BuildHTTPClient(caCertPool *x509.CertPool) (*http.Client, error) {
 	httpClient := &http.Client{Timeout: 10 * time.Second}
 
-	// use the SystemCertPool as a default
-	caCertPool, err := x509.SystemCertPool()
-	if err != nil {
-		return nil, err
-	}
-
-	certs, err := LoadCerts(caDir)
-	if err != nil {
-		return nil, err
-	}
-
-	caCertPool.AppendCertsFromPEM([]byte(certs))
 	tlsConfig := &tls.Config{
 		RootCAs:    caCertPool,
 		MinVersion: tls.VersionTLS12,

internal/rukpak/bundledeployment/bundledeployment.go

@@ -80,6 +80,4 @@ type ImageSource struct {
 	// fetch the specified image reference.
 	// This should not be used in a production environment.
 	InsecureSkipTLSVerify bool
-	// CertificateData contains the PEM data of the certificate that is to be used for the TLS connection
-	CertificateData string
 }

internal/rukpak/source/image_registry.go

@@ -42,7 +42,7 @@ type ImageRegistry struct {
 	AuthNamespace string
 }
 
-func (i *ImageRegistry) Unpack(ctx context.Context, bundle *bd.BundleDeployment) (*Result, error) {
+func (i *ImageRegistry) Unpack(ctx context.Context, bundle *bd.BundleDeployment, caCertPool *x509.CertPool) (*Result, error) {
 	l := log.FromContext(ctx)
 	if bundle.Spec.Source.Type != bd.SourceTypeImage {
 		panic(fmt.Sprintf("programmer error: source type %q is unable to handle specified bundle source type %q", bd.SourceTypeImage, bundle.Spec.Source.Type))
@@ -85,13 +85,8 @@ func (i *ImageRegistry) Unpack(ctx context.Context, bundle *bd.BundleDeployment)
 	if bundle.Spec.Source.Image.InsecureSkipTLSVerify {
 		transport.TLSClientConfig.InsecureSkipVerify = true // nolint:gosec
 	}
-	if bundle.Spec.Source.Image.CertificateData != "" {
-		pool, err := x509.SystemCertPool()
-		if err != nil || pool == nil {
-			pool = x509.NewCertPool()
-		}
-		transport.TLSClientConfig.RootCAs = pool
-		transport.TLSClientConfig.RootCAs.AppendCertsFromPEM([]byte(bundle.Spec.Source.Image.CertificateData))
+	if caCertPool != nil {
+		transport.TLSClientConfig.RootCAs = caCertPool
 	}
 	remoteOpts = append(remoteOpts, remote.WithTransport(transport))
 
@@ -163,7 +158,6 @@ func unpackedResult(fsys fs.FS, bundle *bd.BundleDeployment, ref string) *Result
 				Ref:                   ref,
 				ImagePullSecretName:   bundle.Spec.Source.Image.ImagePullSecretName,
 				InsecureSkipTLSVerify: bundle.Spec.Source.Image.InsecureSkipTLSVerify,
-				CertificateData:       bundle.Spec.Source.Image.CertificateData,
 			},
 		},
 		State: StateUnpacked,

internal/rukpak/source/unpacker.go

@@ -2,6 +2,7 @@ package source
 
 import (
 	"context"
+	"crypto/x509"
 	"fmt"
 	"io/fs"
 
@@ -25,7 +26,7 @@ import (
 // specifications. A source should treat a bundle root directory as an opaque
 // file tree and delegate bundle format concerns to bundle parsers.
 type Unpacker interface {
-	Unpack(context.Context, *bd.BundleDeployment) (*Result, error)
+	Unpack(context.Context, *bd.BundleDeployment, *x509.CertPool) (*Result, error)
 	Cleanup(context.Context, *bd.BundleDeployment) error
 }
 
@@ -79,12 +80,12 @@ func NewUnpacker(sources map[bd.SourceType]Unpacker) Unpacker {
 	return &unpacker{sources: sources}
 }
 
-func (s *unpacker) Unpack(ctx context.Context, bundle *bd.BundleDeployment) (*Result, error) {
+func (s *unpacker) Unpack(ctx context.Context, bundle *bd.BundleDeployment, caCertPool *x509.CertPool) (*Result, error) {
 	source, ok := s.sources[bundle.Spec.Source.Type]
 	if !ok {
 		return nil, fmt.Errorf("source type %q not supported", bundle.Spec.Source.Type)
 	}
-	return source.Unpack(ctx, bundle)
+	return source.Unpack(ctx, bundle, caCertPool)
 }
 
 func (s *unpacker) Cleanup(ctx context.Context, bundle *bd.BundleDeployment) error {