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) + } +}