From 2b83f16b83bb0e344db27168f266a1dd804f3f6a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mazeau?= Date: Thu, 20 Oct 2022 16:05:13 +0200 Subject: [PATCH 01/31] internal/appsec: implement IPFromRequest --- .../dyngo/instrumentation/httpsec/http.go | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/internal/appsec/dyngo/instrumentation/httpsec/http.go b/internal/appsec/dyngo/instrumentation/httpsec/http.go index b070065eeb..8bec41bd88 100644 --- a/internal/appsec/dyngo/instrumentation/httpsec/http.go +++ b/internal/appsec/dyngo/instrumentation/httpsec/http.go @@ -261,3 +261,33 @@ func (OnSDKBodyOperationFinish) ListenedType() reflect.Type { return sdkBodyOper func (f OnSDKBodyOperationFinish) Call(op dyngo.Operation, v interface{}) { f(op.(*SDKBodyOperation), v.(SDKBodyOperationRes)) } + +// IPFromRequest returns the resolved client IP for a specific request. The returned IP can be invalid. +func IPFromRequest(r *http.Request) netaddrIP { + ipHeaders := defaultIPHeaders + if len(clientIPHeader) > 0 { + ipHeaders = []string{clientIPHeader} + } + var headers []string + var ips []string + for _, hdr := range ipHeaders { + if v := r.Header.Get(hdr); v != "" { + headers = append(headers, hdr) + ips = append(ips, v) + } + } + if len(ips) == 0 { + if remoteIP := parseIP(r.RemoteAddr); remoteIP.IsValid() && isGlobal(remoteIP) { + return remoteIP + } + } else if len(ips) == 1 { + for _, ipstr := range strings.Split(ips[0], ",") { + ip := parseIP(strings.TrimSpace(ipstr)) + if ip.IsValid() && isGlobal(ip) { + return ip + } + } + } + + return netaddrIP{} +} From f224b27bf2e461ebb160393a4e044862093c0176 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mazeau?= Date: Thu, 20 Oct 2022 16:07:07 +0200 Subject: [PATCH 02/31] internal/appsec: pass http.clientIP through operation args --- internal/appsec/dyngo/instrumentation/httpsec/http.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/internal/appsec/dyngo/instrumentation/httpsec/http.go b/internal/appsec/dyngo/instrumentation/httpsec/http.go index 8bec41bd88..3809522ebd 100644 --- a/internal/appsec/dyngo/instrumentation/httpsec/http.go +++ b/internal/appsec/dyngo/instrumentation/httpsec/http.go @@ -38,6 +38,8 @@ type ( Query map[string][]string // PathParams corresponds to the address `server.request.path_params` PathParams map[string]string + // ClientIP corresponds to the addres `http.client_ip` + ClientIP netaddrIP } // HandlerOperationRes is the HTTP handler operation results. @@ -123,6 +125,7 @@ func MakeHandlerOperationArgs(r *http.Request, pathParams map[string]string) Han Cookies: cookies, Query: r.URL.Query(), // TODO(Julio-Guerra): avoid actively parsing the query values thanks to dynamic instrumentation PathParams: pathParams, + ClientIP: IPFromRequest(r), } } From c1c1364809d7d74ad7756de512f50e2a191224c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mazeau?= Date: Thu, 20 Oct 2022 16:09:50 +0200 Subject: [PATCH 03/31] internal/appsec: pass htpt.client_ip address to the WAF --- internal/appsec/waf.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/internal/appsec/waf.go b/internal/appsec/waf.go index 95b5494c0b..1d0f58ec51 100644 --- a/internal/appsec/waf.go +++ b/internal/appsec/waf.go @@ -141,6 +141,11 @@ func newHTTPWAFEventListener(handle *waf.Handle, addresses []string, timeout tim } case serverResponseStatusAddr: values[serverResponseStatusAddr] = res.Status + + case httpClientIP: + if args.ClientIP.IsValid() { + values[httpClientIP] = args.ClientIP.String() + } } } matches := runWAF(wafCtx, values, timeout) @@ -277,6 +282,7 @@ const ( serverRequestPathParams = "server.request.path_params" serverRequestBody = "server.request.body" serverResponseStatusAddr = "server.response.status" + httpClientIP = "http.client_ip" ) // List of HTTP rule addresses currently supported by the WAF @@ -288,6 +294,7 @@ var httpAddresses = []string{ serverRequestPathParams, serverRequestBody, serverResponseStatusAddr, + httpClientIP, } // gRPC rule addresses currently supported by the WAF From 566a6c551f5922f9a9da13da3bec7de3364825fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mazeau?= Date: Thu, 3 Nov 2022 12:10:25 +0100 Subject: [PATCH 04/31] internal/appsec: internal/appsec: add support for WAF actions ASM now instanciates an action handler that will perform various actions commanded by the WAF after a match is performed. The "block_request" action type is the only kind of action currently supported, allowing to block an HTTP request. --- contrib/google.golang.org/grpc/appsec.go | 6 ++ .../dyngo/instrumentation/grpcsec/actions.go | 85 +++++++++++++++++++ .../dyngo/instrumentation/grpcsec/grpc.go | 4 + .../dyngo/instrumentation/httpsec/actions.go | 84 ++++++++++++++++++ .../dyngo/instrumentation/httpsec/http.go | 51 ++++++++++- internal/appsec/waf.go | 49 +++++++---- 6 files changed, 261 insertions(+), 18 deletions(-) create mode 100644 internal/appsec/dyngo/instrumentation/grpcsec/actions.go create mode 100644 internal/appsec/dyngo/instrumentation/httpsec/actions.go diff --git a/contrib/google.golang.org/grpc/appsec.go b/contrib/google.golang.org/grpc/appsec.go index 8006107f67..a75a7ff884 100644 --- a/contrib/google.golang.org/grpc/appsec.go +++ b/contrib/google.golang.org/grpc/appsec.go @@ -25,6 +25,9 @@ func appsecUnaryHandlerMiddleware(span ddtrace.Span, handler grpc.UnaryHandler) return func(ctx context.Context, req interface{}) (interface{}, error) { md, _ := metadata.FromIncomingContext(ctx) op := grpcsec.StartHandlerOperation(grpcsec.HandlerOperationArgs{Metadata: md}, nil) + if op.UnaryHandler != nil { + return op.UnaryHandler(ctx, req) + } defer func() { events := op.Finish(grpcsec.HandlerOperationRes{}) instrumentation.SetTags(span, op.Tags()) @@ -44,6 +47,9 @@ func appsecStreamHandlerMiddleware(span ddtrace.Span, handler grpc.StreamHandler return func(srv interface{}, stream grpc.ServerStream) error { md, _ := metadata.FromIncomingContext(stream.Context()) op := grpcsec.StartHandlerOperation(grpcsec.HandlerOperationArgs{Metadata: md}, nil) + if op.StreamHandler != nil { + return op.StreamHandler(srv, stream) + } defer func() { events := op.Finish(grpcsec.HandlerOperationRes{}) instrumentation.SetTags(span, op.Tags()) diff --git a/internal/appsec/dyngo/instrumentation/grpcsec/actions.go b/internal/appsec/dyngo/instrumentation/grpcsec/actions.go new file mode 100644 index 0000000000..35dec93330 --- /dev/null +++ b/internal/appsec/dyngo/instrumentation/grpcsec/actions.go @@ -0,0 +1,85 @@ +// Unless explicitly stated otherwise all files in this repository are licensed +// under the Apache License Version 2.0. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2022 Datadog, Inc. + +//go:build appsec +// +build appsec + +package grpcsec + +import ( + "context" + + "google.golang.org/grpc" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" +) + +type ActionParam interface{} + +type action struct { + id string + params ActionParam +} + +// ActionHandler handles WAF actions registration and execution +type ActionHandler interface { + RegisterAction(id string, params ActionParam) + Exec(id string, op *HandlerOperation) +} + +type actionsHandler struct { + actions map[string]action +} + +// NewActionsHandler returns an action handler holding the default ASM actions. +// Currently, only the default "block" action is supported +func NewActionsHandler() ActionHandler { + defaultBlockAction := action{ + id: "block", + params: BlockRequestParams{ + Status: codes.Aborted, + }, + } + // Register the default "block" action as specified in the RFC for HTTP blocking + actions := map[string]action{defaultBlockAction.id: defaultBlockAction} + + return &actionsHandler{ + actions: actions, + } +} + +// RegisterAction registers a specific action to the actions handler. If the action kind is unknown +// the action will have no effect +func (h *actionsHandler) RegisterAction(id string, params ActionParam) { + h.actions[id] = action{ + id: id, + params: params, + } +} + +// Exec executes the action identified by `id` +func (h *actionsHandler) Exec(id string, op *HandlerOperation) { + a, ok := h.actions[id] + if !ok { + return + } + // Currently, only the "block_request" type is supported, so we only need to check for blockRequestParams + if p, ok := a.params.(BlockRequestParams); ok { + err := status.Error(p.Status, "Request blocked") + op.UnaryHandler = func(ctx context.Context, req interface{}) (interface{}, error) { + return nil, err + } + op.StreamHandler = func(srv interface{}, stream grpc.ServerStream) error { + return err + } + } +} + +// BlockRequestParams is the parameter struct used to perform actions of kind ActionBlockRequest +type BlockRequestParams struct { + ActionParam + // Status is the return code to use when blocking the request + Status codes.Code +} diff --git a/internal/appsec/dyngo/instrumentation/grpcsec/grpc.go b/internal/appsec/dyngo/instrumentation/grpcsec/grpc.go index 8eb2c638f3..ff65b25501 100644 --- a/internal/appsec/dyngo/instrumentation/grpcsec/grpc.go +++ b/internal/appsec/dyngo/instrumentation/grpcsec/grpc.go @@ -15,6 +15,8 @@ import ( "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/dyngo" "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/dyngo/instrumentation" + + "google.golang.org/grpc" ) // Abstract gRPC server handler operation definitions. It is based on two @@ -39,6 +41,8 @@ type ( dyngo.Operation instrumentation.TagsHolder instrumentation.SecurityEventsHolder + UnaryHandler grpc.UnaryHandler + StreamHandler grpc.StreamHandler } // HandlerOperationArgs is the grpc handler arguments. HandlerOperationArgs struct { diff --git a/internal/appsec/dyngo/instrumentation/httpsec/actions.go b/internal/appsec/dyngo/instrumentation/httpsec/actions.go new file mode 100644 index 0000000000..46cf84cb7b --- /dev/null +++ b/internal/appsec/dyngo/instrumentation/httpsec/actions.go @@ -0,0 +1,84 @@ +// Unless explicitly stated otherwise all files in this repository are licensed +// under the Apache License Version 2.0. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2022 Datadog, Inc. + +//go:build appsec +// +build appsec + +package httpsec + +import ( + "net/http" +) + +// Action is used to identify any action kind +type Action interface { + isAction() +} + +// BlockRequestAction is the parameter struct used to perform actions of kind ActionBlockRequest +type BlockRequestAction struct { + Action + // Status is the return code to use when blocking the request + Status int + // Template is the payload template to use to write the response (html or json) + Template string + // handler is the http handler be used to block the request (see wrap()) + handler http.Handler +} + +func (*BlockRequestAction) isAction() {} + +func blockedPayload(a *BlockRequestAction) []byte { + payload := BlockedTemplateJSON + if a.Template == "html" { + payload = BlockedTemplateHTML + } + return payload +} + +// ActionsHandler handles actions registration and their application to operations +type ActionsHandler struct { + actions map[string]Action +} + +// NewActionsHandler returns an action handler holding the default ASM actions. +// Currently, only the default "block" action is supported +func NewActionsHandler() *ActionsHandler { + handler := ActionsHandler{ + actions: map[string]Action{}, + } + // Register the default "block" action as specified in the RFC for HTTP blocking + handler.RegisterAction("block", &BlockRequestAction{ + Status: 403, + Template: "html", + }) + + return &handler +} + +// RegisterAction registers a specific action to the handler. If the action kind is unknown +// the action will not be registered +func (h *ActionsHandler) RegisterAction(id string, action Action) { + switch a := action.(type) { + case *BlockRequestAction: + payload := blockedPayload(a) + a.handler = http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) { + writer.Write(payload) + writer.WriteHeader(a.Status) + }) + h.actions[id] = a + default: + break + } +} + +// Apply applies the action identified by `id` for the given operation +func (h *ActionsHandler) Apply(id string, op *Operation) { + a, ok := h.actions[id] + if !ok { + return + } + op.actions = append(op.actions, a) +} diff --git a/internal/appsec/dyngo/instrumentation/httpsec/http.go b/internal/appsec/dyngo/instrumentation/httpsec/http.go index 3809522ebd..95319cd1a3 100644 --- a/internal/appsec/dyngo/instrumentation/httpsec/http.go +++ b/internal/appsec/dyngo/instrumentation/httpsec/http.go @@ -15,6 +15,7 @@ import ( "encoding/json" "net" "net/http" + "os" "reflect" "strings" @@ -74,6 +75,17 @@ func MonitorParsedBody(ctx context.Context, body interface{}) { // HandlerOperationRes. func WrapHandler(handler http.Handler, span ddtrace.Span, pathParams map[string]string) http.Handler { instrumentation.SetAppSecEnabledTags(span) + applyActions := func(op *Operation) { + for _, action := range op.Actions() { + switch a := action.(type) { + case *BlockRequestAction: + handler = a.handler + op.AddTag("appsec.blocked", true) + default: + log.Warn("appsec: unsupported action %v. Ignoring.", a) + } + } + } return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { SetIPTags(span, r) @@ -81,18 +93,24 @@ func WrapHandler(handler http.Handler, span ddtrace.Span, pathParams map[string] args := MakeHandlerOperationArgs(r, pathParams) ctx, op := StartOperation(r.Context(), args) r = r.WithContext(ctx) + + events := op.Events() + if len(events) > 0 { + applyActions(op) + } defer func() { var status int if mw, ok := w.(interface{ Status() int }); ok { status = mw.Status() } - events := op.Finish(HandlerOperationRes{Status: status}) + events = append(events, op.Finish(HandlerOperationRes{Status: status})...) instrumentation.SetTags(span, op.Tags()) if len(events) == 0 { return } + applyActions(op) remoteIP, _, err := net.SplitHostPort(r.RemoteAddr) if err != nil { remoteIP = r.RemoteAddr @@ -101,6 +119,7 @@ func WrapHandler(handler http.Handler, span ddtrace.Span, pathParams map[string] }() handler.ServeHTTP(w, r) + }) } @@ -152,6 +171,7 @@ type ( dyngo.Operation instrumentation.TagsHolder instrumentation.SecurityEventsHolder + actions []Action } // SDKBodyOperation type representing an SDK body. It must be created with @@ -190,6 +210,10 @@ func (op *Operation) Finish(res HandlerOperationRes) []json.RawMessage { return op.Events() } +func (op *Operation) Actions() []Action { + return op.actions +} + // StartSDKBodyOperation starts the SDKBody operation and emits a start event func StartSDKBodyOperation(parent *Operation, args SDKBodyOperationArgs) *SDKBodyOperation { op := &SDKBodyOperation{Operation: dyngo.NewOperation(parent)} @@ -294,3 +318,28 @@ func IPFromRequest(r *http.Request) netaddrIP { return netaddrIP{} } + +var ( + // BlockedTemplateJSON is the default JSON template used to write responses for blocked requests + BlockedTemplateJSON = []byte(`{"errors": [{"title": "You've been blocked", "detail": "Sorry, you cannot access this page. Please contact the customer service team. Security provided by Datadog."}]}`) + // BlockedTemplateHTML is the default HTML template used to write responses for blocked requests + BlockedTemplateHTML = []byte(` You've been blocked

Sorry, you cannot access this page. Please contact the customer service team.

`) +) + +const ( + envBlockedTemplateHTML = "DD_APPSEC_HTTP_BLOCKED_TEMPLATE_HTML" + envBlockedTemplateJSON = "DD_APPSEC_HTTP_BLOCKED_TEMPLATE_JSON" +) + +func init() { + for env, template := range map[string]*[]byte{envBlockedTemplateJSON: &BlockedTemplateJSON, envBlockedTemplateHTML: &BlockedTemplateHTML} { + if path, ok := os.LookupEnv(env); ok { + if t, err := os.ReadFile(path); err != nil { + log.Warn("Could not read template at %s: %v", path, err) + } else { + *template = t + } + } + + } +} diff --git a/internal/appsec/waf.go b/internal/appsec/waf.go index 1d0f58ec51..4229223376 100644 --- a/internal/appsec/waf.go +++ b/internal/appsec/waf.go @@ -95,9 +95,35 @@ func registerWAF(rules []byte, timeout time.Duration, limiter Limiter, obfCfg *O // newWAFEventListener returns the WAF event listener to register in order to enable it. func newHTTPWAFEventListener(handle *waf.Handle, addresses []string, timeout time.Duration, limiter Limiter) dyngo.EventListener { var monitorRulesOnce sync.Once // per instantiation + actionHandler := httpsec.NewActionsHandler() return httpsec.OnHandlerOperationStart(func(op *httpsec.Operation, args httpsec.HandlerOperationArgs) { var body interface{} + wafCtx := waf.NewContext(handle) + if wafCtx == nil { + // The WAF event listener got concurrently released + return + } + + values := map[string]interface{}{} + for _, addr := range addresses { + if addr == httpClientIP && args.ClientIP.IsValid() { + values[httpClientIP] = args.ClientIP.String() + } + } + + matches, actionIds := runWAF(wafCtx, values, timeout) + actionIds = append(actionIds, "block") + if len(matches) > 0 { + for _, id := range actionIds { + actionHandler.Apply(id, op) + } + op.AddSecurityEvents(matches) + log.Debug("appsec: WAF detected an attack before executing the request") + if len(actionIds) > 0 { + return + } + } op.On(httpsec.OnSDKBodyOperationStart(func(op *httpsec.SDKBodyOperation, args httpsec.SDKBodyOperationArgs) { body = args.Body @@ -106,13 +132,7 @@ func newHTTPWAFEventListener(handle *waf.Handle, addresses []string, timeout tim // 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. op.On(httpsec.OnHandlerOperationFinish(func(op *httpsec.Operation, res httpsec.HandlerOperationRes) { - wafCtx := waf.NewContext(handle) - if wafCtx == nil { - // The WAF event listener got concurrently released - return - } defer wafCtx.Close() - // Run the WAF on the rule addresses available in the request args values := make(map[string]interface{}, len(addresses)) for _, addr := range addresses { @@ -141,14 +161,9 @@ func newHTTPWAFEventListener(handle *waf.Handle, addresses []string, timeout tim } case serverResponseStatusAddr: values[serverResponseStatusAddr] = res.Status - - case httpClientIP: - if args.ClientIP.IsValid() { - values[httpClientIP] = args.ClientIP.String() - } } } - matches := runWAF(wafCtx, values, timeout) + matches, _ := runWAF(wafCtx, values, timeout) // Add WAF metrics. rInfo := handle.RulesetInfo() @@ -222,7 +237,7 @@ func newGRPCWAFEventListener(handle *waf.Handle, _ []string, timeout time.Durati if md := handlerArgs.Metadata; len(md) > 0 { values[grpcServerRequestMetadata] = md } - event := runWAF(wafCtx, values, timeout) + event, _ := runWAF(wafCtx, values, timeout) // WAF run durations are WAF context bound. As of now we need to keep track of those externally since // we use a new WAF context for each callback. When we are able to re-use the same WAF context across @@ -260,17 +275,17 @@ func newGRPCWAFEventListener(handle *waf.Handle, _ []string, timeout time.Durati }) } -func runWAF(wafCtx *waf.Context, values map[string]interface{}, timeout time.Duration) []byte { - matches, _, err := wafCtx.Run(values, timeout) +func runWAF(wafCtx *waf.Context, values map[string]interface{}, timeout time.Duration) ([]byte, []string) { + matches, actions, err := wafCtx.Run(values, timeout) if err != nil { if err == waf.ErrTimeout { log.Debug("appsec: waf timeout value of %s reached", timeout) } else { log.Error("appsec: unexpected waf error: %v", err) - return nil + return nil, nil } } - return matches + return matches, actions } // HTTP rule addresses currently supported by the WAF From dea3413db1d283c5c4b2e759e4c59c89a73a8a77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mazeau?= Date: Wed, 16 Nov 2022 15:00:13 +0100 Subject: [PATCH 05/31] internal/appsec/dyngo: write response header before body in blocking action --- internal/appsec/dyngo/instrumentation/httpsec/actions.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/appsec/dyngo/instrumentation/httpsec/actions.go b/internal/appsec/dyngo/instrumentation/httpsec/actions.go index 46cf84cb7b..5b4593b2e2 100644 --- a/internal/appsec/dyngo/instrumentation/httpsec/actions.go +++ b/internal/appsec/dyngo/instrumentation/httpsec/actions.go @@ -65,8 +65,8 @@ func (h *ActionsHandler) RegisterAction(id string, action Action) { case *BlockRequestAction: payload := blockedPayload(a) a.handler = http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) { - writer.Write(payload) writer.WriteHeader(a.Status) + writer.Write(payload) }) h.actions[id] = a default: From e6f290285af317a131ea45dd7c2e77a987b47c70 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mazeau?= Date: Wed, 16 Nov 2022 15:26:38 +0100 Subject: [PATCH 06/31] internal/appsec/dyngo: clear actions/events before second waf run --- internal/appsec/dyngo/instrumentation/common.go | 15 ++++++++++++--- .../appsec/dyngo/instrumentation/httpsec/http.go | 10 +++++++++- .../appsec/dyngo/instrumentation/httpsec/tags.go | 8 +++++--- .../dyngo/instrumentation/httpsec/tags_test.go | 4 ++-- 4 files changed, 28 insertions(+), 9 deletions(-) diff --git a/internal/appsec/dyngo/instrumentation/common.go b/internal/appsec/dyngo/instrumentation/common.go index 9d0d66736e..4b47ccdab0 100644 --- a/internal/appsec/dyngo/instrumentation/common.go +++ b/internal/appsec/dyngo/instrumentation/common.go @@ -65,6 +65,13 @@ func (s *SecurityEventsHolder) Events() []json.RawMessage { return s.events } +// ClearEvents clears the list of stored events +func (s *SecurityEventsHolder) ClearEvents() { + s.mu.Lock() + defer s.mu.Unlock() + s.events = []json.RawMessage{} +} + // SetTags fills the span tags using the key/value pairs found in `tags` func SetTags(span TagSetter, tags map[string]interface{}) { for k, v := range tags { @@ -102,9 +109,11 @@ func SetEventSpanTags(span TagSetter, events []json.RawMessage) error { // Create the value of the security event tag. // TODO(Julio-Guerra): a future libddwaf version should return something -// avoiding us the following events concatenation logic which currently -// involves unserializing the top-level JSON arrays to concatenate them -// together. +// +// avoiding us the following events concatenation logic which currently +// involves unserializing the top-level JSON arrays to concatenate them +// together. +// // TODO(Julio-Guerra): avoid serializing the json in the request hot path func makeEventTagValue(events []json.RawMessage) (json.RawMessage, error) { var v interface{} diff --git a/internal/appsec/dyngo/instrumentation/httpsec/http.go b/internal/appsec/dyngo/instrumentation/httpsec/http.go index 95319cd1a3..311d8ee8ae 100644 --- a/internal/appsec/dyngo/instrumentation/httpsec/http.go +++ b/internal/appsec/dyngo/instrumentation/httpsec/http.go @@ -80,7 +80,7 @@ func WrapHandler(handler http.Handler, span ddtrace.Span, pathParams map[string] switch a := action.(type) { case *BlockRequestAction: handler = a.handler - op.AddTag("appsec.blocked", true) + op.AddTag(tagBlockedRequest, true) default: log.Warn("appsec: unsupported action %v. Ignoring.", a) } @@ -97,6 +97,8 @@ func WrapHandler(handler http.Handler, span ddtrace.Span, pathParams map[string] events := op.Events() if len(events) > 0 { applyActions(op) + op.ClearEvents() + op.ClearActions() } defer func() { var status int @@ -210,10 +212,16 @@ func (op *Operation) Finish(res HandlerOperationRes) []json.RawMessage { return op.Events() } +// Actions returns the actions linked to the operation func (op *Operation) Actions() []Action { return op.actions } +// ClearActions clears all the actions linked to the operation +func (op *Operation) ClearActions() { + op.actions = []Action{} +} + // StartSDKBodyOperation starts the SDKBody operation and emits a start event func StartSDKBodyOperation(parent *Operation, args SDKBodyOperationArgs) *SDKBodyOperation { op := &SDKBodyOperation{Operation: dyngo.NewOperation(parent)} diff --git a/internal/appsec/dyngo/instrumentation/httpsec/tags.go b/internal/appsec/dyngo/instrumentation/httpsec/tags.go index faf300d950..769c9dc194 100644 --- a/internal/appsec/dyngo/instrumentation/httpsec/tags.go +++ b/internal/appsec/dyngo/instrumentation/httpsec/tags.go @@ -21,9 +21,11 @@ import ( const ( // envClientIPHeader is the name of the env var used to specify the IP header to be used for client IP collection. envClientIPHeader = "DD_TRACE_CLIENT_IP_HEADER" - // multipleIPHeaders sets the multiple ip header tag used internally to tell the backend an error occurred when + // tagMultipleIPHeaders sets the multiple ip header tag used internally to tell the backend an error occurred when // retrieving an HTTP request client IP. - multipleIPHeaders = "_dd.multiple-ip-headers" + tagMultipleIPHeaders = "_dd.multiple-ip-headers" + // tagBlockedRequest used to convey whether a request is blocked + tagBlockedRequest = "appsec.blocked" ) var ( @@ -138,7 +140,7 @@ func SetIPTags(span instrumentation.TagSetter, r *http.Request) { for i := range ips { span.SetTag(ext.HTTPRequestHeaders+"."+headers[i], ips[i]) } - span.SetTag(multipleIPHeaders, strings.Join(headers, ",")) + span.SetTag(tagMultipleIPHeaders, strings.Join(headers, ",")) } } diff --git a/internal/appsec/dyngo/instrumentation/httpsec/tags_test.go b/internal/appsec/dyngo/instrumentation/httpsec/tags_test.go index 1dfe92c6fe..dcbcc06a97 100644 --- a/internal/appsec/dyngo/instrumentation/httpsec/tags_test.go +++ b/internal/appsec/dyngo/instrumentation/httpsec/tags_test.go @@ -225,11 +225,11 @@ func TestIPHeaders(t *testing.T) { SetIPTags(&span, &r) if tc.expectedIP.IsValid() { require.Equal(t, tc.expectedIP.String(), span.Tag(ext.HTTPClientIP)) - require.Nil(t, span.Tag(multipleIPHeaders)) + require.Nil(t, span.Tag(tagMultipleIPHeaders)) } else { require.Nil(t, span.Tag(ext.HTTPClientIP)) if tc.multiHeaders != "" { - require.Equal(t, tc.multiHeaders, span.Tag(multipleIPHeaders)) + require.Equal(t, tc.multiHeaders, span.Tag(tagMultipleIPHeaders)) for hdr, ip := range tc.headers { require.Equal(t, ip, span.Tag(ext.HTTPRequestHeaders+"."+hdr)) } From b7e1d8ad1c8ce0300c95313dad3fca700b909c7f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mazeau?= Date: Mon, 21 Nov 2022 10:48:18 +0100 Subject: [PATCH 07/31] internal/appsec/httpsec: remove appsec build tag constraint for actions --- internal/appsec/dyngo/instrumentation/httpsec/actions.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/internal/appsec/dyngo/instrumentation/httpsec/actions.go b/internal/appsec/dyngo/instrumentation/httpsec/actions.go index 5b4593b2e2..d1b5c530fc 100644 --- a/internal/appsec/dyngo/instrumentation/httpsec/actions.go +++ b/internal/appsec/dyngo/instrumentation/httpsec/actions.go @@ -3,9 +3,6 @@ // This product includes software developed at Datadog (https://www.datadoghq.com/). // Copyright 2022 Datadog, Inc. -//go:build appsec -// +build appsec - package httpsec import ( From 733a1430cd0accb773e2e5d28b7b410b19b6b5de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mazeau?= Date: Wed, 23 Nov 2022 10:12:06 +0100 Subject: [PATCH 08/31] Apply suggestions from code review Co-authored-by: Julio Guerra --- .../appsec/dyngo/instrumentation/common.go | 2 +- .../dyngo/instrumentation/httpsec/actions.go | 6 ++-- .../dyngo/instrumentation/httpsec/http.go | 4 +-- internal/appsec/waf.go | 30 +++++++++++-------- 4 files changed, 24 insertions(+), 18 deletions(-) diff --git a/internal/appsec/dyngo/instrumentation/common.go b/internal/appsec/dyngo/instrumentation/common.go index 4b47ccdab0..50b6dbd8fd 100644 --- a/internal/appsec/dyngo/instrumentation/common.go +++ b/internal/appsec/dyngo/instrumentation/common.go @@ -69,7 +69,7 @@ func (s *SecurityEventsHolder) Events() []json.RawMessage { func (s *SecurityEventsHolder) ClearEvents() { s.mu.Lock() defer s.mu.Unlock() - s.events = []json.RawMessage{} + s.events = s.events[0:0] } // SetTags fills the span tags using the key/value pairs found in `tags` diff --git a/internal/appsec/dyngo/instrumentation/httpsec/actions.go b/internal/appsec/dyngo/instrumentation/httpsec/actions.go index d1b5c530fc..27aa7b256f 100644 --- a/internal/appsec/dyngo/instrumentation/httpsec/actions.go +++ b/internal/appsec/dyngo/instrumentation/httpsec/actions.go @@ -7,6 +7,8 @@ package httpsec import ( "net/http" + + "gopkg.in/DataDog/dd-trace-go.v1/internal/log" ) // Action is used to identify any action kind @@ -16,12 +18,11 @@ type Action interface { // BlockRequestAction is the parameter struct used to perform actions of kind ActionBlockRequest type BlockRequestAction struct { - Action // Status is the return code to use when blocking the request Status int // Template is the payload template to use to write the response (html or json) Template string - // handler is the http handler be used to block the request (see wrap()) + // handler is the http handler to use to block the request handler http.Handler } @@ -75,6 +76,7 @@ func (h *ActionsHandler) RegisterAction(id string, action Action) { func (h *ActionsHandler) Apply(id string, op *Operation) { a, ok := h.actions[id] if !ok { + log.Debug("appsec: ignoring the returned waf action: unknown action id `%s`", id) return } op.actions = append(op.actions, a) diff --git a/internal/appsec/dyngo/instrumentation/httpsec/http.go b/internal/appsec/dyngo/instrumentation/httpsec/http.go index 311d8ee8ae..5a15dc1e3e 100644 --- a/internal/appsec/dyngo/instrumentation/httpsec/http.go +++ b/internal/appsec/dyngo/instrumentation/httpsec/http.go @@ -82,7 +82,7 @@ func WrapHandler(handler http.Handler, span ddtrace.Span, pathParams map[string] handler = a.handler op.AddTag(tagBlockedRequest, true) default: - log.Warn("appsec: unsupported action %v. Ignoring.", a) + log.Error("appsec: ignoring security action: unexpected action type %T", a) } } } @@ -219,7 +219,7 @@ func (op *Operation) Actions() []Action { // ClearActions clears all the actions linked to the operation func (op *Operation) ClearActions() { - op.actions = []Action{} + op.actions = op.actions[0:0] } // StartSDKBodyOperation starts the SDKBody operation and emits a start event diff --git a/internal/appsec/waf.go b/internal/appsec/waf.go index 4229223376..df927d5be3 100644 --- a/internal/appsec/waf.go +++ b/internal/appsec/waf.go @@ -107,13 +107,13 @@ func newHTTPWAFEventListener(handle *waf.Handle, addresses []string, timeout tim values := map[string]interface{}{} for _, addr := range addresses { - if addr == httpClientIP && args.ClientIP.IsValid() { - values[httpClientIP] = args.ClientIP.String() + if addr == httpClientIPAddr && args.ClientIP.IsValid() { + values[httpClientIPAddr] = args.ClientIP.String() } } + // TODO: suspicious request blocking by moving here all the addresses available when the request begins matches, actionIds := runWAF(wafCtx, values, timeout) - actionIds = append(actionIds, "block") if len(matches) > 0 { for _, id := range actionIds { actionHandler.Apply(id, op) @@ -151,18 +151,20 @@ func newHTTPWAFEventListener(handle *waf.Handle, addresses []string, timeout tim if query := args.Query; query != nil { values[serverRequestQueryAddr] = query } - case serverRequestPathParams: + case serverRequestPathParamsAddr: if pathParams := args.PathParams; pathParams != nil { - values[serverRequestPathParams] = pathParams + values[serverRequestPathParamsAddr] = pathParams } - case serverRequestBody: + case serverRequestBodyAddr: if body != nil { - values[serverRequestBody] = body + values[serverRequestBodyAddr] = body } case serverResponseStatusAddr: values[serverResponseStatusAddr] = res.Status } } + // Run the WAF, ignoring the returned actions - if any - since blocking after the request handler's + // response is not supported at the moment. matches, _ := runWAF(wafCtx, values, timeout) // Add WAF metrics. @@ -237,6 +239,8 @@ func newGRPCWAFEventListener(handle *waf.Handle, _ []string, timeout time.Durati if md := handlerArgs.Metadata; len(md) > 0 { values[grpcServerRequestMetadata] = md } + // Run the WAF, ignoring the returned actions - if any - since blocking after the request handler's + // response is not supported at the moment. event, _ := runWAF(wafCtx, values, timeout) // WAF run durations are WAF context bound. As of now we need to keep track of those externally since @@ -294,10 +298,10 @@ const ( serverRequestHeadersNoCookiesAddr = "server.request.headers.no_cookies" serverRequestCookiesAddr = "server.request.cookies" serverRequestQueryAddr = "server.request.query" - serverRequestPathParams = "server.request.path_params" - serverRequestBody = "server.request.body" + serverRequestPathParamsAddr = "server.request.path_params" + serverRequestBodyAddr = "server.request.body" serverResponseStatusAddr = "server.response.status" - httpClientIP = "http.client_ip" + httpClientIPAddr = "http.client_ip" ) // List of HTTP rule addresses currently supported by the WAF @@ -306,10 +310,10 @@ var httpAddresses = []string{ serverRequestHeadersNoCookiesAddr, serverRequestCookiesAddr, serverRequestQueryAddr, - serverRequestPathParams, - serverRequestBody, + serverRequestPathParamsAddr, + serverRequestBodyAddr, serverResponseStatusAddr, - httpClientIP, + httpClientIPAddr, } // gRPC rule addresses currently supported by the WAF From 9f24219a3f1a4480541a4aa18f3b05ed108d2929 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mazeau?= Date: Wed, 23 Nov 2022 15:44:03 +0100 Subject: [PATCH 09/31] internal/appsec/dyngo: simplify BlockRequestAction struct --- .../dyngo/instrumentation/httpsec/actions.go | 64 +++++++++++-------- 1 file changed, 38 insertions(+), 26 deletions(-) diff --git a/internal/appsec/dyngo/instrumentation/httpsec/actions.go b/internal/appsec/dyngo/instrumentation/httpsec/actions.go index 27aa7b256f..109e8c428e 100644 --- a/internal/appsec/dyngo/instrumentation/httpsec/actions.go +++ b/internal/appsec/dyngo/instrumentation/httpsec/actions.go @@ -16,24 +16,48 @@ type Action interface { isAction() } -// BlockRequestAction is the parameter struct used to perform actions of kind ActionBlockRequest +// BlockRequestAction is the action that holds the HTTP handler to use to block the request type BlockRequestAction struct { - // Status is the return code to use when blocking the request - Status int - // Template is the payload template to use to write the response (html or json) - Template string // handler is the http handler to use to block the request handler http.Handler } func (*BlockRequestAction) isAction() {} -func blockedPayload(a *BlockRequestAction) []byte { - payload := BlockedTemplateJSON - if a.Template == "html" { - payload = BlockedTemplateHTML +func NewBlockRequestAction(status int, template string) BlockRequestAction { + htmlHandler := newBlockRequestHandler(status, "application/html", BlockedTemplateHTML) + jsonHandler := newBlockRequestHandler(status, "application/json", BlockedTemplateJSON) + var action BlockRequestAction + switch template { + case "json": + action.handler = newBlockRequestHandler(status, "application/json", BlockedTemplateJSON) + break + case "html": + action.handler = newBlockRequestHandler(status, "application/html", BlockedTemplateHTML) + break + default: + action.handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + h := jsonHandler + for _, value := range r.Header.Values("Accept") { + if value == "application/html" { + h = htmlHandler + break + } + } + h.ServeHTTP(w, r) + }) + break } - return payload + return action + +} + +func newBlockRequestHandler(status int, ct string, payload []byte) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", ct) + w.WriteHeader(status) + w.Write(payload) + }) } // ActionsHandler handles actions registration and their application to operations @@ -48,28 +72,16 @@ func NewActionsHandler() *ActionsHandler { actions: map[string]Action{}, } // Register the default "block" action as specified in the RFC for HTTP blocking - handler.RegisterAction("block", &BlockRequestAction{ - Status: 403, - Template: "html", - }) + block := NewBlockRequestAction(403, "auto") + handler.RegisterAction("block", &block) return &handler } // RegisterAction registers a specific action to the handler. If the action kind is unknown // the action will not be registered -func (h *ActionsHandler) RegisterAction(id string, action Action) { - switch a := action.(type) { - case *BlockRequestAction: - payload := blockedPayload(a) - a.handler = http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) { - writer.WriteHeader(a.Status) - writer.Write(payload) - }) - h.actions[id] = a - default: - break - } +func (h *ActionsHandler) RegisterAction(id string, a Action) { + h.actions[id] = a } // Apply applies the action identified by `id` for the given operation From 36db48611fc495944ca04c13a24f5bd21d9f1334 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mazeau?= Date: Wed, 23 Nov 2022 16:51:03 +0100 Subject: [PATCH 10/31] internal/appsec: actionHandler.Apply returns true if blocking --- .../dyngo/instrumentation/httpsec/actions.go | 14 +++++++++++--- .../appsec/dyngo/instrumentation/httpsec/http.go | 14 +++++++------- internal/appsec/waf.go | 5 +++-- 3 files changed, 21 insertions(+), 12 deletions(-) diff --git a/internal/appsec/dyngo/instrumentation/httpsec/actions.go b/internal/appsec/dyngo/instrumentation/httpsec/actions.go index 109e8c428e..7b7402bb75 100644 --- a/internal/appsec/dyngo/instrumentation/httpsec/actions.go +++ b/internal/appsec/dyngo/instrumentation/httpsec/actions.go @@ -85,11 +85,19 @@ func (h *ActionsHandler) RegisterAction(id string, a Action) { } // Apply applies the action identified by `id` for the given operation -func (h *ActionsHandler) Apply(id string, op *Operation) { +// Returns true if the applied action will interrupt the request flow (block, redirect, etc...) +func (h *ActionsHandler) Apply(id string, op *Operation) bool { a, ok := h.actions[id] if !ok { log.Debug("appsec: ignoring the returned waf action: unknown action id `%s`", id) - return + return false + } + op.AddAction(a) + + switch a.(type) { + case *BlockRequestAction: + return true + default: + return false } - op.actions = append(op.actions, a) } diff --git a/internal/appsec/dyngo/instrumentation/httpsec/http.go b/internal/appsec/dyngo/instrumentation/httpsec/http.go index 5a15dc1e3e..1fd150439a 100644 --- a/internal/appsec/dyngo/instrumentation/httpsec/http.go +++ b/internal/appsec/dyngo/instrumentation/httpsec/http.go @@ -85,6 +85,7 @@ func WrapHandler(handler http.Handler, span ddtrace.Span, pathParams map[string] log.Error("appsec: ignoring security action: unexpected action type %T", a) } } + op.ClearActions() } return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -94,19 +95,14 @@ func WrapHandler(handler http.Handler, span ddtrace.Span, pathParams map[string] ctx, op := StartOperation(r.Context(), args) r = r.WithContext(ctx) - events := op.Events() - if len(events) > 0 { - applyActions(op) - op.ClearEvents() - op.ClearActions() - } + applyActions(op) defer func() { var status int if mw, ok := w.(interface{ Status() int }); ok { status = mw.Status() } - events = append(events, op.Finish(HandlerOperationRes{Status: status})...) + events := op.Finish(HandlerOperationRes{Status: status}) instrumentation.SetTags(span, op.Tags()) if len(events) == 0 { return @@ -217,6 +213,10 @@ func (op *Operation) Actions() []Action { return op.actions } +func (op *Operation) AddAction(a Action) { + op.actions = append(op.actions, a) +} + // ClearActions clears all the actions linked to the operation func (op *Operation) ClearActions() { op.actions = op.actions[0:0] diff --git a/internal/appsec/waf.go b/internal/appsec/waf.go index df927d5be3..a921899377 100644 --- a/internal/appsec/waf.go +++ b/internal/appsec/waf.go @@ -115,12 +115,13 @@ func newHTTPWAFEventListener(handle *waf.Handle, addresses []string, timeout tim matches, actionIds := runWAF(wafCtx, values, timeout) if len(matches) > 0 { + interrupt := false for _, id := range actionIds { - actionHandler.Apply(id, op) + interrupt = actionHandler.Apply(id, op) || interrupt } op.AddSecurityEvents(matches) log.Debug("appsec: WAF detected an attack before executing the request") - if len(actionIds) > 0 { + if interrupt { return } } From 17010e718b8e4f23de34fe293f33f90efc11c2cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mazeau?= Date: Wed, 23 Nov 2022 16:53:47 +0100 Subject: [PATCH 11/31] internal/appsec: add mutex for Operation actions --- internal/appsec/dyngo/instrumentation/httpsec/http.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/internal/appsec/dyngo/instrumentation/httpsec/http.go b/internal/appsec/dyngo/instrumentation/httpsec/http.go index 1fd150439a..8a92a6342a 100644 --- a/internal/appsec/dyngo/instrumentation/httpsec/http.go +++ b/internal/appsec/dyngo/instrumentation/httpsec/http.go @@ -18,6 +18,7 @@ import ( "os" "reflect" "strings" + "sync" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace" "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/dyngo" @@ -169,6 +170,7 @@ type ( dyngo.Operation instrumentation.TagsHolder instrumentation.SecurityEventsHolder + mu sync.RWMutex actions []Action } @@ -210,15 +212,21 @@ func (op *Operation) Finish(res HandlerOperationRes) []json.RawMessage { // Actions returns the actions linked to the operation func (op *Operation) Actions() []Action { + op.mu.RLock() + defer op.mu.RUnlock() return op.actions } func (op *Operation) AddAction(a Action) { + op.mu.Lock() + defer op.mu.Unlock() op.actions = append(op.actions, a) } // ClearActions clears all the actions linked to the operation func (op *Operation) ClearActions() { + op.mu.Lock() + defer op.mu.Unlock() op.actions = op.actions[0:0] } From aad08f89101c8091e378d944a2f148db58d6da40 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mazeau?= Date: Wed, 23 Nov 2022 16:54:56 +0100 Subject: [PATCH 12/31] internal/appsec: add mutex for action handler --- internal/appsec/dyngo/instrumentation/httpsec/actions.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/internal/appsec/dyngo/instrumentation/httpsec/actions.go b/internal/appsec/dyngo/instrumentation/httpsec/actions.go index 7b7402bb75..da1b85a855 100644 --- a/internal/appsec/dyngo/instrumentation/httpsec/actions.go +++ b/internal/appsec/dyngo/instrumentation/httpsec/actions.go @@ -7,6 +7,7 @@ package httpsec import ( "net/http" + "sync" "gopkg.in/DataDog/dd-trace-go.v1/internal/log" ) @@ -62,6 +63,7 @@ func newBlockRequestHandler(status int, ct string, payload []byte) http.Handler // ActionsHandler handles actions registration and their application to operations type ActionsHandler struct { + mu sync.RWMutex actions map[string]Action } @@ -81,13 +83,17 @@ func NewActionsHandler() *ActionsHandler { // RegisterAction registers a specific action to the handler. If the action kind is unknown // the action will not be registered func (h *ActionsHandler) RegisterAction(id string, a Action) { + h.mu.Lock() + defer h.mu.Unlock() h.actions[id] = a } // Apply applies the action identified by `id` for the given operation // Returns true if the applied action will interrupt the request flow (block, redirect, etc...) func (h *ActionsHandler) Apply(id string, op *Operation) bool { + h.mu.RLock() a, ok := h.actions[id] + h.mu.RUnlock() if !ok { log.Debug("appsec: ignoring the returned waf action: unknown action id `%s`", id) return false From 8a48b475d093127eac374d033f4a5781112f55ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mazeau?= Date: Wed, 23 Nov 2022 17:19:46 +0100 Subject: [PATCH 13/31] internal/appsec: add RLock call to security events holder --- internal/appsec/dyngo/instrumentation/common.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/appsec/dyngo/instrumentation/common.go b/internal/appsec/dyngo/instrumentation/common.go index 50b6dbd8fd..20fa4017cb 100644 --- a/internal/appsec/dyngo/instrumentation/common.go +++ b/internal/appsec/dyngo/instrumentation/common.go @@ -49,7 +49,7 @@ func (m *TagsHolder) Tags() map[string]interface{} { // See httpsec/http.go and grpcsec/grpc.go. type SecurityEventsHolder struct { events []json.RawMessage - mu sync.Mutex + mu sync.RWMutex } // AddSecurityEvents adds the security events to the collected events list. @@ -62,6 +62,8 @@ func (s *SecurityEventsHolder) AddSecurityEvents(events ...json.RawMessage) { // Events returns the list of stored events. func (s *SecurityEventsHolder) Events() []json.RawMessage { + s.mu.RLock() + defer s.mu.RUnlock() return s.events } From 9f6411eafd0a2c9b2c6b17b2de6a92ec3ad5a79a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mazeau?= Date: Wed, 23 Nov 2022 18:00:58 +0100 Subject: [PATCH 14/31] appsec: grpc: remove unary/stream handlers and use status code --- contrib/google.golang.org/grpc/appsec.go | 15 +++++++++------ .../dyngo/instrumentation/grpcsec/actions.go | 12 +----------- .../appsec/dyngo/instrumentation/grpcsec/grpc.go | 5 ++--- 3 files changed, 12 insertions(+), 20 deletions(-) diff --git a/contrib/google.golang.org/grpc/appsec.go b/contrib/google.golang.org/grpc/appsec.go index a75a7ff884..633c9acfab 100644 --- a/contrib/google.golang.org/grpc/appsec.go +++ b/contrib/google.golang.org/grpc/appsec.go @@ -17,6 +17,7 @@ import ( "google.golang.org/grpc" "google.golang.org/grpc/metadata" "google.golang.org/grpc/peer" + "google.golang.org/grpc/status" ) // UnaryHandler wrapper to use when AppSec is enabled to monitor its execution. @@ -25,9 +26,6 @@ func appsecUnaryHandlerMiddleware(span ddtrace.Span, handler grpc.UnaryHandler) return func(ctx context.Context, req interface{}) (interface{}, error) { md, _ := metadata.FromIncomingContext(ctx) op := grpcsec.StartHandlerOperation(grpcsec.HandlerOperationArgs{Metadata: md}, nil) - if op.UnaryHandler != nil { - return op.UnaryHandler(ctx, req) - } defer func() { events := op.Finish(grpcsec.HandlerOperationRes{}) instrumentation.SetTags(span, op.Tags()) @@ -36,6 +34,10 @@ func appsecUnaryHandlerMiddleware(span ddtrace.Span, handler grpc.UnaryHandler) } setAppSecTags(ctx, span, events) }() + if op.BlockedCode != nil { + op.AddTag("appsec.blocked", true) + return nil, status.Errorf(*op.BlockedCode, "Request blocked") + } defer grpcsec.StartReceiveOperation(grpcsec.ReceiveOperationArgs{}, op).Finish(grpcsec.ReceiveOperationRes{Message: req}) return handler(ctx, req) } @@ -47,9 +49,6 @@ func appsecStreamHandlerMiddleware(span ddtrace.Span, handler grpc.StreamHandler return func(srv interface{}, stream grpc.ServerStream) error { md, _ := metadata.FromIncomingContext(stream.Context()) op := grpcsec.StartHandlerOperation(grpcsec.HandlerOperationArgs{Metadata: md}, nil) - if op.StreamHandler != nil { - return op.StreamHandler(srv, stream) - } defer func() { events := op.Finish(grpcsec.HandlerOperationRes{}) instrumentation.SetTags(span, op.Tags()) @@ -58,6 +57,10 @@ func appsecStreamHandlerMiddleware(span ddtrace.Span, handler grpc.StreamHandler } setAppSecTags(stream.Context(), span, events) }() + if op.BlockedCode != nil { + op.AddTag("appsec.blocked", true) + return status.Error(*op.BlockedCode, "Request blocked") + } return handler(srv, appsecServerStream{ServerStream: stream, handlerOperation: op}) } } diff --git a/internal/appsec/dyngo/instrumentation/grpcsec/actions.go b/internal/appsec/dyngo/instrumentation/grpcsec/actions.go index 35dec93330..8e2cc90229 100644 --- a/internal/appsec/dyngo/instrumentation/grpcsec/actions.go +++ b/internal/appsec/dyngo/instrumentation/grpcsec/actions.go @@ -9,11 +9,7 @@ package grpcsec import ( - "context" - - "google.golang.org/grpc" "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" ) type ActionParam interface{} @@ -67,13 +63,7 @@ func (h *actionsHandler) Exec(id string, op *HandlerOperation) { } // Currently, only the "block_request" type is supported, so we only need to check for blockRequestParams if p, ok := a.params.(BlockRequestParams); ok { - err := status.Error(p.Status, "Request blocked") - op.UnaryHandler = func(ctx context.Context, req interface{}) (interface{}, error) { - return nil, err - } - op.StreamHandler = func(srv interface{}, stream grpc.ServerStream) error { - return err - } + op.BlockedCode = &p.Status } } diff --git a/internal/appsec/dyngo/instrumentation/grpcsec/grpc.go b/internal/appsec/dyngo/instrumentation/grpcsec/grpc.go index ff65b25501..a61c52322c 100644 --- a/internal/appsec/dyngo/instrumentation/grpcsec/grpc.go +++ b/internal/appsec/dyngo/instrumentation/grpcsec/grpc.go @@ -16,7 +16,7 @@ import ( "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/dyngo" "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/dyngo/instrumentation" - "google.golang.org/grpc" + "google.golang.org/grpc/codes" ) // Abstract gRPC server handler operation definitions. It is based on two @@ -41,8 +41,7 @@ type ( dyngo.Operation instrumentation.TagsHolder instrumentation.SecurityEventsHolder - UnaryHandler grpc.UnaryHandler - StreamHandler grpc.StreamHandler + BlockedCode *codes.Code } // HandlerOperationArgs is the grpc handler arguments. HandlerOperationArgs struct { From 4a7572349c4571d7fcf42f6a28676396d26e5f57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mazeau?= Date: Thu, 24 Nov 2022 10:23:09 +0100 Subject: [PATCH 15/31] internal/appsec: go lint --- internal/appsec/dyngo/instrumentation/httpsec/actions.go | 1 + internal/appsec/dyngo/instrumentation/httpsec/http.go | 1 + 2 files changed, 2 insertions(+) diff --git a/internal/appsec/dyngo/instrumentation/httpsec/actions.go b/internal/appsec/dyngo/instrumentation/httpsec/actions.go index da1b85a855..f2b7da07c6 100644 --- a/internal/appsec/dyngo/instrumentation/httpsec/actions.go +++ b/internal/appsec/dyngo/instrumentation/httpsec/actions.go @@ -25,6 +25,7 @@ type BlockRequestAction struct { func (*BlockRequestAction) isAction() {} +// NewBlockRequestAction creates, initializes and returns a new BlockRequestAction func NewBlockRequestAction(status int, template string) BlockRequestAction { htmlHandler := newBlockRequestHandler(status, "application/html", BlockedTemplateHTML) jsonHandler := newBlockRequestHandler(status, "application/json", BlockedTemplateJSON) diff --git a/internal/appsec/dyngo/instrumentation/httpsec/http.go b/internal/appsec/dyngo/instrumentation/httpsec/http.go index 8a92a6342a..2a80d9ab9d 100644 --- a/internal/appsec/dyngo/instrumentation/httpsec/http.go +++ b/internal/appsec/dyngo/instrumentation/httpsec/http.go @@ -217,6 +217,7 @@ func (op *Operation) Actions() []Action { return op.actions } +// AddAction adds an action to the operation func (op *Operation) AddAction(a Action) { op.mu.Lock() defer op.mu.Unlock() From 49a7c27891225b5518991173223096d897e33461 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mazeau?= Date: Thu, 24 Nov 2022 11:18:33 +0100 Subject: [PATCH 16/31] internal/appsec: move applyActions() to a dedicated function --- .../dyngo/instrumentation/httpsec/http.go | 32 +++++++++++-------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/internal/appsec/dyngo/instrumentation/httpsec/http.go b/internal/appsec/dyngo/instrumentation/httpsec/http.go index 2a80d9ab9d..661c70e872 100644 --- a/internal/appsec/dyngo/instrumentation/httpsec/http.go +++ b/internal/appsec/dyngo/instrumentation/httpsec/http.go @@ -72,23 +72,25 @@ func MonitorParsedBody(ctx context.Context, body interface{}) { } } +// applyActions executes the operation's actions and returns the resulting http handler +func applyActions(op *Operation) http.Handler { + defer op.ClearActions() + for _, action := range op.Actions() { + switch a := action.(type) { + case *BlockRequestAction: + op.AddTag(tagBlockedRequest, true) + return a.handler + default: + log.Error("appsec: ignoring security action: unexpected action type %T", a) + } + } + return nil +} + // WrapHandler wraps the given HTTP handler with the abstract HTTP operation defined by HandlerOperationArgs and // HandlerOperationRes. func WrapHandler(handler http.Handler, span ddtrace.Span, pathParams map[string]string) http.Handler { instrumentation.SetAppSecEnabledTags(span) - applyActions := func(op *Operation) { - for _, action := range op.Actions() { - switch a := action.(type) { - case *BlockRequestAction: - handler = a.handler - op.AddTag(tagBlockedRequest, true) - default: - log.Error("appsec: ignoring security action: unexpected action type %T", a) - } - } - op.ClearActions() - } - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { SetIPTags(span, r) @@ -96,7 +98,9 @@ func WrapHandler(handler http.Handler, span ddtrace.Span, pathParams map[string] ctx, op := StartOperation(r.Context(), args) r = r.WithContext(ctx) - applyActions(op) + if h := applyActions(op); h != nil { + handler = h + } defer func() { var status int if mw, ok := w.(interface{ Status() int }); ok { From 4320cd0a98bdb696485b7bfa06a0fede65128ace Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mazeau?= Date: Thu, 24 Nov 2022 15:04:19 +0100 Subject: [PATCH 17/31] appsec: grpc: retrieve http.client_ip and use action handler --- contrib/google.golang.org/grpc/appsec.go | 7 +- .../dyngo/instrumentation/grpcsec/actions.go | 64 +++++++++---------- .../dyngo/instrumentation/grpcsec/grpc.go | 1 + .../dyngo/instrumentation/httpsec/http.go | 18 ++++-- .../instrumentation/httpsec/ip_default.go | 22 ------- .../dyngo/instrumentation/httpsec/ip_go119.go | 26 -------- .../dyngo/instrumentation/httpsec/tags.go | 16 ++--- .../instrumentation/httpsec/tags_test.go | 59 ++++++++--------- .../dyngo/instrumentation/ip_default.go | 30 +++++++++ .../appsec/dyngo/instrumentation/ip_go119.go | 34 ++++++++++ internal/appsec/waf.go | 31 ++++++++- 11 files changed, 179 insertions(+), 129 deletions(-) delete mode 100644 internal/appsec/dyngo/instrumentation/httpsec/ip_default.go delete mode 100644 internal/appsec/dyngo/instrumentation/httpsec/ip_go119.go create mode 100644 internal/appsec/dyngo/instrumentation/ip_default.go create mode 100644 internal/appsec/dyngo/instrumentation/ip_go119.go diff --git a/contrib/google.golang.org/grpc/appsec.go b/contrib/google.golang.org/grpc/appsec.go index 633c9acfab..9d25329d82 100644 --- a/contrib/google.golang.org/grpc/appsec.go +++ b/contrib/google.golang.org/grpc/appsec.go @@ -12,6 +12,7 @@ import ( "gopkg.in/DataDog/dd-trace-go.v1/ddtrace" "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/dyngo/instrumentation" "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/dyngo/instrumentation/grpcsec" + "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/dyngo/instrumentation/httpsec" "golang.org/x/net/context" "google.golang.org/grpc" @@ -25,7 +26,8 @@ func appsecUnaryHandlerMiddleware(span ddtrace.Span, handler grpc.UnaryHandler) instrumentation.SetAppSecEnabledTags(span) return func(ctx context.Context, req interface{}) (interface{}, error) { md, _ := metadata.FromIncomingContext(ctx) - op := grpcsec.StartHandlerOperation(grpcsec.HandlerOperationArgs{Metadata: md}, nil) + ip := httpsec.IPFromHeaders(httpsec.NormalizeHTTPHeaders(md), "") + op := grpcsec.StartHandlerOperation(grpcsec.HandlerOperationArgs{Metadata: md, ClientIP: ip}, nil) defer func() { events := op.Finish(grpcsec.HandlerOperationRes{}) instrumentation.SetTags(span, op.Tags()) @@ -48,7 +50,8 @@ func appsecStreamHandlerMiddleware(span ddtrace.Span, handler grpc.StreamHandler instrumentation.SetAppSecEnabledTags(span) return func(srv interface{}, stream grpc.ServerStream) error { md, _ := metadata.FromIncomingContext(stream.Context()) - op := grpcsec.StartHandlerOperation(grpcsec.HandlerOperationArgs{Metadata: md}, nil) + ip := httpsec.IPFromHeaders(httpsec.NormalizeHTTPHeaders(md), "") + op := grpcsec.StartHandlerOperation(grpcsec.HandlerOperationArgs{Metadata: md, ClientIP: ip}, nil) defer func() { events := op.Finish(grpcsec.HandlerOperationRes{}) instrumentation.SetTags(span, op.Tags()) diff --git a/internal/appsec/dyngo/instrumentation/grpcsec/actions.go b/internal/appsec/dyngo/instrumentation/grpcsec/actions.go index 8e2cc90229..845ac5d3ca 100644 --- a/internal/appsec/dyngo/instrumentation/grpcsec/actions.go +++ b/internal/appsec/dyngo/instrumentation/grpcsec/actions.go @@ -9,67 +9,61 @@ package grpcsec import ( + "sync" + "google.golang.org/grpc/codes" ) -type ActionParam interface{} - -type action struct { - id string - params ActionParam -} - -// ActionHandler handles WAF actions registration and execution -type ActionHandler interface { - RegisterAction(id string, params ActionParam) - Exec(id string, op *HandlerOperation) +// Action is used to identify any action kind +type Action interface { + isAction() } -type actionsHandler struct { - actions map[string]action +// ActionsHandler handles WAF actions registration and execution +type ActionsHandler struct { + mu sync.RWMutex + actions map[string]Action } // NewActionsHandler returns an action handler holding the default ASM actions. // Currently, only the default "block" action is supported -func NewActionsHandler() ActionHandler { - defaultBlockAction := action{ - id: "block", - params: BlockRequestParams{ - Status: codes.Aborted, - }, - } - // Register the default "block" action as specified in the RFC for HTTP blocking - actions := map[string]action{defaultBlockAction.id: defaultBlockAction} +func NewActionsHandler() ActionsHandler { + // Register the default "block" action as specified in the blocking RFC + actions := map[string]Action{"block": &BlockRequestAction{Status: codes.Aborted}} - return &actionsHandler{ + return ActionsHandler{ actions: actions, } } // RegisterAction registers a specific action to the actions handler. If the action kind is unknown // the action will have no effect -func (h *actionsHandler) RegisterAction(id string, params ActionParam) { - h.actions[id] = action{ - id: id, - params: params, - } +func (h *ActionsHandler) RegisterAction(id string, a Action) { + h.mu.Lock() + defer h.mu.Unlock() + h.actions[id] = a } -// Exec executes the action identified by `id` -func (h *actionsHandler) Exec(id string, op *HandlerOperation) { +// Apply executes the action identified by `id` +func (h *ActionsHandler) Apply(id string, op *HandlerOperation) bool { + h.mu.RLock() a, ok := h.actions[id] + h.mu.RUnlock() if !ok { - return + return false } // Currently, only the "block_request" type is supported, so we only need to check for blockRequestParams - if p, ok := a.params.(BlockRequestParams); ok { + if p, ok := a.(*BlockRequestAction); ok { op.BlockedCode = &p.Status + return true } + return false } -// BlockRequestParams is the parameter struct used to perform actions of kind ActionBlockRequest -type BlockRequestParams struct { - ActionParam +// BlockRequestAction is the struct used to perform the request blocking action +type BlockRequestAction struct { // Status is the return code to use when blocking the request Status codes.Code } + +func (*BlockRequestAction) isAction() {} diff --git a/internal/appsec/dyngo/instrumentation/grpcsec/grpc.go b/internal/appsec/dyngo/instrumentation/grpcsec/grpc.go index a61c52322c..c581afd637 100644 --- a/internal/appsec/dyngo/instrumentation/grpcsec/grpc.go +++ b/internal/appsec/dyngo/instrumentation/grpcsec/grpc.go @@ -48,6 +48,7 @@ type ( // Message received by the gRPC handler. // Corresponds to the address `grpc.server.request.metadata`. Metadata map[string][]string + ClientIP instrumentation.NetaddrIP } // HandlerOperationRes is the grpc handler results. Empty as of today. HandlerOperationRes struct{} diff --git a/internal/appsec/dyngo/instrumentation/httpsec/http.go b/internal/appsec/dyngo/instrumentation/httpsec/http.go index 661c70e872..929fb47844 100644 --- a/internal/appsec/dyngo/instrumentation/httpsec/http.go +++ b/internal/appsec/dyngo/instrumentation/httpsec/http.go @@ -41,7 +41,7 @@ type ( // PathParams corresponds to the address `server.request.path_params` PathParams map[string]string // ClientIP corresponds to the addres `http.client_ip` - ClientIP netaddrIP + ClientIP instrumentation.NetaddrIP } // HandlerOperationRes is the HTTP handler operation results. @@ -310,8 +310,8 @@ func (f OnSDKBodyOperationFinish) Call(op dyngo.Operation, v interface{}) { f(op.(*SDKBodyOperation), v.(SDKBodyOperationRes)) } -// IPFromRequest returns the resolved client IP for a specific request. The returned IP can be invalid. -func IPFromRequest(r *http.Request) netaddrIP { +// IPFromHeaders returns the resolved client IP for set of normalized headers. The returned IP can be invalid. +func IPFromHeaders(hdrs map[string]string, remoteAddr string) instrumentation.NetaddrIP { ipHeaders := defaultIPHeaders if len(clientIPHeader) > 0 { ipHeaders = []string{clientIPHeader} @@ -319,13 +319,13 @@ func IPFromRequest(r *http.Request) netaddrIP { var headers []string var ips []string for _, hdr := range ipHeaders { - if v := r.Header.Get(hdr); v != "" { + if v, ok := hdrs[hdr]; ok && v != "" { headers = append(headers, hdr) ips = append(ips, v) } } if len(ips) == 0 { - if remoteIP := parseIP(r.RemoteAddr); remoteIP.IsValid() && isGlobal(remoteIP) { + if remoteIP := parseIP(remoteAddr); remoteIP.IsValid() && isGlobal(remoteIP) { return remoteIP } } else if len(ips) == 1 { @@ -337,7 +337,13 @@ func IPFromRequest(r *http.Request) netaddrIP { } } - return netaddrIP{} + return instrumentation.NetaddrIP{} +} + +// IPFromRequest returns the resolved client IP for a specific request. The returned IP can be invalid. +func IPFromRequest(r *http.Request) instrumentation.NetaddrIP { + normalized := NormalizeHTTPHeaders(r.Header) + return IPFromHeaders(normalized, r.RemoteAddr) } var ( diff --git a/internal/appsec/dyngo/instrumentation/httpsec/ip_default.go b/internal/appsec/dyngo/instrumentation/httpsec/ip_default.go deleted file mode 100644 index a96f74ef02..0000000000 --- a/internal/appsec/dyngo/instrumentation/httpsec/ip_default.go +++ /dev/null @@ -1,22 +0,0 @@ -// Unless explicitly stated otherwise all files in this repository are licensed -// under the Apache License Version 2.0. -// This product includes software developed at Datadog (https://www.datadoghq.com/). -// Copyright 2022 Datadog, Inc. - -//go:build !go1.19 -// +build !go1.19 - -package httpsec - -import "inet.af/netaddr" - -type netaddrIP = netaddr.IP -type netaddrIPPrefix = netaddr.IPPrefix - -var ( - netaddrParseIP = netaddr.ParseIP - netaddrParseIPPrefix = netaddr.ParseIPPrefix - netaddrMustParseIP = netaddr.MustParseIP - netaddrIPv4 = netaddr.IPv4 - netaddrIPv6Raw = netaddr.IPv6Raw -) diff --git a/internal/appsec/dyngo/instrumentation/httpsec/ip_go119.go b/internal/appsec/dyngo/instrumentation/httpsec/ip_go119.go deleted file mode 100644 index dbea4b60d3..0000000000 --- a/internal/appsec/dyngo/instrumentation/httpsec/ip_go119.go +++ /dev/null @@ -1,26 +0,0 @@ -// Unless explicitly stated otherwise all files in this repository are licensed -// under the Apache License Version 2.0. -// This product includes software developed at Datadog (https://www.datadoghq.com/). -// Copyright 2022 Datadog, Inc. - -//go:build go1.19 -// +build go1.19 - -package httpsec - -import "net/netip" - -type netaddrIP = netip.Addr -type netaddrIPPrefix = netip.Prefix - -var ( - netaddrParseIP = netip.ParseAddr - netaddrParseIPPrefix = netip.ParsePrefix - netaddrMustParseIP = netip.MustParseAddr - netaddrIPv6Raw = netip.AddrFrom16 -) - -func netaddrIPv4(a, b, c, d byte) netaddrIP { - e := [4]byte{a, b, c, d} - return netip.AddrFrom4(e) -} diff --git a/internal/appsec/dyngo/instrumentation/httpsec/tags.go b/internal/appsec/dyngo/instrumentation/httpsec/tags.go index 769c9dc194..65dd5c4b5f 100644 --- a/internal/appsec/dyngo/instrumentation/httpsec/tags.go +++ b/internal/appsec/dyngo/instrumentation/httpsec/tags.go @@ -29,7 +29,7 @@ const ( ) var ( - ipv6SpecialNetworks = []*netaddrIPPrefix{ + ipv6SpecialNetworks = []*instrumentation.NetaddrIPPrefix{ ippref("fec0::/10"), // site local } defaultIPHeaders = []string{ @@ -98,8 +98,8 @@ func NormalizeHTTPHeaders(headers map[string][]string) (normalized map[string]st } // ippref returns the IP network from an IP address string s. If not possible, it returns nil. -func ippref(s string) *netaddrIPPrefix { - if prefix, err := netaddrParseIPPrefix(s); err == nil { +func ippref(s string) *instrumentation.NetaddrIPPrefix { + if prefix, err := instrumentation.NetaddrParseIPPrefix(s); err == nil { return &prefix } return nil @@ -144,19 +144,19 @@ func SetIPTags(span instrumentation.TagSetter, r *http.Request) { } } -func parseIP(s string) netaddrIP { - if ip, err := netaddrParseIP(s); err == nil { +func parseIP(s string) instrumentation.NetaddrIP { + if ip, err := instrumentation.NetaddrParseIP(s); err == nil { return ip } if h, _, err := net.SplitHostPort(s); err == nil { - if ip, err := netaddrParseIP(h); err == nil { + if ip, err := instrumentation.NetaddrParseIP(h); err == nil { return ip } } - return netaddrIP{} + return instrumentation.NetaddrIP{} } -func isGlobal(ip netaddrIP) bool { +func isGlobal(ip instrumentation.NetaddrIP) bool { // IsPrivate also checks for ipv6 ULA. // We care to check for these addresses are not considered public, hence not global. // See https://www.rfc-editor.org/rfc/rfc4193.txt for more details. diff --git a/internal/appsec/dyngo/instrumentation/httpsec/tags_test.go b/internal/appsec/dyngo/instrumentation/httpsec/tags_test.go index dcbcc06a97..06f0c7868c 100644 --- a/internal/appsec/dyngo/instrumentation/httpsec/tags_test.go +++ b/internal/appsec/dyngo/instrumentation/httpsec/tags_test.go @@ -11,6 +11,7 @@ import ( "testing" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext" + "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/dyngo/instrumentation" "github.com/stretchr/testify/require" ) @@ -58,7 +59,7 @@ type ipTestCase struct { name string remoteAddr string headers map[string]string - expectedIP netaddrIP + expectedIP instrumentation.NetaddrIP multiHeaders string clientIPHeader string } @@ -74,12 +75,12 @@ func genIPTestCases() []ipTestCase { tcs = append(tcs, ipTestCase{ name: "ipv4-global." + header, headers: map[string]string{header: ipv4Global}, - expectedIP: netaddrMustParseIP(ipv4Global), + expectedIP: instrumentation.NetaddrMustParseIP(ipv4Global), }) tcs = append(tcs, ipTestCase{ name: "ipv4-private." + header, headers: map[string]string{header: ipv4Private}, - expectedIP: netaddrIP{}, + expectedIP: instrumentation.NetaddrIP{}, }) } // Simple ipv6 test cases over all headers @@ -87,12 +88,12 @@ func genIPTestCases() []ipTestCase { tcs = append(tcs, ipTestCase{ name: "ipv6-global." + header, headers: map[string]string{header: ipv6Global}, - expectedIP: netaddrMustParseIP(ipv6Global), + expectedIP: instrumentation.NetaddrMustParseIP(ipv6Global), }) tcs = append(tcs, ipTestCase{ name: "ipv6-private." + header, headers: map[string]string{header: ipv6Private}, - expectedIP: netaddrIP{}, + expectedIP: instrumentation.NetaddrIP{}, }) } // private and global in same header @@ -100,22 +101,22 @@ func genIPTestCases() []ipTestCase { { name: "ipv4-private+global", headers: map[string]string{"x-forwarded-for": ipv4Private + "," + ipv4Global}, - expectedIP: netaddrMustParseIP(ipv4Global), + expectedIP: instrumentation.NetaddrMustParseIP(ipv4Global), }, { name: "ipv4-global+private", headers: map[string]string{"x-forwarded-for": ipv4Global + "," + ipv4Private}, - expectedIP: netaddrMustParseIP(ipv4Global), + expectedIP: instrumentation.NetaddrMustParseIP(ipv4Global), }, { name: "ipv6-private+global", headers: map[string]string{"x-forwarded-for": ipv6Private + "," + ipv6Global}, - expectedIP: netaddrMustParseIP(ipv6Global), + expectedIP: instrumentation.NetaddrMustParseIP(ipv6Global), }, { name: "ipv6-global+private", headers: map[string]string{"x-forwarded-for": ipv6Global + "," + ipv6Private}, - expectedIP: netaddrMustParseIP(ipv6Global), + expectedIP: instrumentation.NetaddrMustParseIP(ipv6Global), }, }, tcs...) // Invalid IPs (or a mix of valid/invalid over a single or multiple headers) @@ -123,67 +124,67 @@ func genIPTestCases() []ipTestCase { { name: "invalid-ipv4", headers: map[string]string{"x-forwarded-for": "127..0.0.1"}, - expectedIP: netaddrIP{}, + expectedIP: instrumentation.NetaddrIP{}, }, { name: "invalid-ipv4-recover", headers: map[string]string{"x-forwarded-for": "127..0.0.1, " + ipv4Global}, - expectedIP: netaddrMustParseIP(ipv4Global), + expectedIP: instrumentation.NetaddrMustParseIP(ipv4Global), }, { name: "ipv4-multi-header-1", headers: map[string]string{"x-forwarded-for": "127.0.0.1", "forwarded-for": ipv4Global}, - expectedIP: netaddrIP{}, + expectedIP: instrumentation.NetaddrIP{}, multiHeaders: "x-forwarded-for,forwarded-for", }, { name: "ipv4-multi-header-2", headers: map[string]string{"forwarded-for": ipv4Global, "x-forwarded-for": "127.0.0.1"}, - expectedIP: netaddrIP{}, + expectedIP: instrumentation.NetaddrIP{}, multiHeaders: "x-forwarded-for,forwarded-for", }, { name: "invalid-ipv6", headers: map[string]string{"x-forwarded-for": "2001:0db8:2001:zzzz::"}, - expectedIP: netaddrIP{}, + expectedIP: instrumentation.NetaddrIP{}, }, { name: "invalid-ipv6-recover", headers: map[string]string{"x-forwarded-for": "2001:0db8:2001:zzzz::, " + ipv6Global}, - expectedIP: netaddrMustParseIP(ipv6Global), + expectedIP: instrumentation.NetaddrMustParseIP(ipv6Global), }, { name: "ipv6-multi-header-1", headers: map[string]string{"x-forwarded-for": "2001:0db8:2001:zzzz::", "forwarded-for": ipv6Global}, - expectedIP: netaddrIP{}, + expectedIP: instrumentation.NetaddrIP{}, multiHeaders: "x-forwarded-for,forwarded-for", }, { name: "ipv6-multi-header-2", headers: map[string]string{"forwarded-for": ipv6Global, "x-forwarded-for": "2001:0db8:2001:zzzz::"}, - expectedIP: netaddrIP{}, + expectedIP: instrumentation.NetaddrIP{}, multiHeaders: "x-forwarded-for,forwarded-for", }, }, tcs...) tcs = append([]ipTestCase{ { name: "no-headers", - expectedIP: netaddrIP{}, + expectedIP: instrumentation.NetaddrIP{}, }, { name: "header-case", - expectedIP: netaddrMustParseIP(ipv4Global), + expectedIP: instrumentation.NetaddrMustParseIP(ipv4Global), headers: map[string]string{"X-fOrWaRdEd-FoR": ipv4Global}, }, { name: "user-header", - expectedIP: netaddrMustParseIP(ipv4Global), + expectedIP: instrumentation.NetaddrMustParseIP(ipv4Global), headers: map[string]string{"x-forwarded-for": ipv6Global, "custom-header": ipv4Global}, clientIPHeader: "custom-header", }, { name: "user-header-not-found", - expectedIP: netaddrIP{}, + expectedIP: instrumentation.NetaddrIP{}, headers: map[string]string{"x-forwarded-for": ipv4Global}, clientIPHeader: "custom-header", }, @@ -239,12 +240,12 @@ func TestIPHeaders(t *testing.T) { } } -func randIPv4() netaddrIP { - return netaddrIPv4(uint8(rand.Uint32()), uint8(rand.Uint32()), uint8(rand.Uint32()), uint8(rand.Uint32())) +func randIPv4() instrumentation.NetaddrIP { + return instrumentation.NetaddrIPv4(uint8(rand.Uint32()), uint8(rand.Uint32()), uint8(rand.Uint32()), uint8(rand.Uint32())) } -func randIPv6() netaddrIP { - return netaddrIPv6Raw([16]byte{ +func randIPv6() instrumentation.NetaddrIP { + return instrumentation.NetaddrIPv6Raw([16]byte{ uint8(rand.Uint32()), uint8(rand.Uint32()), uint8(rand.Uint32()), uint8(rand.Uint32()), uint8(rand.Uint32()), uint8(rand.Uint32()), uint8(rand.Uint32()), uint8(rand.Uint32()), uint8(rand.Uint32()), uint8(rand.Uint32()), uint8(rand.Uint32()), uint8(rand.Uint32()), @@ -252,7 +253,7 @@ func randIPv6() netaddrIP { }) } -func randGlobalIPv4() netaddrIP { +func randGlobalIPv4() instrumentation.NetaddrIP { for { ip := randIPv4() if isGlobal(ip) { @@ -261,7 +262,7 @@ func randGlobalIPv4() netaddrIP { } } -func randGlobalIPv6() netaddrIP { +func randGlobalIPv6() instrumentation.NetaddrIP { for { ip := randIPv6() if isGlobal(ip) { @@ -270,7 +271,7 @@ func randGlobalIPv6() netaddrIP { } } -func randPrivateIPv4() netaddrIP { +func randPrivateIPv4() instrumentation.NetaddrIP { for { ip := randIPv4() if !isGlobal(ip) && ip.IsPrivate() { @@ -279,7 +280,7 @@ func randPrivateIPv4() netaddrIP { } } -func randPrivateIPv6() netaddrIP { +func randPrivateIPv6() instrumentation.NetaddrIP { for { ip := randIPv6() if !isGlobal(ip) && ip.IsPrivate() { diff --git a/internal/appsec/dyngo/instrumentation/ip_default.go b/internal/appsec/dyngo/instrumentation/ip_default.go new file mode 100644 index 0000000000..3fde0af771 --- /dev/null +++ b/internal/appsec/dyngo/instrumentation/ip_default.go @@ -0,0 +1,30 @@ +// Unless explicitly stated otherwise all files in this repository are licensed +// under the Apache License Version 2.0. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2022 Datadog, Inc. + +//go:build !go1.19 +// +build !go1.19 + +package instrumentation + +import "inet.af/netaddr" + +// NetaddrIP wraps an netaddr.IP value +type NetaddrIP = netaddr.IP + +// NetaddrIPPrefix wraps an netaddr.IPPrefix value +type NetaddrIPPrefix = netaddr.IPPrefix + +var ( + // NetaddrParseIP wraps the netaddr.ParseIP function + NetaddrParseIP = netaddr.ParseIP + // NetaddrParseIPPrefix wraps the netaddr.ParseIPPrefix function + NetaddrParseIPPrefix = netaddr.ParseIPPrefix + // NetaddrMustParseIP wraps the netaddr.MustParseIP function + NetaddrMustParseIP = netaddr.MustParseIP + // NetaddrIPv4 wraps the netaddr.IPv4 function + NetaddrIPv4 = netaddr.IPv4 + // NetaddrIPv6Raw wraps the netaddr.IPv6Raw function + NetaddrIPv6Raw = netaddr.IPv6Raw +) diff --git a/internal/appsec/dyngo/instrumentation/ip_go119.go b/internal/appsec/dyngo/instrumentation/ip_go119.go new file mode 100644 index 0000000000..78b3b0e561 --- /dev/null +++ b/internal/appsec/dyngo/instrumentation/ip_go119.go @@ -0,0 +1,34 @@ +// Unless explicitly stated otherwise all files in this repository are licensed +// under the Apache License Version 2.0. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2022 Datadog, Inc. + +//go:build go1.19 +// +build go1.19 + +package instrumentation + +import "net/netip" + +// NetaddrIP wraps a netip.Addr value +type NetaddrIP = netip.Addr + +// NetaddrIPPrefix wraps a netip.Prefix value +type NetaddrIPPrefix = netip.Prefix + +var ( + // NetaddrParseIP wraps the netip.ParseAddr function + NetaddrParseIP = netip.ParseAddr + // NetaddrParseIPPrefix wraps the netip.ParsePrefix function + NetaddrParseIPPrefix = netip.ParsePrefix + // NetaddrMustParseIP wraps the netip.MustParseAddr function + NetaddrMustParseIP = netip.MustParseAddr + // NetaddrIPv6Raw wraps the netIP.AddrFrom16 function + NetaddrIPv6Raw = netip.AddrFrom16 +) + +// NetaddrIPv4 wraps the netip.AddrFrom4 function +func NetaddrIPv4(a, b, c, d byte) NetaddrIP { + e := [4]byte{a, b, c, d} + return netip.AddrFrom4(e) +} diff --git a/internal/appsec/waf.go b/internal/appsec/waf.go index a921899377..b830887315 100644 --- a/internal/appsec/waf.go +++ b/internal/appsec/waf.go @@ -193,8 +193,9 @@ func newHTTPWAFEventListener(handle *waf.Handle, addresses []string, timeout tim // newGRPCWAFEventListener returns the WAF event listener to register in order // to enable it. -func newGRPCWAFEventListener(handle *waf.Handle, _ []string, timeout time.Duration, limiter Limiter) dyngo.EventListener { +func newGRPCWAFEventListener(handle *waf.Handle, addresses []string, timeout time.Duration, limiter Limiter) dyngo.EventListener { var monitorRulesOnce sync.Once // per instantiation + actionHandler := grpcsec.NewActionsHandler() return grpcsec.OnHandlerOperationStart(func(op *grpcsec.HandlerOperation, handlerArgs grpcsec.HandlerOperationArgs) { // Limit the maximum number of security events, as a streaming RPC could @@ -211,6 +212,34 @@ func newGRPCWAFEventListener(handle *waf.Handle, _ []string, timeout time.Durati mu sync.Mutex // events mutex ) + wafCtx := waf.NewContext(handle) + if wafCtx == nil { + // The WAF event listener got concurrently released + return + } + + // The same address is used for gRPC and http when it comes to client ip + values := map[string]interface{}{} + for _, addr := range addresses { + if addr == httpClientIPAddr && handlerArgs.ClientIP.IsValid() { + values[httpClientIPAddr] = handlerArgs.ClientIP.String() + } + } + + matches, actionIds := runWAF(wafCtx, values, timeout) + wafCtx.Close() + if len(matches) > 0 { + interrupt := false + for _, id := range actionIds { + interrupt = actionHandler.Apply(id, op) || interrupt + } + op.AddSecurityEvents(matches) + log.Debug("appsec: WAF detected an attack before executing the request") + if interrupt { + return + } + } + op.On(grpcsec.OnReceiveOperationFinish(func(_ grpcsec.ReceiveOperation, res grpcsec.ReceiveOperationRes) { if atomic.LoadUint32(&nbEvents) == maxWAFEventsPerRequest { logOnce.Do(func() { From 9663fac11efd1d0f0c732532b30cd5f7f1ce4c70 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mazeau?= Date: Mon, 5 Dec 2022 16:44:02 +0100 Subject: [PATCH 18/31] internal/appsec: dyngo: use go embed for blocked response default templates --- .../dyngo/instrumentation/httpsec/actions.go | 8 ++++---- .../httpsec/blocked-template.html | 1 + .../httpsec/blocked-template.json | 8 ++++++++ .../dyngo/instrumentation/httpsec/http.go | 18 +++++++++++------- 4 files changed, 24 insertions(+), 11 deletions(-) create mode 100644 internal/appsec/dyngo/instrumentation/httpsec/blocked-template.html create mode 100644 internal/appsec/dyngo/instrumentation/httpsec/blocked-template.json diff --git a/internal/appsec/dyngo/instrumentation/httpsec/actions.go b/internal/appsec/dyngo/instrumentation/httpsec/actions.go index f2b7da07c6..7c62608a4e 100644 --- a/internal/appsec/dyngo/instrumentation/httpsec/actions.go +++ b/internal/appsec/dyngo/instrumentation/httpsec/actions.go @@ -27,15 +27,15 @@ func (*BlockRequestAction) isAction() {} // NewBlockRequestAction creates, initializes and returns a new BlockRequestAction func NewBlockRequestAction(status int, template string) BlockRequestAction { - htmlHandler := newBlockRequestHandler(status, "application/html", BlockedTemplateHTML) - jsonHandler := newBlockRequestHandler(status, "application/json", BlockedTemplateJSON) + htmlHandler := newBlockRequestHandler(status, "application/html", blockedTemplateHTML) + jsonHandler := newBlockRequestHandler(status, "application/json", blockedTemplateJSON) var action BlockRequestAction switch template { case "json": - action.handler = newBlockRequestHandler(status, "application/json", BlockedTemplateJSON) + action.handler = newBlockRequestHandler(status, "application/json", blockedTemplateJSON) break case "html": - action.handler = newBlockRequestHandler(status, "application/html", BlockedTemplateHTML) + action.handler = newBlockRequestHandler(status, "application/html", blockedTemplateHTML) break default: action.handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { diff --git a/internal/appsec/dyngo/instrumentation/httpsec/blocked-template.html b/internal/appsec/dyngo/instrumentation/httpsec/blocked-template.html new file mode 100644 index 0000000000..8c48babc80 --- /dev/null +++ b/internal/appsec/dyngo/instrumentation/httpsec/blocked-template.html @@ -0,0 +1 @@ + You've been blocked

Sorry, you cannot access this page. Please contact the customer service team.

diff --git a/internal/appsec/dyngo/instrumentation/httpsec/blocked-template.json b/internal/appsec/dyngo/instrumentation/httpsec/blocked-template.json new file mode 100644 index 0000000000..bbcafb6cb1 --- /dev/null +++ b/internal/appsec/dyngo/instrumentation/httpsec/blocked-template.json @@ -0,0 +1,8 @@ +{ + "errors": [ + { + "title": "You've been blocked", + "detail": "Sorry, you cannot access this page. Please contact the customer service team. Security provided by Datadog." + } + ] +} diff --git a/internal/appsec/dyngo/instrumentation/httpsec/http.go b/internal/appsec/dyngo/instrumentation/httpsec/http.go index 929fb47844..4af849d213 100644 --- a/internal/appsec/dyngo/instrumentation/httpsec/http.go +++ b/internal/appsec/dyngo/instrumentation/httpsec/http.go @@ -12,6 +12,7 @@ package httpsec import ( "context" + _ "embed" "encoding/json" "net" "net/http" @@ -346,12 +347,15 @@ func IPFromRequest(r *http.Request) instrumentation.NetaddrIP { return IPFromHeaders(normalized, r.RemoteAddr) } -var ( - // BlockedTemplateJSON is the default JSON template used to write responses for blocked requests - BlockedTemplateJSON = []byte(`{"errors": [{"title": "You've been blocked", "detail": "Sorry, you cannot access this page. Please contact the customer service team. Security provided by Datadog."}]}`) - // BlockedTemplateHTML is the default HTML template used to write responses for blocked requests - BlockedTemplateHTML = []byte(` You've been blocked

Sorry, you cannot access this page. Please contact the customer service team.

`) -) +// blockedTemplateJSON is the default JSON template used to write responses for blocked requests +// +//go:embed blocked-template.json +var blockedTemplateJSON []byte + +// blockedTemplateHTML is the default HTML template used to write responses for blocked requests +// +//go:embed blocked-template.html +var blockedTemplateHTML []byte const ( envBlockedTemplateHTML = "DD_APPSEC_HTTP_BLOCKED_TEMPLATE_HTML" @@ -359,7 +363,7 @@ const ( ) func init() { - for env, template := range map[string]*[]byte{envBlockedTemplateJSON: &BlockedTemplateJSON, envBlockedTemplateHTML: &BlockedTemplateHTML} { + for env, template := range map[string]*[]byte{envBlockedTemplateJSON: &blockedTemplateJSON, envBlockedTemplateHTML: &blockedTemplateHTML} { if path, ok := os.LookupEnv(env); ok { if t, err := os.ReadFile(path); err != nil { log.Warn("Could not read template at %s: %v", path, err) From 11a4cfb1989d10565972b500711834038b275237 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mazeau?= Date: Tue, 6 Dec 2022 15:53:11 +0100 Subject: [PATCH 19/31] internal/appsec: defer wafCtx.Close() --- internal/appsec/waf.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/appsec/waf.go b/internal/appsec/waf.go index d50a4716d5..a087483637 100644 --- a/internal/appsec/waf.go +++ b/internal/appsec/waf.go @@ -221,6 +221,7 @@ func newGRPCWAFEventListener(handle *waf.Handle, addresses []string, timeout tim // The WAF event listener got concurrently released return } + defer wafCtx.Close() // The same address is used for gRPC and http when it comes to client ip values := map[string]interface{}{} @@ -231,7 +232,6 @@ func newGRPCWAFEventListener(handle *waf.Handle, addresses []string, timeout tim } matches, actionIds := runWAF(wafCtx, values, timeout) - wafCtx.Close() if len(matches) > 0 { interrupt := false for _, id := range actionIds { From 62e93b0d7862e2e13e1c2ccd1abb1f7cb9276e46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mazeau?= Date: Tue, 6 Dec 2022 16:42:48 +0100 Subject: [PATCH 20/31] internal/appsec: add request blocking test case --- internal/appsec/testdata/blocking.json | 42 +++++++++++++++++ internal/appsec/waf_test.go | 64 ++++++++++++++++++++++++++ 2 files changed, 106 insertions(+) create mode 100644 internal/appsec/testdata/blocking.json diff --git a/internal/appsec/testdata/blocking.json b/internal/appsec/testdata/blocking.json new file mode 100644 index 0000000000..8b53f4caf4 --- /dev/null +++ b/internal/appsec/testdata/blocking.json @@ -0,0 +1,42 @@ +{ + "version": "2.2", + "metadata": { + "rules_version": "1.4.2" + }, + "rules": [ + { + "id": "blk-001-001", + "name": "Block IP Addresses", + "tags": { + "type": "block_ip", + "category": "security_response" + }, + "conditions": [ + { + "parameters": { + "inputs": [ + { + "address": "http.client_ip" + } + ], + "data": "blocked_ips" + }, + "operator": "ip_match" + } + ], + "transformers": [], + "on_match": [ + "block" + ] + } + ], + "rules_data": [ + { + "id": "blocked_ips", + "type": "ip_with_expiration", + "data": [ + { "value": "1.2.3.4" } + ] + } + ] +} diff --git a/internal/appsec/waf_test.go b/internal/appsec/waf_test.go index dcf8bb4699..aee4923486 100644 --- a/internal/appsec/waf_test.go +++ b/internal/appsec/waf_test.go @@ -172,3 +172,67 @@ func TestWAF(t *testing.T) { require.NotContains(t, event, sensitivePayloadValue) }) } + +// Test that http blocking works by using custom rules/rules data +func TestBlocking(t *testing.T) { + t.Setenv("DD_APPSEC_RULES", "testdata/blocking.json") + + appsec.Start() + defer appsec.Stop() + if !appsec.Enabled() { + t.Skip("AppSec needs to be enabled for this test") + } + + // Start and trace an HTTP server + mux := httptrace.NewServeMux() + mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte("Hello World!\n")) + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + t.Run("block", func(t *testing.T) { + mt := mocktracer.Start() + defer mt.Stop() + + req, err := http.NewRequest("POST", srv.URL, nil) + if err != nil { + panic(err) + } + // Hardcoded IP header holding an IP that is blocked + req.Header.Set("x-forwarded-for", "1.2.3.4") + res, err := srv.Client().Do(req) + require.NoError(t, err) + + // Check that the request was blocked + b, err := io.ReadAll(res.Body) + require.NoError(t, err) + require.NotEqual(t, "Hello World!\n", string(b)) + require.Equal(t, 403, res.StatusCode) + }) + + t.Run("no-block", func(t *testing.T) { + mt := mocktracer.Start() + defer mt.Stop() + + req1, err := http.NewRequest("POST", srv.URL, nil) + if err != nil { + panic(err) + } + req2, err := http.NewRequest("POST", srv.URL, nil) + if err != nil { + panic(err) + } + req2.Header.Set("x-forwarded-for", "1.2.3.5") + + for _, r := range []*http.Request{req1, req2} { + res, err := srv.Client().Do(r) + require.NoError(t, err) + // Check that the request was not blocked + b, err := io.ReadAll(res.Body) + require.NoError(t, err) + require.Equal(t, "Hello World!\n", string(b)) + + } + }) +} From ca98bff9d2872cd50f274f09220f9c055a1e0470 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mazeau?= Date: Tue, 6 Dec 2022 17:19:21 +0100 Subject: [PATCH 21/31] internal/appsec: golint --- internal/appsec/dyngo/instrumentation/httpsec/http.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/appsec/dyngo/instrumentation/httpsec/http.go b/internal/appsec/dyngo/instrumentation/httpsec/http.go index 4af849d213..120122910c 100644 --- a/internal/appsec/dyngo/instrumentation/httpsec/http.go +++ b/internal/appsec/dyngo/instrumentation/httpsec/http.go @@ -12,6 +12,7 @@ package httpsec import ( "context" + // Blank import needed to use embed for the default blocked response payloads _ "embed" "encoding/json" "net" From e4379bab71138a3f2c622418ea421888bd2bee3e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mazeau?= Date: Thu, 8 Dec 2022 17:30:54 +0100 Subject: [PATCH 22/31] appsec: test grpc IP blocking --- contrib/google.golang.org/grpc/appsec.go | 4 +- contrib/google.golang.org/grpc/appsec_test.go | 89 +++++++++++++++++++ .../dyngo/instrumentation/httpsec/http.go | 15 ++-- internal/appsec/waf.go | 10 ++- 4 files changed, 104 insertions(+), 14 deletions(-) diff --git a/contrib/google.golang.org/grpc/appsec.go b/contrib/google.golang.org/grpc/appsec.go index 9d25329d82..22ba3fd4a9 100644 --- a/contrib/google.golang.org/grpc/appsec.go +++ b/contrib/google.golang.org/grpc/appsec.go @@ -26,7 +26,7 @@ func appsecUnaryHandlerMiddleware(span ddtrace.Span, handler grpc.UnaryHandler) instrumentation.SetAppSecEnabledTags(span) return func(ctx context.Context, req interface{}) (interface{}, error) { md, _ := metadata.FromIncomingContext(ctx) - ip := httpsec.IPFromHeaders(httpsec.NormalizeHTTPHeaders(md), "") + ip := httpsec.IPFromHeaders(md, "") op := grpcsec.StartHandlerOperation(grpcsec.HandlerOperationArgs{Metadata: md, ClientIP: ip}, nil) defer func() { events := op.Finish(grpcsec.HandlerOperationRes{}) @@ -50,7 +50,7 @@ func appsecStreamHandlerMiddleware(span ddtrace.Span, handler grpc.StreamHandler instrumentation.SetAppSecEnabledTags(span) return func(srv interface{}, stream grpc.ServerStream) error { md, _ := metadata.FromIncomingContext(stream.Context()) - ip := httpsec.IPFromHeaders(httpsec.NormalizeHTTPHeaders(md), "") + ip := httpsec.IPFromHeaders(md, "") op := grpcsec.StartHandlerOperation(grpcsec.HandlerOperationArgs{Metadata: md, ClientIP: ip}, nil) defer func() { events := op.Finish(grpcsec.HandlerOperationRes{}) diff --git a/contrib/google.golang.org/grpc/appsec_test.go b/contrib/google.golang.org/grpc/appsec_test.go index 14862e7966..23063eee28 100644 --- a/contrib/google.golang.org/grpc/appsec_test.go +++ b/contrib/google.golang.org/grpc/appsec_test.go @@ -14,7 +14,9 @@ import ( "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec" "github.com/stretchr/testify/require" + "google.golang.org/grpc/codes" "google.golang.org/grpc/metadata" + "google.golang.org/grpc/status" ) func TestAppSec(t *testing.T) { @@ -94,3 +96,90 @@ func TestAppSec(t *testing.T) { require.True(t, strings.Contains(event, "ua0-600-55x")) // canary rule attack attempt }) } + +// Test that http blocking works by using custom rules/rules data +func TestBlocking(t *testing.T) { + t.Setenv("DD_APPSEC_RULES", "../../../internal/appsec/testdata/blocking.json") + appsec.Start() + defer appsec.Stop() + if !appsec.Enabled() { + t.Skip("appsec disabled") + } + + rig, err := newRig(false) + require.NoError(t, err) + defer rig.Close() + + client := rig.client + + t.Run("unary-block", func(t *testing.T) { + mt := mocktracer.Start() + defer mt.Stop() + + // Send a XSS attack in the payload along with the canary value in the RPC metadata + ctx := metadata.NewOutgoingContext(context.Background(), metadata.Pairs("dd-canary", "dd-test-scanner-log", "x-client-ip", "1.2.3.4")) + reply, err := client.Ping(ctx, &FixtureRequest{Name: ""}) + + require.Nil(t, reply) + require.Equal(t, codes.Aborted, status.Code(err)) + + finished := mt.FinishedSpans() + require.Len(t, finished, 1) + // The request should have the attack attempts + event, _ := finished[0].Tag("_dd.appsec.json").(string) + require.NotNil(t, event) + require.True(t, strings.Contains(event, "blk-001-001")) + }) + + t.Run("unary-no-block", func(t *testing.T) { + mt := mocktracer.Start() + defer mt.Stop() + + // Send a XSS attack in the payload along with the canary value in the RPC metadata + ctx := metadata.NewOutgoingContext(context.Background(), metadata.Pairs("dd-canary", "dd-test-scanner-log", "x-client-ip", "1.2.3.5")) + reply, err := client.Ping(ctx, &FixtureRequest{Name: ""}) + + require.Equal(t, "passed", reply.Message) + require.Equal(t, codes.OK, status.Code(err)) + }) + + t.Run("stream-block", func(t *testing.T) { + mt := mocktracer.Start() + defer mt.Stop() + + ctx := metadata.NewOutgoingContext(context.Background(), metadata.Pairs("dd-canary", "dd-test-scanner-log", "x-client-ip", "1.2.3.4")) + stream, err := client.StreamPing(ctx) + require.NoError(t, err) + reply, err := stream.Recv() + + require.Equal(t, codes.Aborted, status.Code(err)) + require.Nil(t, reply) + + finished := mt.FinishedSpans() + require.Len(t, finished, 1) + // The request should have the attack attempts + event, _ := finished[0].Tag("_dd.appsec.json").(string) + require.NotNil(t, event) + require.True(t, strings.Contains(event, "blk-001-001")) + }) + + t.Run("stream-no-block", func(t *testing.T) { + mt := mocktracer.Start() + defer mt.Stop() + + ctx := metadata.NewOutgoingContext(context.Background(), metadata.Pairs("dd-canary", "dd-test-scanner-log", "x-client-ip", "1.2.3.5")) + stream, err := client.StreamPing(ctx) + require.NoError(t, err) + + // Send a XSS attack + err = stream.Send(&FixtureRequest{Name: ""}) + require.NoError(t, err) + reply, err := stream.Recv() + require.Equal(t, codes.OK, status.Code(err)) + require.Equal(t, "passed", reply.Message) + + err = stream.CloseSend() + require.NoError(t, err) + }) + +} diff --git a/internal/appsec/dyngo/instrumentation/httpsec/http.go b/internal/appsec/dyngo/instrumentation/httpsec/http.go index 120122910c..8d8d3d2759 100644 --- a/internal/appsec/dyngo/instrumentation/httpsec/http.go +++ b/internal/appsec/dyngo/instrumentation/httpsec/http.go @@ -149,7 +149,7 @@ func MakeHandlerOperationArgs(r *http.Request, pathParams map[string]string) Han Cookies: cookies, Query: r.URL.Query(), // TODO(Julio-Guerra): avoid actively parsing the query values thanks to dynamic instrumentation PathParams: pathParams, - ClientIP: IPFromRequest(r), + ClientIP: IPFromHeaders(headers, r.RemoteAddr), } } @@ -313,7 +313,7 @@ func (f OnSDKBodyOperationFinish) Call(op dyngo.Operation, v interface{}) { } // IPFromHeaders returns the resolved client IP for set of normalized headers. The returned IP can be invalid. -func IPFromHeaders(hdrs map[string]string, remoteAddr string) instrumentation.NetaddrIP { +func IPFromHeaders(hdrs map[string][]string, remoteAddr string) instrumentation.NetaddrIP { ipHeaders := defaultIPHeaders if len(clientIPHeader) > 0 { ipHeaders = []string{clientIPHeader} @@ -321,9 +321,10 @@ func IPFromHeaders(hdrs map[string]string, remoteAddr string) instrumentation.Ne var headers []string var ips []string for _, hdr := range ipHeaders { - if v, ok := hdrs[hdr]; ok && v != "" { + if v, ok := hdrs[hdr]; ok && len(v) >= 1 { headers = append(headers, hdr) - ips = append(ips, v) + // We currently only use the first entry for one specific header + ips = append(ips, v[0]) } } if len(ips) == 0 { @@ -342,12 +343,6 @@ func IPFromHeaders(hdrs map[string]string, remoteAddr string) instrumentation.Ne return instrumentation.NetaddrIP{} } -// IPFromRequest returns the resolved client IP for a specific request. The returned IP can be invalid. -func IPFromRequest(r *http.Request) instrumentation.NetaddrIP { - normalized := NormalizeHTTPHeaders(r.Header) - return IPFromHeaders(normalized, r.RemoteAddr) -} - // blockedTemplateJSON is the default JSON template used to write responses for blocked requests // //go:embed blocked-template.json diff --git a/internal/appsec/waf.go b/internal/appsec/waf.go index a087483637..0288794bd7 100644 --- a/internal/appsec/waf.go +++ b/internal/appsec/waf.go @@ -360,6 +360,7 @@ const ( var grpcAddresses = []string{ grpcServerRequestMessage, grpcServerRequestMetadata, + httpClientIPAddr, } func init() { @@ -373,11 +374,16 @@ func init() { func supportedAddresses(ruleAddresses []string) (supportedHTTP, supportedGRPC, notSupported []string) { // Filter the supported addresses only for _, addr := range ruleAddresses { + supported := false if i := sort.SearchStrings(httpAddresses, addr); i < len(httpAddresses) && httpAddresses[i] == addr { supportedHTTP = append(supportedHTTP, addr) - } else if i := sort.SearchStrings(grpcAddresses, addr); i < len(grpcAddresses) && grpcAddresses[i] == addr { + supported = true + } + if i := sort.SearchStrings(grpcAddresses, addr); i < len(grpcAddresses) && grpcAddresses[i] == addr { supportedGRPC = append(supportedGRPC, addr) - } else { + supported = true + } + if !supported { notSupported = append(notSupported, addr) } } From 2286b1131a187838e36ab343be1b03f9afbf06ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mazeau?= Date: Thu, 8 Dec 2022 18:50:23 +0100 Subject: [PATCH 23/31] internal/appsec: fix content type when blocking --- .../dyngo/instrumentation/httpsec/actions.go | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/internal/appsec/dyngo/instrumentation/httpsec/actions.go b/internal/appsec/dyngo/instrumentation/httpsec/actions.go index 7c62608a4e..c6d2925071 100644 --- a/internal/appsec/dyngo/instrumentation/httpsec/actions.go +++ b/internal/appsec/dyngo/instrumentation/httpsec/actions.go @@ -7,6 +7,7 @@ package httpsec import ( "net/http" + "strings" "sync" "gopkg.in/DataDog/dd-trace-go.v1/internal/log" @@ -27,24 +28,25 @@ func (*BlockRequestAction) isAction() {} // NewBlockRequestAction creates, initializes and returns a new BlockRequestAction func NewBlockRequestAction(status int, template string) BlockRequestAction { - htmlHandler := newBlockRequestHandler(status, "application/html", blockedTemplateHTML) + htmlHandler := newBlockRequestHandler(status, "text/html", blockedTemplateHTML) jsonHandler := newBlockRequestHandler(status, "application/json", blockedTemplateJSON) var action BlockRequestAction switch template { case "json": - action.handler = newBlockRequestHandler(status, "application/json", blockedTemplateJSON) + action.handler = jsonHandler break case "html": - action.handler = newBlockRequestHandler(status, "application/html", blockedTemplateHTML) + action.handler = htmlHandler break default: action.handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { h := jsonHandler - for _, value := range r.Header.Values("Accept") { - if value == "application/html" { - h = htmlHandler - break - } + hdr := r.Header.Get("Accept") + htmlIdx := strings.Index(hdr, "text/html") + jsonIdx := strings.Index(hdr, "application/json") + // Switch to html handler if text/html comes before application/json in the Accept header + if htmlIdx != -1 && (jsonIdx == -1 || htmlIdx < jsonIdx) { + h = htmlHandler } h.ServeHTTP(w, r) }) From 927267842a4d6ce37da73ed48cecbc4eafa3522e Mon Sep 17 00:00:00 2001 From: Julio Guerra Date: Thu, 8 Dec 2022 23:31:32 +0100 Subject: [PATCH 24/31] grpcsec: report the http.client_ip span tag --- contrib/google.golang.org/grpc/appsec.go | 12 +++++----- .../dyngo/instrumentation/grpcsec/tags.go | 23 +++++++++++-------- 2 files changed, 20 insertions(+), 15 deletions(-) diff --git a/contrib/google.golang.org/grpc/appsec.go b/contrib/google.golang.org/grpc/appsec.go index 22ba3fd4a9..f1e7182706 100644 --- a/contrib/google.golang.org/grpc/appsec.go +++ b/contrib/google.golang.org/grpc/appsec.go @@ -34,7 +34,7 @@ func appsecUnaryHandlerMiddleware(span ddtrace.Span, handler grpc.UnaryHandler) if len(events) == 0 { return } - setAppSecTags(ctx, span, events) + setAppSecTags(ctx, span, ip, events) }() if op.BlockedCode != nil { op.AddTag("appsec.blocked", true) @@ -58,7 +58,7 @@ func appsecStreamHandlerMiddleware(span ddtrace.Span, handler grpc.StreamHandler if len(events) == 0 { return } - setAppSecTags(stream.Context(), span, events) + setAppSecTags(stream.Context(), span, ip, events) }() if op.BlockedCode != nil { op.AddTag("appsec.blocked", true) @@ -84,11 +84,11 @@ func (ss appsecServerStream) RecvMsg(m interface{}) error { } // Set the AppSec tags when security events were found. -func setAppSecTags(ctx context.Context, span ddtrace.Span, events []json.RawMessage) { +func setAppSecTags(ctx context.Context, span ddtrace.Span, clientIP instrumentation.NetaddrIP, events []json.RawMessage) { md, _ := metadata.FromIncomingContext(ctx) - var addr net.Addr + var peerAddr net.Addr if p, ok := peer.FromContext(ctx); ok { - addr = p.Addr + peerAddr = p.Addr } - grpcsec.SetSecurityEventTags(span, events, addr, md) + grpcsec.SetSecurityEventTags(span, events, clientIP, peerAddr, md) } diff --git a/internal/appsec/dyngo/instrumentation/grpcsec/tags.go b/internal/appsec/dyngo/instrumentation/grpcsec/tags.go index 845ab3a044..12ec59d7d3 100644 --- a/internal/appsec/dyngo/instrumentation/grpcsec/tags.go +++ b/internal/appsec/dyngo/instrumentation/grpcsec/tags.go @@ -17,26 +17,31 @@ import ( // SetSecurityEventTags sets the AppSec-specific span tags when a security event // occurred into the service entry span. -func SetSecurityEventTags(span ddtrace.Span, events []json.RawMessage, addr net.Addr, md map[string][]string) { - if err := setSecurityEventTags(span, events, addr, md); err != nil { +func SetSecurityEventTags(span ddtrace.Span, events []json.RawMessage, clientIP instrumentation.NetaddrIP, peerAddr net.Addr, md map[string][]string) { + if err := setSecurityEventTags(span, events, clientIP, peerAddr, md); err != nil { log.Error("appsec: %v", err) } } -func setSecurityEventTags(span ddtrace.Span, events []json.RawMessage, addr net.Addr, md map[string][]string) error { +func setSecurityEventTags(span ddtrace.Span, events []json.RawMessage, clientIP instrumentation.NetaddrIP, peerAddr net.Addr, md map[string][]string) error { if err := instrumentation.SetEventSpanTags(span, events); err != nil { return err } - var ip string - switch actual := addr.(type) { + var peerIP string + switch actual := peerAddr.(type) { case *net.UDPAddr: - ip = actual.IP.String() + peerIP = actual.IP.String() case *net.TCPAddr: - ip = actual.IP.String() + peerIP = actual.IP.String() } - if ip != "" { - span.SetTag("network.client.ip", ip) + if peerIP != "" { + span.SetTag("network.client.ip", peerIP) } + + if clientIP.IsValid() { + span.SetTag("http.client_ip", clientIP.String()) + } + for h, v := range httpsec.NormalizeHTTPHeaders(md) { span.SetTag("grpc.metadata."+h, v) } From c7377af5cf487edbe802bdb1112f1b2b5679828e Mon Sep 17 00:00:00 2001 From: Julio Guerra Date: Fri, 9 Dec 2022 00:20:25 +0100 Subject: [PATCH 25/31] grpcsec: report the http.client_ip span tag --- .../instrumentation/grpcsec/tags_test.go | 77 ++++++++++++------- 1 file changed, 50 insertions(+), 27 deletions(-) diff --git a/internal/appsec/dyngo/instrumentation/grpcsec/tags_test.go b/internal/appsec/dyngo/instrumentation/grpcsec/tags_test.go index 5e0e21db69..442c8bb944 100644 --- a/internal/appsec/dyngo/instrumentation/grpcsec/tags_test.go +++ b/internal/appsec/dyngo/instrumentation/grpcsec/tags_test.go @@ -15,6 +15,7 @@ import ( "gopkg.in/DataDog/dd-trace-go.v1/ddtrace" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext" + "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/dyngo/instrumentation" "gopkg.in/DataDog/dd-trace-go.v1/internal/samplernames" ) @@ -119,33 +120,55 @@ func TestSetSecurityEventTags(t *testing.T) { }, } { metadataCase := metadataCase - t.Run(fmt.Sprintf("%s-%s-%s", eventCase.name, addrCase.name, metadataCase.name), func(t *testing.T) { - var span MockSpan - err := setSecurityEventTags(&span, eventCase.events, addrCase.addr, metadataCase.md) - if eventCase.expectedError { - require.Error(t, err) - return - } - require.NoError(t, err) - - expectedTags := map[string]interface{}{ - "_dd.appsec.json": eventCase.expectedTag, - "manual.keep": true, - "appsec.event": true, - "_dd.origin": "appsec", - } - - if addr := addrCase.expectedTag; addr != "" { - expectedTags["network.client.ip"] = addr - } - - for k, v := range metadataCase.expectedTags { - expectedTags[k] = v - } - - require.Equal(t, expectedTags, span.tags) - require.False(t, span.finished) - }) + for _, clientIPCase := range []struct { + name string + clientIP instrumentation.NetaddrIP + expectedClientIPTag string + }{ + { + name: "invalid-client-ip", + clientIP: instrumentation.NetaddrIP{}, + expectedClientIPTag: "", + }, + { + name: "valid-client-ip", + clientIP: instrumentation.NetaddrIPv4(1, 2, 3, 4), + expectedClientIPTag: "1.2.3.4", + }, + } { + clientIPCase := clientIPCase + t.Run(fmt.Sprintf("%s-%s-%s-%s", eventCase.name, addrCase.name, metadataCase.name, clientIPCase.name), func(t *testing.T) { + var span MockSpan + err := setSecurityEventTags(&span, eventCase.events, clientIPCase.clientIP, addrCase.addr, metadataCase.md) + if eventCase.expectedError { + require.Error(t, err) + return + } + require.NoError(t, err) + + expectedTags := map[string]interface{}{ + "_dd.appsec.json": eventCase.expectedTag, + "manual.keep": true, + "appsec.event": true, + "_dd.origin": "appsec", + } + + if addr := addrCase.expectedTag; addr != "" { + expectedTags["network.client.ip"] = addr + } + + for k, v := range metadataCase.expectedTags { + expectedTags[k] = v + } + + if clientIPCase.expectedClientIPTag != "" { + expectedTags["http.client_ip"] = clientIPCase.expectedClientIPTag + } + + require.Equal(t, expectedTags, span.tags) + require.False(t, span.finished) + }) + } } } } From 7f24120dc41584b2fafdd3beb57544e58f4c195e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mazeau?= Date: Fri, 9 Dec 2022 15:12:05 +0100 Subject: [PATCH 26/31] internal/appsec: close waf ctx when blocking --- internal/appsec/waf.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/appsec/waf.go b/internal/appsec/waf.go index 0288794bd7..851360a7a3 100644 --- a/internal/appsec/waf.go +++ b/internal/appsec/waf.go @@ -126,6 +126,7 @@ func newHTTPWAFEventListener(handle *waf.Handle, addresses []string, timeout tim op.AddSecurityEvents(matches) log.Debug("appsec: WAF detected an attack before executing the request") if interrupt { + wafCtx.Close() return } } From 71c6c7309bffb646191409162e8fdd1eb703a944 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mazeau?= Date: Fri, 9 Dec 2022 15:48:28 +0100 Subject: [PATCH 27/31] internal/appsec: add actions_test.go --- .../instrumentation/httpsec/actions_test.go | 155 ++++++++++++++++++ 1 file changed, 155 insertions(+) create mode 100644 internal/appsec/dyngo/instrumentation/httpsec/actions_test.go diff --git a/internal/appsec/dyngo/instrumentation/httpsec/actions_test.go b/internal/appsec/dyngo/instrumentation/httpsec/actions_test.go new file mode 100644 index 0000000000..5f48a23df2 --- /dev/null +++ b/internal/appsec/dyngo/instrumentation/httpsec/actions_test.go @@ -0,0 +1,155 @@ +// Unless explicitly stated otherwise all files in this repository are licensed +// under the Apache License Version 2.0. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2016 Datadog, Inc. + +package httpsec + +import ( + "io" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestNewBlockRequestAction(t *testing.T) { + mux := http.NewServeMux() + srv := httptest.NewServer(mux) + mux.HandleFunc("/json", NewBlockRequestAction(403, "json").handler.ServeHTTP) + mux.HandleFunc("/html", NewBlockRequestAction(403, "html").handler.ServeHTTP) + mux.HandleFunc("/auto", NewBlockRequestAction(403, "auto").handler.ServeHTTP) + defer srv.Close() + + t.Run("json", func(t *testing.T) { + for _, tc := range []struct { + name string + accept string + }{ + { + name: "no-accept", + }, + { + name: "irrelevant-accept", + accept: "text/html", + }, + { + name: "accept", + accept: "application/json", + }, + } { + t.Run(tc.name, func(t *testing.T) { + req, err := http.NewRequest("POST", srv.URL+"/json", nil) + req.Header.Set("Accept", tc.accept) + require.NoError(t, err) + res, err := srv.Client().Do(req) + require.NoError(t, err) + defer res.Body.Close() + body, err := io.ReadAll(res.Body) + require.Equal(t, 403, res.StatusCode) + require.Equal(t, blockedTemplateJSON, body) + }) + } + }) + + t.Run("html", func(t *testing.T) { + for _, tc := range []struct { + name string + accept string + }{ + { + name: "no-accept", + }, + { + name: "irrelevant-accept", + accept: "application/json", + }, + { + name: "accept", + accept: "text/html", + }, + } { + t.Run(tc.name, func(t *testing.T) { + req, err := http.NewRequest("POST", srv.URL+"/html", nil) + require.NoError(t, err) + res, err := srv.Client().Do(req) + require.NoError(t, err) + defer res.Body.Close() + body, err := io.ReadAll(res.Body) + require.Equal(t, 403, res.StatusCode) + require.Equal(t, blockedTemplateHTML, body) + }) + } + }) + + t.Run("auto", func(t *testing.T) { + for _, tc := range []struct { + name string + accept string + expected []byte + }{ + { + name: "no-accept", + expected: blockedTemplateJSON, + }, + { + name: "json-accept-1", + accept: "application/json", + expected: blockedTemplateJSON, + }, + { + name: "json-accept-2", + accept: "application/json,text/html", + expected: blockedTemplateJSON, + }, + { + name: "json-accept-3", + accept: "irrelevant/content,application/json,text/html", + expected: blockedTemplateJSON, + }, + { + name: "json-accept-4", + accept: "irrelevant/content,application/json,text/html,application/json", + expected: blockedTemplateJSON, + }, + { + name: "html-accept-1", + accept: "text/html", + expected: blockedTemplateHTML, + }, + { + name: "html-accept-2", + accept: "text/html,application/json", + expected: blockedTemplateHTML, + }, + { + name: "html-accept-3", + accept: "irrelevant/content,text/html,application/json", + expected: blockedTemplateHTML, + }, + { + name: "html-accept-4", + accept: "irrelevant/content,text/html,application/json,text/html", + expected: blockedTemplateHTML, + }, + { + name: "irrelevant-accept", + accept: "irrelevant/irrelevant,application/html", + expected: blockedTemplateJSON, + }, + } { + t.Run(tc.name, func(t *testing.T) { + req, err := http.NewRequest("POST", srv.URL+"/auto", nil) + req.Header.Set("Accept", tc.accept) + require.NoError(t, err) + res, err := srv.Client().Do(req) + require.NoError(t, err) + defer res.Body.Close() + body, err := io.ReadAll(res.Body) + require.Equal(t, 403, res.StatusCode) + require.Equal(t, tc.expected, body) + }) + } + }) +} From 7281ec4739fbd87879db6847db405c7bf442cc9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mazeau?= Date: Fri, 9 Dec 2022 16:13:08 +0100 Subject: [PATCH 28/31] internal/appsec: report private IP if no global IP found --- .../dyngo/instrumentation/httpsec/http.go | 36 +++---------- .../dyngo/instrumentation/httpsec/tags.go | 54 +++++++++++++------ .../instrumentation/httpsec/tags_test.go | 4 +- 3 files changed, 48 insertions(+), 46 deletions(-) diff --git a/internal/appsec/dyngo/instrumentation/httpsec/http.go b/internal/appsec/dyngo/instrumentation/httpsec/http.go index 8d8d3d2759..3ac42de62d 100644 --- a/internal/appsec/dyngo/instrumentation/httpsec/http.go +++ b/internal/appsec/dyngo/instrumentation/httpsec/http.go @@ -12,6 +12,7 @@ package httpsec import ( "context" + // Blank import needed to use embed for the default blocked response payloads _ "embed" "encoding/json" @@ -23,6 +24,7 @@ import ( "sync" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace" + "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext" "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/dyngo" "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/dyngo/instrumentation" "gopkg.in/DataDog/dd-trace-go.v1/internal/log" @@ -143,13 +145,14 @@ func MakeHandlerOperationArgs(r *http.Request, pathParams map[string]string) Han } cookies := makeCookies(r) // TODO(Julio-Guerra): avoid actively parsing the cookies thanks to dynamic instrumentation headers["host"] = []string{r.Host} + ip := IPFromHeaders(r.Header, r.RemoteAddr) return HandlerOperationArgs{ RequestURI: r.RequestURI, Headers: headers, Cookies: cookies, Query: r.URL.Query(), // TODO(Julio-Guerra): avoid actively parsing the query values thanks to dynamic instrumentation PathParams: pathParams, - ClientIP: IPFromHeaders(headers, r.RemoteAddr), + ClientIP: ip, } } @@ -312,35 +315,10 @@ func (f OnSDKBodyOperationFinish) Call(op dyngo.Operation, v interface{}) { f(op.(*SDKBodyOperation), v.(SDKBodyOperationRes)) } -// IPFromHeaders returns the resolved client IP for set of normalized headers. The returned IP can be invalid. +// IPFromHeaders tries to resolve and return the user IP from request headers func IPFromHeaders(hdrs map[string][]string, remoteAddr string) instrumentation.NetaddrIP { - ipHeaders := defaultIPHeaders - if len(clientIPHeader) > 0 { - ipHeaders = []string{clientIPHeader} - } - var headers []string - var ips []string - for _, hdr := range ipHeaders { - if v, ok := hdrs[hdr]; ok && len(v) >= 1 { - headers = append(headers, hdr) - // We currently only use the first entry for one specific header - ips = append(ips, v[0]) - } - } - if len(ips) == 0 { - if remoteIP := parseIP(remoteAddr); remoteIP.IsValid() && isGlobal(remoteIP) { - return remoteIP - } - } else if len(ips) == 1 { - for _, ipstr := range strings.Split(ips[0], ",") { - ip := parseIP(strings.TrimSpace(ipstr)) - if ip.IsValid() && isGlobal(ip) { - return ip - } - } - } - - return instrumentation.NetaddrIP{} + ip := IPTagsFromHeaders(hdrs, remoteAddr)[ext.HTTPClientIP] + return parseIP(ip) } // blockedTemplateJSON is the default JSON template used to write responses for blocked requests diff --git a/internal/appsec/dyngo/instrumentation/httpsec/tags.go b/internal/appsec/dyngo/instrumentation/httpsec/tags.go index 65dd5c4b5f..d010a15722 100644 --- a/internal/appsec/dyngo/instrumentation/httpsec/tags.go +++ b/internal/appsec/dyngo/instrumentation/httpsec/tags.go @@ -106,42 +106,66 @@ func ippref(s string) *instrumentation.NetaddrIPPrefix { } // SetIPTags sets the IP related span tags for a given request +func SetIPTags(s instrumentation.TagSetter, r *http.Request) { + for k, v := range IPTagsFromHeaders(r.Header, r.RemoteAddr) { + s.SetTag(k, v) + } + +} + +// IPTagsFromHeaders generates the IP related span tags for a given request's headers // See https://docs.datadoghq.com/tracing/configure_data_security#configuring-a-client-ip-header for more information. -func SetIPTags(span instrumentation.TagSetter, r *http.Request) { +func IPTagsFromHeaders(hdrs map[string][]string, remoteAddr string) map[string]string { + tags := map[string]string{} ipHeaders := defaultIPHeaders if len(clientIPHeader) > 0 { ipHeaders = []string{clientIPHeader} } - var ( headers []string ips []string ) + + // Make sure all headers are lower-case + for k, v := range hdrs { + hdrs[strings.ToLower(k)] = v + } for _, hdr := range ipHeaders { - if v := r.Header.Get(hdr); v != "" { + if v := hdrs[hdr]; len(v) >= 1 && v[0] != "" { headers = append(headers, hdr) - ips = append(ips, v) + ips = append(ips, v[0]) } } - if l := len(ips); l == 0 { - if remoteIP := parseIP(r.RemoteAddr); remoteIP.IsValid() && isGlobal(remoteIP) { - span.SetTag(ext.HTTPClientIP, remoteIP.String()) - } - } else if l == 1 { + var privateIP instrumentation.NetaddrIP + // First try to use the IP from the headers if only one IP was found + if l := len(ips); l == 1 { for _, ipstr := range strings.Split(ips[0], ",") { ip := parseIP(strings.TrimSpace(ipstr)) - if ip.IsValid() && isGlobal(ip) { - span.SetTag(ext.HTTPClientIP, ip.String()) - break + if ip.IsValid() { + if isGlobal(ip) { + tags[ext.HTTPClientIP] = ip.String() + return tags + } else if !privateIP.IsValid() { + privateIP = ip + } } } - } else { + } else if l > 1 { // If more than one IP header, report them for i := range ips { - span.SetTag(ext.HTTPRequestHeaders+"."+headers[i], ips[i]) + tags[ext.HTTPRequestHeaders+"."+headers[i]] = ips[i] } - span.SetTag(tagMultipleIPHeaders, strings.Join(headers, ",")) + tags[tagMultipleIPHeaders] = strings.Join(headers, ",") } + + // Try to get a global IP from remoteAddr. If not, try to use any private IP we found + if remoteIP := parseIP(remoteAddr); remoteIP.IsValid() && (isGlobal(remoteIP) || !privateIP.IsValid()) { + tags[ext.HTTPClientIP] = remoteIP.String() + } else if privateIP.IsValid() { + tags[ext.HTTPClientIP] = privateIP.String() + } + + return tags } func parseIP(s string) instrumentation.NetaddrIP { diff --git a/internal/appsec/dyngo/instrumentation/httpsec/tags_test.go b/internal/appsec/dyngo/instrumentation/httpsec/tags_test.go index 06f0c7868c..9992a4448a 100644 --- a/internal/appsec/dyngo/instrumentation/httpsec/tags_test.go +++ b/internal/appsec/dyngo/instrumentation/httpsec/tags_test.go @@ -80,7 +80,7 @@ func genIPTestCases() []ipTestCase { tcs = append(tcs, ipTestCase{ name: "ipv4-private." + header, headers: map[string]string{header: ipv4Private}, - expectedIP: instrumentation.NetaddrIP{}, + expectedIP: instrumentation.NetaddrMustParseIP(ipv4Private), }) } // Simple ipv6 test cases over all headers @@ -93,7 +93,7 @@ func genIPTestCases() []ipTestCase { tcs = append(tcs, ipTestCase{ name: "ipv6-private." + header, headers: map[string]string{header: ipv6Private}, - expectedIP: instrumentation.NetaddrIP{}, + expectedIP: instrumentation.NetaddrMustParseIP(ipv6Private), }) } // private and global in same header From fde2717c7da9e93694b07d5156f2a36473564afa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mazeau?= Date: Fri, 9 Dec 2022 21:03:28 +0100 Subject: [PATCH 29/31] appsec: retrieve remote addr for grpc --- contrib/google.golang.org/grpc/appsec.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/contrib/google.golang.org/grpc/appsec.go b/contrib/google.golang.org/grpc/appsec.go index f1e7182706..1fed92536f 100644 --- a/contrib/google.golang.org/grpc/appsec.go +++ b/contrib/google.golang.org/grpc/appsec.go @@ -26,7 +26,11 @@ func appsecUnaryHandlerMiddleware(span ddtrace.Span, handler grpc.UnaryHandler) instrumentation.SetAppSecEnabledTags(span) return func(ctx context.Context, req interface{}) (interface{}, error) { md, _ := metadata.FromIncomingContext(ctx) - ip := httpsec.IPFromHeaders(md, "") + var remoteAddr string + if p, ok := peer.FromContext(ctx); ok { + remoteAddr = p.Addr.String() + } + ip := httpsec.IPFromHeaders(md, remoteAddr) op := grpcsec.StartHandlerOperation(grpcsec.HandlerOperationArgs{Metadata: md, ClientIP: ip}, nil) defer func() { events := op.Finish(grpcsec.HandlerOperationRes{}) @@ -50,7 +54,11 @@ func appsecStreamHandlerMiddleware(span ddtrace.Span, handler grpc.StreamHandler instrumentation.SetAppSecEnabledTags(span) return func(srv interface{}, stream grpc.ServerStream) error { md, _ := metadata.FromIncomingContext(stream.Context()) - ip := httpsec.IPFromHeaders(md, "") + var remoteAddr string + if p, ok := peer.FromContext(stream.Context()); ok { + remoteAddr = p.Addr.String() + } + ip := httpsec.IPFromHeaders(md, remoteAddr) op := grpcsec.StartHandlerOperation(grpcsec.HandlerOperationArgs{Metadata: md, ClientIP: ip}, nil) defer func() { events := op.Finish(grpcsec.HandlerOperationRes{}) From a266362277c95292f0f65196005774696f40c568 Mon Sep 17 00:00:00 2001 From: Julio Guerra Date: Sun, 11 Dec 2022 22:03:44 +0100 Subject: [PATCH 30/31] github: update codeowners file to better cover appsec files in contrib --- CODEOWNERS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CODEOWNERS b/CODEOWNERS index 7bfeb6ca6d..87ba38ad00 100644 --- a/CODEOWNERS +++ b/CODEOWNERS @@ -15,7 +15,7 @@ # appsec /appsec @DataDog/appsec-go /internal/appsec @DataDog/appsec-go -/contrib/**/appsec.go @DataDog/appsec-go +/contrib/**/*appsec*.go @DataDog/appsec-go /.github/workflows/appsec.yml @DataDog/appsec-go # telemetry From 3d9d7d4e75021a4cac1c5d2e411d860a12c62820 Mon Sep 17 00:00:00 2001 From: Julio Guerra Date: Mon, 12 Dec 2022 15:09:29 +0100 Subject: [PATCH 31/31] appsec: fix client ip tags with peer ip addresses --- contrib/gin-gonic/gin/appsec.go | 20 +- contrib/google.golang.org/grpc/appsec.go | 33 +-- contrib/labstack/echo.v4/appsec.go | 17 +- .../appsec/dyngo/instrumentation/common.go | 8 + .../dyngo/instrumentation/grpcsec/tags.go | 22 +- .../instrumentation/grpcsec/tags_test.go | 203 +++++++++--------- .../dyngo/instrumentation/httpsec/http.go | 27 +-- .../dyngo/instrumentation/httpsec/tags.go | 134 +++++++----- .../instrumentation/httpsec/tags_test.go | 163 +++++++++----- 9 files changed, 335 insertions(+), 292 deletions(-) diff --git a/contrib/gin-gonic/gin/appsec.go b/contrib/gin-gonic/gin/appsec.go index 27d85e74b9..1b0946343a 100644 --- a/contrib/gin-gonic/gin/appsec.go +++ b/contrib/gin-gonic/gin/appsec.go @@ -6,8 +6,6 @@ package gin import ( - "net" - "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer" "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/dyngo/instrumentation" "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/dyngo/instrumentation/httpsec" @@ -18,8 +16,8 @@ import ( // useAppSec executes the AppSec logic related to the operation start and // returns the function to be executed upon finishing the operation func useAppSec(c *gin.Context, span tracer.Span) func() { - req := c.Request instrumentation.SetAppSecEnabledTags(span) + var params map[string]string if l := len(c.Params); l > 0 { params = make(map[string]string, l) @@ -27,18 +25,20 @@ func useAppSec(c *gin.Context, span tracer.Span) func() { params[p.Key] = p.Value } } - args := httpsec.MakeHandlerOperationArgs(req, params) + + req := c.Request + ipTags, clientIP := httpsec.ClientIPTags(req.Header, req.RemoteAddr) + instrumentation.SetStringTags(span, ipTags) + + args := httpsec.MakeHandlerOperationArgs(req, clientIP, params) ctx, op := httpsec.StartOperation(req.Context(), args) c.Request = req.WithContext(ctx) + return func() { events := op.Finish(httpsec.HandlerOperationRes{Status: c.Writer.Status()}) + instrumentation.SetTags(span, op.Tags()) if len(events) > 0 { - remoteIP, _, err := net.SplitHostPort(req.RemoteAddr) - if err != nil { - remoteIP = req.RemoteAddr - } - httpsec.SetSecurityEventTags(span, events, remoteIP, args.Headers, c.Writer.Header()) + httpsec.SetSecurityEventTags(span, events, args.Headers, c.Writer.Header()) } - instrumentation.SetTags(span, op.Tags()) } } diff --git a/contrib/google.golang.org/grpc/appsec.go b/contrib/google.golang.org/grpc/appsec.go index 1fed92536f..aa60298cf3 100644 --- a/contrib/google.golang.org/grpc/appsec.go +++ b/contrib/google.golang.org/grpc/appsec.go @@ -7,7 +7,6 @@ package grpc import ( "encoding/json" - "net" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace" "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/dyngo/instrumentation" @@ -30,20 +29,24 @@ func appsecUnaryHandlerMiddleware(span ddtrace.Span, handler grpc.UnaryHandler) if p, ok := peer.FromContext(ctx); ok { remoteAddr = p.Addr.String() } - ip := httpsec.IPFromHeaders(md, remoteAddr) - op := grpcsec.StartHandlerOperation(grpcsec.HandlerOperationArgs{Metadata: md, ClientIP: ip}, nil) + ipTags, clientIP := httpsec.ClientIPTags(md, remoteAddr) + instrumentation.SetStringTags(span, ipTags) + + op := grpcsec.StartHandlerOperation(grpcsec.HandlerOperationArgs{Metadata: md, ClientIP: clientIP}, nil) defer func() { events := op.Finish(grpcsec.HandlerOperationRes{}) instrumentation.SetTags(span, op.Tags()) if len(events) == 0 { return } - setAppSecTags(ctx, span, ip, events) + setAppSecEventsTags(ctx, span, events) }() + if op.BlockedCode != nil { - op.AddTag("appsec.blocked", true) + op.AddTag(httpsec.BlockedRequestTag, true) return nil, status.Errorf(*op.BlockedCode, "Request blocked") } + defer grpcsec.StartReceiveOperation(grpcsec.ReceiveOperationArgs{}, op).Finish(grpcsec.ReceiveOperationRes{Message: req}) return handler(ctx, req) } @@ -58,20 +61,24 @@ func appsecStreamHandlerMiddleware(span ddtrace.Span, handler grpc.StreamHandler if p, ok := peer.FromContext(stream.Context()); ok { remoteAddr = p.Addr.String() } - ip := httpsec.IPFromHeaders(md, remoteAddr) - op := grpcsec.StartHandlerOperation(grpcsec.HandlerOperationArgs{Metadata: md, ClientIP: ip}, nil) + ipTags, clientIP := httpsec.ClientIPTags(md, remoteAddr) + instrumentation.SetStringTags(span, ipTags) + + op := grpcsec.StartHandlerOperation(grpcsec.HandlerOperationArgs{Metadata: md, ClientIP: clientIP}, nil) defer func() { events := op.Finish(grpcsec.HandlerOperationRes{}) instrumentation.SetTags(span, op.Tags()) if len(events) == 0 { return } - setAppSecTags(stream.Context(), span, ip, events) + setAppSecEventsTags(stream.Context(), span, events) }() + if op.BlockedCode != nil { - op.AddTag("appsec.blocked", true) + op.AddTag(httpsec.BlockedRequestTag, true) return status.Error(*op.BlockedCode, "Request blocked") } + return handler(srv, appsecServerStream{ServerStream: stream, handlerOperation: op}) } } @@ -92,11 +99,7 @@ func (ss appsecServerStream) RecvMsg(m interface{}) error { } // Set the AppSec tags when security events were found. -func setAppSecTags(ctx context.Context, span ddtrace.Span, clientIP instrumentation.NetaddrIP, events []json.RawMessage) { +func setAppSecEventsTags(ctx context.Context, span ddtrace.Span, events []json.RawMessage) { md, _ := metadata.FromIncomingContext(ctx) - var peerAddr net.Addr - if p, ok := peer.FromContext(ctx); ok { - peerAddr = p.Addr - } - grpcsec.SetSecurityEventTags(span, events, clientIP, peerAddr, md) + grpcsec.SetSecurityEventTags(span, events, md) } diff --git a/contrib/labstack/echo.v4/appsec.go b/contrib/labstack/echo.v4/appsec.go index e5dce33396..1b8b90dfd3 100644 --- a/contrib/labstack/echo.v4/appsec.go +++ b/contrib/labstack/echo.v4/appsec.go @@ -6,8 +6,6 @@ package echo import ( - "net" - "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer" "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/dyngo/instrumentation" "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/dyngo/instrumentation/httpsec" @@ -16,23 +14,24 @@ import ( ) func useAppSec(c echo.Context, span tracer.Span) func() { - req := c.Request() instrumentation.SetAppSecEnabledTags(span) + params := make(map[string]string) for _, n := range c.ParamNames() { params[n] = c.Param(n) } - args := httpsec.MakeHandlerOperationArgs(req, params) + + req := c.Request() + ipTags, clientIP := httpsec.ClientIPTags(req.Header, req.RemoteAddr) + instrumentation.SetStringTags(span, ipTags) + + args := httpsec.MakeHandlerOperationArgs(req, clientIP, params) ctx, op := httpsec.StartOperation(req.Context(), args) c.SetRequest(req.WithContext(ctx)) return func() { events := op.Finish(httpsec.HandlerOperationRes{Status: c.Response().Status}) if len(events) > 0 { - remoteIP, _, err := net.SplitHostPort(req.RemoteAddr) - if err != nil { - remoteIP = req.RemoteAddr - } - httpsec.SetSecurityEventTags(span, events, remoteIP, args.Headers, c.Response().Writer.Header()) + httpsec.SetSecurityEventTags(span, events, args.Headers, c.Response().Writer.Header()) } instrumentation.SetTags(span, op.Tags()) } diff --git a/internal/appsec/dyngo/instrumentation/common.go b/internal/appsec/dyngo/instrumentation/common.go index 20fa4017cb..8d2d06e5d9 100644 --- a/internal/appsec/dyngo/instrumentation/common.go +++ b/internal/appsec/dyngo/instrumentation/common.go @@ -81,6 +81,14 @@ func SetTags(span TagSetter, tags map[string]interface{}) { } } +// SetStringTags fills the span tags using the key/value pairs of strings found +// in `tags` +func SetStringTags(span TagSetter, tags map[string]string) { + for k, v := range tags { + span.SetTag(k, v) + } +} + // SetAppSecEnabledTags sets the AppSec-specific span tags that are expected to be in // the web service entry span (span of type `web`) when AppSec is enabled. func SetAppSecEnabledTags(span TagSetter) { diff --git a/internal/appsec/dyngo/instrumentation/grpcsec/tags.go b/internal/appsec/dyngo/instrumentation/grpcsec/tags.go index 12ec59d7d3..871e81c266 100644 --- a/internal/appsec/dyngo/instrumentation/grpcsec/tags.go +++ b/internal/appsec/dyngo/instrumentation/grpcsec/tags.go @@ -7,7 +7,6 @@ package grpcsec import ( "encoding/json" - "net" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace" "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/dyngo/instrumentation" @@ -17,33 +16,20 @@ import ( // SetSecurityEventTags sets the AppSec-specific span tags when a security event // occurred into the service entry span. -func SetSecurityEventTags(span ddtrace.Span, events []json.RawMessage, clientIP instrumentation.NetaddrIP, peerAddr net.Addr, md map[string][]string) { - if err := setSecurityEventTags(span, events, clientIP, peerAddr, md); err != nil { +func SetSecurityEventTags(span ddtrace.Span, events []json.RawMessage, md map[string][]string) { + if err := setSecurityEventTags(span, events, md); err != nil { log.Error("appsec: %v", err) } } -func setSecurityEventTags(span ddtrace.Span, events []json.RawMessage, clientIP instrumentation.NetaddrIP, peerAddr net.Addr, md map[string][]string) error { +func setSecurityEventTags(span ddtrace.Span, events []json.RawMessage, md map[string][]string) error { if err := instrumentation.SetEventSpanTags(span, events); err != nil { return err } - var peerIP string - switch actual := peerAddr.(type) { - case *net.UDPAddr: - peerIP = actual.IP.String() - case *net.TCPAddr: - peerIP = actual.IP.String() - } - if peerIP != "" { - span.SetTag("network.client.ip", peerIP) - } - - if clientIP.IsValid() { - span.SetTag("http.client_ip", clientIP.String()) - } for h, v := range httpsec.NormalizeHTTPHeaders(md) { span.SetTag("grpc.metadata."+h, v) } + return nil } diff --git a/internal/appsec/dyngo/instrumentation/grpcsec/tags_test.go b/internal/appsec/dyngo/instrumentation/grpcsec/tags_test.go index 442c8bb944..8e34bdb802 100644 --- a/internal/appsec/dyngo/instrumentation/grpcsec/tags_test.go +++ b/internal/appsec/dyngo/instrumentation/grpcsec/tags_test.go @@ -12,10 +12,12 @@ import ( "testing" "github.com/stretchr/testify/require" + "google.golang.org/grpc/metadata" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext" "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/dyngo/instrumentation" + "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/dyngo/instrumentation/httpsec" "gopkg.in/DataDog/dd-trace-go.v1/internal/samplernames" ) @@ -53,127 +55,116 @@ func TestSetSecurityEventTags(t *testing.T) { }, } { eventCase := eventCase - for _, addrCase := range []struct { - name string - addr net.Addr - expectedTag string + for _, metadataCase := range []struct { + name string + md map[string][]string + expectedTags map[string]string }{ { - name: "tcp-ipv4-address", - addr: &net.TCPAddr{IP: net.ParseIP("1.2.3.4"), Port: 6789}, - expectedTag: "1.2.3.4", + name: "zero-metadata", }, { - name: "tcp-ipv6-address", - addr: &net.TCPAddr{IP: net.ParseIP("::1"), Port: 6789}, - expectedTag: "::1", - }, - { - name: "udp-ipv4-address", - addr: &net.UDPAddr{IP: net.ParseIP("1.2.3.4"), Port: 6789}, - expectedTag: "1.2.3.4", + name: "xff-metadata", + md: map[string][]string{ + "x-forwarded-for": {"1.2.3.4", "4.5.6.7"}, + ":authority": {"something"}, + }, + expectedTags: map[string]string{ + "grpc.metadata.x-forwarded-for": "1.2.3.4,4.5.6.7", + }, }, { - name: "udp-ipv6-address", - addr: &net.UDPAddr{IP: net.ParseIP("::1"), Port: 6789}, - expectedTag: "::1", + name: "xff-metadata", + md: map[string][]string{ + "x-forwarded-for": {"1.2.3.4"}, + ":authority": {"something"}, + }, + expectedTags: map[string]string{ + "grpc.metadata.x-forwarded-for": "1.2.3.4", + }, }, { - name: "unix-socket-address", - addr: &net.UnixAddr{Name: "/var/my.sock"}, + name: "no-monitored-metadata", + md: map[string][]string{ + ":authority": {"something"}, + }, }, } { - addrCase := addrCase - for _, metadataCase := range []struct { - name string - md map[string][]string - expectedTags map[string]string - }{ - { - name: "zero-metadata", - }, - { - name: "xff-metadata", - md: map[string][]string{ - "x-forwarded-for": {"1.2.3.4", "4.5.6.7"}, - ":authority": {"something"}, - }, - expectedTags: map[string]string{ - "grpc.metadata.x-forwarded-for": "1.2.3.4,4.5.6.7", - }, - }, - { - name: "xff-metadata", - md: map[string][]string{ - "x-forwarded-for": {"1.2.3.4"}, - ":authority": {"something"}, - }, - expectedTags: map[string]string{ - "grpc.metadata.x-forwarded-for": "1.2.3.4", - }, - }, - { - name: "no-monitored-metadata", - md: map[string][]string{ - ":authority": {"something"}, - }, - }, - } { - metadataCase := metadataCase - for _, clientIPCase := range []struct { - name string - clientIP instrumentation.NetaddrIP - expectedClientIPTag string - }{ - { - name: "invalid-client-ip", - clientIP: instrumentation.NetaddrIP{}, - expectedClientIPTag: "", - }, - { - name: "valid-client-ip", - clientIP: instrumentation.NetaddrIPv4(1, 2, 3, 4), - expectedClientIPTag: "1.2.3.4", - }, - } { - clientIPCase := clientIPCase - t.Run(fmt.Sprintf("%s-%s-%s-%s", eventCase.name, addrCase.name, metadataCase.name, clientIPCase.name), func(t *testing.T) { - var span MockSpan - err := setSecurityEventTags(&span, eventCase.events, clientIPCase.clientIP, addrCase.addr, metadataCase.md) - if eventCase.expectedError { - require.Error(t, err) - return - } - require.NoError(t, err) - - expectedTags := map[string]interface{}{ - "_dd.appsec.json": eventCase.expectedTag, - "manual.keep": true, - "appsec.event": true, - "_dd.origin": "appsec", - } - - if addr := addrCase.expectedTag; addr != "" { - expectedTags["network.client.ip"] = addr - } - - for k, v := range metadataCase.expectedTags { - expectedTags[k] = v - } - - if clientIPCase.expectedClientIPTag != "" { - expectedTags["http.client_ip"] = clientIPCase.expectedClientIPTag - } - - require.Equal(t, expectedTags, span.tags) - require.False(t, span.finished) - }) + metadataCase := metadataCase + t.Run(fmt.Sprintf("%s-%s", eventCase.name, metadataCase.name), func(t *testing.T) { + var span MockSpan + err := setSecurityEventTags(&span, eventCase.events, metadataCase.md) + if eventCase.expectedError { + require.Error(t, err) + return + } + require.NoError(t, err) + + expectedTags := map[string]interface{}{ + "_dd.appsec.json": eventCase.expectedTag, + "manual.keep": true, + "appsec.event": true, + "_dd.origin": "appsec", } - } + + for k, v := range metadataCase.expectedTags { + expectedTags[k] = v + } + + require.Equal(t, expectedTags, span.tags) + require.False(t, span.finished) + }) } } } +func TestClientIP(t *testing.T) { + for _, tc := range []struct { + name string + addr net.Addr + md metadata.MD + expectedClientIP string + }{ + { + name: "tcp-ipv4-address", + addr: &net.TCPAddr{IP: net.ParseIP("1.2.3.4"), Port: 6789}, + expectedClientIP: "1.2.3.4", + }, + { + name: "tcp-ipv4-address", + addr: &net.TCPAddr{IP: net.ParseIP("1.2.3.4"), Port: 6789}, + md: map[string][]string{"x-client-ip": {"127.0.0.1, 2.3.4.5"}}, + expectedClientIP: "2.3.4.5", + }, + { + name: "tcp-ipv6-address", + addr: &net.TCPAddr{IP: net.ParseIP("::1"), Port: 6789}, + expectedClientIP: "::1", + }, + { + name: "udp-ipv4-address", + addr: &net.UDPAddr{IP: net.ParseIP("1.2.3.4"), Port: 6789}, + expectedClientIP: "1.2.3.4", + }, + { + name: "udp-ipv6-address", + addr: &net.UDPAddr{IP: net.ParseIP("::1"), Port: 6789}, + expectedClientIP: "::1", + }, + { + name: "unix-socket-address", + addr: &net.UnixAddr{Name: "/var/my.sock"}, + }, + } { + tc := tc + t.Run(tc.name, func(t *testing.T) { + _, clientIP := httpsec.ClientIPTags(tc.md, tc.addr.String()) + expectedClientIP, _ := instrumentation.NetaddrParseIP(tc.expectedClientIP) + require.Equal(t, expectedClientIP.String(), clientIP.String()) + }) + } +} + type MockSpan struct { tags map[string]interface{} finished bool diff --git a/internal/appsec/dyngo/instrumentation/httpsec/http.go b/internal/appsec/dyngo/instrumentation/httpsec/http.go index 3ac42de62d..17606dee73 100644 --- a/internal/appsec/dyngo/instrumentation/httpsec/http.go +++ b/internal/appsec/dyngo/instrumentation/httpsec/http.go @@ -12,11 +12,9 @@ package httpsec import ( "context" - // Blank import needed to use embed for the default blocked response payloads _ "embed" "encoding/json" - "net" "net/http" "os" "reflect" @@ -24,7 +22,6 @@ import ( "sync" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace" - "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext" "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/dyngo" "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/dyngo/instrumentation" "gopkg.in/DataDog/dd-trace-go.v1/internal/log" @@ -82,7 +79,7 @@ func applyActions(op *Operation) http.Handler { for _, action := range op.Actions() { switch a := action.(type) { case *BlockRequestAction: - op.AddTag(tagBlockedRequest, true) + op.AddTag(BlockedRequestTag, true) return a.handler default: log.Error("appsec: ignoring security action: unexpected action type %T", a) @@ -96,9 +93,10 @@ func applyActions(op *Operation) http.Handler { func WrapHandler(handler http.Handler, span ddtrace.Span, pathParams map[string]string) http.Handler { instrumentation.SetAppSecEnabledTags(span) return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - SetIPTags(span, r) + ipTags, clientIP := ClientIPTags(r.Header, r.RemoteAddr) + instrumentation.SetStringTags(span, ipTags) - args := MakeHandlerOperationArgs(r, pathParams) + args := MakeHandlerOperationArgs(r, clientIP, pathParams) ctx, op := StartOperation(r.Context(), args) r = r.WithContext(ctx) @@ -118,11 +116,7 @@ func WrapHandler(handler http.Handler, span ddtrace.Span, pathParams map[string] } applyActions(op) - remoteIP, _, err := net.SplitHostPort(r.RemoteAddr) - if err != nil { - remoteIP = r.RemoteAddr - } - SetSecurityEventTags(span, events, remoteIP, args.Headers, w.Header()) + SetSecurityEventTags(span, events, args.Headers, w.Header()) }() handler.ServeHTTP(w, r) @@ -133,7 +127,7 @@ func WrapHandler(handler http.Handler, span ddtrace.Span, pathParams map[string] // MakeHandlerOperationArgs creates the HandlerOperationArgs out of a standard // http.Request along with the given current span. It returns an empty structure // when appsec is disabled. -func MakeHandlerOperationArgs(r *http.Request, pathParams map[string]string) HandlerOperationArgs { +func MakeHandlerOperationArgs(r *http.Request, clientIP instrumentation.NetaddrIP, pathParams map[string]string) HandlerOperationArgs { headers := make(http.Header, len(r.Header)) for k, v := range r.Header { k := strings.ToLower(k) @@ -145,14 +139,13 @@ func MakeHandlerOperationArgs(r *http.Request, pathParams map[string]string) Han } cookies := makeCookies(r) // TODO(Julio-Guerra): avoid actively parsing the cookies thanks to dynamic instrumentation headers["host"] = []string{r.Host} - ip := IPFromHeaders(r.Header, r.RemoteAddr) return HandlerOperationArgs{ RequestURI: r.RequestURI, Headers: headers, Cookies: cookies, Query: r.URL.Query(), // TODO(Julio-Guerra): avoid actively parsing the query values thanks to dynamic instrumentation PathParams: pathParams, - ClientIP: ip, + ClientIP: clientIP, } } @@ -315,12 +308,6 @@ func (f OnSDKBodyOperationFinish) Call(op dyngo.Operation, v interface{}) { f(op.(*SDKBodyOperation), v.(SDKBodyOperationRes)) } -// IPFromHeaders tries to resolve and return the user IP from request headers -func IPFromHeaders(hdrs map[string][]string, remoteAddr string) instrumentation.NetaddrIP { - ip := IPTagsFromHeaders(hdrs, remoteAddr)[ext.HTTPClientIP] - return parseIP(ip) -} - // blockedTemplateJSON is the default JSON template used to write responses for blocked requests // //go:embed blocked-template.json diff --git a/internal/appsec/dyngo/instrumentation/httpsec/tags.go b/internal/appsec/dyngo/instrumentation/httpsec/tags.go index d010a15722..5f07f140a7 100644 --- a/internal/appsec/dyngo/instrumentation/httpsec/tags.go +++ b/internal/appsec/dyngo/instrumentation/httpsec/tags.go @@ -8,7 +8,6 @@ package httpsec import ( "encoding/json" "net" - "net/http" "os" "sort" "strings" @@ -21,17 +20,21 @@ import ( const ( // envClientIPHeader is the name of the env var used to specify the IP header to be used for client IP collection. envClientIPHeader = "DD_TRACE_CLIENT_IP_HEADER" - // tagMultipleIPHeaders sets the multiple ip header tag used internally to tell the backend an error occurred when + + // multipleIPHeadersTag sets the multiple ip header tag used internally to tell the backend an error occurred when // retrieving an HTTP request client IP. - tagMultipleIPHeaders = "_dd.multiple-ip-headers" - // tagBlockedRequest used to convey whether a request is blocked - tagBlockedRequest = "appsec.blocked" + multipleIPHeadersTag = "_dd.multiple-ip-headers" + + // BlockedRequestTag used to convey whether a request is blocked + BlockedRequestTag = "appsec.blocked" ) var ( ipv6SpecialNetworks = []*instrumentation.NetaddrIPPrefix{ ippref("fec0::/10"), // site local } + + // List of IP-related headers leveraged to retrieve the public client IP address. defaultIPHeaders = []string{ "x-forwarded-for", "x-real-ip", @@ -43,6 +46,7 @@ var ( "via", "true-client-ip", } + // List of HTTP headers we collect and send. collectedHTTPHeaders = append(defaultIPHeaders, "host", @@ -55,21 +59,22 @@ var ( "accept", "accept-encoding", "accept-language") - clientIPHeader string + + clientIPHeaderCfg string ) func init() { // Required by sort.SearchStrings + sort.Strings(defaultIPHeaders[:]) sort.Strings(collectedHTTPHeaders[:]) - clientIPHeader = os.Getenv(envClientIPHeader) + clientIPHeaderCfg = os.Getenv(envClientIPHeader) } // SetSecurityEventTags sets the AppSec-specific span tags when a security event occurred into the service entry span. -func SetSecurityEventTags(span instrumentation.TagSetter, events []json.RawMessage, remoteIP string, headers, respHeaders map[string][]string) { +func SetSecurityEventTags(span instrumentation.TagSetter, events []json.RawMessage, headers, respHeaders map[string][]string) { if err := instrumentation.SetEventSpanTags(span, events); err != nil { log.Error("appsec: unexpected error while creating the appsec event tags: %v", err) } - span.SetTag("network.client.ip", remoteIP) for h, v := range NormalizeHTTPHeaders(headers) { span.SetTag("http.request.headers."+h, v) } @@ -105,67 +110,80 @@ func ippref(s string) *instrumentation.NetaddrIPPrefix { return nil } -// SetIPTags sets the IP related span tags for a given request -func SetIPTags(s instrumentation.TagSetter, r *http.Request) { - for k, v := range IPTagsFromHeaders(r.Header, r.RemoteAddr) { - s.SetTag(k, v) +// ClientIPTags generates the IP related span tags for a given request headers +func ClientIPTags(hdrs map[string][]string, remoteAddr string) (tags map[string]string, clientIP instrumentation.NetaddrIP) { + tags = map[string]string{} + monitoredHeaders := defaultIPHeaders + if clientIPHeaderCfg != "" { + monitoredHeaders = []string{clientIPHeaderCfg} } -} - -// IPTagsFromHeaders generates the IP related span tags for a given request's headers -// See https://docs.datadoghq.com/tracing/configure_data_security#configuring-a-client-ip-header for more information. -func IPTagsFromHeaders(hdrs map[string][]string, remoteAddr string) map[string]string { - tags := map[string]string{} - ipHeaders := defaultIPHeaders - if len(clientIPHeader) > 0 { - ipHeaders = []string{clientIPHeader} - } - var ( - headers []string - ips []string - ) - - // Make sure all headers are lower-case + // Filter the list of headers + foundHeaders := map[string][]string{} for k, v := range hdrs { - hdrs[strings.ToLower(k)] = v - } - for _, hdr := range ipHeaders { - if v := hdrs[hdr]; len(v) >= 1 && v[0] != "" { - headers = append(headers, hdr) - ips = append(ips, v[0]) + k = strings.ToLower(k) + if i := sort.SearchStrings(monitoredHeaders, k); i < len(monitoredHeaders) && monitoredHeaders[i] == k { + if len(v) >= 1 && v[0] != "" { + foundHeaders[k] = v + } } } - var privateIP instrumentation.NetaddrIP - // First try to use the IP from the headers if only one IP was found - if l := len(ips); l == 1 { - for _, ipstr := range strings.Split(ips[0], ",") { + // If more than one IP header is present, report them and don't return any client ip + if len(foundHeaders) > 1 { + var headers []string + for header, ips := range foundHeaders { + tags[ext.HTTPRequestHeaders+"."+header] = strings.Join(ips, ",") + headers = append(headers, header) + } + sort.Strings(headers) // produce a predictable value + tags[multipleIPHeadersTag] = strings.Join(headers, ",") + return tags, instrumentation.NetaddrIP{} + } + + // Walk IP-related headers + var foundIP instrumentation.NetaddrIP + for _, v := range foundHeaders { + // Handle multi-value headers by flattening the list of values + var ips []string + for _, ip := range v { + ips = append(ips, strings.Split(ip, ",")...) + } + + // Look for the first valid or global IP address in the comma-separated list + for _, ipstr := range ips { ip := parseIP(strings.TrimSpace(ipstr)) - if ip.IsValid() { - if isGlobal(ip) { - tags[ext.HTTPClientIP] = ip.String() - return tags - } else if !privateIP.IsValid() { - privateIP = ip - } + if !ip.IsValid() { + continue + } + // Replace foundIP if still not valid in order to keep the oldest + if !foundIP.IsValid() { + foundIP = ip + } + if isGlobal(ip) { + foundIP = ip + break } } - } else if l > 1 { // If more than one IP header, report them - for i := range ips { - tags[ext.HTTPRequestHeaders+"."+headers[i]] = ips[i] - } - tags[tagMultipleIPHeaders] = strings.Join(headers, ",") } - // Try to get a global IP from remoteAddr. If not, try to use any private IP we found - if remoteIP := parseIP(remoteAddr); remoteIP.IsValid() && (isGlobal(remoteIP) || !privateIP.IsValid()) { - tags[ext.HTTPClientIP] = remoteIP.String() - } else if privateIP.IsValid() { - tags[ext.HTTPClientIP] = privateIP.String() + // Decide which IP address is the client one by starting with the remote IP + remoteIP := parseIP(remoteAddr) + if remoteIP.IsValid() { + tags["network.client.ip"] = remoteIP.String() + clientIP = remoteIP + } + + // The IP address found in the headers supersedes a private remote IP address. + if foundIP.IsValid() && !isGlobal(remoteIP) || isGlobal(foundIP) { + clientIP = foundIP + } + + if clientIP.IsValid() { + tags[ext.HTTPClientIP] = clientIP.String() } - return tags + return tags, clientIP } func parseIP(s string) instrumentation.NetaddrIP { @@ -184,7 +202,7 @@ func isGlobal(ip instrumentation.NetaddrIP) bool { // IsPrivate also checks for ipv6 ULA. // We care to check for these addresses are not considered public, hence not global. // See https://www.rfc-editor.org/rfc/rfc4193.txt for more details. - isGlobal := !ip.IsPrivate() && !ip.IsLoopback() && !ip.IsLinkLocalUnicast() + isGlobal := ip.IsValid() && !ip.IsPrivate() && !ip.IsLoopback() && !ip.IsLinkLocalUnicast() if !isGlobal || !ip.Is6() { return isGlobal } diff --git a/internal/appsec/dyngo/instrumentation/httpsec/tags_test.go b/internal/appsec/dyngo/instrumentation/httpsec/tags_test.go index 9992a4448a..2cb9bee406 100644 --- a/internal/appsec/dyngo/instrumentation/httpsec/tags_test.go +++ b/internal/appsec/dyngo/instrumentation/httpsec/tags_test.go @@ -69,33 +69,87 @@ func genIPTestCases() []ipTestCase { ipv6Global := randGlobalIPv6().String() ipv4Private := randPrivateIPv4().String() ipv6Private := randPrivateIPv6().String() - tcs := []ipTestCase{} - // Simple ipv4 test cases over all headers - for _, header := range defaultIPHeaders { - tcs = append(tcs, ipTestCase{ - name: "ipv4-global." + header, - headers: map[string]string{header: ipv4Global}, + + tcs := []ipTestCase{ + { + name: "ipv4-global-remoteaddr", + remoteAddr: ipv4Global, expectedIP: instrumentation.NetaddrMustParseIP(ipv4Global), - }) - tcs = append(tcs, ipTestCase{ - name: "ipv4-private." + header, - headers: map[string]string{header: ipv4Private}, + }, + { + name: "ipv4-private-remoteaddr", + remoteAddr: ipv4Private, expectedIP: instrumentation.NetaddrMustParseIP(ipv4Private), - }) + }, + { + name: "ipv6-global-remoteaddr", + remoteAddr: ipv6Global, + expectedIP: instrumentation.NetaddrMustParseIP(ipv6Global), + }, + { + name: "ipv6-private-remoteaddr", + remoteAddr: ipv6Private, + expectedIP: instrumentation.NetaddrMustParseIP(ipv6Private), + }, } + + // Simple ipv4 test cases over all headers + for _, header := range defaultIPHeaders { + tcs = append(tcs, + ipTestCase{ + name: "ipv4-global." + header, + remoteAddr: ipv4Private, + headers: map[string]string{header: ipv4Global}, + expectedIP: instrumentation.NetaddrMustParseIP(ipv4Global), + }, + ipTestCase{ + name: "ipv4-private." + header, + headers: map[string]string{header: ipv4Private}, + remoteAddr: ipv6Private, + expectedIP: instrumentation.NetaddrMustParseIP(ipv4Private), + }, + ipTestCase{ + name: "ipv4-global-remoteaddr-local-ip-header." + header, + remoteAddr: ipv4Global, + headers: map[string]string{header: ipv4Private}, + expectedIP: instrumentation.NetaddrMustParseIP(ipv4Global), + }, + ipTestCase{ + name: "ipv4-global-remoteaddr-global-ip-header." + header, + remoteAddr: ipv6Global, + headers: map[string]string{header: ipv4Global}, + expectedIP: instrumentation.NetaddrMustParseIP(ipv4Global), + }) + } + // Simple ipv6 test cases over all headers for _, header := range defaultIPHeaders { tcs = append(tcs, ipTestCase{ name: "ipv6-global." + header, + remoteAddr: ipv4Private, headers: map[string]string{header: ipv6Global}, expectedIP: instrumentation.NetaddrMustParseIP(ipv6Global), - }) - tcs = append(tcs, ipTestCase{ - name: "ipv6-private." + header, - headers: map[string]string{header: ipv6Private}, - expectedIP: instrumentation.NetaddrMustParseIP(ipv6Private), - }) + }, + ipTestCase{ + name: "ipv6-private." + header, + headers: map[string]string{header: ipv6Private}, + remoteAddr: ipv4Private, + expectedIP: instrumentation.NetaddrMustParseIP(ipv6Private), + }, + ipTestCase{ + name: "ipv6-global-remoteaddr-local-ip-header." + header, + remoteAddr: ipv6Global, + headers: map[string]string{header: ipv6Private}, + expectedIP: instrumentation.NetaddrMustParseIP(ipv6Global), + }, + ipTestCase{ + name: "ipv6-global-remoteaddr-global-ip-header." + header, + remoteAddr: ipv4Global, + headers: map[string]string{header: ipv6Global}, + expectedIP: instrumentation.NetaddrMustParseIP(ipv6Global), + }) } + // private and global in same header tcs = append([]ipTestCase{ { @@ -118,7 +172,18 @@ func genIPTestCases() []ipTestCase { headers: map[string]string{"x-forwarded-for": ipv6Global + "," + ipv6Private}, expectedIP: instrumentation.NetaddrMustParseIP(ipv6Global), }, + { + name: "mixed-global+global", + headers: map[string]string{"x-forwarded-for": ipv4Private + "," + ipv6Global + "," + ipv4Global}, + expectedIP: instrumentation.NetaddrMustParseIP(ipv6Global), + }, + { + name: "mixed-global+global", + headers: map[string]string{"x-forwarded-for": ipv4Private + "," + ipv4Global + "," + ipv6Global}, + expectedIP: instrumentation.NetaddrMustParseIP(ipv4Global), + }, }, tcs...) + // Invalid IPs (or a mix of valid/invalid over a single or multiple headers) tcs = append([]ipTestCase{ { @@ -126,22 +191,28 @@ func genIPTestCases() []ipTestCase { headers: map[string]string{"x-forwarded-for": "127..0.0.1"}, expectedIP: instrumentation.NetaddrIP{}, }, + { + name: "invalid-ipv4-header-valid-remoteaddr", + headers: map[string]string{"x-forwarded-for": "127..0.0.1"}, + remoteAddr: ipv4Private, + expectedIP: instrumentation.NetaddrMustParseIP(ipv4Private), + }, { name: "invalid-ipv4-recover", - headers: map[string]string{"x-forwarded-for": "127..0.0.1, " + ipv4Global}, + headers: map[string]string{"x-forwarded-for": "127..0.0.1, " + ipv6Private + "," + ipv4Global}, expectedIP: instrumentation.NetaddrMustParseIP(ipv4Global), }, { name: "ipv4-multi-header-1", headers: map[string]string{"x-forwarded-for": "127.0.0.1", "forwarded-for": ipv4Global}, expectedIP: instrumentation.NetaddrIP{}, - multiHeaders: "x-forwarded-for,forwarded-for", + multiHeaders: "forwarded-for,x-forwarded-for", }, { name: "ipv4-multi-header-2", headers: map[string]string{"forwarded-for": ipv4Global, "x-forwarded-for": "127.0.0.1"}, expectedIP: instrumentation.NetaddrIP{}, - multiHeaders: "x-forwarded-for,forwarded-for", + multiHeaders: "forwarded-for,x-forwarded-for", }, { name: "invalid-ipv6", @@ -157,82 +228,62 @@ func genIPTestCases() []ipTestCase { name: "ipv6-multi-header-1", headers: map[string]string{"x-forwarded-for": "2001:0db8:2001:zzzz::", "forwarded-for": ipv6Global}, expectedIP: instrumentation.NetaddrIP{}, - multiHeaders: "x-forwarded-for,forwarded-for", + multiHeaders: "forwarded-for,x-forwarded-for", }, { name: "ipv6-multi-header-2", headers: map[string]string{"forwarded-for": ipv6Global, "x-forwarded-for": "2001:0db8:2001:zzzz::"}, expectedIP: instrumentation.NetaddrIP{}, - multiHeaders: "x-forwarded-for,forwarded-for", + multiHeaders: "forwarded-for,x-forwarded-for", }, - }, tcs...) - tcs = append([]ipTestCase{ { name: "no-headers", expectedIP: instrumentation.NetaddrIP{}, }, { name: "header-case", - expectedIP: instrumentation.NetaddrMustParseIP(ipv4Global), headers: map[string]string{"X-fOrWaRdEd-FoR": ipv4Global}, + expectedIP: instrumentation.NetaddrMustParseIP(ipv4Global), }, { name: "user-header", - expectedIP: instrumentation.NetaddrMustParseIP(ipv4Global), headers: map[string]string{"x-forwarded-for": ipv6Global, "custom-header": ipv4Global}, clientIPHeader: "custom-header", + expectedIP: instrumentation.NetaddrMustParseIP(ipv4Global), }, { name: "user-header-not-found", - expectedIP: instrumentation.NetaddrIP{}, headers: map[string]string{"x-forwarded-for": ipv4Global}, clientIPHeader: "custom-header", + expectedIP: instrumentation.NetaddrIP{}, }, }, tcs...) return tcs } -type mockspan struct { - tags map[string]interface{} -} - -func (m *mockspan) SetTag(tag string, value interface{}) { - if m.tags == nil { - m.tags = make(map[string]interface{}) - } - m.tags[tag] = value -} - -func (m *mockspan) Tag(tag string) interface{} { - if m.tags == nil { - return nil - } - return m.tags[tag] -} - func TestIPHeaders(t *testing.T) { - // Make sure to restore the real value of clientIPHeader at the end of the test - defer func(s string) { clientIPHeader = s }(clientIPHeader) + // Make sure to restore the real value of clientIPHeaderCfg at the end of the test + defer func(s string) { clientIPHeaderCfg = s }(clientIPHeaderCfg) for _, tc := range genIPTestCases() { t.Run(tc.name, func(t *testing.T) { header := http.Header{} for k, v := range tc.headers { header.Add(k, v) } - r := http.Request{Header: header, RemoteAddr: tc.remoteAddr} - clientIPHeader = tc.clientIPHeader - var span mockspan - SetIPTags(&span, &r) + clientIPHeaderCfg = tc.clientIPHeader + tags, clientIP := ClientIPTags(header, tc.remoteAddr) if tc.expectedIP.IsValid() { - require.Equal(t, tc.expectedIP.String(), span.Tag(ext.HTTPClientIP)) - require.Nil(t, span.Tag(tagMultipleIPHeaders)) + expectedIP := tc.expectedIP.String() + require.Equal(t, expectedIP, tags[ext.HTTPClientIP]) + require.Equal(t, expectedIP, clientIP.String()) + require.NotContains(t, tags, multipleIPHeadersTag) } else { - require.Nil(t, span.Tag(ext.HTTPClientIP)) + require.NotContains(t, tags, ext.HTTPClientIP) if tc.multiHeaders != "" { - require.Equal(t, tc.multiHeaders, span.Tag(tagMultipleIPHeaders)) + require.Equal(t, tc.multiHeaders, tags[multipleIPHeadersTag]) for hdr, ip := range tc.headers { - require.Equal(t, ip, span.Tag(ext.HTTPRequestHeaders+"."+hdr)) + require.Equal(t, ip, tags[ext.HTTPRequestHeaders+"."+hdr]) } } }