Skip to content

Commit

Permalink
Applying some of the feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
RomainMuller committed Apr 9, 2024
1 parent 91a02b9 commit b8ae221
Show file tree
Hide file tree
Showing 11 changed files with 57 additions and 62 deletions.
4 changes: 2 additions & 2 deletions contrib/gin-gonic/gin/appsec.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ func useAppSec(c *gin.Context, span tracer.Span) {
c.Request = r
c.Next()
})
httpsec.WrapHandler(httpWrapper, span, params, func() {
c.Abort()
httpsec.WrapHandler(httpWrapper, span, params, &httpsec.Config{
OnBlock: []func(){func() { c.Abort() }},
}).ServeHTTP(c.Writer, c.Request)
}
6 changes: 3 additions & 3 deletions contrib/go-chi/chi.v5/appsec.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@ import (
"github.com/go-chi/chi/v5"
)

func withAppsec(next http.Handler, r *http.Request, span tracer.Span, opts ...httpsec.WrapHandlerOption) http.Handler {
func withAppsec(next http.Handler, r *http.Request, span tracer.Span, cfg *httpsec.Config) http.Handler {
rctx := chi.RouteContext(r.Context())
if rctx == nil {
return httpsec.WrapHandler(next, span, nil)
return httpsec.WrapHandler(next, span, nil, cfg)
}
var pathParams map[string]string
keys := rctx.URLParams.Keys
Expand All @@ -28,5 +28,5 @@ func withAppsec(next http.Handler, r *http.Request, span tracer.Span, opts ...ht
pathParams[key] = values[i]
}
}
return httpsec.WrapHandlerOpts(next, span, pathParams, opts...)
return httpsec.WrapHandler(next, span, pathParams, cfg)
}
2 changes: 1 addition & 1 deletion contrib/go-chi/chi.v5/chi.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ func Middleware(opts ...Option) func(next http.Handler) http.Handler {

next := next // avoid modifying the value of next in the outer closure scope
if appsec.Enabled() && !cfg.appsecDisabled {
next = withAppsec(next, r, span, cfg.appsecOptions...)
next = withAppsec(next, r, span, &cfg.appsecConfig)
// Note that the following response writer passed to the handler
// implements the `interface { Status() int }` expected by httpsec.
}
Expand Down
22 changes: 13 additions & 9 deletions contrib/go-chi/chi.v5/option.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ type config struct {
headerTags *internal.LockMap
resourceNamer func(r *http.Request) string
appsecDisabled bool
appsecOptions []httpsec.WrapHandlerOption
appsecConfig httpsec.Config
}

// Option represents an option that can be passed to NewRouter.
Expand Down Expand Up @@ -144,17 +144,21 @@ func WithAppsecDisabled(disabled bool) Option {
}
}

// WithAppsecOptions allows for specifying options for the AppSec middleware.
func WithAppsecOptions(opts ...httpsec.WrapHandlerOption) Option {
// WithAppsecOnBlock configures callbacks to be invoked when an AppSec blocking decision is made.
// This has no effect if AppSec is not in use.
func WithAppsecOnBlock(fs ...func()) Option {
return func(cfg *config) {
cfg.appsecOptions = opts
cfg.appsecConfig.OnBlock = append(cfg.appsecConfig.OnBlock, fs...)
}
}

// WithResponseHdrFetcher provides a function to fetch the response headers from the http.ResponseWriter.
// This allows for custom implementations as needed if you over-ride the default http.ResponseWriter.
func WithResponseHdrFetcher(f func(http.ResponseWriter) http.Header) httpsec.WrapHandlerOption {
return func(cfg *httpsec.WrapHandlerCfg) {
cfg.ResponseHdrFetcher = f
// WithResponseHeaderCopier provides a function to fetch the response headers from the
// http.ResponseWriter. This allows for custom implementations as needed if you over-ride the
// default http.ResponseWriter, such as to add synchronization. Provided functions may elect to
// return a copy of the http.Header map instead of a reference to the original (e.g: to not risk
// breaking synchronization).
func WithResponseHeaderCopier(f func(http.ResponseWriter) http.Header) Option {
return func(cfg *config) {
cfg.appsecConfig.ResponseHeaderCopier = f
}
}
4 changes: 2 additions & 2 deletions contrib/go-chi/chi/appsec.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import (
func withAppsec(next http.Handler, r *http.Request, span tracer.Span) http.Handler {
rctx := chi.RouteContext(r.Context())
if rctx == nil {
return httpsec.WrapHandler(next, span, nil)
return httpsec.WrapHandler(next, span, nil, nil)
}
var pathParams map[string]string
keys := rctx.URLParams.Keys
Expand All @@ -28,5 +28,5 @@ func withAppsec(next http.Handler, r *http.Request, span tracer.Span) http.Handl
pathParams[key] = values[i]
}
}
return httpsec.WrapHandler(next, span, pathParams)
return httpsec.WrapHandler(next, span, pathParams, nil)
}
2 changes: 1 addition & 1 deletion contrib/labstack/echo.v4/appsec.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ func withAppSec(next echo.HandlerFunc, span tracer.Span) echo.HandlerFunc {
}
})
// Wrap the echo response to allow monitoring of the response status code in httpsec.WrapHandler()
httpsec.WrapHandler(handler, span, params).ServeHTTP(&statusResponseWriter{Response: c.Response()}, c.Request())
httpsec.WrapHandler(handler, span, params, nil).ServeHTTP(&statusResponseWriter{Response: c.Response()}, c.Request())
// If an error occurred, wrap it under an echo.HTTPError. We need to do this so that APM doesn't override
// the response code tag with 500 in case it doesn't recognize the error type.
if _, ok := err.(*echo.HTTPError); !ok && err != nil {
Expand Down
2 changes: 1 addition & 1 deletion contrib/net/http/trace.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ func TraceAndServe(h http.Handler, w http.ResponseWriter, r *http.Request, cfg *
}()

if appsec.Enabled() {
h = httpsec.WrapHandler(h, span, cfg.RouteParams)
h = httpsec.WrapHandler(h, span, cfg.RouteParams, nil)
}
h.ServeHTTP(rw, r.WithContext(ctx))
}
Expand Down
19 changes: 19 additions & 0 deletions internal/appsec/emitter/httpsec/config.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package httpsec

import "net/http"

type Config struct {
// OnBlock is a list of callbacks to be invoked when a block decision is made.
OnBlock []func()
// ResponseHeaderCopier provides a way to access response headers for reading
// purposes (the value may be provided by copy). This allows customers to
// apply synchronization if they allow http.ResponseWriter objects to be
// accessed by multiple goroutines.
ResponseHeaderCopier func(http.ResponseWriter) http.Header
}

var defaultWrapHandlerConfig = &Config{
ResponseHeaderCopier: func(w http.ResponseWriter) http.Header {
return w.Header()
},
}
32 changes: 14 additions & 18 deletions internal/appsec/emitter/httpsec/http.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,11 +55,18 @@ func ExecuteSDKBodyOperation(parent dyngo.Operation, args types.SDKBodyOperation
return err
}

// WrapHandlerOpts is WrapHandler with support for flexible options.
func WrapHandlerOpts(handler http.Handler, span ddtrace.Span, pathParams map[string]string, opts ...WrapHandlerOption) http.Handler {
cfg := defaultWrapHandlerCfg()
for _, opt := range opts {
opt(cfg)
// WrapHandler wraps the given HTTP handler with the abstract HTTP operation defined by HandlerOperationArgs and
// HandlerOperationRes.
// The onBlock params are used to cleanup the context when needed.
// It is a specific patch meant for Gin, for which we must abort the
// context since it uses a queue of handlers and it's the only way to make
// sure other queued handlers don't get executed.
// TODO: this patch must be removed/improved when we rework our actions/operations system
func WrapHandler(handler http.Handler, span ddtrace.Span, pathParams map[string]string, opts *Config) http.Handler {
if opts == nil {
opts = defaultWrapHandlerConfig
} else if opts.ResponseHeaderCopier == nil {
opts.ResponseHeaderCopier = defaultWrapHandlerConfig.ResponseHeaderCopier
}

trace.SetAppSecEnabledTags(span)
Expand All @@ -86,7 +93,7 @@ func WrapHandlerOpts(handler http.Handler, span ddtrace.Span, pathParams map[str
// in case we are instrumenting the Gin framework
if blocking {
op.SetTag(trace.BlockedRequestTag, true)
for _, f := range cfg.OnBlock {
for _, f := range opts.OnBlock {
f()
}
}
Expand All @@ -99,7 +106,7 @@ func WrapHandlerOpts(handler http.Handler, span ddtrace.Span, pathParams map[str
// extra headers have been added such as the Host header which is removed from the original Go request headers
// map
setRequestHeadersTags(span, args.Headers)
setResponseHeadersTags(span, cfg.ResponseHdrFetcher(w))
setResponseHeadersTags(span, opts.ResponseHeaderCopier(w))
trace.SetTags(span, op.Tags())
if len(events) > 0 {
httptrace.SetSecurityEventsTags(span, events)
Expand All @@ -114,17 +121,6 @@ func WrapHandlerOpts(handler http.Handler, span ddtrace.Span, pathParams map[str
})
}

// WrapHandler wraps the given HTTP handler with the abstract HTTP operation defined by HandlerOperationArgs and
// HandlerOperationRes.
// The onBlock params are used to cleanup the context when needed.
// It is a specific patch meant for Gin, for which we must abort the
// context since it uses a queue of handlers and it's the only way to make
// sure other queued handlers don't get executed.
// TODO: this patch must be removed/improved when we rework our actions/operations system
func WrapHandler(handler http.Handler, span ddtrace.Span, pathParams map[string]string, onBlock ...func()) http.Handler {
return WrapHandlerOpts(handler, span, pathParams, WithOnBlock(onBlock...))
}

// MakeHandlerOperationArgs creates the HandlerOperationArgs value.
func MakeHandlerOperationArgs(r *http.Request, clientIP netip.Addr, pathParams map[string]string) types.HandlerOperationArgs {
cookies := makeCookies(r) // TODO(Julio-Guerra): avoid actively parsing the cookies thanks to dynamic instrumentation
Expand Down
24 changes: 0 additions & 24 deletions internal/appsec/emitter/httpsec/options.go

This file was deleted.

2 changes: 1 addition & 1 deletion internal/version/version.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import (
// Tag specifies the current release tag. It needs to be manually
// updated. A test checks that the value of Tag never points to a
// git tag that is older than HEAD.
const Tag = "v1.62.0-rc.1"
const Tag = "v1.62.0-dev"

// Dissected version number. Filled during init()
var (
Expand Down

0 comments on commit b8ae221

Please sign in to comment.