Apply suggestions from code review

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

appsec/appsec.go

@@ -4,22 +4,29 @@
 // Copyright 2022 Datadog, Inc.
 
 // Package appsec provides application security features in the form of SDK
-// functions that can be manually called to monitor specific code paths and data
+// functions that can be manually called to monitor specific code paths and data.
+// Application Security is currently transparently integrated into the APM tracer
+// and cannot be used nor started alone at the moment.
+// You can read more on how to enable and start Application Security for Go at
+// https://docs.datadoghq.com/security_platform/application_security/getting_started/go
 package appsec
 
 import (
 	"golang.org/x/net/context"
 
+	"gopkg.in/DataDog/dd-trace-go.v1/internal/appsec"
 	"gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/dyngo/instrumentation/httpsec"
 )
 
-// MonitorParsedHTTPBody performs rule matching on the *parsed* HTTP request body.
-// - body: the parsed HTTP body value to be used for rule matching.
-// - ctx: the context of the http.Request for which the matching is to be performed.
-// Note that some HTTP frameworks implement their own version of context.Context,
-// however this function expects this exact type to be used.
-// Also note that passing the HTTP request raw body instead of parsed will result
-// in inaccurate attack detection
+// MonitorParsedHTTPBody runs the security monitoring rules on the given *parsed*
+// HTTP request body. The given context must be the HTTP request context as returned
+// by the Context() method of an HTTP request. Calls to this function are ignored if
+// AppSec is disabled or the given context is incorrect.
+// Note that passing the raw bytes of the HTTP request body is not expected and would
+// result in inaccurate attack detection.
 func MonitorParsedHTTPBody(ctx context.Context, body interface{}) {
-	httpsec.MonitorParsedBody(ctx, body)
+	if appsec.Enabled() {
+		httpsec.MonitorParsedBody(ctx, body)
+	}
+	// bonus: use sync.Once to log a debug message once if AppSec is disabled
 }

appsec/example_test.go

@@ -6,8 +6,8 @@
 package appsec_test
 
 import (
+	"encoding/json"
 	"io"
-	"io/ioutil"
 	"net/http"
 
 	"github.com/labstack/echo/v4"
@@ -17,32 +17,45 @@ import (
 	httptrace "gopkg.in/DataDog/dd-trace-go.v1/contrib/net/http"
 )
 
-func customBodyParser(body io.ReadCloser) string {
-	b, err := ioutil.ReadAll(body)
-	if err != nil {
-		return "Ill formed http body"
-	}
-	return string(b)
+type parsedBodyType struct {
+	Value string `json:"value"`
+}
+
+func customBodyParser(body io.ReadCloser) (*parsedBodyType, error) {
+	var parsedBody parsedBodyType
+	err := json.NewDecoder(body).Decode(&parsedBody)
+
+	return &parsedBody, err
 }
 
 // Monitor HTTP request parsed body
 func ExampleMonitorParsedHTTPBody() {
 	mux := httptrace.NewServeMux()
 	mux.HandleFunc("/body", func(w http.ResponseWriter, r *http.Request) {
 		// Use the SDK to monitor the request's parsed body
-		appsec.MonitorParsedHTTPBody(r.Context(), customBodyParser(r.Body))
+		body, err := customBodyParser(r.Body)
+		if err != nil {
+			http.Error(w, err.Error(), http.StatusInternalServerError)
+			return
+		}
+		appsec.MonitorParsedHTTPBody(r.Context(), body)
 		w.Write([]byte("Body monitored using AppSec SDK\n"))
 	})
 	http.ListenAndServe(":8080", mux)
 }
 
 // Monitor HTTP request parsed body with a framework customized context type
-func ExampleMonitorParsedHTTPBodyCustomContext() {
+func ExampleMonitorParsedHTTPBody_CustomContext() {
 	r := echo.New()
 	r.Use(echotrace.Middleware())
-	r.POST("/body", func(c echo.Context) error {
+	r.POST("/body", func(c echo.Context) (e error) {
+		req := c.Request()
+		body, err := customBodyParser(req.Body)
+		if err != nil {
+			return c.String(http.StatusInternalServerError, err.Error())
+		}
 		// Use the SDK to monitor the request's parsed body
-		appsec.MonitorParsedHTTPBody(c.Request().Context(), customBodyParser(c.Request().Body))
+		appsec.MonitorParsedHTTPBody(c.Request().Context(), body)
 		return c.String(http.StatusOK, "Body monitored using AppSec SDK")
 	})
 

contrib/gin-gonic/gin/appsec.go

@@ -27,8 +27,8 @@ func useAppSec(c *gin.Context, span tracer.Span) func() {
 		}
 	}
 	args := httpsec.MakeHandlerOperationArgs(req, params)
-	ctx, op := httpsec.StartOperation(c.Request.Context(), args)
-	*req = *req.WithContext(ctx)
+	ctx, op := httpsec.StartOperation(req.Context(), args)
+	c.Request = req.WithContext(ctx)
 	return func() {
 		events := op.Finish(httpsec.HandlerOperationRes{Status: c.Writer.Status()})
 		if len(events) > 0 {

contrib/labstack/echo.v4/appsec.go

@@ -28,7 +28,7 @@ func withAppSec(next echo.HandlerFunc) echo.HandlerFunc {
 		}
 		args := httpsec.MakeHandlerOperationArgs(req, params)
 		ctx, op := httpsec.StartOperation(req.Context(), args)
-		*req = *req.WithContext(ctx)
+		c.SetRequest(req.WithContext(ctx))
 		defer func() {
 			events := op.Finish(httpsec.HandlerOperationRes{Status: c.Response().Status})
 			if len(events) > 0 {

internal/appsec/dyngo/instrumentation/httpsec/http.go

@@ -47,22 +47,23 @@ type (
 
 	// SDKBodyOperationArgs is the SDK body operation arguments.
 	SDKBodyOperationArgs struct {
+		// Body corresponds to the address `server.request.body`.
 		Body interface{}
 	}
 
 	// SDKBodyOperationRes is the SDK body operation results.
-	SDKBodyOperationRes struct {
-	}
+	SDKBodyOperationRes struct{}
 )
 
 // MonitorParsedBody starts and finishes the SDK body operation.
+// This function should not be called when AppSec is disabled in order to
+// get preciser error logs.
 func MonitorParsedBody(ctx context.Context, body interface{}) {
 	if parent := fromContext(ctx); parent != nil {
 		op := StartSDKBodyOperation(parent, SDKBodyOperationArgs{Body: body})
 		op.Finish()
 	} else {
-		log.Debug("appsec: no valid parent HTTP context upon SDKBody operation start" +
-			" AppSec may be disabled")
+		log.Error("appsec: parsed http body monitoring ignored: could not find the http handler instrumentation metadata in the request context: the request handler is not being monitored by a middleware function or the provided context is not the expected request context")
 	}
 }
 
@@ -135,14 +136,13 @@ type (
 	// StartSDKBodyOperation() and finished with its Finish() method.
 	SDKBodyOperation struct {
 		dyngo.Operation
-		events json.RawMessage
 	}
 
 	contextKey struct{}
 )
 
 // StartOperation starts an HTTP handler operation, along with the given
-// context and arguments and emits a start event up in the operation stack
+// context and arguments and emits a start event up in the operation stack.
 // The operation is linked to the global root operation since an HTTP operation
 // is always expected to be first in the operation stack.
 func StartOperation(ctx context.Context, args HandlerOperationArgs) (context.Context, *Operation) {
@@ -153,7 +153,7 @@ func StartOperation(ctx context.Context, args HandlerOperationArgs) (context.Con
 }
 
 func fromContext(ctx context.Context) *Operation {
-	// Avoid a runtime panic in case of error by collecting the 2 return values
+	// Avoid a runtime panic in case of type-assertion error by collecting the 2 return values
 	op, _ := ctx.Value(contextKey{}).(*Operation)
 	return op
 }
@@ -173,9 +173,8 @@ func StartSDKBodyOperation(parent *Operation, args SDKBodyOperationArgs) *SDKBod
 }
 
 // Finish finishes the SDKBody operation and emits a finish event
-func (op *SDKBodyOperation) Finish() json.RawMessage {
+func (op *SDKBodyOperation) Finish() {
 	dyngo.FinishOperation(op, SDKBodyOperationRes{})
-	return op.events
 }
 
 // AddSecurityEvent adds the security event to the list of events observed

internal/appsec/waf.go

@@ -84,9 +84,7 @@ func newHTTPWAFEventListener(handle *waf.Handle, addresses []string, timeout tim
 	return httpsec.OnHandlerOperationStart(func(op *httpsec.Operation, args httpsec.HandlerOperationArgs) {
 		var body interface{}
 		op.On(httpsec.OnSDKBodyOperationStart(func(op *httpsec.SDKBodyOperation, args httpsec.SDKBodyOperationArgs) {
-			op.On(httpsec.OnSDKBodyOperationFinish(func(operation *httpsec.SDKBodyOperation, res httpsec.SDKBodyOperationRes) {
-				body = args.Body
-			}))
+			body = args.Body
 		}))
 		// At the moment, AppSec doesn't block the requests, and so we can use the fact we are in monitoring-only mode
 		// to call the WAF only once at the end of the handler operation.