fix(backups): trust only the endpoint CA where the backup writer does

With an endpoint CA the probe trusted the system roots plus that CA.
barman-cloud gets endpointCA as AWS_CA_BUNDLE, which replaces the
default trust store, so a CNPG probe could accept an endpoint the backup
itself would have refused. Let the connection say whether the CA adds to
the system roots or replaces them, and replace them for CNPG.
mariadb-operator appends its CA to the system pool, so MariaDB keeps
that.

Pin in reconcile tests that a Create the apiserver rejects still fails
the BackupJob, so only the read-back of an existing Backup is retried.

Assisted-by: LLM
Signed-off-by: Andrey Kolkov <androndo@gmail.com>

internal/backupcontroller/artifact_size.go

@@ -47,8 +47,11 @@ type s3Connection struct {
 	AccessKey    string
 	SecretKey    string
 	SessionToken string
-	// CAPEM is trusted on top of the system pool when set.
-	CAPEM []byte
+	// CAPEM is an endpoint CA bundle. It is trusted on top of the system
+	// roots, or instead of them when CAOnly is set: the probe must trust
+	// what the writer of the backup trusted, no more.
+	CAPEM  []byte
+	CAOnly bool
 }
 
 func (r *BackupJobReconciler) s3Lister(conn s3Connection) (s3ObjectLister, error) {
@@ -74,16 +77,9 @@ func newMinioObjectLister(conn s3Connection) (s3ObjectLister, error) {
 		if err != nil {
 			return nil, err
 		}
-		pool := tr.TLSClientConfig.RootCAs
-		if pool == nil {
-			if pool, err = x509.SystemCertPool(); err != nil {
-				pool = x509.NewCertPool()
-			}
+		if tr.TLSClientConfig.RootCAs, err = endpointTrustPool(conn, tr.TLSClientConfig.RootCAs); err != nil {
+			return nil, err
 		}
-		if !pool.AppendCertsFromPEM(conn.CAPEM) {
-			return nil, errors.New("no PEM certificates in the endpoint CA bundle")
-		}
-		tr.TLSClientConfig.RootCAs = pool
 		opts.Transport = tr
 	}
 	// minio.New refuses an endpoint with a path prefix, which it cannot
@@ -95,6 +91,26 @@ func newMinioObjectLister(conn s3Connection) (s3ObjectLister, error) {
 	return &minioObjectLister{client: client}, nil
 }
 
+// endpointTrustPool returns the roots to verify the endpoint against: the
+// CA bundle on top of system, or the CA bundle alone with CAOnly. A nil
+// system means the platform's roots.
+func endpointTrustPool(conn s3Connection, system *x509.CertPool) (*x509.CertPool, error) {
+	pool := system
+	switch {
+	case conn.CAOnly:
+		pool = x509.NewCertPool()
+	case pool == nil:
+		var err error
+		if pool, err = x509.SystemCertPool(); err != nil {
+			pool = x509.NewCertPool()
+		}
+	}
+	if !pool.AppendCertsFromPEM(conn.CAPEM) {
+		return nil, errors.New("no PEM certificates in the endpoint CA bundle")
+	}
+	return pool, nil
+}
+
 func (l *minioObjectLister) ListObjects(ctx context.Context, bucket, prefix string, recursive bool) ([]s3ObjectInfo, error) {
 	return collectObjects(ctx, l.client.ListObjects(ctx, bucket, minio.ListObjectsOptions{Prefix: prefix, Recursive: recursive}))
 }

internal/backupcontroller/artifact_size_test.go

@@ -3,8 +3,17 @@ package backupcontroller
 
 import (
 	"context"
+	"crypto/ecdsa"
+	"crypto/elliptic"
+	"crypto/rand"
+	"crypto/x509"
+	"crypto/x509/pkix"
+	"encoding/pem"
 	"errors"
 	"fmt"
+	"math/big"
+	"net/http"
+	"net/http/httptest"
 	"testing"
 	"time"
 
@@ -223,3 +232,56 @@ func TestFailBackupArtifact(t *testing.T) {
 		}
 	})
 }
+
+// TestEndpointTrustPool_CAOnly tells apart "the CA on top of the system
+// roots" and "the CA alone". The system pool holds the endpoint's
+// certificate, and the configured CA is an unrelated one, so the endpoint
+// must stop being trusted only when CAOnly is set.
+func TestEndpointTrustPool_CAOnly(t *testing.T) {
+	endpoint := httptest.NewTLSServer(http.NotFoundHandler())
+	defer endpoint.Close()
+	system := x509.NewCertPool()
+	system.AddCert(endpoint.Certificate())
+
+	// Every httptest server shares one certificate, so the unrelated CA is
+	// minted here.
+	key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+	if err != nil {
+		t.Fatal(err)
+	}
+	tmpl := &x509.Certificate{SerialNumber: big.NewInt(1), Subject: pkix.Name{CommonName: "unrelated CA"}, NotBefore: time.Now(), NotAfter: time.Now().Add(time.Hour), IsCA: true, BasicConstraintsValid: true}
+	der, err := x509.CreateCertificate(rand.Reader, tmpl, tmpl, &key.PublicKey, key)
+	if err != nil {
+		t.Fatal(err)
+	}
+	unrelatedCA := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: der})
+
+	verifies := func(caOnly bool) bool {
+		pool, err := endpointTrustPool(s3Connection{CAPEM: unrelatedCA, CAOnly: caOnly}, system.Clone())
+		if err != nil {
+			t.Fatalf("endpointTrustPool: %v", err)
+		}
+		_, err = endpoint.Certificate().Verify(x509.VerifyOptions{Roots: pool})
+		return err == nil
+	}
+	if !verifies(false) {
+		t.Error("CA on top of the system roots: the endpoint was not trusted")
+	}
+	if verifies(true) {
+		t.Error("CA alone: the endpoint was trusted through the system roots")
+	}
+}
+
+// forbidBackupCreate wraps a fake client whose Create of a Backup is
+// rejected, the way an admission webhook rejects one.
+func forbidBackupCreate(t *testing.T, c client.Client) client.Client {
+	t.Helper()
+	return interceptor.NewClient(c.(client.WithWatch), interceptor.Funcs{
+		Create: func(ctx context.Context, cl client.WithWatch, obj client.Object, opts ...client.CreateOption) error {
+			if _, ok := obj.(*backupsv1alpha1.Backup); ok {
+				return apierrors.NewForbidden(backupsv1alpha1.GroupVersion.WithResource("backups").GroupResource(), obj.GetName(), errors.New("denied by policy"))
+			}
+			return cl.Create(ctx, obj, opts...)
+		},
+	})
+}

internal/backupcontroller/cnpg_artifact_size.go

@@ -105,7 +105,9 @@ func (r *BackupJobReconciler) cnpgS3Connection(ctx context.Context, namespace st
 		if err != nil {
 			return conn, err
 		}
-		conn.CAPEM = []byte(pem)
+		// barman-cloud gets endpointCA as AWS_CA_BUNDLE, which replaces the
+		// default trust store rather than adding to it.
+		conn.CAPEM, conn.CAOnly = []byte(pem), true
 	}
 	return conn, nil
 }

internal/backupcontroller/cnpg_artifact_size_test.go

@@ -126,7 +126,7 @@ func TestCNPGArtifactSizeBytes(t *testing.T) {
 		if got := r.cnpgArtifactSizeBytes(context.Background(), "tenant", store, "srv", "20261021T100000"); got != 5 {
 			t.Errorf("size = %d, want 5", got)
 		}
-		if l.conn.AccessKey != "ID2" || l.conn.SecretKey != "KEY2" || string(l.conn.CAPEM) != "PEM" {
+		if l.conn.AccessKey != "ID2" || l.conn.SecretKey != "KEY2" || string(l.conn.CAPEM) != "PEM" || !l.conn.CAOnly {
 			t.Errorf("connection = %+v", l.conn)
 		}
 	})
@@ -417,3 +417,37 @@ func TestCreateCNPGBackupArtifact_FailedExistenceCheckSkipsProbe(t *testing.T) {
 		t.Errorf("S3 probed %d times after the existence check failed", l.calls)
 	}
 }
+
+// TestReconcileCNPG_RejectedCreateFailsTheJob pins that only the read-back
+// is retried: a Create the apiserver rejects still fails the BackupJob.
+func TestReconcileCNPG_RejectedCreateFailsTheJob(t *testing.T) {
+	f := newCNPGRolloutFixture(t, newRolloutBackupJob(time.Now().Add(-time.Minute)),
+		newRolloutCluster(cnpgClusterHealthyPhase, buildBarmanPlugin(rolloutCluster, rolloutCluster)),
+		newRolloutInstancePod(time.Now().Add(-time.Hour)))
+	_, backups := f.pass()
+	if len(backups) != 1 {
+		t.Fatalf("got %d cnpg.io/Backups after the first pass, want 1", len(backups))
+	}
+	ctx := context.Background()
+	completed := backups[0].DeepCopy()
+	completed.Status.Phase = cnpgBackupPhaseComplete
+	if err := f.c.Update(ctx, completed); err != nil {
+		t.Fatalf("complete cnpg.io/Backup: %v", err)
+	}
+	f.r.Client = forbidBackupCreate(t, f.c)
+
+	j := &backupsv1alpha1.BackupJob{}
+	if err := f.c.Get(ctx, client.ObjectKey{Namespace: rolloutNS, Name: "bj"}, j); err != nil {
+		t.Fatalf("get BackupJob: %v", err)
+	}
+	if _, err := f.r.reconcileCNPG(ctx, j, f.resolved); err != nil {
+		t.Fatalf("reconcileCNPG err = %v, want the BackupJob failed rather than retried", err)
+	}
+	persisted := &backupsv1alpha1.BackupJob{}
+	if err := f.c.Get(ctx, client.ObjectKey{Namespace: rolloutNS, Name: "bj"}, persisted); err != nil {
+		t.Fatalf("get BackupJob: %v", err)
+	}
+	if persisted.Status.Phase != backupsv1alpha1.BackupJobPhaseFailed {
+		t.Errorf("phase = %q, want Failed for a rejected Create", persisted.Status.Phase)
+	}
+}

internal/backupcontroller/mariadb_artifact_size_test.go

@@ -427,3 +427,51 @@ func TestReconcileMariaDB_UnreadableBackupIsRetried(t *testing.T) {
 		t.Error("the BackupJob was failed although its Backup exists")
 	}
 }
+
+// TestReconcileMariaDB_RejectedCreateFailsTheJob pins that only the
+// read-back is retried: a Create the apiserver rejects still fails the
+// BackupJob.
+func TestReconcileMariaDB_RejectedCreateFailsTheJob(t *testing.T) {
+	strategyGroup := strategyv1alpha1.GroupVersion.Group
+	now := metav1.Now()
+	bj := newMariaDBBackupJob("bj-rejected", "tenant")
+	bj.Status.StartedAt = &now
+	bj.Status.Phase = backupsv1alpha1.BackupJobPhaseRunning
+	strategy := &strategyv1alpha1.MariaDB{
+		ObjectMeta: metav1.ObjectMeta{Name: "mdb-strategy"},
+		Spec:       strategyv1alpha1.MariaDBSpec{Template: *newRenderedMariaDBTemplate()},
+	}
+	operatorMariaDB := &mariadbtypes.MariaDB{ObjectMeta: metav1.ObjectMeta{Namespace: "tenant", Name: mariadbNameForApp("mariadb-src")}}
+	operatorBackup := &mariadbtypes.Backup{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: "tenant", Name: "op-bk",
+			Labels: map[string]string{
+				backupsv1alpha1.OwningJobNameLabel:      bj.Name,
+				backupsv1alpha1.OwningJobNamespaceLabel: bj.Namespace,
+			},
+		},
+		Status: mariadbtypes.BackupStatus{Conditions: []metav1.Condition{{
+			Type: mariadbtypes.ConditionTypeComplete, Status: metav1.ConditionTrue, LastTransitionTime: now,
+		}}},
+	}
+	store := newMariaDBStrategyTestClient(t, bj, strategy, newMariaDBApp("mariadb-src", "tenant"), operatorMariaDB, operatorBackup)
+	r := &BackupJobReconciler{
+		Client:   forbidBackupCreate(t, store),
+		Scheme:   store.Scheme(),
+		Recorder: record.NewFakeRecorder(10),
+	}
+	resolved := &ResolvedBackupConfig{StrategyRef: corev1.TypedLocalObjectReference{
+		APIGroup: &strategyGroup, Kind: strategyv1alpha1.MariaDBStrategyKind, Name: strategy.Name,
+	}}
+
+	if _, err := r.reconcileMariaDB(context.Background(), bj, resolved); err != nil {
+		t.Fatalf("reconcileMariaDB err = %v, want the BackupJob failed rather than retried", err)
+	}
+	persisted := &backupsv1alpha1.BackupJob{}
+	if err := store.Get(context.Background(), client.ObjectKeyFromObject(bj), persisted); err != nil {
+		t.Fatalf("get BackupJob: %v", err)
+	}
+	if persisted.Status.Phase != backupsv1alpha1.BackupJobPhaseFailed {
+		t.Errorf("phase = %q, want Failed for a rejected Create", persisted.Status.Phase)
+	}
+}