From 6543955f059647ed45a1e78d50344cd696c2900b Mon Sep 17 00:00:00 2001 From: Julio Guerra Date: Mon, 17 Jan 2022 22:50:59 +0100 Subject: [PATCH 1/4] internal/appsec/config.go: fix waf timeout config along with exhaustive testing --- internal/appsec/config.go | 25 ++++-- internal/appsec/config_test.go | 145 +++++++++++++++++++++++++++++++++ internal/appsec/waf.go | 3 - 3 files changed, 163 insertions(+), 10 deletions(-) create mode 100644 internal/appsec/config_test.go diff --git a/internal/appsec/config.go b/internal/appsec/config.go index 1bd3c74000..25febc01ef 100644 --- a/internal/appsec/config.go +++ b/internal/appsec/config.go @@ -15,6 +15,14 @@ import ( "gopkg.in/DataDog/dd-trace-go.v1/internal/log" ) +const ( + enabledEnvVar = "DD_APPSEC_ENABLED" + rulesEnvVar = "DD_APPSEC_RULES" + wafTimeoutEnvVar = "DD_APPSEC_WAF_TIMEOUT" +) + +const defaultWAFTimeout = 4 * time.Millisecond + // config is the AppSec configuration. type config struct { // rules loaded via the env var DD_APPSEC_RULES. When not set, the builtin rules will be used. @@ -26,13 +34,13 @@ type config struct { // isEnabled returns true when appsec is enabled when the environment variable // DD_APPSEC_ENABLED is set to true. func isEnabled() (bool, error) { - enabledStr := os.Getenv("DD_APPSEC_ENABLED") + enabledStr := os.Getenv(enabledEnvVar) if enabledStr == "" { return false, nil } enabled, err := strconv.ParseBool(enabledStr) if err != nil { - return false, fmt.Errorf("could not parse DD_APPSEC_ENABLED value `%s` as a boolean value", enabledStr) + return false, fmt.Errorf("could not parse %s value `%s` as a boolean value", enabledEnvVar, enabledStr) } return enabled, nil } @@ -40,7 +48,7 @@ func isEnabled() (bool, error) { func newConfig() (*config, error) { cfg := &config{} - filepath := os.Getenv("DD_APPSEC_RULES") + filepath := os.Getenv(rulesEnvVar) if filepath != "" { rules, err := ioutil.ReadFile(filepath) if err != nil { @@ -53,15 +61,18 @@ func newConfig() (*config, error) { log.Info("appsec: starting with the security rules from file %s", filepath) } else { log.Info("appsec: starting with the default recommended security rules") + cfg.rules = []byte(staticRecommendedRule) } - cfg.wafTimeout = 4 * time.Millisecond - if wafTimeout := os.Getenv("DD_APPSEC_WAF_TIMEOUT"); wafTimeout != "" { + cfg.wafTimeout = defaultWAFTimeout + if wafTimeout := os.Getenv(wafTimeoutEnvVar); wafTimeout != "" { timeout, err := time.ParseDuration(wafTimeout) - if err != nil { + if err == nil { cfg.wafTimeout = timeout + } else if timeout <= 0 { + log.Error("appsec: unexpected configuration value of %s=%s: expecting a strictly positive duration. Using default value %s.", wafTimeoutEnvVar, wafTimeout, cfg.wafTimeout) } else { - log.Error("appsec: could not parse the value of DD_APPSEC_WAF_TIMEOUT %s as a duration: %v. Using default value %s.", wafTimeout, err, cfg.wafTimeout) + log.Error("appsec: could not parse the value of %s %s as a duration: %v. Using default value %s.", wafTimeoutEnvVar, wafTimeout, err, cfg.wafTimeout) } } diff --git a/internal/appsec/config_test.go b/internal/appsec/config_test.go new file mode 100644 index 0000000000..1e6641eea2 --- /dev/null +++ b/internal/appsec/config_test.go @@ -0,0 +1,145 @@ +// 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. + +//go:build appsec +// +build appsec + +package appsec + +import ( + "os" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +func TestConfig(t *testing.T) { + expectedDefaultConfig := &config{ + rules: []byte(staticRecommendedRule), + wafTimeout: defaultWAFTimeout, + } + + t.Run("default", func(t *testing.T) { + restoreEnv := cleanEnv() + defer restoreEnv() + cfg, err := newConfig() + require.NoError(t, err) + require.Equal(t, expectedDefaultConfig, cfg) + }) + + t.Run("waf-timeout", func(t *testing.T) { + t.Run("parsable", func(t *testing.T) { + restoreEnv := cleanEnv() + defer restoreEnv() + require.NoError(t, os.Setenv(wafTimeoutEnvVar, "5s")) + cfg, err := newConfig() + require.NoError(t, err) + require.Equal( + t, + &config{ + rules: []byte(staticRecommendedRule), + wafTimeout: 5 * time.Second, + }, + cfg, + ) + }) + + t.Run("not-parsable", func(t *testing.T) { + restoreEnv := cleanEnv() + defer restoreEnv() + require.NoError(t, os.Setenv(wafTimeoutEnvVar, "not a duration string")) + cfg, err := newConfig() + require.NoError(t, err) + require.Equal(t, expectedDefaultConfig, cfg) + }) + + t.Run("negative", func(t *testing.T) { + restoreEnv := cleanEnv() + defer restoreEnv() + require.NoError(t, os.Setenv(wafTimeoutEnvVar, "not a duration string")) + cfg, err := newConfig() + require.NoError(t, err) + require.Equal(t, expectedDefaultConfig, cfg) + }) + + t.Run("empty-string", func(t *testing.T) { + restoreEnv := cleanEnv() + defer restoreEnv() + require.NoError(t, os.Setenv(wafTimeoutEnvVar, "")) + cfg, err := newConfig() + require.NoError(t, err) + require.Equal(t, expectedDefaultConfig, cfg) + }) + }) + + t.Run("rules", func(t *testing.T) { + t.Run("empty-string", func(t *testing.T) { + restoreEnv := cleanEnv() + defer restoreEnv() + os.Setenv(rulesEnvVar, "") + cfg, err := newConfig() + require.NoError(t, err) + require.Equal(t, expectedDefaultConfig, cfg) + }) + + t.Run("file-not-found", func(t *testing.T) { + restoreEnv := cleanEnv() + defer restoreEnv() + os.Setenv(rulesEnvVar, "i do not exist") + cfg, err := newConfig() + require.Error(t, err) + require.Nil(t, cfg) + }) + + t.Run("local-file", func(t *testing.T) { + restoreEnv := cleanEnv() + defer restoreEnv() + file, err := os.CreateTemp("", "example-*") + require.NoError(t, err) + defer func() { + file.Close() + os.Remove(file.Name()) + }() + expectedRules := `custom rule file content` + _, err = file.WriteString(expectedRules) + require.NoError(t, err) + os.Setenv(rulesEnvVar, file.Name()) + cfg, err := newConfig() + require.NoError(t, err) + require.Equal(t, &config{ + rules: []byte(expectedRules), + wafTimeout: defaultWAFTimeout, + }, cfg) + }) + }) +} + +func cleanEnv() func() { + wafTimeout := os.Getenv(wafTimeoutEnvVar) + if err := os.Unsetenv(wafTimeoutEnvVar); err != nil { + panic(err) + } + rules := os.Getenv(rulesEnvVar) + if err := os.Unsetenv(rulesEnvVar); err != nil { + panic(err) + } + return func() { + restoreEnv(wafTimeoutEnvVar, wafTimeout) + restoreEnv(rulesEnvVar, rules) + } +} + +func restoreEnv(key, value string) { + if value != "" { + if err := os.Setenv(key, value); err != nil { + panic(err) + } + } else { + if err := os.Unsetenv(key); err != nil { + panic(err) + } + } +} diff --git a/internal/appsec/waf.go b/internal/appsec/waf.go index 4fd45d9329..9dc3c6a996 100644 --- a/internal/appsec/waf.go +++ b/internal/appsec/waf.go @@ -31,9 +31,6 @@ func registerWAF(rules []byte, timeout time.Duration) (unreg dyngo.UnregisterFun } // Instantiate the WAF - if rules == nil { - rules = []byte(staticRecommendedRule) - } waf, err := waf.NewHandle(rules) if err != nil { return nil, err From 8a633da31430a8fb8136572e2497e16454927974 Mon Sep 17 00:00:00 2001 From: Julio Guerra Date: Mon, 17 Jan 2022 23:11:08 +0100 Subject: [PATCH 2/4] internal/appsec/config.go: fix --- internal/appsec/appsec_disabled.go | 3 +++ internal/appsec/config_test.go | 3 ++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/internal/appsec/appsec_disabled.go b/internal/appsec/appsec_disabled.go index 1a408b6121..12039e0359 100644 --- a/internal/appsec/appsec_disabled.go +++ b/internal/appsec/appsec_disabled.go @@ -33,3 +33,6 @@ func Start() { // Stop AppSec. func Stop() {} + +// Static rule stubs when disabled. +const staticRecommendedRule = "" diff --git a/internal/appsec/config_test.go b/internal/appsec/config_test.go index 1e6641eea2..e04fb4960d 100644 --- a/internal/appsec/config_test.go +++ b/internal/appsec/config_test.go @@ -9,6 +9,7 @@ package appsec import ( + "io/ioutil" "os" "testing" "time" @@ -97,7 +98,7 @@ func TestConfig(t *testing.T) { t.Run("local-file", func(t *testing.T) { restoreEnv := cleanEnv() defer restoreEnv() - file, err := os.CreateTemp("", "example-*") + file, err := ioutil.TempFile("", "example-*") require.NoError(t, err) defer func() { file.Close() From f8c29fd7e5f9388da7bee52277c2a9c7e70826d1 Mon Sep 17 00:00:00 2001 From: Julio Guerra Date: Tue, 18 Jan 2022 10:34:20 +0100 Subject: [PATCH 3/4] internal/appsec/config_test.go: fix test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: François Mazeau --- internal/appsec/config_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/appsec/config_test.go b/internal/appsec/config_test.go index e04fb4960d..9eef6fdbbf 100644 --- a/internal/appsec/config_test.go +++ b/internal/appsec/config_test.go @@ -60,7 +60,7 @@ func TestConfig(t *testing.T) { t.Run("negative", func(t *testing.T) { restoreEnv := cleanEnv() defer restoreEnv() - require.NoError(t, os.Setenv(wafTimeoutEnvVar, "not a duration string")) + require.NoError(t, os.Setenv(wafTimeoutEnvVar, "-1s")) cfg, err := newConfig() require.NoError(t, err) require.Equal(t, expectedDefaultConfig, cfg) From 9277d2a811d425c2741fe587c246149618ca0abf Mon Sep 17 00:00:00 2001 From: Julio Guerra Date: Tue, 18 Jan 2022 10:57:56 +0100 Subject: [PATCH 4/4] internal/appsec/config.go: fix negative condition --- internal/appsec/config.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/internal/appsec/config.go b/internal/appsec/config.go index 25febc01ef..25a1929f22 100644 --- a/internal/appsec/config.go +++ b/internal/appsec/config.go @@ -66,11 +66,12 @@ func newConfig() (*config, error) { cfg.wafTimeout = defaultWAFTimeout if wafTimeout := os.Getenv(wafTimeoutEnvVar); wafTimeout != "" { - timeout, err := time.ParseDuration(wafTimeout) - if err == nil { - cfg.wafTimeout = timeout - } else if timeout <= 0 { - log.Error("appsec: unexpected configuration value of %s=%s: expecting a strictly positive duration. Using default value %s.", wafTimeoutEnvVar, wafTimeout, cfg.wafTimeout) + if timeout, err := time.ParseDuration(wafTimeout); err == nil { + if timeout <= 0 { + log.Error("appsec: unexpected configuration value of %s=%s: expecting a strictly positive duration. Using default value %s.", wafTimeoutEnvVar, wafTimeout, cfg.wafTimeout) + } else { + cfg.wafTimeout = timeout + } } else { log.Error("appsec: could not parse the value of %s %s as a duration: %v. Using default value %s.", wafTimeoutEnvVar, wafTimeout, err, cfg.wafTimeout) }