Add optional TLS certificate checking when talking to Proxmox (#336)

* Add optional TLS certificate checking when talking to Proxmox

* Fix: Define behavior for containers without RootCA store

* Fix: Allow all true-ish YAML values

Author: Luzifer

cmd/main.go

@@ -48,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"
@@ -69,6 +70,9 @@ var (
 	ProxmoxTokenID string
 	// ProxmoxSecret env variable that defines the Proxmox secret for the given token id.
 	ProxmoxSecret string
+
+	proxmoxInsecure     bool
+	proxmoxRootCertFile string
 )
 
 func init() {
@@ -190,9 +194,17 @@ func setupProxmoxClient(ctx context.Context, logger logr.Logger) (capmox.Client,
 	if ProxmoxURL == "" || ProxmoxTokenID == "" || ProxmoxSecret == "" {
 		return nil, nil
 	}
-	// TODO, check if we need to delete tls config
+
+	rootCerts, err := tlshelper.SystemRootsWithFile(proxmoxRootCertFile)
+	if err != nil {
+		return nil, fmt.Errorf("loading cert pool: %w", err)
+	}
+
 	tr := &http.Transport{
-		TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, //nolint:gosec
+		TLSClientConfig: &tls.Config{
+			InsecureSkipVerify: proxmoxInsecure, //#nosec:G402 // Default retained, user can enable cert checking
+			RootCAs:            rootCerts,
+		},
 	}
 
 	httpClient := &http.Client{Transport: tr}
@@ -209,6 +221,12 @@ func initFlagsAndEnv(fs *pflag.FlagSet) {
 	ProxmoxTokenID = env.GetString("PROXMOX_TOKEN", "")
 	ProxmoxSecret = env.GetString("PROXMOX_SECRET", "")
 
+	fs.BoolVar(&proxmoxInsecure, "proxmox-insecure",
+		env.GetString("PROXMOX_INSECURE", "true") == "true",
+		"Skip TLS verification when connecting to Proxmox")
+	fs.StringVar(&proxmoxRootCertFile, "proxmox-root-cert-file", "",
+		"Root-Certificate to use to verify server TLS certificate")
+
 	fs.StringVar(&metricsAddr, "metrics-bind-address", ":8080", "The address the metric endpoint binds to.")
 	fs.StringVar(&probeAddr, "health-probe-bind-address", ":8081", "The address the probe endpoint binds to.")
 	fs.BoolVar(&enableLeaderElection, "leader-elect", false,

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,10 @@ package scope
 import (
 	"context"
 	"crypto/tls"
+	"fmt"
 	"net/http"
+	"slices"
+	"strings"
 
 	"github.com/go-logr/logr"
 	"github.com/luthermonson/go-proxmox"
@@ -36,6 +39,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"
@@ -151,9 +155,24 @@ func (s *ClusterScope) setupProxmoxClient(ctx context.Context) (capmox.Client, e
 	tokenSecret := string(secret.Data["secret"])
 	url := string(secret.Data["url"])
 
-	// TODO, check if we need to delete tls config
+	tlsInsecure, tlsInsecureSet := secret.Data["insecure"]
+	tlsRootCA := secret.Data["root_ca"]
+
+	rootCerts, err := tlshelper.SystemRootsWithCert(tlsRootCA)
+	if err != nil {
+		return nil, fmt.Errorf("loading cert pool: %w", err)
+	}
+
 	tr := &http.Transport{
-		TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, //nolint:gosec
+		TLSClientConfig: &tls.Config{
+			// When "insecure" is unset we retain the pre-v0.7 behavior of
+			// setting the connection insecure. If it is set we compare
+			// against YAML true-ish values.
+			//
+			//#nosec:G402 // Intended to enable insecure mode for unknown CAs
+			InsecureSkipVerify: !tlsInsecureSet || slices.Contains([]string{"1", "on", "true", "yes", "y"}, strings.ToLower(string(tlsInsecure))),
+			RootCAs:            rootCerts,
+		},
 	}
 
 	httpClient := &http.Client{Transport: tr}