From 9ac06cd1fc204bbf502acdbdaaebc073fe4bca77 Mon Sep 17 00:00:00 2001 From: Aleksei Sviridkin Date: Tue, 21 Jul 2026 13:51:25 +0300 Subject: [PATCH] fix(controller): withdraw the CA projection and refuse contested releases MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two gaps in the sentinel-to-canonical-name lifecycle: Withdrawal — the reconciler published each declared entry but never deleted one. A sentinel that keeps existing with its CACert entry removed (a chart gating it on tls.enabled, say) left the old .tenant-ca served forever, because owner-reference GC only fires when the whole sentinel is pruned. A sentinel that reconciles with zero CACert entries now deletes the projection at the canonical name, but only when it still carries that sentinel's owner reference, so a foreign Secret or another sentinel's projection is never touched. Contention — two sentinels in one namespace with the same release label both resolve to the single canonical name. Whichever reconciled first would publish an arbitrary CA and the two could flap ownership of one projection. Refuse both, mirroring the more-than-one-CACert-entry refusal, so nothing is published until the declaration lives on exactly one sentinel. The ClusterRole comments claimed the reconciler never deletes a Secret; corrected to describe the owner-scoped withdrawal delete. Signed-off-by: Aleksei Sviridkin --- internal/controller/cacert/reconciler.go | 85 +++++++++++++++++++ internal/controller/cacert/reconciler_test.go | 67 +++++++++++++++ .../cozystack-controller/templates/rbac.yaml | 15 ++-- 3 files changed, 161 insertions(+), 6 deletions(-) diff --git a/internal/controller/cacert/reconciler.go b/internal/controller/cacert/reconciler.go index b9e59826e..33b0b8fdb 100644 --- a/internal/controller/cacert/reconciler.go +++ b/internal/controller/cacert/reconciler.go @@ -318,6 +318,7 @@ const ( reasonNoRelease = "NoRelease" reasonUnsupportedProjection = "UnsupportedProjectionType" reasonMultipleCACert = "MultipleCACertProjections" + reasonReleaseContested = "ReleaseContested" ) // privateKeyHeader matches a PEM private-key header of any flavour (PKCS#1, @@ -454,6 +455,38 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu return ctrl.Result{}, nil } + if caCertEntries == 0 { + // No CACert entry declares the trust anchor any more — a chart that gated its + // CACert entry (say on tls.enabled) while still rendering the sentinel lands + // here. Withdraw a projection a previous declaration published: garbage + // collection only fires when the whole sentinel is pruned, so a sentinel that + // keeps existing with its CACert entry removed would otherwise serve a stale + // anchor forever. Only a projection this sentinel owns is deleted; a foreign + // Secret, or another sentinel's projection at the name, is left untouched. + if err := r.withdrawProjection(ctx, tp, target); err != nil { + return ctrl.Result{}, err + } + } else { + // Exactly one CACert entry. Refuse if another sentinel in this namespace + // declares one for the same release: both resolve to the single canonical + // name, so publishing from either would silently pick an arbitrary winner and + // let the two flap ownership of one projection, while deleting either would + // garbage-collect an anchor the other still declares. Refuse both — neither + // writes — until the declaration lives on exactly one sentinel. This mirrors + // the more-than-one-CACert-entry refusal above, one level up. + other, err := r.anotherSentinelForRelease(ctx, tp, release) + if err != nil { + return ctrl.Result{}, err + } + if other != "" { + if err := r.setReady(ctx, tp, notReady(reasonReleaseContested, + fmt.Sprintf("another TenantProjection %q in this namespace declares a CACert projection for release %q; both resolve to the canonical name %q, so neither is published — declare the trust anchor from exactly one sentinel", other, release, target))); err != nil { + return ctrl.Result{}, err + } + return ctrl.Result{}, nil + } + } + // The sentinel's Ready condition aggregates its projections: True only when // every declared projection is published, otherwise the first failure. ready := metav1.Condition{ @@ -554,6 +587,58 @@ func (r *Reconciler) reconcileCACert(ctx context.Context, tp *internalv1alpha1.T }, nil } +// withdrawProjection deletes the canonical trust-anchor Secret when the sentinel +// no longer declares one, but ONLY when it still carries this sentinel's owner +// reference. Deleting the whole sentinel is handled by garbage collection through +// that reference; this covers the narrower case of a sentinel that keeps existing +// with its CACert declaration removed, which GC never sees. A foreign Secret at the +// name, or a projection another sentinel owns, is never deleted — the owner- +// reference check is what keeps the withdrawal from destroying data that is not +// this sentinel's to withdraw. +func (r *Reconciler) withdrawProjection(ctx context.Context, tp *internalv1alpha1.TenantProjection, target string) error { + existing := &corev1.Secret{} + if err := r.Reader.Get(ctx, types.NamespacedName{Namespace: tp.Namespace, Name: target}, existing); err != nil { + if apierrors.IsNotFound(err) { + return nil + } + return fmt.Errorf("get CA projection %s/%s: %w", tp.Namespace, target, err) + } + if !hasOwner(existing.OwnerReferences, sentinelOwnerRef(tp)) { + return nil + } + if err := r.Delete(ctx, existing); err != nil && !apierrors.IsNotFound(err) { + return fmt.Errorf("withdraw CA projection %s/%s: %w", tp.Namespace, target, err) + } + return nil +} + +// anotherSentinelForRelease returns the name of a DIFFERENT sentinel in the same +// namespace that carries the same release label and also declares a CACert +// projection, or "" when this sentinel is the only one for the release. Two such +// sentinels resolve to the same canonical projection name, so the reconcile refuses +// to publish from either; see the caller. +func (r *Reconciler) anotherSentinelForRelease(ctx context.Context, tp *internalv1alpha1.TenantProjection, release string) (string, error) { + list := &internalv1alpha1.TenantProjectionList{} + if err := r.List(ctx, list, client.InNamespace(tp.Namespace)); err != nil { + return "", fmt.Errorf("list TenantProjections in %s: %w", tp.Namespace, err) + } + for i := range list.Items { + other := &list.Items[i] + if other.Name == tp.Name || other.DeletionTimestamp != nil { + continue + } + if other.Labels[helmNameLabel] != release { + continue + } + for j := range other.Spec.Projections { + if other.Spec.Projections[j].Type == internalv1alpha1.ProjectionTypeCACert { + return other.Name, nil + } + } + } + return "", nil +} + // reconcileSourceAtCanonicalName handles the degenerate case where the source // itself sits at the canonical projection name — a Secret cannot be projected // over itself. diff --git a/internal/controller/cacert/reconciler_test.go b/internal/controller/cacert/reconciler_test.go index ee237cb86..6dfd740bf 100644 --- a/internal/controller/cacert/reconciler_test.go +++ b/internal/controller/cacert/reconciler_test.go @@ -505,6 +505,73 @@ func TestReconcile_MultipleCACertEntries_Rejected(t *testing.T) { assertReady(t, c, metav1.ConditionFalse, reasonMultipleCACert) } +// TestReconcile_EntryRemoved_WithdrawsProjection exercises the withdrawal path: a +// sentinel that stops declaring a CACert projection must have the anchor it +// published removed. Garbage collection only fires when the whole sentinel is +// pruned, so without an explicit delete a sentinel that keeps existing with its +// CACert entry gated off (e.g. on tls.enabled) would serve a stale trust anchor +// forever. +func TestReconcile_EntryRemoved_WithdrawsProjection(t *testing.T) { + c := newClient(t, helmRelease(), appDef(), sentinel(), + operatorSecret(testOperatorCA, map[string][]byte{caCertKey: []byte(testCA), caKeyKey: []byte(testKey)}), + ) + mustReconcile(t, c) + if _, ok := getSecret(t, c, testProjection); !ok { + t.Fatalf("the projection must exist while the CACert entry is declared") + } + + // Drop the CACert entry through the internal type. The CRD's MinItems=1 forbids + // an empty projections list at the API server, but a chart that gates the entry + // — or a future non-CACert entry replacing it — leaves the sentinel with zero + // CACert projections while the old anchor still stands. + tp := &internalv1alpha1.TenantProjection{} + if err := c.Get(context.TODO(), types.NamespacedName{Namespace: testNamespace, Name: testSentinel}, tp); err != nil { + t.Fatalf("get sentinel: %v", err) + } + tp.Spec.Projections = nil + if err := c.Update(context.TODO(), tp); err != nil { + t.Fatalf("drop the CACert entry: %v", err) + } + mustReconcile(t, c) + + if _, ok := getSecret(t, c, testProjection); ok { + t.Errorf("withdrawing the last CACert entry must delete the projection it published") + } +} + +// TestReconcile_TwoSentinelsForOneRelease_Refused pins the two-sentinel guard: two +// sentinels in one namespace carrying the same release label both resolve to the +// single canonical name. Without the guard, whichever reconciles first publishes an +// arbitrary CA (a silent winner) and the two flap ownership of one projection. Both +// must refuse instead, so nothing is published until the declaration lives on +// exactly one sentinel. +func TestReconcile_TwoSentinelsForOneRelease_Refused(t *testing.T) { + second := &internalv1alpha1.TenantProjection{ + ObjectMeta: metav1.ObjectMeta{ + Name: testSentinel + "-dup", + Namespace: testNamespace, + UID: types.UID("77777777-8888-9999-aaaa-bbbbbbbbbbbb"), + Labels: map[string]string{helmNameLabel: testRelease}, + }, + Spec: internalv1alpha1.TenantProjectionSpec{ + Projections: []internalv1alpha1.TenantProjectionEntry{{ + Type: internalv1alpha1.ProjectionTypeCACert, + SourceSecretName: testOperatorCA, + SourceKey: caCertKey, + }}, + }, + } + c := newClient(t, helmRelease(), appDef(), sentinel(), second, + operatorSecret(testOperatorCA, map[string][]byte{caCertKey: []byte(testCA), caKeyKey: []byte(testKey)}), + ) + mustReconcile(t, c) + + if _, ok := getSecret(t, c, testProjection); ok { + t.Errorf("two sentinels contesting one release must publish nothing, not an arbitrary winner") + } + assertReady(t, c, metav1.ConditionFalse, reasonReleaseContested) +} + // --- Reconcile: the write-path guards, exercised through a real source --- // TestReconcile_RefusesPrivateKeyUnderLiftedKey pins the reason the controller diff --git a/packages/system/cozystack-controller/templates/rbac.yaml b/packages/system/cozystack-controller/templates/rbac.yaml index adee3d4f2..d8d176930 100644 --- a/packages/system/cozystack-controller/templates/rbac.yaml +++ b/packages/system/cozystack-controller/templates/rbac.yaml @@ -51,10 +51,11 @@ rules: # terminates TLS; tenant ingress controllers / Gateways still read only the # replica in their own namespace. # - CACertReconciler projects an application's CA certificate into the key-free -# ".tenant-ca" Secret of the same namespace. It only ever creates or -# updates that projection; the projection is garbage-collected through its -# owner reference to the TenantProjection sentinel when the application (and so -# the sentinel) is deleted, so this reconciler itself never deletes a Secret. +# ".tenant-ca" Secret of the same namespace. It creates and updates +# that projection, and deletes it to WITHDRAW a retired declaration — but only +# a projection it owns by owner reference, never a foreign Secret. Deleting the +# whole sentinel is handled instead by garbage collection, through the +# projection's owner reference to the TenantProjection. # # The read verbs (get/list/watch) are listed explicitly so the rule fully # describes both reconcilers' Secret access on its own and keeps working if the @@ -82,8 +83,10 @@ rules: # compromise of every Secret in the cluster — including the CA private keys the # CA-extraction controller exists to keep away from tenants. The controller's own # code is what confines it: the CA reconciler lifts exactly one whitelisted key, -# re-guards the bytes on every write, and never deletes a projection at all — -# garbage collection removes it through the sentinel owner reference. That is a +# re-guards the bytes on every write, and deletes only a projection it owns by +# owner reference — to withdraw a retired declaration — never a foreign Secret; +# garbage collection removes the projection through the sentinel owner reference +# when the sentinel itself goes. That is a # code-level confinement of an RBAC-level power, which is # strictly weaker than not holding the power — the honest reading is that the # guard is the boundary, and the RBAC is not.