New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

appsec: suspicious request blocking #1797

Merged

Julio-Guerra merged 31 commits into main from francois.mazeau/suspicious-request-blocking

Apr 13, 2023

Contributor

Hellzy commented Mar 14, 2023 •

edited

What does this PR do?

This change adds the capability to block any request by providing users with the ability to update the security
rules configs through remote configuration (ASM_DD, ASM and ASM_DATA products).

Motivation

Describe how to test/QA your changes

Reviewer's Checklist

If known, an appropriate milestone has been selected; otherwise the Triage milestone is set.
Changed code has unit tests for its functionality.
If this interacts with the agent in a new way, a system test has been added.

Hellzy added the appsec label

Hellzy force-pushed the francois.mazeau/suspicious-request-blocking branch from 41cee28 to 9cc1df5 Compare

March 23, 2023 09:21

pr-commenter bot commented Mar 23, 2023 •

edited

Benchmarks

Comparing candidate commit 70543c9 in PR branch francois.mazeau/suspicious-request-blocking with baseline commit 8251480 in branch main.

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

scenario:BenchmarkConcurrentTracing-24

🟥 execution_time [+0.134ms; +0.159ms] or [+11.305%; +13.468%]

Hellzy force-pushed the francois.mazeau/suspicious-request-blocking branch 2 times, most recently from e71ef60 to 05166dc Compare

March 28, 2023 13:48

Hellzy added this to the v1.50.0 milestone

Hellzy force-pushed the francois.mazeau/suspicious-request-blocking branch 3 times, most recently from 7221cd8 to fa5aef4 Compare

April 3, 2023 13:25

Hellzy force-pushed the francois.mazeau/suspicious-request-blocking branch 2 times, most recently from 0fffa72 to b37d797 Compare

April 5, 2023 13:29

Hellzy marked this pull request as ready for review

April 7, 2023 08:50

Hellzy requested review from a team as code owners

April 7, 2023 08:50

Hellzy changed the title ~~[WIP] appsec: suspicious request blocking~~ appsec: suspicious request blocking

Julio-Guerra requested changes

View reviewed changes

go.mod Show resolved Hide resolved

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

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

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

internal/appsec/remoteconfig.go Show resolved Hide resolved

internal/appsec/remoteconfig.go Show resolved Hide resolved

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

internal/remoteconfig/remoteconfig.go Show resolved Hide resolved

internal/remoteconfig/remoteconfig.go Outdated Show resolved Hide resolved

internal/remoteconfig/remoteconfig_test.go Outdated Show resolved Hide resolved

Hellzy force-pushed the francois.mazeau/suspicious-request-blocking branch 7 times, most recently from e906e86 to 6735a84 Compare

April 12, 2023 13:50

Julio-Guerra reviewed

View reviewed changes

internal/appsec/remoteconfig.go Show resolved Hide resolved

internal/appsec/remoteconfig.go Show resolved Hide resolved

Julio-Guerra requested changes

View reviewed changes

Contributor

Julio-Guerra left a comment

Our system tests CI in dd-trace-go doesn't run the APPSEC_REQUEST_BLOCKING scenario. Did you run it locally? Should we add it in our CI (along with all the others)?

internal/appsec/remoteconfig.go Show resolved Hide resolved

go.mod Outdated Show resolved Hide resolved

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

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

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

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

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

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

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

internal/appsec/remoteconfig_test.go Show resolved Hide resolved

Hellzy force-pushed the francois.mazeau/suspicious-request-blocking branch from cc5abf5 to a0e1a7c Compare

April 13, 2023 09:15

Contributor Author

Hellzy commented Apr 13, 2023

Our system tests CI in dd-trace-go doesn't run the APPSEC_REQUEST_BLOCKING scenario. Did you run it locally? Should we add it in our CI (along with all the others)?

Yes on both accounts.

Hellzy force-pushed the francois.mazeau/suspicious-request-blocking branch 2 times, most recently from 5fb6f18 to 7c83dc9 Compare

April 13, 2023 11:22


          internal/remoteconfig: add ASM request/response blocking capabilities

Hellzy and others added 8 commits

April 13, 2023 13:24


          internal/appsec: review refactor

fb47438

- Add debug log (some remain to be done)
- Rename rulesetFragment to rulesFragment
- Rename ruleset to rulesManager
- Rename ASMFeaturesCallback to handleASMFeatures
- Fix bug in umbrella callback testing
- Use a temporary rulesManager when and update cfg.rulesManager only
when swapping the WAF is successful


          Apply suggestions from code review

58f4daa

Co-authored-by: Julio Guerra <julio@datadog.com>


          internal/remoteconfig: merge status updates together

f0870f5


          internal/appsec/remoteconfig: only apply configs if no error on all u…

4b43af2

…pdates


          internal/appsec: store mergeMaps result in first param

637604b


          internal/appsec: add apply status rc tests

57acea8


          go.mod: upgrade go-libbdwaf to v1.1.0

4a4547c


          internal/appsec: add e2e waf update test

e8d7a89

Hellzy force-pushed the francois.mazeau/suspicious-request-blocking branch 2 times, most recently from 4c6eb73 to f4c9ec8 Compare

April 13, 2023 12:04


          internal/appsec: review patch

- Update exported capabilities
- Split ASM_FEATURES handling appart from rules produts handling
- Simplify rules-toggle test
- Update rulesManager with RC configs in a separate function for easier
  testing

Hellzy force-pushed the francois.mazeau/suspicious-request-blocking branch from f4c9ec8 to 6017257 Compare

April 13, 2023 12:05

Julio-Guerra previously approved these changes

View reviewed changes

Contributor

Julio-Guerra left a comment

Congratulations 🛡️

Hellzy dismissed Julio-Guerra’s stale review via

b863a59

April 13, 2023 12:43

Hellzy requested a review from Julio-Guerra

April 13, 2023 12:43


          internal/appsec: add test for rules update after appsec deactivation

f047103

Hellzy force-pushed the francois.mazeau/suspicious-request-blocking branch from b863a59 to f047103 Compare

April 13, 2023 12:46

Julio-Guerra approved these changes

View reviewed changes

Hellzy enabled auto-merge (squash)

April 13, 2023 13:32

Hellzy disabled auto-merge

April 13, 2023 13:37


          Merge branch 'main' into francois.mazeau/suspicious-request-blocking

01f41d0

Julio-Guerra requested changes

View reviewed changes

Contributor

Julio-Guerra left a comment

let's check what this perf regression is about

Contributor Author

Hellzy commented Apr 13, 2023

let's check what this perf regression is about

Seems to be unrelated when. Running the benchmark locally shows no call to appsec or the remote configuration client (internal/appsec, internal/remoteconfig)

Hellzy requested a review from Julio-Guerra

April 13, 2023 15:20


          Merge branch 'main' into francois.mazeau/suspicious-request-blocking

70543c9

Julio-Guerra approved these changes

View reviewed changes

Julio-Guerra merged commit 1cfcf4f into main

272 of 273 checks passed

Julio-Guerra deleted the francois.mazeau/suspicious-request-blocking branch

April 13, 2023 19:34

eliottness mentioned this pull request

appsec: add blocking SDK body operation #1901

Merged

3 tasks

zARODz11z added a commit that referenced this pull request


          contrib/aws/aws-sdk-go-v2: add aws_service, region, <resourcename> su…

93310c3

…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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment