Use x509.CertPools instead of PEM strings (#1052)

Signed-off-by: Todd Short <[email protected]>

Author: tmshort

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")
 	}
@@ -191,6 +197,7 @@ func main() {
 		BaseCachePath: filepath.Join(cachePath, "unpack"),
 		// TODO: This needs to be derived per extension via ext.Spec.InstallNamespace
 		AuthNamespace: systemNamespace,
+		CaCertPool:    certPool,
 	}
 
 	domain := ocv1alpha1.GroupVersion.Group
@@ -220,7 +227,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,7 +273,7 @@ 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)
 	if err != nil {
@@ -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/httputil/httputil.go

@@ -3,51 +3,79 @@ package httputil
 import (
 	"crypto/tls"
 	"crypto/x509"
+	"encoding/pem"
+	"fmt"
 	"net/http"
 	"os"
 	"path/filepath"
-	"strings"
 	"time"
 )
 
-func LoadCerts(caDir string) (string, error) {
+// This version of (*x509.CertPool).AppendCertsFromPEM() will error out if parsing fails
+func appendCertsFromPEM(s *x509.CertPool, pemCerts []byte) error {
+	n := 1
+	for len(pemCerts) > 0 {
+		var block *pem.Block
+		block, pemCerts = pem.Decode(pemCerts)
+		if block == nil {
+			return fmt.Errorf("unable to PEM decode cert %d", n)
+		}
+		// ignore non-certificates (e.g. keys)
+		if block.Type != "CERTIFICATE" {
+			continue
+		}
+		if len(block.Headers) != 0 {
+			// This is a cert, but we're ignoring it, so bump the counter
+			n++
+			continue
+		}
+
+		cert, err := x509.ParseCertificate(block.Bytes)
+		if err != nil {
+			return fmt.Errorf("unable to parse cert %d: %w", n, err)
+		}
+		// no return values - panics or always succeeds
+		s.AddCert(cert)
+		n++
+	}
+
+	return nil
+}
+
+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()))
+		file := filepath.Join(caDir, 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)
 		if err != nil {
-			return "", err
+			return nil, fmt.Errorf("error adding cert file %q: %w", file, err)
 		}
-		certs = append(certs, string(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

@@ -40,6 +40,7 @@ func NewUnrecoverable(err error) *Unrecoverable {
 type ImageRegistry struct {
 	BaseCachePath string
 	AuthNamespace string
+	CaCertPool    *x509.CertPool
 }
 
 func (i *ImageRegistry) Unpack(ctx context.Context, bundle *bd.BundleDeployment) (*Result, error) {
@@ -85,13 +86,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 i.CaCertPool != nil {
+		transport.TLSClientConfig.RootCAs = i.CaCertPool
 	}
 	remoteOpts = append(remoteOpts, remote.WithTransport(transport))
 
@@ -163,7 +159,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,