From b0160e2252894edf17fa72d7540b7ce001319d1c Mon Sep 17 00:00:00 2001 From: Romain Marcadier Date: Mon, 5 Feb 2024 14:16:07 +0100 Subject: [PATCH] feedback from @Julio-Guerra --- internal/appsec/dyngo/operation.go | 8 +-- internal/appsec/dyngo/operation_test.go | 64 +++++++++---------- .../appsec/emitter/graphqlsec/types/types.go | 6 +- internal/appsec/emitter/grpcsec/grpc_test.go | 2 +- .../appsec/emitter/grpcsec/types/types.go | 4 +- internal/appsec/emitter/httpsec/http.go | 2 +- .../appsec/emitter/httpsec/types/types.go | 4 +- internal/appsec/emitter/sharedsec/shared.go | 2 +- .../appsec/listener/graphqlsec/graphql.go | 6 +- internal/appsec/listener/grpcsec/grpc.go | 6 +- internal/appsec/listener/httpsec/http.go | 6 +- internal/appsec/waf.go | 4 ++ 12 files changed, 57 insertions(+), 57 deletions(-) diff --git a/internal/appsec/dyngo/operation.go b/internal/appsec/dyngo/operation.go index a7696a86f6..e16d357e9d 100644 --- a/internal/appsec/dyngo/operation.go +++ b/internal/appsec/dyngo/operation.go @@ -150,14 +150,10 @@ func StartOperation[O Operation, E ArgOf[O]](op O, args E) { } } -func newOperation(parent Operation) *operation { - return &operation{parent: parent.unwrap()} -} - -// Finish finishes the operation along with its results and emits a +// FinishOperation finishes the operation along with its results and emits a // finish event with the operation results. // The operation is then disabled and its event listeners removed. -func Finish[O Operation, E ResultOf[O]](op O, results E) { +func FinishOperation[O Operation, E ResultOf[O]](op O, results E) { o := op.unwrap() defer o.disable() // This will need the RLock below to be released... diff --git a/internal/appsec/dyngo/operation_test.go b/internal/appsec/dyngo/operation_test.go index 6a710dd9d6..08f1150c9b 100644 --- a/internal/appsec/dyngo/operation_test.go +++ b/internal/appsec/dyngo/operation_test.go @@ -340,7 +340,7 @@ func TestUsage(t *testing.T) { t.Run("recursive-operation", func(t *testing.T) { root := startOperation(RootArgs{}, nil) - defer dyngo.Finish(root, RootRes{}) + defer dyngo.FinishOperation(root, RootRes{}) called := 0 dyngo.On(root, func(operation, HTTPHandlerArgs) { called++ }) @@ -362,7 +362,7 @@ func TestUsage(t *testing.T) { t.Run("concurrency", func(t *testing.T) { // root is the shared operation having concurrent accesses in this test root := startOperation(RootArgs{}, nil) - defer dyngo.Finish(root, RootRes{}) + defer dyngo.FinishOperation(root, RootRes{}) // Create nbGoroutines registering event listeners concurrently nbGoroutines := 1000 @@ -405,7 +405,7 @@ func TestUsage(t *testing.T) { startBarrier.Wait() defer done.Done() op := startOperation(MyOperationArgs{}, root) - defer dyngo.Finish(op, MyOperationRes{}) + defer dyngo.FinishOperation(op, MyOperationRes{}) }() } @@ -463,7 +463,7 @@ func startOperation[T dyngo.ArgOf[operation]](args T, parent dyngo.Operation) op // Helper function to run operations recursively. func runOperation[A dyngo.ArgOf[operation], R dyngo.ResultOf[operation]](parent dyngo.Operation, args A, res R, child func(dyngo.Operation)) { op := startOperation(args, parent) - defer dyngo.Finish(op, res) + defer dyngo.FinishOperation(op, res) if child != nil { child(op) } @@ -479,7 +479,7 @@ func TestOperationData(t *testing.T) { for i := 0; i < 10; i++ { dyngo.EmitData(op, &data) } - dyngo.Finish(op, MyOperationRes{}) + dyngo.FinishOperation(op, MyOperationRes{}) require.Equal(t, 10, data) }) @@ -493,8 +493,8 @@ func TestOperationData(t *testing.T) { for i := 0; i < 10; i++ { dyngo.EmitData(op2, &data) } - dyngo.Finish(op2, MyOperation2Res{}) - dyngo.Finish(op1, MyOperationRes{}) + dyngo.FinishOperation(op2, MyOperation2Res{}) + dyngo.FinishOperation(op1, MyOperationRes{}) require.Equal(t, 10, data) }) @@ -507,8 +507,8 @@ func TestOperationData(t *testing.T) { for i := 0; i < 10; i++ { dyngo.EmitData(op2, &data) } - dyngo.Finish(op2, MyOperation2Res{}) - dyngo.Finish(op1, MyOperationRes{}) + dyngo.FinishOperation(op2, MyOperation2Res{}) + dyngo.FinishOperation(op1, MyOperationRes{}) require.Equal(t, 20, data) }) }) @@ -524,22 +524,22 @@ func TestOperationEvents(t *testing.T) { }) op2 := startOperation(MyOperation2Args{}, op1) - dyngo.Finish(op2, MyOperation2Res{}) + dyngo.FinishOperation(op2, MyOperation2Res{}) // Called once require.Equal(t, 1, called) op2 = startOperation(MyOperation2Args{}, op1) - dyngo.Finish(op2, MyOperation2Res{}) + dyngo.FinishOperation(op2, MyOperation2Res{}) // Called again require.Equal(t, 2, called) // Finish the operation so that it gets disabled and its listeners removed - dyngo.Finish(op1, MyOperationRes{}) + dyngo.FinishOperation(op1, MyOperationRes{}) op2 = startOperation(MyOperation2Args{}, op1) - dyngo.Finish(op2, MyOperation2Res{}) + dyngo.FinishOperation(op2, MyOperation2Res{}) // No longer called require.Equal(t, 2, called) @@ -554,35 +554,35 @@ func TestOperationEvents(t *testing.T) { }) op2 := startOperation(MyOperation2Args{}, op1) - dyngo.Finish(op2, MyOperation2Res{}) + dyngo.FinishOperation(op2, MyOperation2Res{}) // Called once require.Equal(t, 1, called) op2 = startOperation(MyOperation2Args{}, op1) - dyngo.Finish(op2, MyOperation2Res{}) + dyngo.FinishOperation(op2, MyOperation2Res{}) // Called again require.Equal(t, 2, called) op3 := startOperation(MyOperation3Args{}, op2) - dyngo.Finish(op3, MyOperation3Res{}) + dyngo.FinishOperation(op3, MyOperation3Res{}) // Not called require.Equal(t, 2, called) op2 = startOperation(MyOperation2Args{}, op3) - dyngo.Finish(op2, MyOperation2Res{}) + dyngo.FinishOperation(op2, MyOperation2Res{}) // Called again require.Equal(t, 3, called) // Finish the operation so that it gets disabled and its listeners removed - dyngo.Finish(op1, MyOperationRes{}) + dyngo.FinishOperation(op1, MyOperationRes{}) op2 = startOperation(MyOperation2Args{}, op3) - dyngo.Finish(op2, MyOperation2Res{}) + dyngo.FinishOperation(op2, MyOperation2Res{}) // No longer called require.Equal(t, 3, called) op2 = startOperation(MyOperation2Args{}, op2) - dyngo.Finish(op2, MyOperation2Res{}) + dyngo.FinishOperation(op2, MyOperation2Res{}) // No longer called require.Equal(t, 3, called) }) @@ -605,16 +605,16 @@ func TestOperationEvents(t *testing.T) { // Trigger the registered events op2 := startOperation(MyOperation2Args{}, op) - dyngo.Finish(op2, MyOperation2Res{}) + dyngo.FinishOperation(op2, MyOperation2Res{}) // We should have 4 calls require.Equal(t, 2, calls) // Finish the operation to disable it. Its event listeners should then be removed. - dyngo.Finish(op, MyOperationRes{}) + dyngo.FinishOperation(op, MyOperationRes{}) // Trigger the same events op2 = startOperation(MyOperation2Args{}, op) - dyngo.Finish(op2, MyOperation2Res{}) + dyngo.FinishOperation(op2, MyOperation2Res{}) // The number of calls should be unchanged require.Equal(t, 2, calls) @@ -622,7 +622,7 @@ func TestOperationEvents(t *testing.T) { registerTo(op) // Trigger the same events op2 = startOperation(MyOperation2Args{}, op) - dyngo.Finish(op2, MyOperation2Res{}) + dyngo.FinishOperation(op2, MyOperation2Res{}) // The number of calls should be unchanged require.Equal(t, 2, calls) }) @@ -630,7 +630,7 @@ func TestOperationEvents(t *testing.T) { t.Run("event-listener-panic", func(t *testing.T) { t.Run("start", func(t *testing.T) { op := startOperation(MyOperationArgs{}, nil) - defer dyngo.Finish(op, MyOperationRes{}) + defer dyngo.FinishOperation(op, MyOperationRes{}) // Panic on start calls := 0 @@ -643,14 +643,14 @@ func TestOperationEvents(t *testing.T) { require.NotPanics(t, func() { op := startOperation(MyOperationArgs{}, op) require.NotNil(t, op) - defer dyngo.Finish(op, MyOperationRes{}) + defer dyngo.FinishOperation(op, MyOperationRes{}) require.Equal(t, calls, 1) }) }) t.Run("finish", func(t *testing.T) { op := startOperation(MyOperationArgs{}, nil) - defer dyngo.Finish(op, MyOperationRes{}) + defer dyngo.FinishOperation(op, MyOperationRes{}) // Panic on finish calls := 0 dyngo.OnFinish(op, func(operation, MyOperationRes) { @@ -662,7 +662,7 @@ func TestOperationEvents(t *testing.T) { require.NotPanics(t, func() { op := startOperation(MyOperationArgs{}, op) require.NotNil(t, op) - dyngo.Finish(op, MyOperationRes{}) + dyngo.FinishOperation(op, MyOperationRes{}) require.Equal(t, calls, 1) }) }) @@ -675,12 +675,12 @@ func BenchmarkEvents(b *testing.B) { for length := 1; length <= 64; length *= 2 { b.Run(fmt.Sprintf("stack=%d", length), func(b *testing.B) { root := startOperation(MyOperationArgs{}, nil) - defer dyngo.Finish(root, MyOperationRes{}) + defer dyngo.FinishOperation(root, MyOperationRes{}) op := root for i := 0; i < length-1; i++ { op = startOperation(MyOperationArgs{}, op) - defer dyngo.Finish(op, MyOperationRes{}) + defer dyngo.FinishOperation(op, MyOperationRes{}) } b.Run("start event", func(b *testing.B) { @@ -700,7 +700,7 @@ func BenchmarkEvents(b *testing.B) { b.ResetTimer() for n := 0; n < b.N; n++ { leafOp := startOperation(MyOperationArgs{}, op) - dyngo.Finish(leafOp, MyOperationRes{}) + dyngo.FinishOperation(leafOp, MyOperationRes{}) } }) }) @@ -709,7 +709,7 @@ func BenchmarkEvents(b *testing.B) { b.Run("registering", func(b *testing.B) { op := startOperation(MyOperationArgs{}, nil) - defer dyngo.Finish(op, MyOperationRes{}) + defer dyngo.FinishOperation(op, MyOperationRes{}) b.Run("start event", func(b *testing.B) { b.ReportAllocs() diff --git a/internal/appsec/emitter/graphqlsec/types/types.go b/internal/appsec/emitter/graphqlsec/types/types.go index 93f94b7121..d8b0d1948c 100644 --- a/internal/appsec/emitter/graphqlsec/types/types.go +++ b/internal/appsec/emitter/graphqlsec/types/types.go @@ -35,7 +35,7 @@ type ( // Finish the GraphQL query operation, along with the given results, and emit a finish event up in // the operation stack. func (q *RequestOperation) Finish(res RequestOperationRes) { - dyngo.Finish(q, res) + dyngo.FinishOperation(q, res) } func (RequestOperationArgs) IsArgOf(*RequestOperation) {} @@ -69,7 +69,7 @@ type ( // Finish the GraphQL query operation, along with the given results, and emit a finish event up in // the operation stack. func (q *ExecutionOperation) Finish(res ExecutionOperationRes) { - dyngo.Finish(q, res) + dyngo.FinishOperation(q, res) } func (ExecutionOperationArgs) IsArgOf(*ExecutionOperation) {} @@ -105,7 +105,7 @@ type ( // Finish the GraphQL Field operation, along with the given results, and emit a finish event up in // the operation stack. func (q *ResolveOperation) Finish(res ResolveOperationRes) { - dyngo.Finish(q, res) + dyngo.FinishOperation(q, res) } func (ResolveOperationArgs) IsArgOf(*ResolveOperation) {} diff --git a/internal/appsec/emitter/grpcsec/grpc_test.go b/internal/appsec/emitter/grpcsec/grpc_test.go index 1c6cd8bcb8..c5d8d0916d 100644 --- a/internal/appsec/emitter/grpcsec/grpc_test.go +++ b/internal/appsec/emitter/grpcsec/grpc_test.go @@ -30,7 +30,7 @@ func TestUsage(t *testing.T) { return func(t *testing.T) { localRootOp := dyngo.NewRootOperation() dyngo.StartOperation(localRootOp, rootArgs{}) - defer dyngo.Finish(localRootOp, rootRes{}) + defer dyngo.FinishOperation(localRootOp, rootRes{}) var handlerStarted, handlerFinished, recvStarted, recvFinished int defer func() { diff --git a/internal/appsec/emitter/grpcsec/types/types.go b/internal/appsec/emitter/grpcsec/types/types.go index 0fd27bb6dc..7d3f47d82c 100644 --- a/internal/appsec/emitter/grpcsec/types/types.go +++ b/internal/appsec/emitter/grpcsec/types/types.go @@ -91,14 +91,14 @@ func (e *MonitoringError) Error() string { // Finish the gRPC handler operation, along with the given results, and emit a // finish event up in the operation stack. func (op *HandlerOperation) Finish(res HandlerOperationRes) []any { - dyngo.Finish(op, res) + dyngo.FinishOperation(op, res) return op.Events() } // Finish the gRPC handler operation, along with the given results, and emits a // finish event up in the operation stack. func (op ReceiveOperation) Finish(res ReceiveOperationRes) { - dyngo.Finish(op, res) + dyngo.FinishOperation(op, res) } func (HandlerOperationArgs) IsArgOf(*HandlerOperation) {} diff --git a/internal/appsec/emitter/httpsec/http.go b/internal/appsec/emitter/httpsec/http.go index 280778d81a..d721d67ec0 100644 --- a/internal/appsec/emitter/httpsec/http.go +++ b/internal/appsec/emitter/httpsec/http.go @@ -51,7 +51,7 @@ func ExecuteSDKBodyOperation(parent dyngo.Operation, args types.SDKBodyOperation err = e }) dyngo.StartOperation(op, args) - dyngo.Finish(op, types.SDKBodyOperationRes{}) + dyngo.FinishOperation(op, types.SDKBodyOperationRes{}) return err } diff --git a/internal/appsec/emitter/httpsec/types/types.go b/internal/appsec/emitter/httpsec/types/types.go index c77bfe5ba0..04e481c124 100644 --- a/internal/appsec/emitter/httpsec/types/types.go +++ b/internal/appsec/emitter/httpsec/types/types.go @@ -32,7 +32,7 @@ type ( // Finish the HTTP handler operation, along with the given results and emits a // finish event up in the operation stack. func (op *Operation) Finish(res HandlerOperationRes) []any { - dyngo.Finish(op, res) + dyngo.FinishOperation(op, res) return op.Events() } @@ -80,7 +80,7 @@ type ( // Finish finishes the SDKBody operation and emits a finish event func (op *SDKBodyOperation) Finish() { - dyngo.Finish(op, SDKBodyOperationRes{}) + dyngo.FinishOperation(op, SDKBodyOperationRes{}) } // Error implements the Error interface diff --git a/internal/appsec/emitter/sharedsec/shared.go b/internal/appsec/emitter/sharedsec/shared.go index 61348ca739..715afc45cd 100644 --- a/internal/appsec/emitter/sharedsec/shared.go +++ b/internal/appsec/emitter/sharedsec/shared.go @@ -41,7 +41,7 @@ func ExecuteUserIDOperation(parent dyngo.Operation, args UserIDOperationArgs) er op := &UserIDOperation{Operation: dyngo.NewOperation(parent)} dyngo.OnData(op, func(e error) { err = e }) dyngo.StartOperation(op, args) - dyngo.Finish(op, UserIDOperationRes{}) + dyngo.FinishOperation(op, UserIDOperationRes{}) return err } diff --git a/internal/appsec/listener/graphqlsec/graphql.go b/internal/appsec/listener/graphqlsec/graphql.go index 722c6e713d..19a7762543 100644 --- a/internal/appsec/listener/graphqlsec/graphql.go +++ b/internal/appsec/listener/graphqlsec/graphql.go @@ -35,7 +35,7 @@ var supportedAddresses = listener.AddressSet{ // Install registers the GraphQL WAF Event Listener on the given root operation. func Install(wafHandle *waf.Handle, _ sharedsec.Actions, cfg *config.Config, lim limiter.Limiter, root dyngo.Operation) { if listener := newWafEventListener(wafHandle, cfg, lim); listener != nil { - log.Debug("[appsec] registering the GraphQL WAF Event Listener") + log.Debug("appsec: registering the GraphQL WAF Event Listener") dyngo.On(root, listener.onEvent) } } @@ -51,13 +51,13 @@ type wafEventListener struct { func newWafEventListener(wafHandle *waf.Handle, cfg *config.Config, limiter limiter.Limiter) *wafEventListener { if wafHandle == nil { - log.Debug("[appsec] no WAF Handle available, the GraphQL WAF Event Listener will not be registered") + log.Debug("appsec: no WAF Handle available, the GraphQL WAF Event Listener will not be registered") return nil } addresses := listener.FilterAddressSet(supportedAddresses, wafHandle) if len(addresses) == 0 { - log.Debug("[appsec] no supported GraphQL address is used by currently loaded WAF rules, the GraphQL WAF Event Listener will not be registered") + log.Debug("appsec: no supported GraphQL address is used by currently loaded WAF rules, the GraphQL WAF Event Listener will not be registered") return nil } diff --git a/internal/appsec/listener/grpcsec/grpc.go b/internal/appsec/listener/grpcsec/grpc.go index 488857b45d..fb401fbf49 100644 --- a/internal/appsec/listener/grpcsec/grpc.go +++ b/internal/appsec/listener/grpcsec/grpc.go @@ -43,7 +43,7 @@ var supportedAddresses = listener.AddressSet{ // Install registers the gRPC WAF Event Listener on the given root operation. func Install(wafHandle *waf.Handle, actions sharedsec.Actions, cfg *config.Config, lim limiter.Limiter, root dyngo.Operation) { if listener := newWafEventListener(wafHandle, actions, cfg, lim); listener != nil { - log.Debug("[appsec] registering the gRPC WAF Event Listener") + log.Debug("appsec: registering the gRPC WAF Event Listener") dyngo.On(root, listener.onEvent) } } @@ -60,13 +60,13 @@ type wafEventListener struct { func newWafEventListener(wafHandle *waf.Handle, actions sharedsec.Actions, cfg *config.Config, limiter limiter.Limiter) *wafEventListener { if wafHandle == nil { - log.Debug("[appsec] no WAF Handle available, the gRPC WAF Event Listener will not be registered") + log.Debug("appsec: no WAF Handle available, the gRPC WAF Event Listener will not be registered") return nil } addresses := listener.FilterAddressSet(supportedAddresses, wafHandle) if len(addresses) == 0 { - log.Debug("[appsec] no supported gRPC address is used by currently loaded WAF rules, the gRPC WAF Event Listener will not be registered") + log.Debug("appsec: no supported gRPC address is used by currently loaded WAF rules, the gRPC WAF Event Listener will not be registered") return nil } diff --git a/internal/appsec/listener/httpsec/http.go b/internal/appsec/listener/httpsec/http.go index 03e2679580..eca709185e 100644 --- a/internal/appsec/listener/httpsec/http.go +++ b/internal/appsec/listener/httpsec/http.go @@ -56,7 +56,7 @@ var supportedAddresses = listener.AddressSet{ // Install registers the HTTP WAF Event Listener on the given root operation. func Install(wafHandle *waf.Handle, actions sharedsec.Actions, cfg *config.Config, lim limiter.Limiter, root dyngo.Operation) { if listener := newWafEventListener(wafHandle, actions, cfg, lim); listener != nil { - log.Debug("[appsec] registering the HTTP WAF Event Listener") + log.Debug("appsec: registering the HTTP WAF Event Listener") dyngo.On(root, listener.onEvent) } } @@ -73,13 +73,13 @@ type wafEventListener struct { func newWafEventListener(wafHandle *waf.Handle, actions sharedsec.Actions, cfg *config.Config, limiter limiter.Limiter) *wafEventListener { if wafHandle == nil { - log.Debug("[appsec] no WAF Handle available, the HTTP WAF Event Listener will not be registered") + log.Debug("appsec: no WAF Handle available, the HTTP WAF Event Listener will not be registered") return nil } addresses := listener.FilterAddressSet(supportedAddresses, wafHandle) if len(addresses) == 0 { - log.Debug("[appsec] no supported HTTP address is used by currently loaded WAF rules, the HTTP WAF Event Listener will not be registered") + log.Debug("appsec: no supported HTTP address is used by currently loaded WAF rules, the HTTP WAF Event Listener will not be registered") return nil } diff --git a/internal/appsec/waf.go b/internal/appsec/waf.go index a58480ef3e..8e74bfca89 100644 --- a/internal/appsec/waf.go +++ b/internal/appsec/waf.go @@ -95,6 +95,10 @@ func newWAFHandle(rules config.RulesFragment, cfg *config.Config) (*wafHandle, e type wafEventListener func(*waf.Handle, sharedsec.Actions, *config.Config, limiter.Limiter, dyngo.Operation) +// wafEventListeners is the global list of event listeners registered by contribs at init time. This +// is thread-safe assuming all writes (via AddWAFEventListener) are performed within `init` +// functions; so this is written to only during initialization, and is read from concurrently only +// during runtime when no writes are happening anymore. var wafEventListeners []wafEventListener // AddWAFEventListener adds a new WAF event listener to be registered whenever a new root operation