Repository: coreruleset/go-ftw
Pull request #132 (2023-02-21 19:47:28)

feat(config): adds override_empty_host_header flag

The feature added in https://github.com/coreruleset/go-ftw/pull/63 replaces the empty host field with the destination address. It has conflicts with some CRS test that are trying to send on purpose empty or missing host headers.
It happens only when `dest_addr` override is in place, for this reason CRS tests against apache are still running smoothly.
This PR implements @theseion [idea](https://github.com/coreruleset/go-ftw/issues/129#issuecomment-1421347248), adding the `override_empty_host_header` flag that enables this feature, permitting, by default, to keep running CRS tests as intended. 
More context in the issue: https://github.com/coreruleset/go-ftw/issues/129.

Thanks the feedbacks!


tentatively closes https://github.com/coreruleset/go-ftw/issues/129

theseion: Thanks @M4tteoP, looks good. I've requested a review from @fzipi, just to be sure we're all on the same page.

Patch:
diff --git a/README.md b/README.md
index 01f1276..4a4b5b4 100644
--- a/README.md
+++ b/README.md
@@ -296,7 +296,19 @@ Other interesting functions you can use are: `randBytes`, `htpasswd`, `encryptAE
 Sometimes you have tests that work well for some platform combinations, e.g. Apache + modsecurity2, but fail for others, e.g. NGiNX + modsecurity3. Taking that into account, you can override test results using the `testoverride` config param. The test will be skipped, and the result forced as configured.
 
 Tests can be altered using four lists:
-- `input` allows you to override global parameters in tests. An example usage is if you want to change the `dest_addr` of all tests to point to an external IP or host
+- `input` allows you to override global parameters in tests. The following ones can be overridden:
+  - `dest_addr`: overrides the destination address (accepts IP or hostname)
+  - `override_empty_host_header`: if true and `dest_addr` override is _also_ set, empty `Host` headers will be replaced with `dest_addr`
+  - `port`: overrides the port number
+  - `protocol`: overrides the protocol
+  - `uri`: overrides the uri
+  - `version`: overrides the HTTP version. E.g. "HTTP/1.1"
+  - `headers`: overrides headers, the format is a map of strings
+  - `method`: overrides the method used to perform the request
+  - `data`: overrides data sent in the request
+  - `stopmagic`: prevent header autocompletion (currently sets `Connection: close` and `Content-Length` for requests with body data)
+  - `encodedrequest`: overrides base64 encoded request
+  - `rawrequest`: permits to provide a raw request. `method`, `uri` and `version` values will be ignored
 - `ignore` is for tests you want to ignore. You should add a comment on why you ignore the test
 - `forcepass` is for tests you want to pass unconditionally. You should add a comment on why you force to pass the test
 - `forcefail` is for tests you want to fail unconditionally. You should add a comment on why you force to fail the test
diff --git a/config/types.go b/config/types.go
index 3bdaa00..f8d1dff 100644
--- a/config/types.go
+++ b/config/types.go
@@ -41,12 +41,12 @@ type FTWConfiguration struct {
 
 // FTWTestOverride holds four lists:
 //
-//	Input allows you to override input parameters in tests. An example usage is if you want to change the `dest_addr` of all tests to point to an external IP or host.
+//	Overrides allows you to override input parameters in tests. An example usage is if you want to change the `dest_addr` of all tests to point to an external IP or host.
 //	Ignore is for tests you want to ignore. You should add a comment on why you ignore the test
 //	ForcePass is for tests you want to pass unconditionally. You should add a comment on why you force to pass the test
 //	ForceFail is for tests you want to fail unconditionally. You should add a comment on why you force to fail the test
 type FTWTestOverride struct {
-	Input     test.Input            `koanf:"input"`
+	Overrides test.Overrides        `koanf:"input"`
 	Ignore    map[*FTWRegexp]string `koanf:"ignore"`
 	ForcePass map[*FTWRegexp]string `koanf:"forcepass"`
 	ForceFail map[*FTWRegexp]string `koanf:"forcefail"`
diff --git a/runner/run.go b/runner/run.go
index ba4780e..2427f5a 100644
--- a/runner/run.go
+++ b/runner/run.go
@@ -379,16 +379,26 @@ func getRequestFromTest(testRequest test.Input) *ftwhttp.Request {
 
 // applyInputOverride will check if config had global overrides and write that into the test.
 func applyInputOverride(o config.FTWTestOverride, testRequest *test.Input) error {
-	overrides := o.Input
+	overrides := o.Overrides
 	if overrides.Port != nil {
 		testRequest.Port = overrides.Port
 	}
+
+	if overrides.Headers != nil {
+		if testRequest.Headers == nil {
+			testRequest.Headers = ftwhttp.Header{}
+		}
+		for k, v := range overrides.Headers {
+			testRequest.Headers.Set(k, v)
+		}
+	}
+
 	if overrides.DestAddr != nil {
 		testRequest.DestAddr = overrides.DestAddr
 		if testRequest.Headers == nil {
 			testRequest.Headers = ftwhttp.Header{}
 		}
-		if testRequest.Headers.Get("Host") == "" {
+		if overrides.OverrideEmptyHostHeader && testRequest.Headers.Get("Host") == "" {
 			testRequest.Headers.Set("Host", *overrides.DestAddr)
 		}
 	}


Test patch:
diff --git a/config/config_test.go b/config/config_test.go
index de570ad..5aeede6 100644
--- a/config/config_test.go
+++ b/config/config_test.go
@@ -54,14 +54,14 @@ func TestNewConfigConfig(t *testing.T) {
 
 	assert.NoError(t, err)
 	assert.NotNil(t, cfg)
-	assert.NotEmpty(t, cfg.TestOverride.Input, "Ignore list must not be empty")
+	assert.NotEmpty(t, cfg.TestOverride.Overrides, "Ignore list must not be empty")
 
 	for id, text := range cfg.TestOverride.Ignore {
 		assert.Contains(t, (*regexp.Regexp)(id).String(), "920400-1$", "Looks like we could not find item to ignore")
 		assert.Equal(t, "This test must be ignored", text, "Text doesn't match")
 	}
 
-	overrides := cfg.TestOverride.Input
+	overrides := cfg.TestOverride.Overrides
 	assert.NotNil(t, overrides.DestAddr, "Looks like we are not overriding destination address")
 	assert.Equal(t, "httpbin.org", *overrides.DestAddr, "Looks like we are not overriding destination address")
 }
@@ -161,7 +161,7 @@ func TestNewDefaultConfigWithParams(t *testing.T) {
 	cfg.WithLogfile("mylogfile.log")
 	assert.Equal(t, "mylogfile.log", cfg.LogFile)
 	overrides := FTWTestOverride{
-		Input:     test.Input{},
+		Overrides: test.Overrides{},
 		Ignore:    nil,
 		ForcePass: nil,
 		ForceFail: nil,
diff --git a/runner/run_test.go b/runner/run_test.go
index 1131c20..d6357b1 100644
--- a/runner/run_test.go
+++ b/runner/run_test.go
@@ -36,6 +36,34 @@ testoverride:
     protocol: "http"
 `
 
+var yamlConfigEmptyHostHeaderOverride = `
+---
+testoverride:
+  input:
+    dest_addr: %s
+    headers:
+      Host: %s
+    override_empty_host_header: true
+`
+
+var yamlConfigHostHeaderOverride = `
+---
+testoverride:
+  input:
+    dest_addr: address.org
+    headers:
+      unique_id: %s
+`
+
+var yamlConfigHeaderOverride = `
+---
+testoverride:
+  input:
+    dest_addr: address.org
+    headers:
+      unique_id: %s
+`
+
 var yamlConfigOverride = `
 ---
 testoverride:
@@ -419,12 +447,12 @@ func replaceDestinationInConfiguration(override *config.FTWTestOverride, dest ft
 	replaceableAddress := "TEST_ADDR"
 	replaceablePort := -1
 
-	input := &override.Input
-	if input.DestAddr != nil && *input.DestAddr == replaceableAddress {
-		input.DestAddr = &dest.DestAddr
+	overriddenInputs := &override.Overrides
+	if overriddenInputs.DestAddr != nil && *overriddenInputs.DestAddr == replaceableAddress {
+		overriddenInputs.DestAddr = &dest.DestAddr
 	}
-	if input.Port != nil && *input.Port == replaceablePort {
-		input.Port = &dest.Port
+	if overriddenInputs.Port != nil && *overriddenInputs.Port == replaceablePort {
+		overriddenInputs.Port = &dest.Port
 	}
 }
 
@@ -721,7 +749,7 @@ func TestFailedTestsRun(t *testing.T) {
 	assert.Equal(t, 1, res.Stats.TotalFailed())
 }
 
-func TestApplyInputOverrideSetHostFromDestAddr(t *testing.T) {
+func TestApplyInputOverrideHostFromDestAddr(t *testing.T) {
 	originalHost := "original.com"
 	overrideHost := "override.com"
 	testInput := test.Input{
@@ -729,7 +757,7 @@ func TestApplyInputOverrideSetHostFromDestAddr(t *testing.T) {
 	}
 	cfg := &config.FTWConfiguration{
 		TestOverride: config.FTWTestOverride{
-			Input: test.Input{
+			Overrides: test.Overrides{
 				DestAddr: &overrideHost,
 			},
 		},
@@ -743,8 +771,98 @@ func TestApplyInputOverrideSetHostFromDestAddr(t *testing.T) {
 	assert.NotNil(t, testInput.Headers, "Header map must exist after overriding `dest_addr`")
 
 	hostHeader := testInput.Headers.Get("Host")
-	assert.NotEqual(t, "", hostHeader, "Host header must be set after overriding `dest_addr`")
-	assert.Equal(t, overrideHost, hostHeader, "Host header must be identical to `dest_addr` after overrding `dest_addr`")
+	assert.Equal(t, "", hostHeader, "Without OverrideEmptyHostHeader, Host header must not be set after overriding `dest_addr`")
+}
+
+func TestApplyInputOverrideEmptyHostHeaderSetHostFromDestAddr(t *testing.T) {
+	originalHost := "original.com"
+	overrideHost := "override.com"
+	testInput := test.Input{
+		DestAddr: &originalHost,
+	}
+	cfg := &config.FTWConfiguration{
+		TestOverride: config.FTWTestOverride{
+			Overrides: test.Overrides{
+				DestAddr:                &overrideHost,
+				OverrideEmptyHostHeader: true,
+			},
+		},
+	}
+
+	err := applyInputOverride(cfg.TestOverride, &testInput)
+	assert.NoError(t, err, "Failed to apply input overrides")
+
+	assert.Equal(t, overrideHost, *testInput.DestAddr, "`dest_addr` should have been overridden")
+
+	assert.NotNil(t, testInput.Headers, "Header map must exist after overriding `dest_addr`")
+
+	hostHeader := testInput.Headers.Get("Host")
+	assert.NotEqual(t, "", hostHeader, "Host header must be set after overriding `dest_addr` and setting `override_empty_host_header` to `true`")
+	assert.Equal(t, overrideHost, hostHeader, "Host header must be identical to `dest_addr` after overriding `dest_addr` and setting `override_emtpy_host_header` to `true`")
+}
+
+func TestApplyInputOverrideSetHostFromHostHeaderOverride(t *testing.T) {
+	originalDestAddr := "original.com"
+	overrideDestAddress := "wrong.org"
+	overrideHostHeader := "override.com"
+
+	cfg, err1 := config.NewConfigFromString(fmt.Sprintf(yamlConfigEmptyHostHeaderOverride, overrideDestAddress, overrideHostHeader))
+	assert.NoError(t, err1)
+
+	testInput := test.Input{
+		DestAddr: &originalDestAddr,
+	}
+
+	err := applyInputOverride(cfg.TestOverride, &testInput)
+	assert.NoError(t, err, "Failed to apply input overrides")
+
+	hostHeader := testInput.Headers.Get("Host")
+	assert.NotEqual(t, "", hostHeader, "Host header must be set after overriding the `Host` header")
+	if hostHeader == overrideDestAddress {
+		assert.Equal(t, overrideHostHeader, hostHeader, "Host header override must take precence over OverrideEmptyHostHeader")
+	} else {
+		assert.Equal(t, overrideHostHeader, hostHeader, "Host header must be identical to overridden `Host` header.")
+	}
+}
+
+func TestApplyInputOverrideSetHeaderOverridingExistingOne(t *testing.T) {
+	originalHeaderValue := "original"
+	overrideHeaderValue := "override"
+	cfg, err1 := config.NewConfigFromString(fmt.Sprintf(yamlConfigHostHeaderOverride, overrideHeaderValue))
+	assert.NoError(t, err1)
+
+	testInput := test.Input{
+		Headers: ftwhttp.Header{"unique_id": originalHeaderValue},
+	}
+
+	assert.NotNil(t, testInput.Headers, "Header map must exist before overriding any header")
+
+	err := applyInputOverride(cfg.TestOverride, &testInput)
+	assert.NoError(t, err, "Failed to apply input overrides")
+
+	overriddenHeader := testInput.Headers.Get("unique_id")
+	assert.NotEqual(t, "", overriddenHeader, "unique_id header must be set after overriding it")
+	assert.Equal(t, overrideHeaderValue, overriddenHeader, "Host header must be identical to overridden `Host` header.")
+}
+
+func TestApplyInputOverrides(t *testing.T) {
+	originalHeaderValue := "original"
+	overrideHeaderValue := "override"
+	cfg, err1 := config.NewConfigFromString(fmt.Sprintf(yamlConfigHeaderOverride, overrideHeaderValue))
+	assert.NoError(t, err1)
+
+	testInput := test.Input{
+		Headers: ftwhttp.Header{"unique_id": originalHeaderValue},
+	}
+
+	assert.NotNil(t, testInput.Headers, "Header map must exist before overriding any header")
+
+	err := applyInputOverride(cfg.TestOverride, &testInput)
+	assert.NoError(t, err, "Failed to apply input overrides")
+
+	overriddenHeader := testInput.Headers.Get("unique_id")
+	assert.NotEqual(t, "", overriddenHeader, "unique_id header must be set after overriding it")
+	assert.Equal(t, overrideHeaderValue, overriddenHeader, "Host header must be identical to overridden `Host` header.")
 }
 
 func TestIgnoredTestsRun(t *testing.T) {
diff --git a/test/types.go b/test/types.go
index 44a5e3f..fc51528 100644
--- a/test/types.go
+++ b/test/types.go
@@ -4,6 +4,7 @@ import "github.com/coreruleset/go-ftw/ftwhttp"
 
 // Input represents the input request in a stage
 // The fields `Version`, `Method` and `URI` we want to explicitly now when they are set to ""
+
 type Input struct {
 	DestAddr       *string        `yaml:"dest_addr,omitempty" koanf:"dest_addr,omitempty"`
 	Port           *int           `yaml:"port,omitempty" koanf:"port,omitempty"`
@@ -19,6 +20,23 @@ type Input struct {
 	RAWRequest     string         `yaml:"raw_request,omitempty" koanf:"raw_request,omitempty"`
 }
 
+// Overrides represents the overridden inputs that have to be applied to tests
+type Overrides struct {
+	DestAddr *string `yaml:"dest_addr,omitempty" koanf:"dest_addr,omitempty"`
+	Port     *int    `yaml:"port,omitempty" koanf:"port,omitempty"`
+	Protocol *string `yaml:"protocol,omitempty" koanf:"protocol,omitempty"`
+	// URI      *string `yaml:"uri,omitempty" koanf:"uri,omitempty"`
+	// Version  *string        `yaml:"version,omitempty" koanf:"version,omitempty"`
+	Headers ftwhttp.Header `yaml:"headers,omitempty" koanf:"headers,omitempty"`
+	// Method   *string        `yaml:"method,omitempty" koanf:"method,omitempty"`
+	// Data     *string        `yaml:"data,omitempty" koanf:"data,omitempty"`
+	// SaveCookie     bool           `yaml:"save_cookie,omitempty" koanf:"save_cookie,omitempty"`
+	// StopMagic      bool           `yaml:"stop_magic" koanf:"stop_magic,omitempty"`
+	// EncodedRequest string         `yaml:"encoded_request,omitempty" koanf:"encoded_request,omitempty"`
+	// RAWRequest     string         `yaml:"raw_request,omitempty" koanf:"raw_request,omitempty"`
+	OverrideEmptyHostHeader bool `yaml:"override_empty_host_header,omitempty" koanf:"override_empty_host_header,omitempty"`
+}
+
 // Output is the response expected from the test
 type Output struct {
 	Status           []int  `yaml:"status,flow,omitempty"`