diff --git a/pkg/controller/source_reconciler.go b/pkg/controller/source_reconciler.go index 4464062..ead83c1 100644 --- a/pkg/controller/source_reconciler.go +++ b/pkg/controller/source_reconciler.go @@ -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 diff --git a/pkg/controller/source_reconciler_test.go b/pkg/controller/source_reconciler_test.go index 8a9a84e..230ec2d 100644 --- a/pkg/controller/source_reconciler_test.go +++ b/pkg/controller/source_reconciler_test.go @@ -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) +}