LOG-9751: fix(filter:drop): add validation to restrict inclusion of s…

api/observability/v1/filter_types.go

@@ -119,6 +119,7 @@ type DropCondition struct {
 	// Must define only one of matches OR notMatches
 	//
 	// +kubebuilder:validation:Optional
+	// +kubebuilder:validation:Pattern:=`^[^'\n\r]*$`
 	// +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Drop Match Expression"
 	Matches string `json:"matches,omitempty"`
 
@@ -127,6 +128,7 @@ type DropCondition struct {
 	// Must define only one of matches or notMatches
 	//
 	// +kubebuilder:validation:Optional
+	// +kubebuilder:validation:Pattern:=`^[^'\n\r]*$`
 	// +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Keep Match Expression"
 	NotMatches string `json:"notMatches,omitempty"`
 }

bundle/manifests/observability.openshift.io_clusterlogforwarders.yaml

@@ -1115,12 +1115,14 @@ spec:
                                     A regular expression that the field will match.
                                     If the value of the field defined in the DropTest matches the regular expression, the log record will be dropped.
                                     Must define only one of matches OR notMatches
+                                  pattern: ^[^'\n\r]*$
                                   type: string
                                 notMatches:
                                   description: |-
                                     A regular expression that the field does not match.
                                     If the value of the field defined in the DropTest does not match the regular expression, the log record will be dropped.
                                     Must define only one of matches or notMatches
+                                  pattern: ^[^'\n\r]*$
                                   type: string
                               type: object
                               x-kubernetes-validations:

config/crd/bases/observability.openshift.io_clusterlogforwarders.yaml

@@ -1115,12 +1115,14 @@ spec:
                                     A regular expression that the field will match.
                                     If the value of the field defined in the DropTest matches the regular expression, the log record will be dropped.
                                     Must define only one of matches OR notMatches
+                                  pattern: ^[^'\n\r]*$
                                   type: string
                                 notMatches:
                                   description: |-
                                     A regular expression that the field does not match.
                                     If the value of the field defined in the DropTest does not match the regular expression, the log record will be dropped.
                                     Must define only one of matches or notMatches
+                                  pattern: ^[^'\n\r]*$
                                   type: string
                               type: object
                               x-kubernetes-validations:

docs/reference/datamodels/viaq/v1.adoc

@@ -973,7 +973,7 @@ to be sent with the log Records
 
 |string
 
-a|  Sequence is increasing id used in conjunction with the timestamp to estblish a linear timeline
+a|  Sequence is an increasing ID used in conjunction with the timestamp to estblish a linear timeline
 of log records.  This was added as a workaround for logstores that do not have nano-second precision.
 
 |======================
@@ -1003,7 +1003,7 @@ to be sent with the log Records
 
 ===== Description
 
-Sequence is increasing id used in conjunction with the timestamp to estblish a linear timeline
+Sequence is an increasing ID used in conjunction with the timestamp to estblish a linear timeline
 of log records.  This was added as a workaround for logstores that do not have nano-second precision.
 
 =====  Type
@@ -1664,7 +1664,7 @@ to be sent with the log Records
 
 |string
 
-a|  Sequence is increasing id used in conjunction with the timestamp to estblish a linear timeline
+a|  Sequence is an increasing ID used in conjunction with the timestamp to estblish a linear timeline
 of log records.  This was added as a workaround for logstores that do not have nano-second precision.
 
 |======================
@@ -1694,7 +1694,7 @@ to be sent with the log Records
 
 ===== Description
 
-Sequence is increasing id used in conjunction with the timestamp to estblish a linear timeline
+Sequence is an increasing ID used in conjunction with the timestamp to estblish a linear timeline
 of log records.  This was added as a workaround for logstores that do not have nano-second precision.
 
 =====  Type
@@ -2648,7 +2648,7 @@ to be sent with the log Records
 
 |string
 
-a|  Sequence is increasing id used in conjunction with the timestamp to estblish a linear timeline
+a|  Sequence is an increasing ID used in conjunction with the timestamp to estblish a linear timeline
 of log records.  This was added as a workaround for logstores that do not have nano-second precision.
 
 |======================
@@ -2678,7 +2678,7 @@ to be sent with the log Records
 
 ===== Description
 
-Sequence is increasing id used in conjunction with the timestamp to estblish a linear timeline
+Sequence is an increasing ID used in conjunction with the timestamp to estblish a linear timeline
 of log records.  This was added as a workaround for logstores that do not have nano-second precision.
 
 =====  Type
@@ -4267,7 +4267,7 @@ to be sent with the log Records
 
 |string
 
-a|  Sequence is increasing id used in conjunction with the timestamp to estblish a linear timeline
+a|  Sequence is an increasing ID used in conjunction with the timestamp to estblish a linear timeline
 of log records.  This was added as a workaround for logstores that do not have nano-second precision.
 
 |======================
@@ -4297,7 +4297,7 @@ to be sent with the log Records
 
 ===== Description
 
-Sequence is increasing id used in conjunction with the timestamp to estblish a linear timeline
+Sequence is an increasing ID used in conjunction with the timestamp to estblish a linear timeline
 of log records.  This was added as a workaround for logstores that do not have nano-second precision.
 
 =====  Type

internal/generator/vector/filter/drop/filter.go

@@ -15,17 +15,34 @@ func NewFilter(dropTestsSpec []obs.DropTest) *Filter {
 	return &Filter{dropTestsSpec}
 }
 
+func buildMatchCondition(field, pattern string, negate bool) (string, error) {
+	if strings.ContainsAny(pattern, "'\n\r") {
+		return "", fmt.Errorf("match pattern must not contain single quotes, newlines, or carriage returns: %q", pattern)
+	}
+	prefix := ""
+	if negate {
+		prefix = "!"
+	}
+	return fmt.Sprintf(`%smatch(to_string(%s) ?? "", r'%s')`, prefix, field, pattern), nil
+}
+
 func (f *Filter) VRL() (string, error) {
 	vrlTests := []string{}
 	for _, test := range f.tests {
 		condList := []string{}
 		for _, cond := range test.DropConditions {
 			field := fmt.Sprintf("._internal%s", cond.Field)
+			var matchExpr string
+			var err error
 			if cond.Matches != "" {
-				condList = append(condList, fmt.Sprintf(`match(to_string(%s) ?? "", r'%s')`, field, cond.Matches))
+				matchExpr, err = buildMatchCondition(field, cond.Matches, false)
 			} else {
-				condList = append(condList, fmt.Sprintf(`!match(to_string(%s) ?? "", r'%s')`, field, cond.NotMatches))
+				matchExpr, err = buildMatchCondition(field, cond.NotMatches, true)
+			}
+			if err != nil {
+				return "", err
 			}
+			condList = append(condList, matchExpr)
 		}
 		// Concatenate the conditions with ANDs and add Vector's error coalescing.
 		// If any errors arise from the match such as, `cond.Field` not being a string or a field

internal/generator/vector/filter/drop/filter_test.go

@@ -10,6 +10,38 @@ import (
 var _ = Describe("drop filter", func() {
 
 	Context("#VRL", func() {
+		It("should reject matches containing single quotes", func() {
+			spec := []obs.DropTest{
+				{
+					DropConditions: []obs.DropCondition{
+						{
+							Field:   ".kubernetes.namespace_name",
+							Matches: "foo'bar",
+						},
+					},
+				},
+			}
+			_, err := NewFilter(spec).VRL()
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("single quotes"))
+		})
+
+		It("should reject notMatches containing single quotes", func() {
+			spec := []obs.DropTest{
+				{
+					DropConditions: []obs.DropCondition{
+						{
+							Field:      ".kubernetes.namespace_name",
+							NotMatches: "x'''[sources.evil]",
+						},
+					},
+				},
+			}
+			_, err := NewFilter(spec).VRL()
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("single quotes"))
+		})
+
 		It("should generate valid VRL for dropping", func() {
 			spec := []obs.DropTest{
 				{

internal/pkg/generator/forwarder/generator.go

@@ -6,11 +6,14 @@ import (
 
 	obs "github.com/openshift/cluster-logging-operator/api/observability/v1"
 	"github.com/openshift/cluster-logging-operator/internal/api/initialize"
+	internalobs "github.com/openshift/cluster-logging-operator/internal/api/observability"
 	"github.com/openshift/cluster-logging-operator/internal/factory"
 	forwardergenerator "github.com/openshift/cluster-logging-operator/internal/generator/forwarder"
 	"github.com/openshift/cluster-logging-operator/internal/generator/framework"
 	"github.com/openshift/cluster-logging-operator/internal/utils"
+	filtervalidation "github.com/openshift/cluster-logging-operator/internal/validations/observability/filters"
 	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/yaml"
 
 	log "github.com/ViaQ/logerr/v2/log/static"
@@ -43,6 +46,19 @@ func Generate(clfYaml string, debugOutput bool, client client.Client) (string, e
 	//}
 	forwarder = initialize.ClusterLogForwarder(forwarder, utils.NoOptions)
 	log.V(3).Info("Initialized ClusterLogForwarder", "cr", forwarder)
+
+	filterMap := internalobs.FilterMap(forwarder.Spec)
+	var filterErrors []error
+	for _, filter := range filterMap {
+		cond := filtervalidation.ValidateFilter(*filter)
+		if cond.Status == metav1.ConditionFalse {
+			filterErrors = append(filterErrors, errors.New(cond.Message))
+		}
+	}
+	if len(filterErrors) > 0 {
+		return "", fmt.Errorf("invalid filter spec: %w", errors.Join(filterErrors...))
+	}
+
 	// TODO: enable secrets
 	//secrets := internalobs.FetchSecrets(forwarder.Spec.Outputs, client)
 	secrets := map[string]*corev1.Secret{}

internal/validations/observability/filters/validate_filters.go

@@ -54,11 +54,14 @@ func validateDropFilter(filterSpec obs.FilterSpec) (results []string) {
 			if testCondition.Matches != "" && testCondition.NotMatches != "" {
 				testErrors = append(testErrors, "only one of matches or notMatches can be defined at once")
 			}
+			if strings.ContainsAny(testCondition.Matches, "'\n\r") || strings.ContainsAny(testCondition.NotMatches, "'\n\r") {
+				testErrors = append(testErrors, "matches/notMatches must not contain single quotes, newlines, or carriage returns")
+			}
 			// Validate provided regex
 			if testCondition.Matches != "" {
 				_, err = regexp.Compile(testCondition.Matches)
 			} else if testCondition.NotMatches != "" {
-				_, err = regexp.Compile(testCondition.Matches)
+				_, err = regexp.Compile(testCondition.NotMatches)
 			}
 			if err != nil {
 				testErrors = append(testErrors, "matches/notMatches must be a valid regular expression.")

internal/validations/observability/filters/validate_filters_test.go

@@ -95,6 +95,45 @@ var _ = Describe("[internal][validations][observability][filters]", func() {
 				},
 				"[matches/notMatches must be a valid regular expression.]",
 			),
+			Entry("should fail validation if notMatches contains an invalid regular expression",
+				[]obs.DropTest{
+					{
+						DropConditions: []obs.DropCondition{
+							{
+								Field:      ".kubernetes.namespace_name",
+								NotMatches: "[invalid",
+							},
+						},
+					},
+				},
+				"[matches/notMatches must be a valid regular expression.]",
+			),
+			Entry("should fail validation if matches contains a single quote",
+				[]obs.DropTest{
+					{
+						DropConditions: []obs.DropCondition{
+							{
+								Field:   ".kubernetes.namespace_name",
+								Matches: "foo'bar",
+							},
+						},
+					},
+				},
+				"[matches/notMatches must not contain single quotes, newlines, or carriage returns]",
+			),
+			Entry("should fail validation if notMatches contains a single quote",
+				[]obs.DropTest{
+					{
+						DropConditions: []obs.DropCondition{
+							{
+								Field:      ".kubernetes.namespace_name",
+								NotMatches: "x'''[sources.evil]",
+							},
+						},
+					},
+				},
+				"[matches/notMatches must not contain single quotes, newlines, or carriage returns]",
+			),
 		)
 
 		DescribeTable("valid drop filter spec", func(dropTests []obs.DropTest) {

test/e2e/collection/apivalidations/api_validations_test.go

@@ -126,5 +126,16 @@ var _ = Describe("", func() {
 			Expect(err).To(HaveOccurred())
 			Expect(err.Error()).To(MatchRegexp("azureMonitor.logType: Required value"))
 		}),
+		Entry("should pass for drop filter with valid matches", "drop-filter-valid.yaml", func(out string, err error) {
+			Expect(err).ToNot(HaveOccurred())
+		}),
+		Entry("should fail for drop filter with single quote in matches", "drop-filter-single-quote-matches.yaml", func(out string, err error) {
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("Invalid value"))
+		}),
+		Entry("should fail for drop filter with single quote in notMatches", "drop-filter-single-quote-notmatches.yaml", func(out string, err error) {
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("Invalid value"))
+		}),
 	)
 })

test/e2e/collection/apivalidations/drop-filter-single-quote-matches.yaml

@@ -0,0 +1,35 @@
+apiVersion: observability.openshift.io/v1
+kind: ClusterLogForwarder
+metadata:
+  name: clf-validation-test
+spec:
+  filters:
+    - name: my-drop-filter
+      type: drop
+      drop:
+        - test:
+            - field: .kubernetes.namespace_name
+              matches: "foo'bar"
+  managementState: Managed
+  outputs:
+    - name: splunk-aosqe
+      splunk:
+        authentication:
+          token:
+            key: hecToken
+            secretName: to-splunk-secret-54980
+        index: main
+        tuning:
+          compression: none
+        url: http://to-nowhere.svc:8088
+      type: splunk
+  pipelines:
+    - filterRefs:
+        - my-drop-filter
+      inputRefs:
+        - application
+      name: forward-log-splunk
+      outputRefs:
+        - splunk-aosqe
+  serviceAccount:
+    name: clf-validation-test

test/e2e/collection/apivalidations/drop-filter-single-quote-notmatches.yaml

@@ -0,0 +1,35 @@
+apiVersion: observability.openshift.io/v1
+kind: ClusterLogForwarder
+metadata:
+  name: clf-validation-test
+spec:
+  filters:
+    - name: my-drop-filter
+      type: drop
+      drop:
+        - test:
+            - field: .kubernetes.namespace_name
+              notMatches: "x'''[sources.evil]"
+  managementState: Managed
+  outputs:
+    - name: splunk-aosqe
+      splunk:
+        authentication:
+          token:
+            key: hecToken
+            secretName: to-splunk-secret-54980
+        index: main
+        tuning:
+          compression: none
+        url: http://to-nowhere.svc:8088
+      type: splunk
+  pipelines:
+    - filterRefs:
+        - my-drop-filter
+      inputRefs:
+        - application
+      name: forward-log-splunk
+      outputRefs:
+        - splunk-aosqe
+  serviceAccount:
+    name: clf-validation-test