Fix: Define behavior for containers without RootCA store

Author: Luzifer

cmd/main.go

@@ -20,7 +20,6 @@ package main
 import (
 	"context"
 	"crypto/tls"
-	"crypto/x509"
 	"flag"
 	"fmt"
 	"net/http"
@@ -49,6 +48,7 @@ import (
 
 	infrastructurev1alpha1 "github.com/ionos-cloud/cluster-api-provider-proxmox/api/v1alpha1"
 	"github.com/ionos-cloud/cluster-api-provider-proxmox/internal/controller"
+	"github.com/ionos-cloud/cluster-api-provider-proxmox/internal/tlshelper"
 	"github.com/ionos-cloud/cluster-api-provider-proxmox/internal/webhook"
 	capmox "github.com/ionos-cloud/cluster-api-provider-proxmox/pkg/proxmox"
 	"github.com/ionos-cloud/cluster-api-provider-proxmox/pkg/proxmox/goproxmox"
@@ -195,21 +195,9 @@ func setupProxmoxClient(ctx context.Context, logger logr.Logger) (capmox.Client,
 		return nil, nil
 	}
 
-	rootCerts, err := x509.SystemCertPool()
+	rootCerts, err := tlshelper.SystemRootsWithFile(proxmoxRootCertFile)
 	if err != nil {
-		return nil, fmt.Errorf("loading system cert pool: %w", err)
-	}
-
-	if proxmoxRootCertFile != "" {
-		// There is a custom root cert, we need to load it
-		pemBlock, err := os.ReadFile(proxmoxRootCertFile) //#nosec:G304 // File provided by system operator
-		if err != nil {
-			return nil, fmt.Errorf("reading proxmox-root-cert-file: %w", err)
-		}
-
-		if !rootCerts.AppendCertsFromPEM(pemBlock) {
-			return nil, fmt.Errorf("failure adding cert to root cert list")
-		}
+		return nil, fmt.Errorf("loading cert pool: %w", err)
 	}
 
 	tr := &http.Transport{

internal/tlshelper/roots.go

@@ -0,0 +1,54 @@
+// Package tlshelper wraps loading and modifying the root-CA store for
+// use in tls.Config
+package tlshelper
+
+import (
+	"crypto/x509"
+	"fmt"
+	"os"
+)
+
+// SystemRootsWithFile reads the pemBlock for SystemRootsWithCert from
+// the given file and then calls SystemRootsWithCert.
+func SystemRootsWithFile(filepath string) (*x509.CertPool, error) {
+	if len(filepath) == 0 {
+		// No filename given, we fall back to default behavior: returning
+		// nil and letting Go itself take care of everything
+		return nil, nil
+	}
+
+	pemBlock, err := os.ReadFile(filepath) //#nosec:G304 // Intended to read the given file
+	if err != nil {
+		return nil, fmt.Errorf("loading certificate file: %w", err)
+	}
+
+	return SystemRootsWithCert(pemBlock)
+}
+
+// SystemRootsWithCert tries to load the system root store and to
+// append the given certificate from a PEM encoded block into it. In
+// case there is no pool an empty pool is used to add the certificate.
+func SystemRootsWithCert(pemBlock []byte) (*x509.CertPool, error) {
+	if len(pemBlock) == 0 {
+		// Zero length / nil, we fall back to default behavior: returning
+		// nil and letting Go itself take care of everything
+		return nil, nil
+	}
+
+	rootCerts, err := x509.SystemCertPool()
+	if err != nil && !os.IsNotExist(err) {
+		return nil, fmt.Errorf("loading system cert pool: %w", err)
+	}
+
+	if os.IsNotExist(err) {
+		// The system pool is not available but that's fine as we are
+		// supposed to add one own cert
+		rootCerts = x509.NewCertPool()
+	}
+
+	if !rootCerts.AppendCertsFromPEM(pemBlock) {
+		return nil, fmt.Errorf("certificate was not appended")
+	}
+
+	return rootCerts, nil
+}

internal/tlshelper/roots_test.go

@@ -0,0 +1,49 @@
+package tlshelper
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// This block contains a 100Y valid self-signed ECDSA CA with key no
+// longer existent.
+const exampleRootCA = `-----BEGIN CERTIFICATE-----
+MIIB6DCCAY+gAwIBAgIUIgQxz/rn8WLH3zyHICHh8Us552AwCgYIKoZIzj0EAwIw
+STELMAkGA1UEBhMCREUxEDAOBgNVBAgMB0hhbWJ1cmcxEzARBgNVBAoMCkV4YW1w
+bGUgQ0ExEzARBgNVBAMMCkV4YW1wbGUgQ0EwIBcNMjQxMTI4MTExMzM3WhgPMjEy
+NDExMDQxMTEzMzdaMEkxCzAJBgNVBAYTAkRFMRAwDgYDVQQIDAdIYW1idXJnMRMw
+EQYDVQQKDApFeGFtcGxlIENBMRMwEQYDVQQDDApFeGFtcGxlIENBMFkwEwYHKoZI
+zj0CAQYIKoZIzj0DAQcDQgAEvQIYmiX4z2sv8sVqyC/eQ7e2JH1WQgIRuSWYVEOL
+YKEQDHwMg1KKu3+McgHXLiZ0af0JDyd00em/g7k39RzNhqNTMFEwHQYDVR0OBBYE
+FPDaKA2+m5nRhWESuw+61HdvmZiGMB8GA1UdIwQYMBaAFPDaKA2+m5nRhWESuw+6
+1HdvmZiGMA8GA1UdEwEB/wQFMAMBAf8wCgYIKoZIzj0EAwIDRwAwRAIgQtj0ZhXK
+RYrWwHMzIHXqggsU3NhnOUa/yeeQOExDVbMCIAFDdaz3v5jOmWm7LR5BwQwLRWhO
+37ky2VQr/1GhU42q
+-----END CERTIFICATE-----`
+
+func TestRootPoolLoading(t *testing.T) {
+	// Empty file
+	roots, err := SystemRootsWithFile("")
+	require.NoError(t, err)
+	assert.Nil(t, roots)
+
+	// Empty pemBlock
+	roots, err = SystemRootsWithCert(nil)
+	require.NoError(t, err)
+	assert.Nil(t, roots)
+
+	// CA from system plus PEM
+	roots, err = SystemRootsWithCert([]byte(exampleRootCA))
+	require.NoError(t, err)
+	assert.NotNil(t, roots)
+
+	// Error from broken file
+	_, err = SystemRootsWithFile("/tmp/this/file/will/never/exist.notexist")
+	require.Error(t, err)
+
+	// Error from broken PEM
+	_, err = SystemRootsWithCert([]byte("I'm certainly not a PEM block"))
+	require.Error(t, err)
+}

pkg/scope/cluster.go

@@ -20,7 +20,6 @@ package scope
 import (
 	"context"
 	"crypto/tls"
-	"crypto/x509"
 	"fmt"
 	"net/http"
 
@@ -38,6 +37,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/log"
 
 	infrav1alpha1 "github.com/ionos-cloud/cluster-api-provider-proxmox/api/v1alpha1"
+	"github.com/ionos-cloud/cluster-api-provider-proxmox/internal/tlshelper"
 	"github.com/ionos-cloud/cluster-api-provider-proxmox/pkg/kubernetes/ipam"
 	capmox "github.com/ionos-cloud/cluster-api-provider-proxmox/pkg/proxmox"
 	"github.com/ionos-cloud/cluster-api-provider-proxmox/pkg/proxmox/goproxmox"
@@ -156,15 +156,9 @@ func (s *ClusterScope) setupProxmoxClient(ctx context.Context) (capmox.Client, e
 	tlsInsecure := string(secret.Data["insecure"]) != "false"
 	tlsRootCA := secret.Data["root_ca"]
 
-	rootCerts, err := x509.SystemCertPool()
+	rootCerts, err := tlshelper.SystemRootsWithCert(tlsRootCA)
 	if err != nil {
-		return nil, fmt.Errorf("loading system cert pool: %w", err)
-	}
-
-	if len(tlsRootCA) > 0 {
-		if !rootCerts.AppendCertsFromPEM(tlsRootCA) {
-			return nil, fmt.Errorf("failure adding cert to root cert list")
-		}
+		return nil, fmt.Errorf("loading cert pool: %w", err)
 	}
 
 	tr := &http.Transport{