From a90326cd68a9d0d35f71bed26fe80d3020170937 Mon Sep 17 00:00:00 2001 From: Paul Logston Date: Fri, 3 Jun 2022 00:59:55 +0800 Subject: [PATCH 1/2] util:envutil: Add external env var accessor This commit adds a method `ExternalEnvString` that can be used for fetching explicitly non-CRDB environment variables. These variables are stored in the same cache as used CRDB environment variables but are guaranteed to not be prefixed with `COCKROACH_` --- pkg/util/envutil/env.go | 56 +++++++++++++++++-- pkg/util/envutil/env_test.go | 101 +++++++++++++++++++++++++++++++++++ 2 files changed, 152 insertions(+), 5 deletions(-) diff --git a/pkg/util/envutil/env.go b/pkg/util/envutil/env.go index d10ef98a13c8..ebaa553dcc58 100644 --- a/pkg/util/envutil/env.go +++ b/pkg/util/envutil/env.go @@ -43,10 +43,9 @@ func init() { func checkVarName(name string) { // Env vars must: - // - start with COCKROACH_ // - be uppercase // - only contain letters, digits, and _ - valid := strings.HasPrefix(name, "COCKROACH_") + valid := true for i := 0; valid && i < len(name); i++ { c := name[i] valid = ((c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || c == '_') @@ -56,15 +55,51 @@ func checkVarName(name string) { } } -// getEnv retrieves an environment variable, keeps track of where +func checkInternalVarName(name string) { + // Env vars must: + // - start with COCKROACH_ + // - pass basic validity checks in checkVarName + if !strings.HasPrefix(name, "COCKROACH_") { + panic("invalid env var name " + name) + } + checkVarName(name) +} + +func checkExternalVarName(name string) { + // Env vars must: + // - not start with COCKROACH_ + // - pass basic validity checks in checkVarName + if strings.HasPrefix(name, "COCKROACH_") { + panic("invalid env var name " + name) + } + checkVarName(name) +} + +// getEnv performs all of the same actions as getAndCacheEnv but also includes +// a validity check of the variable name. +func getEnv(varName string, depth int) (string, bool) { + checkInternalVarName(varName) + return getAndCacheEnv(varName, depth+1) +} + +// getExternalEnv performs all of the same actions as getEnv but also asserts +// that the variable is not of the form of an internal environment variable, +// eg. "COCKROACH_". +func getExternalEnv(varName string, depth int) (string, bool) { + checkExternalVarName(varName) + return getAndCacheEnv(varName, depth+1) +} + +// getAndCacheEnv retrieves an environment variable, keeps track of where // it was accessed, and checks that each environment variable is accessed // from at most one place. // The bookkeeping enables a report of all influential environment // variables with "cockroach debug env". To keep this report useful, // all relevant environment variables should be read during start up. -func getEnv(varName string, depth int) (string, bool) { +// This function should not be used directly; getEnv or getExternalEnv should +// be used instead. +func getAndCacheEnv(varName string, depth int) (string, bool) { _, consumer, _, _ := runtime.Caller(depth + 1) - checkVarName(varName) envVarRegistry.mu.Lock() defer envVarRegistry.mu.Unlock() @@ -166,6 +201,7 @@ var valueReportableUnsafeVarRegistry = map[redact.SafeString]struct{}{ "DEBUG_HTTP2_GOROUTINES": {}, "GRPC_GO_LOG_SEVERITY_LEVEL": {}, "GRPC_GO_LOG_VERBOSITY_LEVEL": {}, + "HOST_IP": {}, "LANG": {}, "LC_ALL": {}, "LC_COLLATE": {}, @@ -272,6 +308,16 @@ func EnvString(name string, depth int) (string, bool) { return getEnv(name, depth+1) } +// ExternalEnvString returns the value set by the specified environment +// variable. Only non-CRDB environment variables should be accessed via this +// method. CRDB specific variables should be accessed via EnvString. The depth +// argument indicates the stack depth of the caller that should be associated +// with the variable. The returned boolean flag indicates if the variable is +// set. +func ExternalEnvString(name string, depth int) (string, bool) { + return getExternalEnv(name, depth+1) +} + // EnvOrDefaultString returns the value set by the specified // environment variable, if any, otherwise the specified default // value. diff --git a/pkg/util/envutil/env_test.go b/pkg/util/envutil/env_test.go index 264425cae322..dfc615758ace 100644 --- a/pkg/util/envutil/env_test.go +++ b/pkg/util/envutil/env_test.go @@ -30,6 +30,107 @@ func TestEnvOrDefault(t *testing.T) { } } +func TestCheckVarName(t *testing.T) { + t.Run("checkVarName", func(t *testing.T) { + for _, tc := range []struct { + name string + valid bool + }{ + { + name: "abc123", + valid: false, + }, + { + name: "ABC 123", + valid: false, + }, + { + name: "@&) 123", + valid: false, + }, + { + name: "ABC123", + valid: true, + }, + { + name: "ABC_123", + valid: true, + }, + } { + func() { + defer func() { + r := recover() + if !tc.valid && r == nil { + t.Errorf("expected panic for name %q, got none", tc.name) + } else if tc.valid && r != nil { + t.Errorf("unexpected panic for name %q, got %q", tc.name, r) + } + }() + + checkVarName(tc.name) + }() + } + }) + + t.Run("checkInternalVarName", func(t *testing.T) { + for _, tc := range []struct { + name string + valid bool + }{ + { + name: "ABC_123", + valid: false, + }, + { + name: "COCKROACH_X", + valid: true, + }, + } { + func() { + defer func() { + r := recover() + if !tc.valid && r == nil { + t.Errorf("expected panic for name %q, got none", tc.name) + } else if tc.valid && r != nil { + t.Errorf("unexpected panic for name %q, got %q", tc.name, r) + } + }() + + checkInternalVarName(tc.name) + }() + } + }) + + t.Run("checkExternalVarName", func(t *testing.T) { + for _, tc := range []struct { + name string + valid bool + }{ + { + name: "COCKROACH_X", + valid: false, + }, + { + name: "ABC_123", + valid: true, + }, + } { + func() { + defer func() { + r := recover() + if !tc.valid && r == nil { + t.Errorf("expected panic for name %q, got none", tc.name) + } else if tc.valid && r != nil { + t.Errorf("unexpected panic for name %q, got %q", tc.name, r) + } + }() + + checkExternalVarName(tc.name) + }() + } + }) +} + func TestTestSetEnvExists(t *testing.T) { key := "COCKROACH_ENVUTIL_TESTSETTING" require.NoError(t, os.Setenv(key, "before")) From 36d940d51e3ba97d91c0ebec485b04610a058f99 Mon Sep 17 00:00:00 2001 From: Paul Logston Date: Tue, 31 May 2022 22:45:33 +0800 Subject: [PATCH 2/2] logging: Expand env vars in logging config This commit adds a command line flag `--log-config-vars` that is used to specify those environment variables that will be expanded in the logging config file contents. This flag enables operators to indirectly define an allow list of sorts of trusted environment variables. A desired use case of this flag is one where `HOST_IP` is pulled from the running container environment and used to direct logs to a node local instance of FluentBit. In this snippet below, the log.yaml configuration file can be defined with a generic `fluent-server` sink that specifies `${HOST_IP}:5170` as its address. CRDB will then pull the IP from the environment and replace the variable with the IP in the logging configuration. ``` containers: - name: cockroach image: cockroachdb/cockroach args: - --log-config-file log.yaml - --log-config-vars HOST_IP env: - name: HOST_IP valueFrom: fieldRef: fieldPath: status.hostIP ``` Closes #81146. Release note (cli change): The CLI now contains a flag (--log-config-vars) that allows for environment variables to be specified for expansion within the logging configuration file. This change allows for a single logging configuration file to service an array of sinks without further manipulation of the configuration file. --- pkg/cli/cliflags/flags.go | 6 ++ pkg/cli/context.go | 4 ++ pkg/cli/flags.go | 1 + pkg/cli/flags_test.go | 32 +++++---- pkg/cli/log_flags.go | 60 +++++++++++++++- pkg/cli/log_flags_test.go | 141 +++++++++++++++++++++++++++++++++++--- pkg/cli/testdata/logflags | 43 ++++++++++++ 7 files changed, 263 insertions(+), 24 deletions(-) diff --git a/pkg/cli/cliflags/flags.go b/pkg/cli/cliflags/flags.go index 3b5b904ff72a..8af26bcee001 100644 --- a/pkg/cli/cliflags/flags.go +++ b/pkg/cli/cliflags/flags.go @@ -1576,6 +1576,12 @@ This has the same effect as passing the content of the file via the --log flag.`, } + LogConfigVars = FlagInfo{ + Name: "log-config-vars", + Description: `Environment variables that will be expanded if +present in the body of the logging configuration.`, + } + DeprecatedStderrThreshold = FlagInfo{ Name: "logtostderr", Description: `Write log messages beyond the specified severity to stderr.`, diff --git a/pkg/cli/context.go b/pkg/cli/context.go index 1c207a9b83ed..2e563c7928a5 100644 --- a/pkg/cli/context.go +++ b/pkg/cli/context.go @@ -187,6 +187,9 @@ type cliContext struct { // logConfigInput is the YAML input for the logging configuration. logConfigInput settableString + // logConfigVars is an array of environment variables used in the logging + // configuration that will be expanded by CRDB. + logConfigVars []string // logConfig is the resulting logging configuration after the input // configuration has been parsed and validated. logConfig logconfig.Config @@ -233,6 +236,7 @@ func setCliContextDefaults() { cliCtx.sqlConnDBName = "" cliCtx.allowUnencryptedClientPassword = false cliCtx.logConfigInput = settableString{s: ""} + cliCtx.logConfigVars = nil cliCtx.logConfig = logconfig.Config{} cliCtx.logShutdownFn = func() {} cliCtx.ambiguousLogDir = false diff --git a/pkg/cli/flags.go b/pkg/cli/flags.go index efc04711dd7e..5f2da077859a 100644 --- a/pkg/cli/flags.go +++ b/pkg/cli/flags.go @@ -400,6 +400,7 @@ func init() { // Logging configuration. varFlag(pf, &stringValue{settableString: &cliCtx.logConfigInput}, cliflags.Log) varFlag(pf, &fileContentsValue{settableString: &cliCtx.logConfigInput, fileName: ""}, cliflags.LogConfigFile) + stringSliceFlag(pf, &cliCtx.logConfigVars, cliflags.LogConfigVars) // Pre-v21.1 overrides. Deprecated. // TODO(knz): Remove this. diff --git a/pkg/cli/flags_test.go b/pkg/cli/flags_test.go index 603b0d5b7759..e2b10301309d 100644 --- a/pkg/cli/flags_test.go +++ b/pkg/cli/flags_test.go @@ -1274,26 +1274,30 @@ Available Commands: help Help about any command Flags: - -h, --help help for cockroach - --log - Logging configuration, expressed using YAML syntax. For example, you can - change the default logging directory with: --log='file-defaults: {dir: ...}'. - See the documentation for more options and details. To preview how the log - configuration is applied, or preview the default configuration, you can use - the 'cockroach debug check-log-config' sub-command. - - --log-config-file - File name to read the logging configuration from. This has the same effect as - passing the content of the file via the --log flag. - (default ) - --version version for cockroach + -h, --help help for cockroach + --log + Logging configuration, expressed using YAML syntax. For example, you can + change the default logging directory with: --log='file-defaults: {dir: ...}'. + See the documentation for more options and details. To preview how the log + configuration is applied, or preview the default configuration, you can use + the 'cockroach debug check-log-config' sub-command. + + --log-config-file + File name to read the logging configuration from. This has the same effect as + passing the content of the file via the --log flag. + (default ) + --log-config-vars strings + Environment variables that will be expanded if present in the body of the + logging configuration. + + --version version for cockroach Use "cockroach [command] --help" for more information about a command. ` helpExpected := fmt.Sprintf("CockroachDB command-line interface and server.\n\n%s", // Due to a bug in spf13/cobra, 'cockroach help' does not include the --version // flag. Strangely, 'cockroach --help' does, as well as usage error messages. - strings.ReplaceAll(expUsage, " --version version for cockroach\n", "")) + strings.ReplaceAll(expUsage, " --version version for cockroach\n", "")) badFlagExpected := fmt.Sprintf("%s\nError: unknown flag: --foo\n", expUsage) testCases := []struct { diff --git a/pkg/cli/log_flags.go b/pkg/cli/log_flags.go index 709fef54a350..3c4ed6d84d26 100644 --- a/pkg/cli/log_flags.go +++ b/pkg/cli/log_flags.go @@ -17,9 +17,11 @@ import ( "os" "path/filepath" "strconv" + "strings" "github.com/cockroachdb/cockroach/pkg/base" "github.com/cockroachdb/cockroach/pkg/cli/cliflags" + "github.com/cockroachdb/cockroach/pkg/util/envutil" "github.com/cockroachdb/cockroach/pkg/util/humanizeutil" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/cockroach/pkg/util/log/channel" @@ -45,6 +47,10 @@ func setupLogging(ctx context.Context, cmd *cobra.Command, isServerCmd, applyCon return errors.Newf("--%s is incompatible with legacy discrete logging flags", cliflags.Log.Name) } + if err := validateLogConfigVars(cliCtx.logConfigVars); err != nil { + return errors.Wrap(err, "invalid logging configuration") + } + // Sanity check to prevent misuse of API. if active, firstUse := log.IsActive(); active { panic(errors.Newf("logging already active; first used at:\n%s", firstUse)) @@ -118,7 +124,17 @@ func setupLogging(ctx context.Context, cmd *cobra.Command, isServerCmd, applyCon // If a configuration was specified via --log, load it. if cliCtx.logConfigInput.isSet { - if err := h.Set(cliCtx.logConfigInput.s); err != nil { + s := cliCtx.logConfigInput.s + + if len(cliCtx.logConfigVars) > 0 { + var err error + s, err = expandEnvironmentVariables(s, cliCtx.logConfigVars) + if err != nil { + return errors.Wrap(err, "unable to expand environment variables") + } + } + + if err := h.Set(s); err != nil { return err } if h.Config.FileDefaults.Dir != nil { @@ -458,3 +474,45 @@ sinks: max-file-size: 102400 max-group-size: 1048576 ` + +// validateLogConfigVars return an error if any of the passed logging +// configuration variables are are not permissible. For security, variables +// that start with COCKROACH_ are explicitly disallowed. See #81146 for more. +func validateLogConfigVars(vars []string) error { + for _, v := range vars { + if strings.HasPrefix(strings.ToUpper(v), "COCKROACH_") { + return errors.Newf("use of %s is not allowed as a logging configuration variable", v) + } + } + return nil +} + +// expandEnvironmentVariables replaces variables used in string with their +// values pulled from the environment. If there are variables in the string +// that are not contained in vars, they will be replaced with the empty string. +func expandEnvironmentVariables(s string, vars []string) (string, error) { + var err error + + m := map[string]string{} + // Only pull variable values from the environment if their key is present + // in vars to create an allow list. + for _, k := range vars { + v, ok := envutil.ExternalEnvString(k, 1) + if !ok { + err = errors.CombineErrors(err, errors.Newf("variable %q is not defined in environment", k)) + continue + } + m[k] = v + } + + s = os.Expand(s, func(k string) string { + v, ok := m[k] + if !ok { + err = errors.CombineErrors(err, errors.Newf("unknown variable %q used in configuration", k)) + return "" + } + return v + }) + + return s, err +} diff --git a/pkg/cli/log_flags_test.go b/pkg/cli/log_flags_test.go index f7421debb7de..c9407f78d073 100644 --- a/pkg/cli/log_flags_test.go +++ b/pkg/cli/log_flags_test.go @@ -15,6 +15,7 @@ import ( "io/ioutil" "os" "path/filepath" + "reflect" "regexp" "strings" "testing" @@ -24,7 +25,9 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/cockroach/pkg/util/log/logconfig" "github.com/cockroachdb/datadriven" + "github.com/cockroachdb/errors" "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -128,6 +131,11 @@ func TestSetupLogging(t *testing.T) { t.Fatal(err) } + require.NoError(t, os.Setenv("HOST_IP", "1.2.3.4")) + defer func() { + require.NoError(t, os.Unsetenv("HOST_IP")) + }() + ctx := context.Background() datadriven.RunTest(t, testutils.TestDataPath(t, "logflags"), func(t *testing.T, td *datadriven.TestData) string { @@ -207,8 +215,89 @@ func isServerCmd(thisCmd *cobra.Command) bool { return false } +func TestValidateLogConfigVars(t *testing.T) { + defer leaktest.AfterTest(t)() + for i, tc := range []struct { + vars []string + expectedErr error + }{ + { + vars: []string{"HOST_IP"}, + }, + { + vars: []string{"COCKROACH_TEST"}, + expectedErr: errors.Newf(`use of COCKROACH_TEST is not allowed as a logging configuration variable`), + }, + } { + err := validateLogConfigVars(tc.vars) + + if !errors.Is(tc.expectedErr, err) { + t.Errorf("%d. validateLogConfigVars err expected '%s', but got '%s'.", + i, tc.expectedErr, err) + } + } +} + +func TestExpandEnvironmentVariables(t *testing.T) { + defer leaktest.AfterTest(t)() + require.NoError(t, os.Setenv("HOST_IP1", "1.2.3.4")) + require.NoError(t, os.Setenv("HOST_IP2", "5.6.7.8")) + defer func() { + require.NoError(t, os.Unsetenv("HOST_IP1")) + require.NoError(t, os.Unsetenv("HOST_IP2")) + }() + require.NoError(t, os.Unsetenv("EXPAND_ABSENT_VAR1")) + require.NoError(t, os.Unsetenv("EXPAND_ABSENT_VAR2")) + + for _, tc := range []struct { + in string + vars []string + expectedOut string + expectedErrMsg string + }{ + { + in: "$HOST_IP1", + vars: []string{"HOST_IP1"}, + expectedOut: "1.2.3.4", + }, + { + in: "${HOST_IP2}", + vars: []string{"HOST_IP2"}, + expectedOut: "5.6.7.8", + }, + { + in: "$EXPAND_ABSENT_VAR1", + vars: []string{"EXPAND_ABSENT_VAR1"}, + expectedErrMsg: `variable "EXPAND_ABSENT_VAR1" is not defined in environment`, + }, + { + in: "${EXPAND_ABSENT_VAR2}", + vars: []string{"EXPAND_ABSENT_VAR2"}, + expectedErrMsg: `variable "EXPAND_ABSENT_VAR2" is not defined in environment`, + }, + { + in: "$HOST_IP3", + expectedErrMsg: `unknown variable "HOST_IP3" used in configuration`, + }, + { + in: "${HOST_IP4}", + expectedErrMsg: `unknown variable "HOST_IP4" used in configuration`, + }, + } { + out, err := expandEnvironmentVariables(tc.in, tc.vars) + + if tc.expectedErrMsg != "" { + assert.EqualError(t, err, tc.expectedErrMsg) + } else { + assert.Nil(t, err) + } + assert.Equal(t, tc.expectedOut, out) + } +} + // TestLogFlagCombinations checks that --log and --log-config-file properly -// override each other. +// override each other and that --log-config-vars stores the appropriate values +// in the cliContext struct. func TestLogFlagCombinations(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) @@ -238,15 +327,44 @@ func TestLogFlagCombinations(t *testing.T) { f := startCmd.Flags() testData := []struct { - args []string - expectedLogCfg string + args []string + expectedLogCfg string + expectedLogVars []string }{ - {[]string{"start"}, ""}, - {[]string{"start", "--log=foo"}, "foo"}, - {[]string{"start", "--log-config-file=" + tmpfile.Name()}, filecontents}, - {[]string{"start", "--log=foo", "--log=bar"}, "bar"}, - {[]string{"start", "--log=foo", "--log-config-file=" + tmpfile.Name()}, filecontents}, - {[]string{"start", "--log-config-file=" + tmpfile.Name(), "--log=bar"}, "bar"}, + { + args: []string{"start"}, + expectedLogCfg: "", + }, + { + args: []string{"start", "--log=foo"}, + expectedLogCfg: "foo", + }, + { + args: []string{"start", "--log-config-file=" + tmpfile.Name()}, + expectedLogCfg: filecontents, + }, + { + args: []string{"start", "--log=foo", "--log=bar"}, + expectedLogCfg: "bar", + }, + { + args: []string{"start", "--log=foo", "--log-config-file=" + tmpfile.Name()}, + expectedLogCfg: filecontents, + }, + { + args: []string{"start", "--log-config-file=" + tmpfile.Name(), "--log=bar"}, + expectedLogCfg: "bar", + }, + { + args: []string{"start", "--log-config-file=" + tmpfile.Name(), "--log-config-vars=HOST_IP"}, + expectedLogCfg: filecontents, + expectedLogVars: []string{"HOST_IP"}, + }, + { + args: []string{"start", "--log-config-file=" + tmpfile.Name(), "--log-config-vars=HOST_IP,POD_NAME"}, + expectedLogCfg: filecontents, + expectedLogVars: []string{"HOST_IP", "POD_NAME"}, + }, } for i, td := range testData { @@ -259,6 +377,11 @@ func TestLogFlagCombinations(t *testing.T) { t.Errorf("%d. cliCtx.logConfigInput.s expected '%s', but got '%s'. td.args was '%#v'.", i, td.expectedLogCfg, cliCtx.logConfigInput.s, td.args) } + + if !reflect.DeepEqual(td.expectedLogVars, cliCtx.logConfigVars) { + t.Errorf("%d. cliCtx.logConfigVars expected '%s', but got '%s'. td.args was '%#v'.", + i, td.expectedLogCfg, cliCtx.logConfigVars, td.args) + } } } diff --git a/pkg/cli/testdata/logflags b/pkg/cli/testdata/logflags index 82ec8b8dcf9e..bb1d803a4f83 100644 --- a/pkg/cli/testdata/logflags +++ b/pkg/cli/testdata/logflags @@ -325,6 +325,49 @@ telemetry: )>}, }, )>} +# Ensure variable expansion works. +run +start +--log=sinks: {fluent-servers: {health: {channels: [DEV], address: ${HOST_IP}:5170}}} +--log-config-vars=HOST_IP +---- +config: {)>, +, +, +sinks: {file-groups: {default: ,true,crdb-v2)>, +health: ,true,crdb-v2)>, +pebble: ,true,crdb-v2)>, +security: ,false,crdb-v2)>, +sql-audit: ,false,crdb-v2)>, +sql-auth: ,false,crdb-v2)>, +sql-exec: ,true,crdb-v2)>, +sql-schema: ,true,crdb-v2)>, +sql-slow: ,true,crdb-v2)>, +sql-slow-internal-only: ,true,crdb-v2)>, +telemetry: )>}, +fluent-servers: {health: {channels: {INFO: [DEV]}, +net: tcp, +address: '1.2.3.4:5170', +filter: INFO, +format: json-fluent-compact, +redactable: true, +exit-on-error: false, +buffering: NONE}}, +}, +)>} # It's possible to disable the stderr capture. run