internal/appsec: add new AppSec beta functionality and support for contrib/net/http #1007
Conversation
This draft instrumentation gateway allows to listen and react to specific
operation stacks. In this first attempt, it tries to avoid maps of operation
attributes and rather relies on Go structures. This allows to simplify the
event listener and emitter API by relying on the operation arguments/results
structure type. It also allows have strongly types lists of operation
arguments and results.
The main limitation comes from the absence of Go generics for now, that
implies the use of empty interface{} values along with some calls to the
reflection package.
Quick POC of the WAF using an operation event listener on the HTTP handler operation. It passes some of its values to the WAF and runs it. For now, it panics when an error or an attack occurs.
By enforcing the order of the WAF listener so that it properly comes after the data events, it is then possible to only rely on them and have a cleaner callback. This shows that a better abstraction should be developped in the future so that no such manual hacks in the listener order is required.
Remove the use of reflect.Type as map key type to avoid possible panics if in the future the `reflect` package returns a non-hashable type descriptor. To uniquely identify the operation argument/result type as it was done with reflect.Type, a struct is now used which includes the type package path and name. This avoids run-time concatenation between reflect.(Type).PkgPath() and reflect.(Type).Name() whereas the struct simply references those two strings. Note that anonymous structs shouldn't be allowed as they have no name nor package path.
- Instrumentation Gateway implemented by package dyngo - WAF integrated behing dyngo's operation events - Simple HTTP client and security event batching - Separate AppSec Go module
Start/Finish the abstract HTTP handler operation using dyngo along with the data required for the AppSec WAF and its security event context.
Integrate AppSec behing the APM tracer so that existing APM users can transparently benefit from AppSec by simply updating the APM module. It is disabled by default, unless DD_APPSEC_ENABLED is true.
There was a problem hiding this comment.
I reviewed purely from a structural point of view addressing the modules approach. Next, it'd be nice to talk about:
- Package structure and examining new public API (
godocoutput) - Code grooming to ensure coding conventions are more or less kept even with the rest of the repository. This makes it easer to navigate for collaborators. I left some guidelines here.
It would also be nice to have some sort of benchmark to understand the overhead of having AppSec enabled. Something basic, perhaps testing the net/http package.
Co-authored-by: Gabriel Aszalos <[email protected]>
There was a problem hiding this comment.
@Julio-Guerra I looked through a lot of this code. Here are the immediate comments I have. If you can help me sort out the build issues so that I can run things I'll take a closer look.
Considering dd-trace-go as the overall Datadog's client library for Go, appsec can move to internal/ and doesn't need to be a standalone module. Another consequence is that dyngo can go into internal/, where it acutally belonged as it is not an appsec-specific package.
There was a problem hiding this comment.
A few quick looks. I see @felixge has done a quite thorough review already, and I'm hoping to do the same soon.
Until then, to save time on nitty comments, can you please look through our style guidelines and try to apply as much as possible of them? I realise some of them might be inconvenient and change the way you're used to writing this code, but we have to stay consistent. And I'm happy (as is everyone else I think) to re-assess and change any rule from there as long as we do it cross-repo and it makes sense.
Remove the context as it is already possible to stop the appsec goroutine without the context.
Register() functions now return their unregister function closures to call in order to unregister the instrumentations and their callbacks. dyngo.Unregister() was removed.
|
This is starting to look good. Can we make sure that the CircleCI steps make sense when reading their titles? As they are now they don't: Let's have duplicates of each using the prefix. Can we do that?
|
|
Lastly, can we also run |
There was a problem hiding this comment.
This is looking much better.
It still needs some review, but as it's hidden behind a compile flag and an environment variable as a "beta" feature, and will not interfere with existing customer applications, I think we can go ahead and merge it.
- It's all internal
- The package surface has been dramatically reduced
- It's an off-by-default beta feature
I think this is pretty much what @felixge said as well.
Before we go GA, we should go through again and tidy up.
There was a problem hiding this comment.
Same as above. Not going to echo.
I think we've come a long way regardless, and this is in a much better shape now. Thank you @Julio-Guerra for all your patience with us (especially me 😁) and looking forward to doing more work together.
I will take care of merging & tagging as soon as CI is good.
gbbr
changed the title
AppSec Milestone 0 is about implementing:
DD_APPSEC_ENABLEDis true.The main building block is the "instrumentation gateway" - the
dyngopackage - which allows to:We firstly want to support the standard
net/httplibrary by defining the HTTP handler operation incontrib/internal/httputil.TraceAndServe(). Every existing integration relying on itTraceAndServecan therefore benefit from it.The WAF listens to the HTTP handler operation and scans some request data for now (more will be added in milestone 1). When the request matches a WAF rule, a security event is emitted through the instrumentation gateway as a "data" event to let listeners further decorate the event with extra metadata context.
This has the huge benefit of keeping the protection code (the WAF code here) focused on what it needs to do and know (modularity and separation of concerns) compared to Sqreen's implementation where we had to have all the request context available in the protections only to add this request metadata to the events. This was an unexpected benefit and use of the instrumentation gateway "data" event.
So the HTTP handler, once it has started the HTTP handler operation, can listen to those security events so that it can add all the context we need in our final intake payloads, but also enforce keeping the trace, and in the future block the request.
Finally, the AppSec agent also listens to the security events through the instrumentation gateway to batch the events, that gets sent every second or when the maximum length of 1024 events is reached (those values are configurable).
Note that the WAF callback was simplified for now and relies on HTTP request data being individually emitted as a data event in order to map them to the rule addresses. The WAF is therefore called once per data event while it should be called once per "group of newly available data". This is not optimal and will be worked out in milestone 1.
Here is a simplified sequence diagram of it showing the chain of events: