From 92fa97969016ca92a4b123bcd4c11c710f6cfd32 Mon Sep 17 00:00:00 2001 From: Andrey Kolkov Date: Thu, 22 Oct 2026 20:08:29 +0400 Subject: [PATCH] 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 --- internal/backupcontroller/artifact_size.go | 38 ++++++++---- .../backupcontroller/artifact_size_test.go | 62 +++++++++++++++++++ .../backupcontroller/cnpg_artifact_size.go | 4 +- .../cnpg_artifact_size_test.go | 36 ++++++++++- .../mariadb_artifact_size_test.go | 48 ++++++++++++++ 5 files changed, 175 insertions(+), 13 deletions(-) diff --git a/internal/backupcontroller/artifact_size.go b/internal/backupcontroller/artifact_size.go index 534019184..1a822699c 100644 --- a/internal/backupcontroller/artifact_size.go +++ b/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})) } diff --git a/internal/backupcontroller/artifact_size_test.go b/internal/backupcontroller/artifact_size_test.go index 523d56469..3d9c35b23 100644 --- a/internal/backupcontroller/artifact_size_test.go +++ b/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...) + }, + }) +} diff --git a/internal/backupcontroller/cnpg_artifact_size.go b/internal/backupcontroller/cnpg_artifact_size.go index c7d91bdb2..0756e2bb9 100644 --- a/internal/backupcontroller/cnpg_artifact_size.go +++ b/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 } diff --git a/internal/backupcontroller/cnpg_artifact_size_test.go b/internal/backupcontroller/cnpg_artifact_size_test.go index 7b3852317..85e55e8c2 100644 --- a/internal/backupcontroller/cnpg_artifact_size_test.go +++ b/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) + } +} diff --git a/internal/backupcontroller/mariadb_artifact_size_test.go b/internal/backupcontroller/mariadb_artifact_size_test.go index b49bcae66..084abe31c 100644 --- a/internal/backupcontroller/mariadb_artifact_size_test.go +++ b/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) + } +}