appsec: add blocking SDK body operation #1901

eliottness · 2023-04-14T09:15:54Z

What does this PR do?

This PR adds the http sdk body blocking feature the feature is divided as follows:

Modifying the appsec api: adding appsec.MonitorParsedHTTPBody an error as return value
Adding a call to the WAF to check for security event synchronously with a call to appsec.MonitorParsedHTTPBody on the body passed as parameter
Removing the call to the WAF done on the body an the end of a request becaused we moved it.

Motivation

This feature is planed with ASM_BLOCKING and follows suspicious request blocking (#1797) PR.

Describe how to test/QA your changes

There is no recommended rule which blocks on the body, so I added a rule from the recommended ruleset in the blocking.json ruleset test data.

Reviewer's Checklist

Maybe wait for the sytem-tests SRB PR to be merged and is passing before merging this
Changed code has unit tests for its functionality.
If this interacts with the agent in a new way, a system test has been added.

pr-commenter · 2023-04-14T09:27:22Z

Benchmarks

Comparing candidate commit eec0e1c in PR branch eliott.bouhana/sdk-body-blocking with baseline commit a48dc66 in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 18 metrics, 0 unstable metrics.

Hellzy

Nice job. A few things to fix below

internal/appsec/waf.go

internal/appsec/waf_test.go

internal/appsec/dyngo/instrumentation/sharedsec/shared.go

internal/appsec/waf.go

internal/appsec/dyngo/instrumentation/httpsec/http.go

Julio-Guerra

Somes [important] nits

Signed-off-by: Eliott Bouhana <eliott.bouhana@epita.fr>

Signed-off-by: Eliott Bouhana <eliott.bouhana@datadoghq.com>

Julio-Guerra · 2023-04-17T19:13:00Z

internal/appsec/waf.go

-				for _, id := range actionIds {
-					if actionHandler.Apply(id, op) {
-						operation.Error = sharedsec.NewUserMonitoringError("Request blocked")
+		if _, ok := httpAddresses[userIDAddr]; ok {


The test is done on the global list of HTTP addresses rather than the local list of addresses present in the rules addresses.
Same comment on the other conditions you added.

Feel free to revive addressesContains to simplify the if a bit.

Signed-off-by: Eliott Bouhana <eliott.bouhana@datadoghq.com>

#1901 adds the http sdk body blocking feature the feature is divided as follows: * Modifying the appsec api: adding appsec.MonitorParsedHTTPBody an error as return value * Adding a call to the WAF to check for security event synchronously with a call to appsec.MonitorParsedHTTPBody on the body passed as parameter * Removing the call to the WAF done on the body an the end of a request because we moved it. * Refactoring the waf addresses storage and access Signed-off-by: Eliott Bouhana <eliott.bouhana@datadoghq.com>

Signed-off-by: Eliott Bouhana <eliott.bouhana@datadoghq.com> Co-authored-by: Eliott Bouhana <47679741+eliottness@users.noreply.github.com>

…ch as queuename tags contrib: upgrade labstack/echo/v4 from v4.2.0 to v4.9.0 (#1891) ci: fix flaky lint job (#1892) contrib/elasticsearch: use naming schema (#1897) ci: introduce golangci (#1898) appsec: suspicious request blocking (#1797) Co-authored-by: Julio Guerra <julio@datadog.com> ci/golangci-lint: skip google.golang.org/grpc.v12 (#1899) .github/workflows: run ASM and RC system-tests scenarios (#1900) contrib/hashicorp/vault: use naming schema (#1868) contrib/database/sql: add WithIgnoreQueryTypes option (#1823) Co-authored-by: Zarir Hamza <zarir.hamza@datadoghq.com> Co-authored-by: Rodrigo Argüello <rodrigo.arguello@datadoghq.com> contrib/database/sql: use naming schema (#1895) internal/appsec: add server.request.method address (#1893) Signed-off-by: Eliott Bouhana <eliott.bouhana@datadoghq.com> Co-authored-by: François Mazeau <francois.mazeau@datadoghq.com> internal/appsec/dyngo: atomic instrumentation swapping (#1873) Co-authored-by: François Mazeau <francois.mazeau@datadoghq.com> go.mod: datadog-agent/pkg/remoteconfig/state 7.45.0-rc.1 (#1902) internal/version: bump to v1.51.0 (#1912) ddtrace/tracer: don't set empty tracestate propagation tag (#1910) go.mod: github.com/DataDog/datadog-agent/pkg/obfuscate 7.45.0-rc.1 (#1916) appsec: add blocking SDK body operation (#1901) * Modifying the appsec api: adding appsec.MonitorParsedHTTPBody an error as return value * Adding a call to the WAF to check for security event synchronously with a call to appsec.MonitorParsedHTTPBody on the body passed as parameter * Removing the call to the WAF done on the body an the end of a request because we moved it. * Refactoring the waf addresses storage and access Signed-off-by: Eliott Bouhana <eliott.bouhana@datadoghq.com> ddtrace/{opentelemetry,opentracer}: add telemetry (#1909) internal/appsec: fix user ID event detection (#1918) internal/telemetry: track tracer init time metric (#1896) Co-authored-by: Andrew Glaude <andrew.glaude@datadoghq.com> internal/appsec/remoteconfig: fix rules overrides (#1921) contrib/mongodb: use naming schema (#1908) contrib/syndtr/goleveldb/leveldb: use naming schema (#1914) contrib/tidwall/buntdb: use naming schema (#1913) internal/appsec: do not ignore the appsec events rate limiter (#1927) remoteconfig: remove empty products and don't override appsec rules data (#1925) contrib/kafka: refactor tests (#1907) contrib/google.golang.org/grpc: use naming schema (#1919) contrib/twitchtv/twirp: use naming schema (#1920) contrib/http: use naming schema (#1929) ddtrace/tracer: reset decision maker during fallback behavior of w3c header extraction (#1933) contrib/cassandra: use naming schema (#1911) Co-authored-by: Diana Shevchenko <40775148+dianashevchenko@users.noreply.github.com> contrib/redis: use naming schema (#1906) Co-authored-by: Andrew Glaude <andrew.glaude@datadoghq.com> ci/system-tests: more scenarios with parallel jobs (#1938) ci: update linter job and add bodyclose (#1942) contrib/redis/go-redis.v9: support v9 (#1730) Add support for new go-redis version v9. It does 2 things: Copy existing version 8 files to a new path, /redis/go-redis.v9. Make changes to support version 9. Fixes #1710 format and rerun go tidy get rid of prints add topLevelRegion assertions remove confusing named return values and todo comment ddtrace/tracer: ensure access to trace tags is concurrency-safe (#1948) Spancontext marshaling was accessing tracer internal structures without a lock, resulting in a data race and panic. This commit adds a few methods to trace to allow safe access to the tags and propagatingTags members of trace to the marshaling code. Fixes #1944 ddtrace/tracer: mark context updated when SetUser is called (#1949) Fixes a minor logic mistake when setting a user on a span lint and add default switch case refactor resourceNameKey and value assignments restructure functions to be left aligned use internal logger, be less verbose with function names go back to normal switch type and format Set keyTraceID128 on first span in the chunk only (#1946) go.mod: upgrade go-libddwaf to v1.2.0 (#1953) Co-authored-by: Julio Guerra <julio@datadog.com> contrib/database/sql: fix bug where options were always overwritten by register options (#1904) Co-authored-by: Diana Shevchenko <40775148+dianashevchenko@users.noreply.github.com> ci/smoke-tests: update the go.sum file after go get -u (#1957) contrib/net/http: don't set empty string values as span tags (#1956) Do not set span fields when they are not configured so the tracer can put the defaults in. use normal string then derefence rever go.mod and go.sum changes contrib/internal/httptrace: remove naming schema from init (#1960) contrib/graphql: use naming schema (#1926) internal/telemetry: trim the dependencies version prefix v (#1963) contrib/aws: use naming schema (#1931) contrib/cloud.google.com/go/pubsub.v1: use naming schema (#1937) go mod tidy lint and fix test

eliottness requested a review from a team as a code owner April 14, 2023 09:15

eliottness force-pushed the eliott.bouhana/sdk-body-blocking branch from 63dcac0 to fbb83dc Compare April 14, 2023 09:20

eliottness force-pushed the eliott.bouhana/sdk-body-blocking branch from fbb83dc to 9593c68 Compare April 14, 2023 09:28

Hellzy suggested changes Apr 14, 2023

View reviewed changes

eliottness force-pushed the eliott.bouhana/sdk-body-blocking branch 2 times, most recently from f2be3c1 to 8920ed1 Compare April 14, 2023 11:48

eliottness requested a review from Hellzy April 14, 2023 11:52

eliottness force-pushed the eliott.bouhana/sdk-body-blocking branch 3 times, most recently from afb922f to 515fa77 Compare April 14, 2023 14:54

Hellzy previously approved these changes Apr 17, 2023

View reviewed changes

Julio-Guerra reviewed Apr 17, 2023

View reviewed changes

internal/appsec/waf.go Outdated Show resolved Hide resolved

Julio-Guerra reviewed Apr 17, 2023

View reviewed changes

internal/appsec/waf.go Outdated Show resolved Hide resolved

Julio-Guerra reviewed Apr 17, 2023

View reviewed changes

internal/appsec/dyngo/instrumentation/httpsec/http.go Show resolved Hide resolved

Julio-Guerra requested changes Apr 17, 2023

View reviewed changes

eliottness dismissed Hellzy’s stale review via 2c85850 April 17, 2023 14:37

eliottness force-pushed the eliott.bouhana/sdk-body-blocking branch 3 times, most recently from baf4b88 to 9c553e8 Compare April 17, 2023 15:10

Hellzy previously approved these changes Apr 17, 2023

View reviewed changes

eliottness enabled auto-merge (squash) April 17, 2023 15:21

eliottness and others added 2 commits April 17, 2023 17:27

appsec: add blocking SDK body operation

d896c9d

Signed-off-by: Eliott Bouhana <eliott.bouhana@epita.fr>

appsec: add sync.Once warn log is appsec is disabled and add doc

8b2f059

Signed-off-by: Eliott Bouhana <eliott.bouhana@datadoghq.com>

eliottness force-pushed the eliott.bouhana/sdk-body-blocking branch from 9c553e8 to 20bc06a Compare April 17, 2023 15:27

eliottness requested a review from Julio-Guerra April 17, 2023 16:06

Julio-Guerra requested changes Apr 17, 2023

View reviewed changes

eliottness dismissed Hellzy’s stale review via 53f34f1 April 18, 2023 09:44

eliottness force-pushed the eliott.bouhana/sdk-body-blocking branch 2 times, most recently from 53f34f1 to d2a105b Compare April 18, 2023 09:50

eliottness force-pushed the eliott.bouhana/sdk-body-blocking branch from d2a105b to 60de475 Compare April 18, 2023 09:51

internal/appsec: refactor waf addresses: []string -> map[string]struct{}

Unverified

This user has not yet uploaded their public signing key.

Learn about vigilant mode

0d9e844

Signed-off-by: Eliott Bouhana <eliott.bouhana@datadoghq.com>

eliottness force-pushed the eliott.bouhana/sdk-body-blocking branch from 60de475 to 0d9e844 Compare April 18, 2023 11:58

Merge branch 'main' into eliott.bouhana/sdk-body-blocking

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

eec0e1c

Julio-Guerra added the appsec label Apr 18, 2023

Julio-Guerra approved these changes Apr 18, 2023

View reviewed changes

eliottness merged commit 3452a55 into main Apr 18, 2023

eliottness deleted the eliott.bouhana/sdk-body-blocking branch April 18, 2023 12:15

This was referenced Apr 18, 2023

appsec: add blocking SDK body operation (#1901) #1917

Merged

internal/appsec: fix user ID event detection & other appsec tests #1918

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

appsec: add blocking SDK body operation #1901

appsec: add blocking SDK body operation #1901

eliottness commented Apr 14, 2023

pr-commenter bot commented Apr 14, 2023 •

edited

Loading

Hellzy left a comment

Julio-Guerra left a comment

Julio-Guerra Apr 17, 2023

appsec: add blocking SDK body operation #1901

appsec: add blocking SDK body operation #1901

Conversation

eliottness commented Apr 14, 2023

What does this PR do?

Motivation

Describe how to test/QA your changes

Reviewer's Checklist

pr-commenter bot commented Apr 14, 2023 • edited Loading

Benchmarks

Hellzy left a comment

Choose a reason for hiding this comment

Julio-Guerra left a comment

Choose a reason for hiding this comment

Julio-Guerra Apr 17, 2023

Choose a reason for hiding this comment

pr-commenter bot commented Apr 14, 2023 •

edited

Loading