appsec/graphql: add support for Threat Monitoring #2309

RomainMuller · 2023-10-30T12:42:15Z

What does this PR do?

Adds a new in-app WAF integration for GraphQL to dyngo. Also adds a new contrib for github.com/grapgql-go/graphql to ensure the integration is not unnecessarily middleware-dependent.

The graphql.server.all_resolvers address can currently only be sent to the WAF after all resolvers have executed, as doing otherwise would require re-parsing the query and matching fields to variables, which is unnecessarily expensive.

Blocking capabilities will require further work (possibly dynamic instrumentation) as the tracing hooks provided by both supported middleware fail to allow altering the response flow (e.g: returning an error instead of invoking the field resolvers).

Motivation

This provides in-app WAF support for GraphQL operations.

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!

Adds a new in-app WAF integration for GraphQL to `dyngo`. Also adds a new contrib for `github.com/grapgql-go/graphql` to ensure the integration is not unnecessarily middleware-dependent. The `graphql.server.all_resolvers` address can currently only be sent to the WAF after all resolvers have executed, as doing otherwise would require re-parsing the query and matching fields to variables, which is unnecessarily expensive. Blocking capabilities will require further work (possibly dynamic instrumentation) as the tracing hooks provided by both supported middleware fail to allow altering the response flow (e.g: returning an error instead of invoking the field resolvers).

contrib/graphql-go/graphql/graphql_test.go

contrib/graphql-go/graphql/graphql.go

internal/appsec/waf.go

contrib/graphql-go/graphql/graphql.go

…hql/APPSEC-11164

pr-commenter · 2023-10-30T13:06:54Z

Benchmarks

Benchmark execution time: 2023-12-19 10:16:39

Comparing candidate commit e0bb880 in PR branch romain.marcadier/graphql/APPSEC-11164 with baseline commit 337ded8 in branch main.

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

contrib/google.golang.org/grpc/stats_client.go

…OTel semantics

…hql/APPSEC-11164 # Conflicts: # go.mod # go.sum # internal/appsec/dyngo/instrumentation/common.go # internal/appsec/remoteconfig_test.go # internal/appsec/waf.go # internal/appsec/waf_unit_test.go

…hql/APPSEC-11164

Move dyngo listeners into per-protocol packages to make them easier to maintain and to improve overall separation of concerns within dyngo listener settings. This is an initial step in preparing for the addition of GraphQL support, and to make listener activation be contrib-driven instead of being unconditional.

…graphql/APPSEC-11164 # Conflicts: # internal/appsec/waf.go

github-actions · 2023-11-22T17:35:55Z

contrib/graph-gophers/graphql-go/graphql.go

 	}
 }

 // TraceField traces a GraphQL field access.
-func (t *Tracer) TraceField(ctx context.Context, _ string, typeName string, fieldName string, trivial bool, _ map[string]interface{}) (context.Context, trace.TraceFieldFinishFunc) {
+func (t *Tracer) TraceField(ctx context.Context, label string, typeName string, fieldName string, trivial bool, arguments map[string]interface{}) (context.Context, trace.FieldFinishFunc) {


🚫 [golangci] _{reported by reviewdog 🐶}
unused-parameter: parameter 'label' seems to be unused, consider removing or renaming it as _ (revive)

…ctor-dyngo # Conflicts: # internal/appsec/listener/util/util_test.go # internal/appsec/waf.go

Julio-Guerra

First pass done - I reviewed everything on the dyngo side of things. I have all the contribs left to review.

internal/appsec/listener/graphqlsec/graphql.go

Julio-Guerra · 2023-12-11T15:58:38Z

internal/appsec/emitter/graphqlsec/execution.go

+	if span == nil {
+		// The span may be nil (e.g: in case of GraphQL subscriptions with certian contribs)
+		span = trace.BlackHoleTagSetter{}
+	}


What about skipping them if in the end we don't have any data transport available?

Also, in Q1, we will start relying on the GLS so this case shouldn't happen anymore.

GLS standing for Goroutine Local Storage? I don't think that'll change anything in this posture... The problem this works around is that the tracer INTENTIONALLY does not create a span for GraphQL Subscriptions because they may be open forever (preventing anything from being emitted).

internal/appsec/emitter/graphqlsec/execution.go

internal/appsec/emitter/graphqlsec/result.go

…hql/APPSEC-11164 # Conflicts: # contrib/google.golang.org/grpc/stats_client.go # internal/appsec/trace/tagsholder.go # internal/appsec/waf.go

contrib/99designs/gqlgen/tracer.go

contrib/graph-gophers/graphql-go/option.go

ahmed-mez

First round of review, mainly focused on the non-appsec code.
Does it make sense to add an integration test for the new contrib? Let's also add an example_test.go file for it.

I didn't add comments for style but please read the style guidelines of the repo. There are multiple places where the Stay compact rule wasn't respected.

RomainMuller · 2023-12-14T13:36:44Z

@ahmed-mez: There are multiple places where the Stay compact rule wasn't respected.

Ah - you got me there... I guess I am aware of the style guide but the whole Stay Compact aspect I have a pretty strong disagreement with at this stage (and sadly, the style guide provides no motivation for this guideline, so I can't even say if I agree with the general principles of why it's there)... Nevertheless, my strong opinions aren't a justification to simply ignore guidelines (and this is also not the place to debate on said guidelines), so I'll go around and remove excessive blank lines 🫣

ahmed-mez

Looks good for tracing. If @rarguelloF or @zarirhamza could review on behalf of the FI team that would be great!

zarirhamza

Contrib looks good too, glad to see better graphQL instrumentation

Julio-Guerra

Congrats - very amazing work and super happy to see such a good level of testing.

I left side notes only for the record, nothing to address immediately.

Julio-Guerra · 2023-12-15T13:31:57Z

contrib/99designs/gqlgen/appsec_test.go

+	"gopkg.in/DataDog/dd-trace-go.v1/internal/appsec"
+)
+
+func TestAppSec(t *testing.T) {


amazing test 👏

Julio-Guerra · 2023-12-15T13:33:27Z

contrib/99designs/gqlgen/appsec_test.go

+// returns a function to restore the previous environment state.
+func enableAppSec(t *testing.T) func() {
+	const rules = `{
+		"version": "2.2",


Note: there's a helper function template somewhere in internal/appsec that you could leverage too.
The reason is that the rule format changed a few times so we created constructors instead at some point.

Julio-Guerra · 2023-12-15T13:34:43Z

contrib/99designs/gqlgen/appsec_test.go

+	err = os.WriteFile(rulesFile, []byte(rules), 0644)
+	require.NoError(t, err)
+	t.Setenv("DD_APPSEC_ENABLED", "1")
+	t.Setenv("DD_APPSEC_RULES", rulesFile)


Would we useful to accept inlined rules in the string value - worth adding this idea in the backlog:

github-actions bot reviewed Oct 30, 2023

View reviewed changes

Merge remote-tracking branch 'origin/main' into romain.marcadier/grap…

3e0651c

…hql/APPSEC-11164

gci the imports

ffb2ddf

github-actions bot reviewed Oct 30, 2023

View reviewed changes

contrib/google.golang.org/grpc/stats_client.go Outdated Show resolved Hide resolved

contrib/google.golang.org/grpc/stats_client.go Outdated Show resolved Hide resolved

Merge branch 'main' into romain.marcadier/graphql/APPSEC-11164

2f0d043

RomainMuller force-pushed the romain.marcadier/graphql/APPSEC-11164 branch from ed9ae81 to 2f0d043 Compare October 31, 2023 11:07

RomainMuller added 13 commits October 31, 2023 17:15

standardize tag names according to the RFC

9767a09

restore old contrib to previous names to avoid a breaking change per …

e75d4dd

…OTel semantics

more tweaks and fixes

8c89791

defer span finish so WAF events get sent properly

edb98f7

fix bad query close type

2d44f1d

repack context data struct to minimize size

5ac2b26

upgrade to latest WAF

2b47db4

Merge remote-tracking branch 'origin/main' into romain.marcadier/grap…

f50921f

…hql/APPSEC-11164 # Conflicts: # go.mod # go.sum # internal/appsec/dyngo/instrumentation/common.go # internal/appsec/remoteconfig_test.go # internal/appsec/waf.go # internal/appsec/waf_unit_test.go

refactor listeners for better encapsulation

bd6e49a

document that omiting trivial also omits them for appsec

a60a419

Merge remote-tracking branch 'origin/main' into romain.marcadier/grap…

d5ce39d

…hql/APPSEC-11164

Merge branch 'romain.marcadier/refactor-dyngo' into romain.marcadier/…

c542113

…graphql/APPSEC-11164 # Conflicts: # internal/appsec/waf.go

RomainMuller changed the base branch from main to romain.marcadier/refactor-dyngo November 22, 2023 10:06

add test scenarios

80e6c3a

github-actions bot reviewed Nov 22, 2023

View reviewed changes

RomainMuller added 6 commits November 23, 2023 11:13

Merge remote-tracking branch 'origin/main' into romain.marcadier/refa…

ff36f02

…ctor-dyngo # Conflicts: # internal/appsec/listener/util/util_test.go # internal/appsec/waf.go

use limiter from github.com/DataDog/appsec-internal-go/limiter

9f29075

use tagged appsec-internal-go

7baf324

fix potential integer conversion issue

4a995ce

restore response headers no cookies feature (lost in merge commit)

670ecd1

preallocate one more map

8c2d896

Julio-Guerra requested changes Dec 11, 2023

View reviewed changes

Julio-Guerra added apm:ecosystem contrib/* related feature requests or bugs appsec labels Dec 12, 2023

have bench_test run in the gitlab benchmarking

4c8211e

RomainMuller requested a review from a team as a code owner December 12, 2023 10:25

RomainMuller added 5 commits December 13, 2023 12:20

NoopTagSetter

b4cce00

use common naming patterns

1affe2e

make parenting relationship a construction concern

b737b29

re-duplicate the error structure

910000c

Merge remote-tracking branch 'origin/main' into romain.marcadier/grap…

4e473e3

…hql/APPSEC-11164 # Conflicts: # contrib/google.golang.org/grpc/stats_client.go # internal/appsec/trace/tagsholder.go # internal/appsec/waf.go

RomainMuller requested a review from Julio-Guerra December 13, 2023 12:30

RomainMuller added 2 commits December 13, 2023 13:53

stop producing all_resolvers

88bd250

hoist wafDaigs up one closure context

ecc8439

ahmed-mez reviewed Dec 13, 2023

View reviewed changes

contrib/99designs/gqlgen/tracer.go Show resolved Hide resolved

contrib/graph-gophers/graphql-go/option.go Show resolved Hide resolved

ahmed-mez reviewed Dec 13, 2023

View reviewed changes

early feedback from @ahmed-mez

344769b

stay compact 🫥

1f74eba

ahmed-mez approved these changes Dec 14, 2023

View reviewed changes

zarirhamza approved these changes Dec 14, 2023

View reviewed changes

Julio-Guerra approved these changes Dec 15, 2023

View reviewed changes

cbeauchesne approved these changes Dec 18, 2023

View reviewed changes

RomainMuller enabled auto-merge (squash) December 18, 2023 09:46

Merge branch 'main' into romain.marcadier/graphql/APPSEC-11164

b91bc73

auto-merge was automatically disabled December 18, 2023 09:51
Pull Request is not mergeable

Merge branch 'main' into romain.marcadier/graphql/APPSEC-11164

e0bb880

RomainMuller enabled auto-merge (squash) December 19, 2023 10:01

auto-merge was automatically disabled December 19, 2023 10:04
Pull Request is not mergeable

RomainMuller merged commit 3eafa89 into main Dec 19, 2023
334 of 335 checks passed

RomainMuller deleted the romain.marcadier/graphql/APPSEC-11164 branch December 19, 2023 10:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

appsec/graphql: add support for Threat Monitoring #2309

appsec/graphql: add support for Threat Monitoring #2309

RomainMuller commented Oct 30, 2023 •

edited

Loading

pr-commenter bot commented Oct 30, 2023 •

edited

Loading

github-actions bot Nov 22, 2023

Julio-Guerra left a comment

Julio-Guerra Dec 11, 2023

Julio-Guerra Dec 11, 2023

RomainMuller Dec 13, 2023

ahmed-mez left a comment

RomainMuller commented Dec 14, 2023

ahmed-mez left a comment

zarirhamza left a comment

Julio-Guerra left a comment

Julio-Guerra Dec 15, 2023

Julio-Guerra Dec 15, 2023

Julio-Guerra Dec 15, 2023

appsec/graphql: add support for Threat Monitoring #2309

appsec/graphql: add support for Threat Monitoring #2309

Conversation

RomainMuller commented Oct 30, 2023 • edited Loading

What does this PR do?

Motivation

Reviewer's Checklist

pr-commenter bot commented Oct 30, 2023 • edited Loading

Benchmarks

github-actions bot Nov 22, 2023

Choose a reason for hiding this comment

Julio-Guerra left a comment

Choose a reason for hiding this comment

Julio-Guerra Dec 11, 2023

Choose a reason for hiding this comment

Julio-Guerra Dec 11, 2023

Choose a reason for hiding this comment

RomainMuller Dec 13, 2023

Choose a reason for hiding this comment

ahmed-mez left a comment

Choose a reason for hiding this comment

RomainMuller commented Dec 14, 2023

ahmed-mez left a comment

Choose a reason for hiding this comment

zarirhamza left a comment

Choose a reason for hiding this comment

Julio-Guerra left a comment

Choose a reason for hiding this comment

Julio-Guerra Dec 15, 2023

Choose a reason for hiding this comment

Julio-Guerra Dec 15, 2023

Choose a reason for hiding this comment

Julio-Guerra Dec 15, 2023

Choose a reason for hiding this comment

RomainMuller commented Oct 30, 2023 •

edited

Loading

pr-commenter bot commented Oct 30, 2023 •

edited

Loading