feat(remediation): add quarantine action (deny-all NetworkPolicy + dry-run)

mainhandler/actionhandler.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"strings"
 	"time"
 
 	"github.com/armosec/armoapi-go/apis"
@@ -12,6 +13,7 @@ import (
 	"github.com/kubescape/operator/mainhandler/remediators"
 	"github.com/kubescape/operator/utils"
 	corev1 "k8s.io/api/core/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
@@ -80,36 +82,69 @@ func (actionHandler *ActionHandler) handleOperatorAction(ctx context.Context) er
 		helpers.String("dryRun", fmt.Sprintf("%t", dryRun)))
 
 	switch args.Action {
-	case apis.OperatorActionAnnotate:
-		r := registry[apis.OperatorActionAnnotate]
-		plan, err := r.Plan(ctx, remediators.Request{Target: target, Reason: args.Reason, FindingRef: args.FindingRef})
-		if err != nil {
-			return err
-		}
-		result, err := r.Apply(ctx, plan, dryRun)
-		if err != nil {
-			return err
-		}
-		return actionHandler.recordActionResult(ctx, result)
+	case apis.OperatorActionAnnotate, apis.OperatorActionQuarantine:
+		req := remediators.Request{Target: target, Reason: args.Reason, FindingRef: args.FindingRef}
+		return actionHandler.applyRemediation(ctx, registry[args.Action], req, dryRun)
 
 	case apis.OperatorActionRevert:
-		// Phase 1: the only applied action is annotate, so revert undoes it.
 		// Pass dryRun so a default (no --confirm) revert previews instead of writing.
-		r := registry[apis.OperatorActionAnnotate]
-		result, err := r.Revert(ctx, target, dryRun)
-		if err != nil {
-			return err
-		}
-		return actionHandler.recordActionResult(ctx, result)
+		return actionHandler.revertTarget(ctx, registry, target, dryRun)
 
-	case apis.OperatorActionQuarantine, apis.OperatorActionCordon:
+	case apis.OperatorActionCordon:
 		return fmt.Errorf("operatorAction: action %q is not implemented yet (planned for a later phase)", args.Action)
 
 	default:
 		return fmt.Errorf("operatorAction: unknown action %q", args.Action)
 	}
 }
 
+// applyRemediation runs a remediator's Plan -> Apply and records the result.
+func (actionHandler *ActionHandler) applyRemediation(ctx context.Context, r remediators.Remediator, req remediators.Request, dryRun bool) error {
+	plan, err := r.Plan(ctx, req)
+	if err != nil {
+		return err
+	}
+	result, err := r.Apply(ctx, plan, dryRun)
+	if err != nil {
+		return err
+	}
+	return actionHandler.recordActionResult(ctx, result)
+}
+
+// revertTarget undoes every reversible action on the target. Each Revert is
+// idempotent — a missing annotation or NetworkPolicy is a no-op — so revert is
+// safe to call without knowing which action was originally applied. A target
+// object that no longer exists (NotFound) is skipped, not treated as an error,
+// so a leftover artifact (e.g. a NetworkPolicy whose workload was deleted) is
+// still cleaned up.
+func (actionHandler *ActionHandler) revertTarget(ctx context.Context, registry map[apis.OperatorActionType]remediators.Remediator, target remediators.Target, dryRun bool) error {
+	reversible := []apis.OperatorActionType{apis.OperatorActionAnnotate, apis.OperatorActionQuarantine}
+	var descriptions []string
+	applied := false
+	for _, action := range reversible {
+		r, ok := registry[action]
+		if !ok {
+			continue
+		}
+		result, err := r.Revert(ctx, target, dryRun)
+		if err != nil {
+			if apierrors.IsNotFound(err) {
+				continue
+			}
+			return err
+		}
+		descriptions = append(descriptions, result.Description)
+		applied = applied || result.Applied
+	}
+	return actionHandler.recordActionResult(ctx, remediators.Result{
+		Action:      string(apis.OperatorActionRevert),
+		Target:      target,
+		DryRun:      dryRun,
+		Applied:     applied,
+		Description: strings.Join(descriptions, "; "),
+	})
+}
+
 // recordActionResult writes the result to the OperatorCommand status payload
 // (a no-op when the command did not originate from a CRD) and emits a
 // best-effort Kubernetes Event.

mainhandler/actionhandler_test.go

@@ -13,6 +13,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	appsv1 "k8s.io/api/apps/v1"
+	networkingv1 "k8s.io/api/networking/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/client-go/kubernetes"
@@ -22,6 +23,17 @@ import (
 
 func boolPtr(b bool) *bool { return &b }
 
+func deploymentWithSelectorForHandler(ns, name string, selector map[string]string) *appsv1.Deployment {
+	return &appsv1.Deployment{
+		ObjectMeta: metav1.ObjectMeta{Namespace: ns, Name: name},
+		Spec:       appsv1.DeploymentSpec{Selector: &metav1.LabelSelector{MatchLabels: selector}},
+	}
+}
+
+func networkPolicyForHandler(ns, name string) *networkingv1.NetworkPolicy {
+	return &networkingv1.NetworkPolicy{ObjectMeta: metav1.ObjectMeta{Namespace: ns, Name: name}}
+}
+
 func newTestConfig(serviceConfig config.Config) config.IConfig {
 	return config.NewOperatorConfig(config.CapabilitiesConfig{}, utilsmetadata.ClusterConfig{}, &beUtils.Credentials{}, serviceConfig)
 }
@@ -178,7 +190,7 @@ func TestHandleOperatorAction_TargetRequired(t *testing.T) {
 func TestHandleOperatorAction_UnimplementedActions(t *testing.T) {
 	client := k8sfake.NewClientset()
 	cfg := newTestConfig(config.Config{Namespace: "kubescape"})
-	for _, action := range []apis.OperatorActionType{apis.OperatorActionQuarantine, apis.OperatorActionCordon} {
+	for _, action := range []apis.OperatorActionType{apis.OperatorActionCordon} {
 		ah := newActionHandlerForTest(t, client, cfg, apis.OperatorActionArgs{
 			Action: action,
 			Target: &apis.OperatorActionTarget{Kind: "Deployment", Namespace: "payments", Name: "api"},
@@ -189,6 +201,77 @@ func TestHandleOperatorAction_UnimplementedActions(t *testing.T) {
 	}
 }
 
+// quarantine without --confirm must create the deny-all NetworkPolicy as a
+// server-side dry-run, never a real write.
+func TestHandleOperatorAction_QuarantineDefaultsToDryRun(t *testing.T) {
+	client := k8sfake.NewClientset(deploymentWithSelectorForHandler("payments", "api", map[string]string{"app": "api"}))
+	var dryRun []string
+	client.PrependReactor("create", "networkpolicies", func(action clienttesting.Action) (bool, runtime.Object, error) {
+		dryRun = action.(clienttesting.CreateActionImpl).CreateOptions.DryRun
+		return false, nil, nil
+	})
+
+	ah := newActionHandlerForTest(t, client, newTestConfig(config.Config{Namespace: "kubescape"}), apis.OperatorActionArgs{
+		Action: apis.OperatorActionQuarantine,
+		Target: &apis.OperatorActionTarget{Kind: "Deployment", Namespace: "payments", Name: "api"},
+		Reason: "C-0016",
+	})
+
+	require.NoError(t, ah.handleOperatorAction(context.Background()))
+	assert.Equal(t, []string{metav1.DryRunAll}, dryRun, "quarantine without dryRun must default to server-side dry-run")
+}
+
+// quarantine --confirm writes the NetworkPolicy isolating the workload's pods.
+func TestHandleOperatorAction_QuarantineConfirmWrites(t *testing.T) {
+	client := k8sfake.NewClientset(deploymentWithSelectorForHandler("payments", "api", map[string]string{"app": "api"}))
+	ah := newActionHandlerForTest(t, client, newTestConfig(config.Config{Namespace: "kubescape"}), apis.OperatorActionArgs{
+		Action: apis.OperatorActionQuarantine,
+		Target: &apis.OperatorActionTarget{Kind: "Deployment", Namespace: "payments", Name: "api"},
+		DryRun: boolPtr(false),
+	})
+
+	require.NoError(t, ah.handleOperatorAction(context.Background()))
+	got, err := client.NetworkingV1().NetworkPolicies("payments").Get(context.Background(), "kubescape-quarantine-deployment-api", metav1.GetOptions{})
+	require.NoError(t, err)
+	assert.Equal(t, map[string]string{"app": "api"}, got.Spec.PodSelector.MatchLabels)
+}
+
+// A quarantine target in an excluded namespace must be rejected before any write.
+func TestHandleOperatorAction_QuarantineExcludedNamespace(t *testing.T) {
+	client := k8sfake.NewClientset()
+	cfg := newTestConfig(config.Config{Namespace: "kubescape", ExcludeNamespaces: []string{"kube-system"}})
+	ah := newActionHandlerForTest(t, client, cfg, apis.OperatorActionArgs{
+		Action: apis.OperatorActionQuarantine,
+		Target: &apis.OperatorActionTarget{Kind: "Deployment", Namespace: "kube-system", Name: "api"},
+		DryRun: boolPtr(false),
+	})
+	err := ah.handleOperatorAction(context.Background())
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "excluded from remediation")
+
+	// The rejection must happen before any cluster write — no NetworkPolicy may
+	// have been created in the excluded namespace.
+	nps, listErr := client.NetworkingV1().NetworkPolicies("kube-system").List(context.Background(), metav1.ListOptions{})
+	require.NoError(t, listErr)
+	assert.Empty(t, nps.Items, "excluded namespace must be rejected before any NetworkPolicy write")
+}
+
+// revert undoes quarantine (deletes the NetworkPolicy) even when the workload was
+// never annotated — without the caller naming which action to undo.
+func TestHandleOperatorAction_RevertDeletesQuarantine(t *testing.T) {
+	np := networkPolicyForHandler("payments", "kubescape-quarantine-deployment-api")
+	client := k8sfake.NewClientset(np)
+	ah := newActionHandlerForTest(t, client, newTestConfig(config.Config{Namespace: "kubescape"}), apis.OperatorActionArgs{
+		Action: apis.OperatorActionRevert,
+		Target: &apis.OperatorActionTarget{Kind: "Deployment", Namespace: "payments", Name: "api"},
+		DryRun: boolPtr(false),
+	})
+
+	require.NoError(t, ah.handleOperatorAction(context.Background()))
+	_, err := client.NetworkingV1().NetworkPolicies("payments").Get(context.Background(), "kubescape-quarantine-deployment-api", metav1.GetOptions{})
+	assert.Error(t, err, "revert must delete the quarantine NetworkPolicy")
+}
+
 func TestHandleOperatorAction_UnknownAction(t *testing.T) {
 	client := k8sfake.NewClientset()
 	ah := newActionHandlerForTest(t, client, newTestConfig(config.Config{Namespace: "kubescape"}), apis.OperatorActionArgs{

mainhandler/remediators/annotate.go

@@ -73,10 +73,29 @@ func (r *AnnotateRemediator) Apply(ctx context.Context, p Plan, dryRun bool) (Re
 // Apply, dryRun=true issues a server-side dry-run (validated against admission,
 // never persisted); only dryRun=false performs a real write — so the
 // safe-by-default contract is honored for revert too.
+//
+// The contract's revert verb undoes every reversible action without naming which
+// one was applied, so revert runs annotate even on a workload that was only
+// quarantined. To keep the audit trail honest, a workload carrying none of the
+// remediation annotations is reported as a no-op ("nothing to revert",
+// Applied=false) rather than claiming annotations were removed.
 func (r *AnnotateRemediator) Revert(ctx context.Context, t Target, dryRun bool) (Result, error) {
 	if err := validateTarget(t); err != nil {
 		return Result{}, err
 	}
+	annotations, err := r.getAnnotations(ctx, t)
+	if err != nil {
+		return Result{}, err
+	}
+	if !hasAnyKey(annotations, AnnotationRemediated, AnnotationReason, AnnotationFindingRef) {
+		return Result{
+			Action:      string(apis.OperatorActionRevert),
+			Target:      t,
+			DryRun:      dryRun,
+			Applied:     false,
+			Description: fmt.Sprintf("no kubescape remediation annotations on %s; nothing to revert", t),
+		}, nil
+	}
 	// A JSON merge patch deletes a key by setting it to null.
 	patch, err := annotationPatch(map[string]interface{}{
 		AnnotationRemediated: nil,
@@ -109,6 +128,40 @@ func (r *AnnotateRemediator) desiredAnnotations(req Request) map[string]string {
 	return annotations
 }
 
+// getAnnotations reads the live target's annotations so Revert can tell whether
+// there is anything to undo. A NotFound is returned to the caller (the action
+// handler skips a target that no longer exists).
+func (r *AnnotateRemediator) getAnnotations(ctx context.Context, t Target) (map[string]string, error) {
+	switch strings.ToLower(t.Kind) {
+	case "deployment":
+		o, err := r.client.AppsV1().Deployments(t.Namespace).Get(ctx, t.Name, metav1.GetOptions{})
+		if err != nil {
+			return nil, err
+		}
+		return o.Annotations, nil
+	case "statefulset":
+		o, err := r.client.AppsV1().StatefulSets(t.Namespace).Get(ctx, t.Name, metav1.GetOptions{})
+		if err != nil {
+			return nil, err
+		}
+		return o.Annotations, nil
+	case "daemonset":
+		o, err := r.client.AppsV1().DaemonSets(t.Namespace).Get(ctx, t.Name, metav1.GetOptions{})
+		if err != nil {
+			return nil, err
+		}
+		return o.Annotations, nil
+	case "pod":
+		o, err := r.client.CoreV1().Pods(t.Namespace).Get(ctx, t.Name, metav1.GetOptions{})
+		if err != nil {
+			return nil, err
+		}
+		return o.Annotations, nil
+	default:
+		return nil, fmt.Errorf("annotate: unsupported target kind %q (supported: Deployment, StatefulSet, DaemonSet, Pod)", t.Kind)
+	}
+}
+
 // patch applies a JSON merge patch to the target object using the typed client
 // for its kind. Server-side dry-run is requested when dryRun is true.
 func (r *AnnotateRemediator) patch(ctx context.Context, t Target, patch []byte, dryRun bool) error {
@@ -164,6 +217,16 @@ func toInterfaceMap(in map[string]string) map[string]interface{} {
 	return out
 }
 
+// hasAnyKey reports whether m contains at least one of keys.
+func hasAnyKey(m map[string]string, keys ...string) bool {
+	for _, k := range keys {
+		if _, ok := m[k]; ok {
+			return true
+		}
+	}
+	return false
+}
+
 func validateTarget(t Target) error {
 	if t.Kind == "" || t.Name == "" {
 		return fmt.Errorf("target requires both kind and name (got %+v)", t)

mainhandler/remediators/annotate_test.go

@@ -145,6 +145,26 @@ func TestAnnotateRevert(t *testing.T) {
 	assert.Equal(t, "keep-me", got.Annotations["unrelated"], "unrelated annotations must be preserved")
 }
 
+// revert runs on every target regardless of which action was applied, so a
+// workload that was never annotated must be reported as a no-op ("nothing to
+// revert", Applied=false) instead of claiming annotations were removed — and it
+// must not issue a patch.
+func TestAnnotateRevertNoAnnotationsIsNoop(t *testing.T) {
+	client := k8sfake.NewClientset(annotatedDeployment("payments", "api", map[string]string{"unrelated": "keep-me"}))
+	patched := false
+	client.PrependReactor("patch", "deployments", func(action clienttesting.Action) (bool, runtime.Object, error) {
+		patched = true
+		return false, nil, nil
+	})
+	r := NewAnnotateRemediator(client)
+
+	res, err := r.Revert(context.Background(), Target{Kind: "Deployment", Namespace: "payments", Name: "api"}, false)
+	require.NoError(t, err)
+	assert.False(t, res.Applied, "nothing to revert")
+	assert.Contains(t, res.Description, "nothing to revert")
+	assert.False(t, patched, "revert must not patch a workload with no kubescape annotations")
+}
+
 // A dry-run revert must request server-side dry-run and never persist, mirroring
 // Apply — so the safe-by-default contract holds for revert too.
 func TestAnnotateRevertDryRun(t *testing.T) {