internal/appsec/config.go: fix waf timeout configuration (#1129)

Co-authored-by: François Mazeau <[email protected]>

internal/appsec/appsec_disabled.go

@@ -33,3 +33,6 @@ func Start() {
 
 // Stop AppSec.
 func Stop() {}
+
+// Static rule stubs when disabled.
+const staticRecommendedRule = ""

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,21 +34,21 @@ 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
 }
 
 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)
 		}
 	}
 

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)
+		}
+	}
+}

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