From d6a489c672730577bf2daf5ca8790734c93b6a06 Mon Sep 17 00:00:00 2001 From: Julio Guerra Date: Tue, 18 Jan 2022 11:20:16 +0100 Subject: [PATCH] internal/appsec/config.go: fix waf timeout configuration (#1129) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: François Mazeau --- internal/appsec/appsec_disabled.go | 3 + internal/appsec/config.go | 30 ++++-- internal/appsec/config_test.go | 146 +++++++++++++++++++++++++++++ internal/appsec/waf.go | 3 - 4 files changed, 170 insertions(+), 12 deletions(-) create mode 100644 internal/appsec/config_test.go 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.go b/internal/appsec/config.go index 1bd3c74000..25a1929f22 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,19 @@ 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 != "" { - timeout, err := time.ParseDuration(wafTimeout) - if err != nil { - cfg.wafTimeout = timeout + cfg.wafTimeout = defaultWAFTimeout + if wafTimeout := os.Getenv(wafTimeoutEnvVar); 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 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..9eef6fdbbf --- /dev/null +++ b/internal/appsec/config_test.go @@ -0,0 +1,146 @@ +// 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 ( + "io/ioutil" + "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, "-1s")) + 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 := ioutil.TempFile("", "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