fix(controller): guard mirror deletion + enforce secret blacklist

C1: deleteAllMirrors used to issue a blind Delete on every namespace matching the source name+GVK, which would destroy unrelated resources (e.g. a 'default' SA, 'ca-bundle' ConfigMap) sharing the source name. Now reads each candidate, verifies managed-by label and source-reference annotation, and only deletes confirmed mirrors. M1: BlacklistedSecretTypes was declared but never enforced. Enabling mirroring on a service-account-token / bootstrap-token / helm release Secret would mirror credentials cluster-wide. Now refused at Reconcile. M3: deleteAllMirrors swallowed per-namespace errors and returned nil, so callers removed the finalizer even on partial failure (orphans). Errors are now joined and returned.
2026-06-05 22:43:51 +00:00 · 2026-05-02 22:35:40 +01:00
parent b555d84d32
commit 4277c8ac39
2 changed files with 292 additions and 17 deletions
@@ -3,6 +3,7 @@ package controller

 import (
 	"context"
+	stderrors "errors"
 	"fmt"
 	"slices"
 	"time"
@@ -197,6 +198,22 @@ func (r *SourceReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 		return ctrl.Result{}, nil
 	}

+	// Refuse to mirror sensitive Secret types (service-account tokens, bootstrap
+	// tokens, helm release blobs). Mirroring these to other namespaces is a
+	// credential exposure path. If a finalizer is already set (the resource was
+	// enabled before we started enforcing this list), tear down via handleDisabled
+	// so any prior mirrors are cleaned up.
+	if isBlacklistedSecret(sourceObj) {
+		secretType, _, _ := unstructured.NestedString(sourceObj.Object, "type")
+		logger.Info("refusing to mirror blacklisted Secret type",
+			"type", secretType,
+			"reason", "credential exposure risk")
+		if slices.Contains(sourceObj.GetFinalizers(), constants.FinalizerName) {
+			return r.handleDisabled(ctx, sourceObj)
+		}
+		return ctrl.Result{}, nil
+	}
+
 	// Add finalizer if not present
 	if !slices.Contains(sourceObj.GetFinalizers(), constants.FinalizerName) {
 		logger.Info("adding finalizer to source resource")
@@ -401,44 +418,65 @@ func (r *SourceReconciler) reconcileMirror(ctx context.Context, source runtime.O
 	return nil
 }

-// deleteAllMirrors deletes all mirrors for a source resource.
+// deleteAllMirrors deletes all mirrors that this source owns across the cluster.
+// It verifies ownership (managed-by label + source-reference annotation) before
+// deleting anything to avoid destroying unrelated resources that happen to share
+// the source's name. Per-namespace failures are aggregated so callers can defer
+// finalizer removal until cleanup actually succeeds.
 func (r *SourceReconciler) deleteAllMirrors(ctx context.Context, sourceObj metav1.Object) error {
 	logger := log.FromContext(ctx)

-	// List all namespaces
 	allNamespaces, err := r.NamespaceLister.ListNamespaces(ctx)
 	if err != nil {
 		return fmt.Errorf("failed to list namespaces: %w", err)
 	}

-	// Get GVK from source object
 	sourceUnstructured, ok := sourceObj.(*unstructured.Unstructured)
 	if !ok {
 		return fmt.Errorf("source object is not unstructured")
 	}

-	var deleteCount int
+	var (
+		deleteCount int
+		deleteErrs  []error
+	)
 	for _, ns := range allNamespaces {
-		// Skip source namespace
 		if ns == sourceObj.GetNamespace() {
 			continue
 		}

-		// Create mirror reference for deletion
-		mirror := &unstructured.Unstructured{}
-		mirror.SetGroupVersionKind(sourceUnstructured.GroupVersionKind())
-		mirror.SetNamespace(ns)
-		mirror.SetName(sourceObj.GetName())
-
-		err := r.Delete(ctx, mirror)
-		if err == nil {
-			deleteCount++
-		} else if !errors.IsNotFound(err) {
-			logger.Error(err, "failed to delete mirror", "namespace", ns)
+		existing := &unstructured.Unstructured{}
+		existing.SetGroupVersionKind(sourceUnstructured.GroupVersionKind())
+		getErr := r.Get(ctx, client.ObjectKey{Namespace: ns, Name: sourceObj.GetName()}, existing)
+		if errors.IsNotFound(getErr) {
+			continue
 		}
+		if getErr != nil {
+			logger.Error(getErr, "failed to fetch potential mirror", "namespace", ns)
+			deleteErrs = append(deleteErrs, fmt.Errorf("get mirror %s/%s: %w", ns, sourceObj.GetName(), getErr))
+			continue
+		}
+
+		if !IsManagedByUs(existing) {
+			continue
+		}
+		srcNs, srcName, _, found := GetSourceReference(existing)
+		if !found || srcNs != sourceObj.GetNamespace() || srcName != sourceObj.GetName() {
+			continue
+		}
+
+		if delErr := r.Delete(ctx, existing); delErr != nil && !errors.IsNotFound(delErr) {
+			logger.Error(delErr, "failed to delete mirror", "namespace", ns)
+			deleteErrs = append(deleteErrs, fmt.Errorf("delete mirror %s/%s: %w", ns, sourceObj.GetName(), delErr))
+			continue
+		}
+		deleteCount++
 	}

-	logger.Info("deleted mirrors", "count", deleteCount)
+	logger.Info("deleted mirrors", "count", deleteCount, "errors", len(deleteErrs))
+	if len(deleteErrs) > 0 {
+		return stderrors.Join(deleteErrs...)
+	}
 	return nil
 }

@@ -603,6 +641,19 @@ func (r *SourceReconciler) updateLastSyncStatus(ctx context.Context, source runt
 	return r.Update(ctx, source.(*unstructured.Unstructured))
 }

+// isBlacklistedSecret reports whether the given source is a core/v1 Secret with
+// a Type that must never be mirrored across namespaces.
+func isBlacklistedSecret(obj *unstructured.Unstructured) bool {
+	if obj.GetKind() != "Secret" || obj.GetAPIVersion() != "v1" {
+		return false
+	}
+	secretType, found, err := unstructured.NestedString(obj.Object, "type")
+	if err != nil || !found {
+		return false
+	}
+	return slices.Contains(constants.BlacklistedSecretTypes, secretType)
+}
+
 // isEnabledForMirroring checks if a resource has both the label and annotation for mirroring.
 func isEnabledForMirroring(obj metav1.Object) bool {
 	// Check label
@@ -879,3 +879,227 @@ func TestSourceReconciler_Reconcile_AnnotationChange_PatternChange(t *testing.T)
 	mockClient.AssertExpectations(t)
 	mockLister.AssertExpectations(t)
 }
+func TestSourceReconciler_deleteAllMirrors_skipsUnmanagedResources(t *testing.T) {
+	// Regression test: deleteAllMirrors must NOT delete a resource it does not own,
+	// even if the name and GVK happen to match the source.
+	sourceObj := &unstructured.Unstructured{
+		Object: map[string]interface{}{
+			"apiVersion": "v1",
+			"kind":       "Secret",
+			"metadata": map[string]interface{}{
+				"name":      "shared-name",
+				"namespace": "default",
+				"uid":       "source-uid",
+			},
+		},
+	}
+
+	mockClient := new(MockClient)
+	mockLister := new(MockNamespaceLister)
+	mockLister.On("ListNamespaces", mock.Anything).Return([]string{"default", "ns-other"}, nil)
+
+	// In ns-other a resource with the same name/GVK exists but is NOT managed
+	// by kubemirror — pretend it's a regular Secret created by another operator.
+	otherSecret := &unstructured.Unstructured{
+		Object: map[string]interface{}{
+			"apiVersion": "v1",
+			"kind":       "Secret",
+			"metadata": map[string]interface{}{
+				"name":      "shared-name",
+				"namespace": "ns-other",
+			},
+		},
+	}
+	mockClient.On("Get", mock.Anything,
+		types.NamespacedName{Namespace: "ns-other", Name: "shared-name"},
+		mock.AnythingOfType("*unstructured.Unstructured")).
+		Return(nil, otherSecret)
+
+	r := &SourceReconciler{Client: mockClient, NamespaceLister: mockLister}
+
+	err := r.deleteAllMirrors(context.Background(), sourceObj)
+	require.NoError(t, err)
+
+	// The critical assertion: Delete was NEVER called.
+	mockClient.AssertNotCalled(t, "Delete", mock.Anything, mock.Anything, mock.Anything)
+	mockLister.AssertExpectations(t)
+}
+
+func TestSourceReconciler_deleteAllMirrors_aggregatesErrors(t *testing.T) {
+	// Regression test: per-namespace deletion failures must be returned (joined),
+	// otherwise callers will remove the finalizer and orphan the failed mirrors.
+	sourceObj := &unstructured.Unstructured{
+		Object: map[string]interface{}{
+			"apiVersion": "v1",
+			"kind":       "Secret",
+			"metadata": map[string]interface{}{
+				"name":      "test-secret",
+				"namespace": "default",
+				"uid":       "source-uid",
+			},
+		},
+	}
+
+	managedMirror := func(ns string) *unstructured.Unstructured {
+		return &unstructured.Unstructured{
+			Object: map[string]interface{}{
+				"apiVersion": "v1",
+				"kind":       "Secret",
+				"metadata": map[string]interface{}{
+					"name":      "test-secret",
+					"namespace": ns,
+					"labels": map[string]interface{}{
+						constants.LabelManagedBy: constants.ControllerName,
+						constants.LabelMirror:    "true",
+					},
+					"annotations": map[string]interface{}{
+						constants.AnnotationSourceNamespace: "default",
+						constants.AnnotationSourceName:      "test-secret",
+						constants.AnnotationSourceUID:       "source-uid",
+					},
+				},
+			},
+		}
+	}
+
+	mockClient := new(MockClient)
+	mockLister := new(MockNamespaceLister)
+	mockLister.On("ListNamespaces", mock.Anything).Return([]string{"default", "ns-ok", "ns-fail"}, nil)
+
+	mockClient.On("Get", mock.Anything,
+		types.NamespacedName{Namespace: "ns-ok", Name: "test-secret"},
+		mock.AnythingOfType("*unstructured.Unstructured")).
+		Return(nil, managedMirror("ns-ok"))
+	mockClient.On("Get", mock.Anything,
+		types.NamespacedName{Namespace: "ns-fail", Name: "test-secret"},
+		mock.AnythingOfType("*unstructured.Unstructured")).
+		Return(nil, managedMirror("ns-fail"))
+
+	mockClient.On("Delete", mock.Anything, mock.MatchedBy(func(obj client.Object) bool {
+		u, ok := obj.(*unstructured.Unstructured)
+		return ok && u.GetNamespace() == "ns-ok"
+	}), mock.Anything).Return(nil)
+
+	mockClient.On("Delete", mock.Anything, mock.MatchedBy(func(obj client.Object) bool {
+		u, ok := obj.(*unstructured.Unstructured)
+		return ok && u.GetNamespace() == "ns-fail"
+	}), mock.Anything).Return(fmt.Errorf("webhook denied"))
+
+	r := &SourceReconciler{Client: mockClient, NamespaceLister: mockLister}
+	err := r.deleteAllMirrors(context.Background(), sourceObj)
+	require.Error(t, err, "must surface deletion failure so finalizer is retained")
+	assert.Contains(t, err.Error(), "ns-fail")
+}
+
+func TestIsBlacklistedSecret(t *testing.T) {
+	cases := []struct {
+		obj      *unstructured.Unstructured
+		name     string
+		expected bool
+	}{
+		{
+			name: "service-account-token blacklisted",
+			obj: &unstructured.Unstructured{Object: map[string]interface{}{
+				"apiVersion": "v1", "kind": "Secret",
+				"type": "kubernetes.io/service-account-token",
+			}},
+			expected: true,
+		},
+		{
+			name: "bootstrap token blacklisted",
+			obj: &unstructured.Unstructured{Object: map[string]interface{}{
+				"apiVersion": "v1", "kind": "Secret",
+				"type": "bootstrap.kubernetes.io/token",
+			}},
+			expected: true,
+		},
+		{
+			name: "helm release blacklisted",
+			obj: &unstructured.Unstructured{Object: map[string]interface{}{
+				"apiVersion": "v1", "kind": "Secret",
+				"type": "helm.sh/release.v1",
+			}},
+			expected: true,
+		},
+		{
+			name: "opaque secret allowed",
+			obj: &unstructured.Unstructured{Object: map[string]interface{}{
+				"apiVersion": "v1", "kind": "Secret",
+				"type": "Opaque",
+			}},
+			expected: false,
+		},
+		{
+			name: "secret without type allowed",
+			obj: &unstructured.Unstructured{Object: map[string]interface{}{
+				"apiVersion": "v1", "kind": "Secret",
+			}},
+			expected: false,
+		},
+		{
+			name: "configmap with matching type ignored",
+			obj: &unstructured.Unstructured{Object: map[string]interface{}{
+				"apiVersion": "v1", "kind": "ConfigMap",
+				"type": "kubernetes.io/service-account-token",
+			}},
+			expected: false,
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			assert.Equal(t, tc.expected, isBlacklistedSecret(tc.obj))
+		})
+	}
+}
+
+func TestSourceReconciler_Reconcile_RefusesBlacklistedSecret(t *testing.T) {
+	// Regression test: enabling mirroring on a service-account-token Secret
+	// must NOT cause it to be mirrored anywhere.
+	mockClient := new(MockClient)
+	mockLister := new(MockNamespaceLister)
+
+	tokenSecret := &unstructured.Unstructured{
+		Object: map[string]interface{}{
+			"apiVersion": "v1",
+			"kind":       "Secret",
+			"metadata": map[string]interface{}{
+				"name":      "sa-token",
+				"namespace": "default",
+				"labels": map[string]interface{}{
+					constants.LabelEnabled: "true",
+				},
+				"annotations": map[string]interface{}{
+					constants.AnnotationSync:             "true",
+					constants.AnnotationTargetNamespaces: "all",
+				},
+			},
+			"type": "kubernetes.io/service-account-token",
+		},
+	}
+
+	mockClient.On("Get", mock.Anything,
+		types.NamespacedName{Namespace: "default", Name: "sa-token"},
+		mock.AnythingOfType("*unstructured.Unstructured")).
+		Return(nil, tokenSecret)
+
+	r := &SourceReconciler{
+		Client:          mockClient,
+		Scheme:          runtime.NewScheme(),
+		Config:          &config.Config{},
+		Filter:          filter.NewNamespaceFilter([]string{}, []string{}),
+		NamespaceLister: mockLister,
+		GVK:             schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Secret"},
+	}
+
+	result, err := r.Reconcile(context.Background(),
+		ctrl.Request{NamespacedName: types.NamespacedName{Namespace: "default", Name: "sa-token"}})
+
+	require.NoError(t, err)
+	assert.Equal(t, ctrl.Result{}, result)
+
+	// Critical: no namespace listing, no Create, no Update — the Secret was
+	// rejected before anything mirroring-related happened.
+	mockLister.AssertNotCalled(t, "ListNamespacesWithLabels", mock.Anything)
+	mockClient.AssertNotCalled(t, "Create", mock.Anything, mock.Anything, mock.Anything)
+}