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

@@ -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()
+			jsonSec[keyName] = setting.RedactedValue(keyName, 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
 		}
 	}
 

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

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

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() {
+	for _, testCase := range sqlStoreTestCases {
-		t.Helper()
+		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 {
+			for _, connSubStr := range testCase.connStrValues {
-			Convey(testCase.name, func() {
+				require.Contains(t, connStr, connSubStr)
-				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)
-				}
-			})
-		}
-	})
 }
 
-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
 }

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 {
+func RedactedValue(key, value string) string {
-	uppercased := strings.ToUpper(s)
+	uppercased := strings.ToUpper(key)
-	return strings.Contains(uppercased, "PASSWORD") || strings.Contains(uppercased, "SECRET") || strings.Contains(uppercased, "PROVIDER_CONFIG")
+	// Sensitive information: password, secrets etc
-}
+	for _, pattern := range []string{
-
+		"PASSWORD",
-func shouldRedactURLKey(s string) bool {
+		"SECRET",
-	uppercased := strings.ToUpper(s)
+		"PROVIDER_CONFIG",
-	return strings.Contains(uppercased, "DATABASE_URL")
+		"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) {
+				appliedEnvOverrides = append(appliedEnvOverrides, fmt.Sprintf("%s=%s", envKey, RedactedValue(envKey, envValue)))
-					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))
 			}
 		}
 	}
@@ -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) {
+				appliedCommandLineProperties = append(appliedCommandLineProperties,
-					value = redactedPassword
+					fmt.Sprintf("%s=%s", keyString, RedactedValue(keyString, value)))
-				}
-				appliedCommandLineProperties = append(appliedCommandLineProperties, fmt.Sprintf("%s=%s", keyString, value))
 			}
 		}
 	}
@@ -1059,10 +1059,7 @@ func (s *DynamicSection) Key(k string) *ini.Key {
 	}
 
 	key.SetValue(envValue)
-	if shouldRedactKey(envKey) {
+	s.Logger.Info("Config overridden from Environment variable", "var", fmt.Sprintf("%s=%s", envKey, RedactedValue(envKey, envValue)))
-		envValue = redactedPassword
-	}
-	s.Logger.Info("Config overridden from Environment variable", "var", fmt.Sprintf("%s=%s", envKey, envValue))
 
 	return key
 }

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() {