From f8f5f8c3b18d701bacd2c636c8133fd6d55a319a Mon Sep 17 00:00:00 2001 From: Matteo Pace Date: Fri, 11 Nov 2022 14:27:15 +0100 Subject: [PATCH 1/6] Export engine status --- internal/corazawaf/transaction.go | 5 +++++ types/transaction.go | 3 +++ 2 files changed, 8 insertions(+) diff --git a/internal/corazawaf/transaction.go b/internal/corazawaf/transaction.go index 39c1050ee..6056d3ce5 100644 --- a/internal/corazawaf/transaction.go +++ b/internal/corazawaf/transaction.go @@ -939,6 +939,11 @@ func (tx *Transaction) ProcessLogging() { } } +// RuleEngineStatus returns the status of the rule engine for the transaction +func (tx *Transaction) RuleEngineStatus() types.RuleEngineStatus { + return tx.RuleEngine +} + // RequestBodyAccessible will return true if RequestBody access has been enabled by RequestBodyAccess func (tx *Transaction) RequestBodyAccessible() bool { return tx.RequestBodyAccess diff --git a/types/transaction.go b/types/transaction.go index 84246a0c5..3081aa9ba 100644 --- a/types/transaction.go +++ b/types/transaction.go @@ -103,6 +103,9 @@ type Transaction interface { // delivered prior to the execution of this method. ProcessLogging() + // RuleEngineStatus returns the status of the rule engine for the transaction + RuleEngineStatus() RuleEngineStatus + // RequestBodyAccessible will return true if RequestBody access has been enabled by RequestBodyAccess RequestBodyAccessible() bool From c6216506e825260f23151b4d3fc2ac7c68497051 Mon Sep 17 00:00:00 2001 From: Matteo Pace Date: Fri, 11 Nov 2022 15:33:17 +0100 Subject: [PATCH 2/6] adding notes about current status returned --- internal/corazawaf/transaction.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/internal/corazawaf/transaction.go b/internal/corazawaf/transaction.go index 6056d3ce5..3009e7fa8 100644 --- a/internal/corazawaf/transaction.go +++ b/internal/corazawaf/transaction.go @@ -940,16 +940,19 @@ func (tx *Transaction) ProcessLogging() { } // RuleEngineStatus returns the status of the rule engine for the transaction +// Note that it returns the current status of the engine, later rules may still change it via ctl actions func (tx *Transaction) RuleEngineStatus() types.RuleEngineStatus { return tx.RuleEngine } // RequestBodyAccessible will return true if RequestBody access has been enabled by RequestBodyAccess +// Note that it returns the current status, later rules may still change it via ctl actions func (tx *Transaction) RequestBodyAccessible() bool { return tx.RequestBodyAccess } // ResponseBodyAccessible will return true if ResponseBody access has been enabled by ResponseBodyAccess +// Note that it returns the current status, later rules may still change it via ctl actions func (tx *Transaction) ResponseBodyAccessible() bool { return tx.ResponseBodyAccess } From 8b1c17587f4bf7e7b338ef01cb8a1230a424ae96 Mon Sep 17 00:00:00 2001 From: Matteo Pace Date: Sat, 12 Nov 2022 23:14:04 +0100 Subject: [PATCH 3/6] adds example with RuleEngineStatus and RequestBodyAccessible in middleware, adds docs --- http/middleware.go | 12 ++++++++++++ http/middleware_test.go | 16 ++++++++++++++++ internal/corazawaf/transaction.go | 22 +++++++++++++++++++++- 3 files changed, 49 insertions(+), 1 deletion(-) diff --git a/http/middleware.go b/http/middleware.go index 1d0bb0a98..8c9435916 100644 --- a/http/middleware.go +++ b/http/middleware.go @@ -89,6 +89,18 @@ func processRequest(tx types.Transaction, req *http.Request) (*types.Interruptio } } + // Calling tx.RuleEngineStatus() and tx.RequestBodyAccessible() is possible to + // anticipate some checks performed inside ProcessRequestBody(), avoiding + // to call the latter if no inspections are going to happen. + // It is performed here as a matter of example. It is recommended to avoid doing it + // if not strictly needed for server/proxy side actions. + switch { + case tx.RuleEngineStatus() == types.RuleEngineOff: + return nil, nil + case !tx.RequestBodyAccessible(): + return tx.Interruption(), nil + } + return tx.ProcessRequestBody() } diff --git a/http/middleware_test.go b/http/middleware_test.go index c17b1b684..938794afd 100644 --- a/http/middleware_test.go +++ b/http/middleware_test.go @@ -43,6 +43,22 @@ func TestProcessRequest(t *testing.T) { } } +func TestProcessRequestEngineOff(t *testing.T) { + req, _ := http.NewRequest("POST", "https://www.coraza.io/test", strings.NewReader("test=456")) + waf := corazawaf.NewWAF() + waf.RuleEngine = types.RuleEngineOff + tx := waf.NewTransaction() + if _, err := processRequest(tx, req); err != nil { + t.Fatal(err) + } + if tx.Variables().RequestMethod().String() != "POST" { + t.Fatal("failed to set request from request object") + } + if err := tx.Close(); err != nil { + t.Fatal(err) + } +} + func TestProcessRequestMultipart(t *testing.T) { req, _ := http.NewRequest("POST", "/some", nil) if err := multipartRequest(t, req); err != nil { diff --git a/internal/corazawaf/transaction.go b/internal/corazawaf/transaction.go index 3009e7fa8..300fe25e2 100644 --- a/internal/corazawaf/transaction.go +++ b/internal/corazawaf/transaction.go @@ -940,19 +940,39 @@ func (tx *Transaction) ProcessLogging() { } // RuleEngineStatus returns the status of the rule engine for the transaction +// +// It is suggested to perform early checks on the status only if the API consumer +// requires it for specific server/proxy actions (such as avoiding proxy side buffering). +// Otherwise do not use this method in order to avoid any risk of performing wrong early assumptions. +// +// Three values can be returned: +// types.RuleEngineOn: no early assumptions have to be made at all +// types.RuleEngineDetectionOnly: it may be possible to assume that no disruptive actions will be performed +// types.RuleEngineOff: it is safe to assume that no rules will be processed +// // Note that it returns the current status of the engine, later rules may still change it via ctl actions func (tx *Transaction) RuleEngineStatus() types.RuleEngineStatus { return tx.RuleEngine } // RequestBodyAccessible will return true if RequestBody access has been enabled by RequestBodyAccess +// +// It is suggested to perform early checks only if the API consumer requires them for specific +// server/proxy actions (such as avoiding proxy side buffering). +// Otherwise do not use this method in order to avoid any risk of performing wrong early assumptions. +// // Note that it returns the current status, later rules may still change it via ctl actions func (tx *Transaction) RequestBodyAccessible() bool { return tx.RequestBodyAccess } // ResponseBodyAccessible will return true if ResponseBody access has been enabled by ResponseBodyAccess -// Note that it returns the current status, later rules may still change it via ctl actions +// +// It is suggested to perform early checks only if the API consumer requires them for specific +// server/proxy actions (such as avoiding proxy side buffering). +// Otherwise do not use this method in order to avoid any risk of performing wrong early assumptions. +// +// Note that it returns the current status, later rules may still change it via ctl actions. func (tx *Transaction) ResponseBodyAccessible() bool { return tx.ResponseBodyAccess } From 1a4f10370a6573a2daaa46570b0224f86d3a0b39 Mon Sep 17 00:00:00 2001 From: Matteo Pace Date: Fri, 18 Nov 2022 12:38:46 +0100 Subject: [PATCH 4/6] reverts status export, exports only engine off --- http/middleware.go | 12 ------------ internal/corazawaf/transaction.go | 29 +++-------------------------- types/transaction.go | 16 ++++++++++++++-- 3 files changed, 17 insertions(+), 40 deletions(-) diff --git a/http/middleware.go b/http/middleware.go index 8c9435916..1d0bb0a98 100644 --- a/http/middleware.go +++ b/http/middleware.go @@ -89,18 +89,6 @@ func processRequest(tx types.Transaction, req *http.Request) (*types.Interruptio } } - // Calling tx.RuleEngineStatus() and tx.RequestBodyAccessible() is possible to - // anticipate some checks performed inside ProcessRequestBody(), avoiding - // to call the latter if no inspections are going to happen. - // It is performed here as a matter of example. It is recommended to avoid doing it - // if not strictly needed for server/proxy side actions. - switch { - case tx.RuleEngineStatus() == types.RuleEngineOff: - return nil, nil - case !tx.RequestBodyAccessible(): - return tx.Interruption(), nil - } - return tx.ProcessRequestBody() } diff --git a/internal/corazawaf/transaction.go b/internal/corazawaf/transaction.go index 300fe25e2..d32a3d6cb 100644 --- a/internal/corazawaf/transaction.go +++ b/internal/corazawaf/transaction.go @@ -939,40 +939,17 @@ func (tx *Transaction) ProcessLogging() { } } -// RuleEngineStatus returns the status of the rule engine for the transaction -// -// It is suggested to perform early checks on the status only if the API consumer -// requires it for specific server/proxy actions (such as avoiding proxy side buffering). -// Otherwise do not use this method in order to avoid any risk of performing wrong early assumptions. -// -// Three values can be returned: -// types.RuleEngineOn: no early assumptions have to be made at all -// types.RuleEngineDetectionOnly: it may be possible to assume that no disruptive actions will be performed -// types.RuleEngineOff: it is safe to assume that no rules will be processed -// -// Note that it returns the current status of the engine, later rules may still change it via ctl actions -func (tx *Transaction) RuleEngineStatus() types.RuleEngineStatus { - return tx.RuleEngine +// IsEngineRuleOff will return true if RuleEngine is set to Off +func (tx *Transaction) IsEngineRuleOff() bool { + return tx.RuleEngine == types.RuleEngineOff } // RequestBodyAccessible will return true if RequestBody access has been enabled by RequestBodyAccess -// -// It is suggested to perform early checks only if the API consumer requires them for specific -// server/proxy actions (such as avoiding proxy side buffering). -// Otherwise do not use this method in order to avoid any risk of performing wrong early assumptions. -// -// Note that it returns the current status, later rules may still change it via ctl actions func (tx *Transaction) RequestBodyAccessible() bool { return tx.RequestBodyAccess } // ResponseBodyAccessible will return true if ResponseBody access has been enabled by ResponseBodyAccess -// -// It is suggested to perform early checks only if the API consumer requires them for specific -// server/proxy actions (such as avoiding proxy side buffering). -// Otherwise do not use this method in order to avoid any risk of performing wrong early assumptions. -// -// Note that it returns the current status, later rules may still change it via ctl actions. func (tx *Transaction) ResponseBodyAccessible() bool { return tx.ResponseBodyAccess } diff --git a/types/transaction.go b/types/transaction.go index 3081aa9ba..1d1a6b8f5 100644 --- a/types/transaction.go +++ b/types/transaction.go @@ -103,13 +103,25 @@ type Transaction interface { // delivered prior to the execution of this method. ProcessLogging() - // RuleEngineStatus returns the status of the rule engine for the transaction - RuleEngineStatus() RuleEngineStatus + // IsEngineRuleOff will return true if RuleEngine is set to Off + IsEngineRuleOff() bool // RequestBodyAccessible will return true if RequestBody access has been enabled by RequestBodyAccess + // + // This can be used to perform checks just before calling request body related functions. + // In order to avoid any risk of performing wrong early assumptions, perform early checks on this value + // only if the API consumer requires them for specific server/proxy actions + // (such as avoiding proxy side buffering). + // Note: it returns the current status, later rules may still change it via ctl actions. RequestBodyAccessible() bool // ResponseBodyAccessible will return true if ResponseBody access has been enabled by ResponseBodyAccess + // + // This can be used to perform checks just before calling response body related functions. + // In order to avoid any risk of performing wrong early assumptions, perform early checks on this value + // only if the API consumer requires them for specific server/proxy actions + // (such as avoiding proxy side buffering). + // Note: it returns the current status, later rules may still change it via ctl actions. ResponseBodyAccessible() bool // Interrupted will return true if the transaction was interrupted From 165dd9df10813ed0ecc732d89595da00f7e92444 Mon Sep 17 00:00:00 2001 From: Matteo Pace Date: Sat, 19 Nov 2022 00:52:56 +0100 Subject: [PATCH 5/6] adds IsRuleEngineOff check into middleware, fixes name convention RuleEngine --- http/interceptor.go | 1 + http/middleware.go | 5 +++++ internal/corazawaf/transaction.go | 4 ++-- types/transaction.go | 4 ++-- 4 files changed, 10 insertions(+), 4 deletions(-) diff --git a/http/interceptor.go b/http/interceptor.go index 172cd6524..d3cf92566 100644 --- a/http/interceptor.go +++ b/http/interceptor.go @@ -79,6 +79,7 @@ func wrap(w http.ResponseWriter, r *http.Request, tx types.Transaction) ( http.ResponseWriter, func(types.Transaction, *http.Request) error, ) { // nolint:gocyclo + i := &rwInterceptor{w: w, tx: tx, proto: r.Proto} responseProcessor := func(tx types.Transaction, r *http.Request) error { diff --git a/http/middleware.go b/http/middleware.go index 726a97140..220aed757 100644 --- a/http/middleware.go +++ b/http/middleware.go @@ -112,6 +112,11 @@ func WrapHandler(waf coraza.WAF, l Logger, h http.Handler) http.Handler { } }() + // Early return, Coraza is not going to process any rule + if tx.IsRuleEngineOff() { + return + } + // ProcessRequest is just a wrapper around ProcessConnection, ProcessURI, // ProcessRequestHeaders and ProcessRequestBody. // It fails if any of these functions returns an error and it stops on interruption. diff --git a/internal/corazawaf/transaction.go b/internal/corazawaf/transaction.go index d32a3d6cb..1ec050a26 100644 --- a/internal/corazawaf/transaction.go +++ b/internal/corazawaf/transaction.go @@ -939,8 +939,8 @@ func (tx *Transaction) ProcessLogging() { } } -// IsEngineRuleOff will return true if RuleEngine is set to Off -func (tx *Transaction) IsEngineRuleOff() bool { +// IsRuleEngineOff will return true if RuleEngine is set to Off +func (tx *Transaction) IsRuleEngineOff() bool { return tx.RuleEngine == types.RuleEngineOff } diff --git a/types/transaction.go b/types/transaction.go index 1d1a6b8f5..e7b46d3b0 100644 --- a/types/transaction.go +++ b/types/transaction.go @@ -103,8 +103,8 @@ type Transaction interface { // delivered prior to the execution of this method. ProcessLogging() - // IsEngineRuleOff will return true if RuleEngine is set to Off - IsEngineRuleOff() bool + // IsRuleEngineOff will return true if RuleEngine is set to Off + IsRuleEngineOff() bool // RequestBodyAccessible will return true if RequestBody access has been enabled by RequestBodyAccess // From eeee825466495cef858ced4d762eca6215ad97d2 Mon Sep 17 00:00:00 2001 From: Matteo Pace Date: Mon, 21 Nov 2022 12:26:02 +0100 Subject: [PATCH 6/6] fix IsRuleEngineOff usage inside middleware, adds tests --- http/middleware.go | 4 + http/middleware_test.go | 185 +++++++++++++++++++++++++--------------- 2 files changed, 122 insertions(+), 67 deletions(-) diff --git a/http/middleware.go b/http/middleware.go index 220aed757..907184165 100644 --- a/http/middleware.go +++ b/http/middleware.go @@ -101,6 +101,7 @@ func processRequest(tx types.Transaction, req *http.Request) (*types.Interruptio } func WrapHandler(waf coraza.WAF, l Logger, h http.Handler) http.Handler { + fn := func(w http.ResponseWriter, r *http.Request) { tx := waf.NewTransaction() defer func() { @@ -114,6 +115,9 @@ func WrapHandler(waf coraza.WAF, l Logger, h http.Handler) http.Handler { // Early return, Coraza is not going to process any rule if tx.IsRuleEngineOff() { + // response writer is not going to be wrapped, but used as-is + // to generate the response + h.ServeHTTP(w, r) return } diff --git a/http/middleware_test.go b/http/middleware_test.go index 259a6f30d..1d8e690ca 100644 --- a/http/middleware_test.go +++ b/http/middleware_test.go @@ -244,17 +244,19 @@ func createWAF(t *testing.T) coraza.WAF { return waf } +type httpTest struct { + http2 bool + reqURI string + reqBody string + respHeaders map[string]string + respBody string + expectedProto string + expectedStatus int + expectedRespBody string +} + func TestHttpServer(t *testing.T) { - tests := map[string]struct { - http2 bool - reqURI string - reqBody string - respHeaders map[string]string - respBody string - expectedProto string - expectedStatus int - expectedRespBody string - }{ + tests := map[string]httpTest{ "no blocking": { reqURI: "/hello", expectedProto: "HTTP/1.1", @@ -302,71 +304,120 @@ func TestHttpServer(t *testing.T) { // Perform tests for name, tCase := range tests { t.Run(name, func(t *testing.T) { - serverErrC := make(chan error, 1) - defer close(serverErrC) - - // Spin up the test server - ts := httptest.NewUnstartedServer(WrapHandler(createWAF(t), t.Logf, http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - if want, have := tCase.expectedProto, req.Proto; want != have { - t.Errorf("unexpected proto, want: %s, have: %s", want, have) - } - - w.Header().Set("Content-Type", "text/plain") - w.Header().Add("coraza-middleware", "true") - for k, v := range tCase.respHeaders { - w.Header().Set(k, v) - } - w.WriteHeader(201) - _, err := w.Write([]byte(tCase.respBody)) - if err != nil { - serverErrC <- err - } - }))) - if tCase.http2 { - ts.EnableHTTP2 = true - ts.StartTLS() - } else { - ts.Start() - } - defer ts.Close() + runAgainstWaf(t, tCase, createWAF(t)) - var reqBody io.Reader - if tCase.reqBody != "" { - reqBody = strings.NewReader(tCase.reqBody) - } - req, _ := http.NewRequest("POST", ts.URL+tCase.reqURI, reqBody) - // TODO(jcchavezs): Fix it once the discussion in https://github.com/corazawaf/coraza/issues/438 is settled - req.Header.Add("content-type", "application/x-www-form-urlencoded") - res, err := ts.Client().Do(req) + }) + } +} + +func TestHttpServerWithRuleEngineOff(t *testing.T) { + tests := map[string]httpTest{ + "no blocking true negative": { + reqURI: "/hello", + expectedProto: "HTTP/1.1", + expectedStatus: 201, + respBody: "Hello!", + expectedRespBody: "Hello!", + }, + "no blocking true positive header phase": { + reqURI: "/hello?id=0", + expectedProto: "HTTP/1.1", + expectedStatus: 201, + respBody: "Downstream works!", + expectedRespBody: "Downstream works!", + }, + "no blocking true positive body phase": { + reqURI: "/hello", + reqBody: "eval('cat /etc/passwd')", + expectedProto: "HTTP/1.1", + expectedStatus: 201, + respBody: "Waf is Off!", + expectedRespBody: "Waf is Off!", + }, + } + // Perform tests + for name, tCase := range tests { + t.Run(name, func(t *testing.T) { + waf, err := coraza.NewWAF(coraza.NewWAFConfig(). + WithDirectives(` + SecRuleEngine Off + SecRequestBodyAccess On + SecRule ARGS:id "@eq 0" "id:1, phase:1,deny, status:403,msg:'Invalid id',log,auditlog" + SecRule REQUEST_BODY "@contains eval" "id:100, phase:2,deny, status:403,msg:'Invalid request body',log,auditlog" + `).WithErrorLogger(errLogger(t)).WithDebugLogger(&debugLogger{t: t})) if err != nil { - t.Fatalf("unexpected error when performing the request: %v", err) + t.Fatal(err) } + runAgainstWaf(t, tCase, waf) + }) + } +} - if want, have := tCase.expectedStatus, res.StatusCode; want != have { - t.Errorf("unexpected status code, want: %d, have: %d", want, have) - } +func runAgainstWaf(t *testing.T, tCase httpTest, waf coraza.WAF) { + t.Helper() + serverErrC := make(chan error, 1) + defer close(serverErrC) - resBody, err := io.ReadAll(res.Body) - if err != nil { - t.Fatalf("unexpected error when reading the response body: %v", err) - } + // Spin up the test server + ts := httptest.NewUnstartedServer(WrapHandler(waf, t.Logf, http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + if want, have := tCase.expectedProto, req.Proto; want != have { + t.Errorf("unexpected proto, want: %s, have: %s", want, have) + } - if want, have := tCase.expectedRespBody, string(resBody); want != have { - t.Errorf("unexpected response body, want: %q, have %q", want, have) - } + w.Header().Set("Content-Type", "text/plain") + w.Header().Add("coraza-middleware", "true") + for k, v := range tCase.respHeaders { + w.Header().Set(k, v) + } + w.WriteHeader(201) + _, err := w.Write([]byte(tCase.respBody)) + if err != nil { + serverErrC <- err + } + }))) + if tCase.http2 { + ts.EnableHTTP2 = true + ts.StartTLS() + } else { + ts.Start() + } + defer ts.Close() - err = res.Body.Close() - if err != nil { - t.Errorf("failed to close the body: %v", err) - } + var reqBody io.Reader + if tCase.reqBody != "" { + reqBody = strings.NewReader(tCase.reqBody) + } + req, _ := http.NewRequest("POST", ts.URL+tCase.reqURI, reqBody) + // TODO(jcchavezs): Fix it once the discussion in https://github.com/corazawaf/coraza/issues/438 is settled + req.Header.Add("content-type", "application/x-www-form-urlencoded") + res, err := ts.Client().Do(req) + if err != nil { + t.Fatalf("unexpected error when performing the request: %v", err) + } - select { - case err = <-serverErrC: - t.Errorf("unexpected error from server when writing response body: %v", err) - default: - return - } - }) + if want, have := tCase.expectedStatus, res.StatusCode; want != have { + t.Errorf("unexpected status code, want: %d, have: %d", want, have) + } + + resBody, err := io.ReadAll(res.Body) + if err != nil { + t.Fatalf("unexpected error when reading the response body: %v", err) + } + + if want, have := tCase.expectedRespBody, string(resBody); want != have { + t.Errorf("unexpected response body, want: %q, have %q", want, have) + } + + err = res.Body.Close() + if err != nil { + t.Errorf("failed to close the body: %v", err) + } + + select { + case err = <-serverErrC: + t.Errorf("unexpected error from server when writing response body: %v", err) + default: + return } }