Content based block filter #2002

MustafaSaber · 2022-05-09T10:23:36Z

Intro

Body filter is a used to block requests based on body content

Implementation can be tested with proxy
./bin/skipper -address=:9999 -inline-routes='r: * -> block("foo") -> "http://127.0.0.1:9090";'

szuecs · 2022-05-18T14:15:24Z

filters/block/matcher.go

+
+func newMatcher(
+	input io.ReadCloser,
+	matchList []string,


We need to store the []byte converted strings

type matchData struct { b []byte }

matchList []matchData,

see also https://github.com/zalando/skipper/pull/2002/files/38b573a768e06ac3b7397e4a97e5a205a1bcef7f#r877428661

szuecs · 2022-05-18T14:16:13Z

filters/block/matcher.go

+	var consumed int
+
+	for _, s := range m.matchList {
+		if bytes.Contains(b, []byte(s)) {


We can prepare string s from matchList to be a []byte already, such that we do the conversion once.

see also https://github.com/zalando/skipper/pull/2002/files/38b573a768e06ac3b7397e4a97e5a205a1bcef7f#r877428661

szuecs · 2022-05-19T18:55:30Z

filters/block/matcher.go

+	log "github.com/sirupsen/logrus"
+)
+
+type maxBufferHandling int


please move the type to the iota const, such that these are close together

szuecs · 2022-05-19T19:01:19Z

filters/block/matcher.go

+
+	// init:
+	input             io.ReadCloser
+	matchList         []string


this should be either a map[int][]byte or a []buffer with type buffer []byte
see also https://go.dev/play/p/IKAa6eA91YD
I think the type is more memory efficient, but it does not really matter if we do not store millions of strings.

szuecs · 2022-06-08T10:23:06Z

docs/reference/filters.md

+
+Block a request based on it's body content.
+
+The filter max request body is 2MiB by default and can be overidden with `-max-matcher-buffer-size=<int>`.


The sentence is not correct because it is only using a 2MiB buffer as default and does not stop processing the body.

szuecs · 2022-06-08T10:23:57Z

docs/reference/filters.md

+
+Parameters:
+
+* toblockList (List of []bytes)


It's a list of string instead of list of []byte from the user perspective, which is what we document here.

szuecs · 2022-06-08T10:24:44Z

docs/reference/filters.md

+Example:
+
+```
+* -> block("maliciousCotent") -> "http://example.com";


maliciousCotent -> malicious Content

szuecs · 2022-06-08T10:32:11Z

filters/block/matcher.go

+	return n, nil
+}
+
+// Closes closes the undelrying reader if it implements io.Closer.


godoc: Closes -> Close first has to match the func name

Also tell in the godoc that it's not safe for concurrent use and it is reentrant.

szuecs · 2022-06-08T10:32:35Z

filters/block/matcher.go

+	ready   *bytes.Buffer
+	pending *bytes.Buffer
+
+	// metrices:


drop this comment

szuecs · 2022-06-08T10:32:50Z

filters/block/matcher.go

+//
+type matcher struct {
+
+	// init:


drop this comment

szuecs · 2022-06-08T10:32:55Z

filters/block/matcher.go

+	// metrices:
+	metrics metrics.Metrics
+
+	// final:


drop this comment

szuecs · 2022-06-08T10:34:36Z

filters/block/matcher.go

+
+	if m.err == proxy.ErrBlocked {
+		m.metrics.IncCounter("blocked.requests")
+		log.Errorf("Content blocked: %v", proxy.ErrBlocked)


We should not log here, because it's more the library part.

szuecs · 2022-06-08T10:34:55Z

filters/block/matcher.go

+
+		if err == proxy.ErrBlocked {
+			m.metrics.IncCounter("blocked.requests")
+			log.Errorf("Content blocked: %v", proxy.ErrBlocked)


same here drop the log

szuecs · 2022-06-08T10:39:20Z

skipper.go

@@ -774,6 +775,10 @@ type Options struct {
 	// MaxAuditBody sets the maximum read size of the body read by the audit log filter
 	MaxAuditBody int

+	// MaxMatcherBufferSize sets the maximum read size of the body read by the block filter


I would say:

MaxMatcherBufferSize sets the maximum read buffer size of the block filter defaults to 2MiB in case of a zero value

MustafaSaber · 2022-08-29T10:22:39Z

👍

Signed-off-by: Sandor Szücs <sandor.szuecs@zalando.de>

Make test pass when err is expected Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

Return the int output of the reader if the error was block otherwise it implies failing in reading. Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

Support having chunks of data for the same request. Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

Passing valid request without reading extra nil chunks of data. Blocking non valid requests without going into infinite loops. Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

Move block entrypoint with with a global flag in config to control max body size. Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

Change string to []bytes for better memory utilization on the long term. Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

Update docs & godocs. Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

MustafaSaber · 2022-08-29T10:44:46Z

👍

szuecs · 2022-08-29T16:13:09Z

filters/block/matcher.go

+
+// Close closes the undelrying reader if it implements io.Closer.
+// Not safe for concurrent usage and it's reentrant.
+func (m *matcher) Close() error {


We could use sync.Once to only run this once. see also: https://github.com/zalando/skipper/blob/master/filters/shedder/admission.go#L324-L329

docs/reference/filters.md

Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

szuecs · 2022-08-29T17:18:54Z

👍

MustafaSaber · 2022-08-29T18:02:15Z

👍

MustafaSaber force-pushed the feature/body-filtering-v2 branch from c9bc52a to 2a12c3e Compare May 10, 2022 14:36

MustafaSaber requested review from AlexanderYastrebov and szuecs May 10, 2022 15:03

MustafaSaber self-assigned this May 10, 2022

MustafaSaber marked this pull request as ready for review May 18, 2022 13:23

szuecs reviewed May 18, 2022

View reviewed changes

szuecs reviewed May 19, 2022

View reviewed changes

szuecs reviewed Jun 8, 2022

View reviewed changes

filters/block/matcher.go Outdated

ready *bytes.Buffer

pending *bytes.Buffer

// metrices:

Copy link

Member

szuecs Jun 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

drop this comment

szuecs reviewed Jun 8, 2022

View reviewed changes

filters/block/matcher.go Outdated

//

type matcher struct {

// init:

Copy link

Member

szuecs Jun 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

drop this comment

szuecs reviewed Jun 8, 2022

View reviewed changes

filters/block/matcher.go Outdated

// metrices:

metrics metrics.Metrics

// final:

Copy link

Member

szuecs Jun 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

drop this comment

szuecs reviewed Jun 8, 2022

View reviewed changes

szuecs assigned AlexanderYastrebov Jun 16, 2022

szuecs added the ready-for-review label Aug 1, 2022

szuecs and others added 8 commits August 29, 2022 12:25

simple body filtering filter

9d24b83

Signed-off-by: Sandor Szücs <sandor.szuecs@zalando.de>

chore: fix body test

dab7c65

Make test pass when err is expected Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

fix: matchbody return

b901624

Return the int output of the reader if the error was block otherwise it implies failing in reading. Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

feat: Add benchmarks for bodyMatch

13d29a0

Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

feat: Init a new bodyMatch filter implementation

c5f44e1

Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

chore: update block filter with mater utilites

8d0346a

Support having chunks of data for the same request. Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

fix: reading empty buffer and infinite loop

9e0ee4e

Passing valid request without reading extra nil chunks of data. Blocking non valid requests without going into infinite loops. Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

chore: checking testcases after refactor

d81ae00

Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

MustafaSaber added 21 commits August 29, 2022 12:25

chore: use different tests styles

f5adf83

Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

chore: change test to MiB numbers

a1e994d

Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

chore: clean old filter code

2850b0e

Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

chore: update matcher go docs

5693a96

Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

fix: tests variable renaming

188e984

Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

chore: update regular docs

371bbe1

Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

chore: change variable name for better ctx

e4bbb09

Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

chore: return constants to matcher

3c42a1c

Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

chore: continue cleaning bodyMatch filter

3a5a4ee

Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

fix: builtin after cleaning old code.

8567ec3

Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

chore: change block filter entrypoint

6ad0529

Move block entrypoint with with a global flag in config to control max body size. Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

chore: update config & block tests.

7f1651f

Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

chore: remove matcher tests.

0fa0cfb

Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

fix: block tests types

73e4ec9

Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

feat: add metrics to blockContent filter

1cb3889

Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

chore: change the way we handle content

1e52306

Change string to []bytes for better memory utilization on the long term. Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

chore: rename block filter to blockContent for better readability

9c95338

Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

chore: update docs

80effd9

Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

chore: catch metrics in all right places.

62855c3

Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

chore: move blocked error to proxy for better http error handling.

748c915

Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

chore: address review comments

4eecf33

Update docs & godocs. Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

MustafaSaber force-pushed the feature/body-filtering-v2 branch from 4db42ca to 4eecf33 Compare August 29, 2022 10:25

szuecs reviewed Aug 29, 2022

View reviewed changes

docs/reference/filters.md Outdated Show resolved Hide resolved

make close safe for concurrency

5eae265

Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

MustafaSaber merged commit 92ba2ed into master Aug 29, 2022

MustafaSaber deleted the feature/body-filtering-v2 branch August 29, 2022 18:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Content based block filter #2002

Content based block filter #2002

MustafaSaber commented May 9, 2022 •

edited

Loading

szuecs May 18, 2022

szuecs May 19, 2022

szuecs May 18, 2022

szuecs May 19, 2022

szuecs May 19, 2022

szuecs May 19, 2022

szuecs Jun 8, 2022

szuecs Jun 8, 2022

szuecs Jun 8, 2022

szuecs Jun 8, 2022

szuecs Jun 8, 2022 •

edited

Loading

szuecs Jun 8, 2022

szuecs Jun 8, 2022

szuecs Jun 8, 2022

szuecs Jun 8, 2022

szuecs Jun 8, 2022

szuecs Jun 8, 2022

MustafaSaber commented Aug 29, 2022

MustafaSaber commented Aug 29, 2022

szuecs Aug 29, 2022 •

edited

Loading

szuecs commented Aug 29, 2022

MustafaSaber commented Aug 29, 2022


		Block a request based on it's body content.

		The filter max request body is 2MiB by default and can be overidden with `-max-matcher-buffer-size=<int>`.

Content based block filter #2002

Content based block filter #2002

Conversation

MustafaSaber commented May 9, 2022 • edited Loading

Intro

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szuecs Jun 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MustafaSaber commented Aug 29, 2022

MustafaSaber commented Aug 29, 2022

szuecs Aug 29, 2022 • edited Loading

Choose a reason for hiding this comment

szuecs commented Aug 29, 2022

MustafaSaber commented Aug 29, 2022

MustafaSaber commented May 9, 2022 •

edited

Loading

szuecs Jun 8, 2022 •

edited

Loading

szuecs Aug 29, 2022 •

edited

Loading