Skip to content
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/api-security: http request schema collection and sensitive data scanning #2381

Merged
merged 23 commits into from
Dec 12, 2023
Merged

appsec/api-security: http request schema collection and sensitive data scanning #2381

merged 23 commits into from
Dec 12, 2023

Conversation

Hellzy
Copy link
Contributor

@Hellzy Hellzy commented Nov 24, 2023 •
edited
Loading

What does this PR do?

  • Ugrade to go-libddwaf v2.2.0
  • Upgrade to appsec-internal-go v1.4.0
  • Security rules v1.10.0
  • Switch to shared config from appsec-internal-go
  • Extract request schemas and add them to tags if API Security config allows it
  • Handle serializable tags in tagsholder, which get marshaled to JSON strings when calling Tags()
  • Add processors and scanners entries to rules manager
  • Run APPSEC_API_SECURITY system-tests scenario

Motivation

Reviewer's Checklist

  • Changed code has unit tests for its functionality at or near 100% coverage.
  • System-Tests covering this feature have been added and enabled with the va.b.c-dev version tag.
  • There is a benchmark for any new code, or changes to existing code.
  • If this interacts with the agent in a new way, a system test has been added.
  • Add an appropriate team label so this PR gets put in the right place for the release notes.

For Datadog employees:

  • If this PR touches code that handles credentials of any kind, such as Datadog API keys, I've requested a review from @DataDog/security-design-and-guidance.
  • This PR doesn't touch any of that.

Unsure? Have a question? Request a review!

@Hellzy Hellzy requested review from a team as code owners November 24, 2023 15:25
@pr-commenter
Copy link

pr-commenter bot commented Nov 24, 2023 •
edited
Loading

Benchmarks

Benchmark execution time: 2023-12-12 09:49:00

Comparing candidate commit 9aa2e40 in PR branch francois.mazeau/apisec/APPSEC-11884 with baseline commit aada6eb in branch main.

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

scenario:BenchmarkSingleSpanRetention/no-rules-24

  • 🟥 execution_time [+7.393µs; +8.731µs] or [+2.998%; +3.540%]

scenario:BenchmarkSingleSpanRetention/with-rules/match-all-24

  • 🟥 execution_time [+6.958µs; +8.949µs] or [+2.794%; +3.593%]

scenario:BenchmarkSingleSpanRetention/with-rules/match-half-24

  • 🟥 execution_time [+7.483µs; +9.038µs] or [+3.004%; +3.629%]

@Hellzy Hellzy changed the title appsec: HTTP schema extraction for API Security [Do not merge] appsec: HTTP schema extraction for API Security Nov 24, 2023
@Hellzy Hellzy marked this pull request as draft November 24, 2023 16:19
@Hellzy Hellzy force-pushed the francois.mazeau/apisec/APPSEC-11884 branch from a0f4b9f to 1e1d3ff Compare November 24, 2023 16:30
@Hellzy Hellzy force-pushed the francois.mazeau/apisec/APPSEC-11884 branch 4 times, most recently from bb3eb6f to 1e9e091 Compare December 6, 2023 11:30
@Hellzy Hellzy changed the title [Do not merge] appsec: HTTP schema extraction for API Security appsec: HTTP schema extraction for API Security Dec 6, 2023
@Hellzy Hellzy marked this pull request as ready for review December 7, 2023 08:47
@Hellzy Hellzy force-pushed the francois.mazeau/apisec/APPSEC-11884 branch from f977c1d to 379fed1 Compare December 7, 2023 09:07
@Hellzy
Copy link
Contributor Author

Hellzy commented Dec 7, 2023

System-tests workflow: https://github.com/DataDog/dd-trace-go/actions/runs/7126006639
Using DataDog/system-tests#1870

RomainMuller
RomainMuller requested changes Dec 7, 2023
View reviewed changes
internal/apps/unit-of-work/go.sum Outdated Show resolved Hide resolved
internal/appsec/listener/httpsec/http.go Outdated Show resolved Hide resolved
internal/appsec/listener/httpsec/http.go Outdated Show resolved Hide resolved
internal/appsec/listener/httpsec/http.go Outdated Show resolved Hide resolved
internal/appsec/listener/httpsec/http.go Outdated Show resolved Hide resolved
internal/appsec/listener/sharedsec/shared.go Outdated Show resolved Hide resolved
internal/appsec/waf_test.go Outdated Show resolved Hide resolved
internal/appsec/waf_test.go Show resolved Hide resolved
internal/appsec/waf_test.go Outdated Show resolved Hide resolved
@Hellzy Hellzy force-pushed the francois.mazeau/apisec/APPSEC-11884 branch 2 times, most recently from 156e93e to b6f411c Compare December 7, 2023 11:06
@Hellzy Hellzy force-pushed the francois.mazeau/apisec/APPSEC-11884 branch from b6f411c to b30e192 Compare December 7, 2023 14:36
Julio-Guerra
Julio-Guerra requested changes Dec 8, 2023
View reviewed changes
Copy link
Contributor

@Julio-Guerra Julio-Guerra left a comment

Choose a reason for hiding this comment

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

Good job.
I did a couple of suggestions. Only one is major: the use of AddAPISecurityTags in the request path instead of being at the end of the request like we do for events.

internal/appsec/config.go Outdated Show resolved Hide resolved
internal/appsec/listener/httpsec/http.go Show resolved Hide resolved
internal/appsec/appsec.go Outdated Show resolved Hide resolved
internal/appsec/remoteconfig_test.go Outdated Show resolved Hide resolved
internal/appsec/listener/httpsec/http.go Show resolved Hide resolved
internal/appsec/listener/sharedsec/shared.go Outdated Show resolved Hide resolved
internal/appsec/remoteconfig.go Outdated Show resolved Hide resolved
@Hellzy Hellzy force-pushed the francois.mazeau/apisec/APPSEC-11884 branch 2 times, most recently from d4d356c to 8816481 Compare December 11, 2023 14:54
Hellzy and others added 15 commits December 11, 2023 17:05
@Hellzy
ci: run appsec API security system-tests scenario
8d5c1d5
@Hellzy
internal/appsec: serialize API Sec schemas
e8c63ef
@Hellzy
internal/appsec: add SDK body schema test
7648d3f
@Hellzy
internal/appsec: add waf unit test for schema collection
7907531
@Hellzy
internal/appsec: simplify WAF addresses handling
0794412
@Hellzy @RomainMuller
Apply suggestions from code review
0a92cab 
Co-authored-by: Romain Marcadier <romain@datadoghq.com>
@Hellzy
internal/appsec: force appsec enabled for API Sec test
e9538b9
@Hellzy
go.mod: appsec-internal-go@v1.4.0
c8b0c11
@Hellzy
appsec/internal: test tags values for API Sec
8f9b811
@Hellzy
internal/appsec: set logger backend
8bb502a
@Hellzy
Add serializable tags to tag holder
6b24780
@Hellzy
appsec: log: use fmt.Errorf
390d3cd
@Hellzy
appsec: export enablement env var
a8a966f
@Hellzy
appsec: clarify schema generation with a comment
ca0b461
@Hellzy
tagholder: add serializableTag wrapper
5402395
@Hellzy Hellzy force-pushed the francois.mazeau/apisec/APPSEC-11884 branch from cf59a5a to 5402395 Compare December 11, 2023 16:11
@Hellzy Hellzy merged commit 2c3a644 into main Dec 12, 2023
185 of 235 checks passed
@Hellzy Hellzy deleted the francois.mazeau/apisec/APPSEC-11884 branch December 12, 2023 13:32
@Julio-Guerra Julio-Guerra changed the title appsec: HTTP schema extraction for API Security appsec/api-security: http request schema collection and sensitive data scanning Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Julio-Guerra Julio-Guerra Julio-Guerra approved these changes

@RomainMuller RomainMuller RomainMuller approved these changes

Assignees
No one assigned
Labels
appsec
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Hellzy @Julio-Guerra @RomainMuller