From da13f88862e60297127b2e6a9a5f038dc99fa701 Mon Sep 17 00:00:00 2001 From: Serge Zaitsev Date: Mon, 10 May 2021 17:03:10 +0200 Subject: [PATCH] Redact sensitive values before logging them (#33829) * use a common way to redact sensitive values before logging them * fix panic on missing testCase.err, simplify require checks * fix a silly typo * combine readConfig and buildConnectionString methods, as they are closely related --- pkg/api/admin.go | 18 +------ pkg/infra/remotecache/redis_storage.go | 2 +- pkg/services/sqlstore/sqlstore.go | 14 ++++-- pkg/services/sqlstore/sqlstore_test.go | 58 ++++++++++++---------- pkg/setting/setting.go | 67 ++++++++++++-------------- pkg/setting/setting_test.go | 12 +---- 6 files changed, 77 insertions(+), 94 deletions(-) diff --git a/pkg/api/admin.go b/pkg/api/admin.go index c7f06ecf112..090e4f07886 100644 --- a/pkg/api/admin.go +++ b/pkg/api/admin.go @@ -1,9 +1,6 @@ package api import ( - "regexp" - "strings" - "github.com/grafana/grafana/pkg/api/response" "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/models" @@ -19,20 +16,7 @@ func AdminGetSettings(c *models.ReqContext) response.Response { for _, key := range section.Keys() { keyName := key.Name() - value := key.Value() - if strings.Contains(keyName, "secret") || strings.Contains(keyName, "password") || (strings.Contains(keyName, "provider_config")) { - value = "************" - } - if strings.Contains(keyName, "url") { - var rgx = regexp.MustCompile(`.*:\/\/([^:]*):([^@]*)@.*?$`) - var subs = rgx.FindAllSubmatch([]byte(value), -1) - if subs != nil && len(subs[0]) == 3 { - value = strings.Replace(value, string(subs[0][1]), "******", 1) - value = strings.Replace(value, string(subs[0][2]), "******", 1) - } - } - - jsonSec[keyName] = value + jsonSec[keyName] = setting.RedactedValue(keyName, key.Value()) } } diff --git a/pkg/infra/remotecache/redis_storage.go b/pkg/infra/remotecache/redis_storage.go index 20400004873..59d751c3db0 100644 --- a/pkg/infra/remotecache/redis_storage.go +++ b/pkg/infra/remotecache/redis_storage.go @@ -28,7 +28,7 @@ func parseRedisConnStr(connStr string) (*redis.Options, error) { if len(keyValueTuple) != 2 { if strings.HasPrefix(rawKeyValue, "password") { // don't log the password - rawKeyValue = "password******" + rawKeyValue = "password" + setting.RedactedPassword } return nil, fmt.Errorf("incorrect redis connection string format detected for '%v', format is key=value,key=value", rawKeyValue) } diff --git a/pkg/services/sqlstore/sqlstore.go b/pkg/services/sqlstore/sqlstore.go index 73992c0fece..57dac9fd86b 100644 --- a/pkg/services/sqlstore/sqlstore.go +++ b/pkg/services/sqlstore/sqlstore.go @@ -74,8 +74,6 @@ func (ss *SQLStore) Register() { func (ss *SQLStore) Init() error { ss.log = log.New("sqlstore") - ss.readConfig() - if err := ss.initEngine(); err != nil { return errutil.Wrap("failed to connect to database", err) } @@ -204,6 +202,10 @@ func (ss *SQLStore) buildExtraConnectionString(sep rune) string { } func (ss *SQLStore) buildConnectionString() (string, error) { + if err := ss.readConfig(); err != nil { + return "", err + } + cnnstr := ss.dbCfg.ConnectionString // special case used by integration tests @@ -339,12 +341,15 @@ func (ss *SQLStore) initEngine() error { } // readConfig initializes the SQLStore from its configuration. -func (ss *SQLStore) readConfig() { +func (ss *SQLStore) readConfig() error { sec := ss.Cfg.Raw.Section("database") cfgURL := sec.Key("url").String() if len(cfgURL) != 0 { - dbURL, _ := url.Parse(cfgURL) + dbURL, err := url.Parse(cfgURL) + if err != nil { + return err + } ss.dbCfg.Type = dbURL.Scheme ss.dbCfg.Host = dbURL.Host @@ -382,6 +387,7 @@ func (ss *SQLStore) readConfig() { ss.dbCfg.CacheMode = sec.Key("cache_mode").MustString("private") ss.dbCfg.SkipMigrations = sec.Key("skip_migrations").MustBool() + return nil } // ITestDB is an interface of arguments for testing db diff --git a/pkg/services/sqlstore/sqlstore_test.go b/pkg/services/sqlstore/sqlstore_test.go index 9ff37c2a41d..5579d7aba7e 100644 --- a/pkg/services/sqlstore/sqlstore_test.go +++ b/pkg/services/sqlstore/sqlstore_test.go @@ -3,18 +3,21 @@ package sqlstore import ( + "errors" + "net/url" "testing" - . "github.com/smartystreets/goconvey/convey" - "github.com/grafana/grafana/pkg/setting" + "github.com/stretchr/testify/require" ) type sqlStoreTest struct { name string dbType string dbHost string + dbURL string connStrValues []string + err error } var sqlStoreTestCases = []sqlStoreTest{ @@ -66,44 +69,47 @@ var sqlStoreTestCases = []sqlStoreTest{ dbHost: "[::1]", connStrValues: []string{"host=::1", "port=5432"}, }, + { + name: "Invalid database URL", + dbURL: "://invalid.com/", + err: &url.Error{Op: "parse", URL: "://invalid.com/", Err: errors.New("missing protocol scheme")}, + }, } func TestSQLConnectionString(t *testing.T) { - Convey("Testing SQL Connection Strings", t, func() { - t.Helper() + for _, testCase := range sqlStoreTestCases { + t.Run(testCase.name, func(t *testing.T) { + sqlstore := &SQLStore{} + sqlstore.Cfg = makeSQLStoreTestConfig(t, testCase.dbType, testCase.dbHost, testCase.dbURL) + connStr, err := sqlstore.buildConnectionString() + require.Equal(t, testCase.err, err) - for _, testCase := range sqlStoreTestCases { - Convey(testCase.name, func() { - sqlstore := &SQLStore{} - sqlstore.Cfg = makeSQLStoreTestConfig(testCase.dbType, testCase.dbHost) - sqlstore.readConfig() - - connStr, err := sqlstore.buildConnectionString() - - So(err, ShouldBeNil) - for _, connSubStr := range testCase.connStrValues { - So(connStr, ShouldContainSubstring, connSubStr) - } - }) - } - }) + for _, connSubStr := range testCase.connStrValues { + require.Contains(t, connStr, connSubStr) + } + }) + } } -func makeSQLStoreTestConfig(dbType string, host string) *setting.Cfg { +func makeSQLStoreTestConfig(t *testing.T, dbType, host, dbURL string) *setting.Cfg { + t.Helper() + cfg := setting.NewCfg() sec, err := cfg.Raw.NewSection("database") - So(err, ShouldBeNil) + require.NoError(t, err) _, err = sec.NewKey("type", dbType) - So(err, ShouldBeNil) + require.NoError(t, err) _, err = sec.NewKey("host", host) - So(err, ShouldBeNil) + require.NoError(t, err) + _, err = sec.NewKey("url", dbURL) + require.NoError(t, err) _, err = sec.NewKey("user", "user") - So(err, ShouldBeNil) + require.NoError(t, err) _, err = sec.NewKey("name", "test_db") - So(err, ShouldBeNil) + require.NoError(t, err) _, err = sec.NewKey("password", "pass") - So(err, ShouldBeNil) + require.NoError(t, err) return cfg } diff --git a/pkg/setting/setting.go b/pkg/setting/setting.go index e90dd6d4260..fbcd4c2633a 100644 --- a/pkg/setting/setting.go +++ b/pkg/setting/setting.go @@ -36,7 +36,7 @@ const ( ) const ( - redactedPassword = "*********" + RedactedPassword = "*********" DefaultHTTPAddr = "0.0.0.0" Dev = "development" Prod = "production" @@ -431,14 +431,33 @@ func ToAbsUrl(relativeUrl string) string { return AppUrl + relativeUrl } -func shouldRedactKey(s string) bool { - uppercased := strings.ToUpper(s) - return strings.Contains(uppercased, "PASSWORD") || strings.Contains(uppercased, "SECRET") || strings.Contains(uppercased, "PROVIDER_CONFIG") -} - -func shouldRedactURLKey(s string) bool { - uppercased := strings.ToUpper(s) - return strings.Contains(uppercased, "DATABASE_URL") +func RedactedValue(key, value string) string { + uppercased := strings.ToUpper(key) + // Sensitive information: password, secrets etc + for _, pattern := range []string{ + "PASSWORD", + "SECRET", + "PROVIDER_CONFIG", + "PRIVATE_KEY", + "SECRET_KEY", + "CERTIFICATE", + } { + if strings.Contains(uppercased, pattern) { + return RedactedPassword + } + } + // Sensitive URLs that might contain username and password + for _, pattern := range []string{ + "DATABASE_URL", + } { + if strings.Contains(uppercased, pattern) { + if u, err := url.Parse(value); err == nil { + return u.Redacted() + } + } + } + // Otherwise return unmodified value + return value } func applyEnvVariableOverrides(file *ini.File) error { @@ -450,24 +469,7 @@ func applyEnvVariableOverrides(file *ini.File) error { if len(envValue) > 0 { key.SetValue(envValue) - if shouldRedactKey(envKey) { - envValue = redactedPassword - } - if shouldRedactURLKey(envKey) { - u, err := url.Parse(envValue) - if err != nil { - return fmt.Errorf("could not parse environment variable. key: %s, value: %s. error: %v", envKey, envValue, err) - } - ui := u.User - if ui != nil { - _, exists := ui.Password() - if exists { - u.User = url.UserPassword(ui.Username(), "-redacted-") - envValue = u.String() - } - } - } - appliedEnvOverrides = append(appliedEnvOverrides, fmt.Sprintf("%s=%s", envKey, envValue)) + appliedEnvOverrides = append(appliedEnvOverrides, fmt.Sprintf("%s=%s", envKey, RedactedValue(envKey, envValue))) } } } @@ -549,10 +551,8 @@ func applyCommandLineDefaultProperties(props map[string]string, file *ini.File) value, exists := props[keyString] if exists { key.SetValue(value) - if shouldRedactKey(keyString) { - value = redactedPassword - } - appliedCommandLineProperties = append(appliedCommandLineProperties, fmt.Sprintf("%s=%s", keyString, value)) + appliedCommandLineProperties = append(appliedCommandLineProperties, + fmt.Sprintf("%s=%s", keyString, RedactedValue(keyString, value))) } } } @@ -1059,10 +1059,7 @@ func (s *DynamicSection) Key(k string) *ini.Key { } key.SetValue(envValue) - if shouldRedactKey(envKey) { - envValue = redactedPassword - } - s.Logger.Info("Config overridden from Environment variable", "var", fmt.Sprintf("%s=%s", envKey, envValue)) + s.Logger.Info("Config overridden from Environment variable", "var", fmt.Sprintf("%s=%s", envKey, RedactedValue(envKey, envValue))) return key } diff --git a/pkg/setting/setting_test.go b/pkg/setting/setting_test.go index 85e801eb605..aa52cb60a14 100644 --- a/pkg/setting/setting_test.go +++ b/pkg/setting/setting_test.go @@ -78,16 +78,6 @@ func TestLoadingSettings(t *testing.T) { So(appliedEnvOverrides, ShouldContain, "GF_SECURITY_ADMIN_PASSWORD=*********") }) - Convey("Should return an error when url is invalid", func() { - err := os.Setenv("GF_DATABASE_URL", "postgres.%31://grafana:secret@postgres:5432/grafana") - require.NoError(t, err) - - cfg := NewCfg() - err = cfg.Load(&CommandLineArgs{HomePath: "../../"}) - - So(err, ShouldNotBeNil) - }) - Convey("Should replace password in URL when url environment is defined", func() { err := os.Setenv("GF_DATABASE_URL", "mysql://user:secret@localhost:3306/database") require.NoError(t, err) @@ -96,7 +86,7 @@ func TestLoadingSettings(t *testing.T) { err = cfg.Load(&CommandLineArgs{HomePath: "../../"}) So(err, ShouldBeNil) - So(appliedEnvOverrides, ShouldContain, "GF_DATABASE_URL=mysql://user:-redacted-@localhost:3306/database") + So(appliedEnvOverrides, ShouldContain, "GF_DATABASE_URL=mysql://user:xxxxx@localhost:3306/database") }) Convey("Should get property map from command line args array", func() {