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.

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

appsec: support server.response.headers.no_cookies WAF address #2347

Merged
merged 2 commits into from Nov 22, 2023

Conversation

eliottness
Copy link
Contributor

@eliottness eliottness commented Nov 13, 2023

What does this PR do?

Adds a field to HandlerOperationRes in appsec/internal/dyngo/instrumentation/httpsec/http.go for response headers and send them to the WAF in appsec/internal/waf.go

Motivation

API Security is around the corner and we need to build support schemas for user responses. As such, two new addresses were created:

  • server.response.headers.no_cookies
  • server.response.body

This PR does half the job since the second address will be tricker to implement.

Reviewer's Checklist

  • Changed code has unit tests for its functionality at or near 100% coverage.
  • 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.

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!

@eliottness eliottness changed the base branch from main to romain.marcadier/waf-upgrade-ephemeral/APPSEC-12062 November 13, 2023 14:02
@pr-commenter
Copy link

pr-commenter bot commented Nov 13, 2023

Benchmarks

Benchmark execution time: 2023-11-22 09:51:29

Comparing candidate commit e15864a in PR branch eliott.bouhana/APPSEC-11880/response-no-cookies with baseline commit 9cf50b3 in branch main.

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

@eliottness eliottness force-pushed the eliott.bouhana/APPSEC-11880/response-no-cookies branch from 1ac6f7b to fa0edaa Compare November 14, 2023 10:51
@eliottness eliottness force-pushed the eliott.bouhana/APPSEC-11880/response-no-cookies branch 2 times, most recently from 3addde9 to f911c59 Compare November 14, 2023 12:36
@eliottness eliottness force-pushed the romain.marcadier/waf-upgrade-ephemeral/APPSEC-12062 branch from dbaa2fc to 2aca4bc Compare November 14, 2023 14:12
Base automatically changed from romain.marcadier/waf-upgrade-ephemeral/APPSEC-12062 to main November 14, 2023 14:27
@eliottness
Copy link
Contributor Author

@eliottness eliottness force-pushed the eliott.bouhana/APPSEC-11880/response-no-cookies branch from f911c59 to 3502eeb Compare November 17, 2023 16:55
@eliottness eliottness marked this pull request as ready for review November 17, 2023 17:17
@eliottness eliottness requested a review from a team as a code owner November 17, 2023 17:17
@eliottness eliottness force-pushed the eliott.bouhana/APPSEC-11880/response-no-cookies branch from 3502eeb to 9dd83eb Compare November 20, 2023 09:09
@eliottness eliottness enabled auto-merge (squash) November 20, 2023 10:47
Copy link
Contributor

@Hellzy Hellzy left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Signed-off-by: Eliott Bouhana <eliott.bouhana@datadoghq.com>
Signed-off-by: Eliott Bouhana <eliott.bouhana@datadoghq.com>
@eliottness eliottness force-pushed the eliott.bouhana/APPSEC-11880/response-no-cookies branch from 9dd83eb to e15864a Compare November 22, 2023 09:35
@eliottness eliottness merged commit c418382 into main Nov 22, 2023
344 checks passed
@eliottness eliottness deleted the eliott.bouhana/APPSEC-11880/response-no-cookies branch November 22, 2023 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants