From 5b389c522494f8607a579b4335c8673e0adc09c7 Mon Sep 17 00:00:00 2001 From: Ben Schumacher Date: Fri, 6 Jun 2025 15:07:54 +0200 Subject: [PATCH] [MM-63760] Only partially sanitize DB datasources for Support Packet (#30728) Co-authored-by: Claude --- server/channels/app/config.go | 4 +- server/channels/app/platform/config.go | 6 +- .../channels/app/platform/support_packet.go | 2 +- .../app/platform/support_packet_test.go | 7 +- server/config/database.go | 2 +- server/config/diff.go | 8 +- server/config/diff_test.go | 4 +- server/public/model/config.go | 104 +++++++++++++++++- server/public/model/config_test.go | 68 +++++++++++- server/public/utils/sql/sql_utils.go | 37 +------ server/public/utils/sql/sql_utils_test.go | 52 --------- 11 files changed, 183 insertions(+), 111 deletions(-) diff --git a/server/channels/app/config.go b/server/channels/app/config.go index 63382e29a1..8eda0c3c28 100644 --- a/server/channels/app/config.go +++ b/server/channels/app/config.go @@ -226,11 +226,11 @@ func (a *App) SanitizedConfig(cfg *model.Config) { if err != nil { // GetPluginManifests might error, e.g. when plugins are disabled. // Sanitize all plugin settings in this case. - cfg.Sanitize(nil) + cfg.Sanitize(nil, nil) return } - cfg.Sanitize(manifests) + cfg.Sanitize(manifests, nil) } // GetEnvironmentConfig returns a map of configuration keys whose values have been overridden by an environment variable. diff --git a/server/channels/app/platform/config.go b/server/channels/app/platform/config.go index 0b499c7f1e..764d188435 100644 --- a/server/channels/app/platform/config.go +++ b/server/channels/app/platform/config.go @@ -42,7 +42,7 @@ func (ps *PlatformService) Config() *model.Config { } // getSanitizedConfig gets the configuration without any secrets. -func (ps *PlatformService) getSanitizedConfig(rctx request.CTX) *model.Config { +func (ps *PlatformService) getSanitizedConfig(rctx request.CTX, opts *model.SanitizeOptions) *model.Config { cfg := ps.Config().Clone() manifests, err := ps.getPluginManifests() @@ -50,9 +50,9 @@ func (ps *PlatformService) getSanitizedConfig(rctx request.CTX) *model.Config { // getPluginManifests might error, e.g. when plugins are disabled. // Sanitize all plugin settings in this case. rctx.Logger().Warn("Failed to get plugin manifests for config sanitization. Will sanitize all plugin settings.", mlog.Err(err)) - cfg.Sanitize(nil) + cfg.Sanitize(nil, opts) } else { - cfg.Sanitize(manifests) + cfg.Sanitize(manifests, opts) } return cfg diff --git a/server/channels/app/platform/support_packet.go b/server/channels/app/platform/support_packet.go index f6a45705d1..a4606db612 100644 --- a/server/channels/app/platform/support_packet.go +++ b/server/channels/app/platform/support_packet.go @@ -196,7 +196,7 @@ func (ps *PlatformService) getSupportPacketDiagnostics(rctx request.CTX) (*model } func (ps *PlatformService) getSanitizedConfigFile(rctx request.CTX) (*model.FileData, error) { - config := ps.getSanitizedConfig(rctx) + config := ps.getSanitizedConfig(rctx, &model.SanitizeOptions{PartiallyRedactDataSources: true}) spConfig := model.SupportPacketConfig{ Config: config, FeatureFlags: *config.FeatureFlags, diff --git a/server/channels/app/platform/support_packet_test.go b/server/channels/app/platform/support_packet_test.go index 1dd2276922..f6c30989ce 100644 --- a/server/channels/app/platform/support_packet_test.go +++ b/server/channels/app/platform/support_packet_test.go @@ -360,13 +360,18 @@ func TestGetSanitizedConfigFile(t *testing.T) { require.NoError(t, err) // Ensure sensitive fields are redacted - assert.Equal(t, model.FakeSetting, *config.SqlSettings.DataSource) + assert.Equal(t, model.FakeSetting, *config.FileSettings.PublicLinkSalt) // Ensure non-sensitive fields are present assert.Equal(t, "example.com", *config.ServiceSettings.AllowedUntrustedInternalConnections) // Ensure feature flags are present assert.Equal(t, "true", config.FeatureFlags.TestFeature) + + // Ensure DataSource is partially sanitized (not completely replaced with FakeSetting) + // The default test database connection string should have username/password redacted + assert.Contains(t, *config.SqlSettings.DataSource, "****:****") + assert.NotEqual(t, model.FakeSetting, *config.SqlSettings.DataSource) } func TestGetCPUProfile(t *testing.T) { diff --git a/server/config/database.go b/server/config/database.go index c7174f462a..187cbe8bce 100644 --- a/server/config/database.go +++ b/server/config/database.go @@ -409,7 +409,7 @@ func (ds *DatabaseStore) RemoveFile(name string) error { func (ds *DatabaseStore) String() string { // This is called during the running of MM, so we expect the parsing of DSN // to be successful. - sanitized, _ := sqlUtils.SanitizeDataSource(ds.driverName, ds.originalDsn) + sanitized, _ := model.SanitizeDataSource(ds.driverName, ds.originalDsn) return sanitized } diff --git a/server/config/diff.go b/server/config/diff.go index 524a404474..c62cff5f63 100644 --- a/server/config/diff.go +++ b/server/config/diff.go @@ -65,19 +65,19 @@ func (cd ConfigDiffs) Sanitize() ConfigDiffs { cfgPtr, ok := cd[0].BaseVal.(*model.Config) if ok { - cfgPtr.Sanitize(pluginManifests) + cfgPtr.Sanitize(pluginManifests, nil) } cfgPtr, ok = cd[0].ActualVal.(*model.Config) if ok { - cfgPtr.Sanitize(pluginManifests) + cfgPtr.Sanitize(pluginManifests, nil) } cfgVal, ok := cd[0].BaseVal.(model.Config) if ok { - cfgVal.Sanitize(pluginManifests) + cfgVal.Sanitize(pluginManifests, nil) } cfgVal, ok = cd[0].ActualVal.(model.Config) if ok { - cfgVal.Sanitize(pluginManifests) + cfgVal.Sanitize(pluginManifests, nil) } } diff --git a/server/config/diff_test.go b/server/config/diff_test.go index 4346bfa71f..ba585f949d 100644 --- a/server/config/diff_test.go +++ b/server/config/diff_test.go @@ -113,7 +113,7 @@ func TestDiffSanitized(t *testing.T) { Path: "", BaseVal: func() model.Config { cfg := defaultConfigGen() - cfg.Sanitize(nil) + cfg.Sanitize(nil, nil) return *cfg }(), ActualVal: model.Config{}, @@ -131,7 +131,7 @@ func TestDiffSanitized(t *testing.T) { BaseVal: model.Config{}, ActualVal: func() model.Config { cfg := defaultConfigGen() - cfg.Sanitize(nil) + cfg.Sanitize(nil, nil) return *cfg }(), }, diff --git a/server/public/model/config.go b/server/public/model/config.go index 638eaa7ffc..5083a5ebdf 100644 --- a/server/public/model/config.go +++ b/server/public/model/config.go @@ -20,7 +20,9 @@ import ( "strings" "time" + "github.com/go-sql-driver/mysql" "github.com/mattermost/ldap" + "github.com/pkg/errors" "github.com/mattermost/mattermost/server/public/shared/mlog" "github.com/mattermost/mattermost/server/public/utils" @@ -73,6 +75,9 @@ const ( FakeSetting = "********************************" + // SanitizedPassword is the placeholder used for redacting passwords in data sources + SanitizedPassword = "****" + RestrictEmojiCreationAll = "all" RestrictEmojiCreationAdmin = "admin" RestrictEmojiCreationSystemAdmin = "system_admin" @@ -4681,6 +4686,14 @@ func (s *ImageProxySettings) isValid() *AppError { return nil } +// SanitizeOptions specifies options for the [Config.Sanitize] method. +type SanitizeOptions struct { + // PartiallyRedactDataSources, when true, only redacts usernames and passwords + // from data sources, keeping other connection parameters visible. + // When false, replaces the entire data source with FakeSetting. + PartiallyRedactDataSources bool +} + func (o *Config) GetSanitizeOptions() map[string]bool { options := map[string]bool{} options["fullname"] = *o.PrivacySettings.ShowFullName @@ -4689,7 +4702,32 @@ func (o *Config) GetSanitizeOptions() map[string]bool { return options } -func (o *Config) Sanitize(pluginManifests []*Manifest) { +// Sanitize removes sensitive information from the configuration object. +// It replaces sensitive fields with [FakeSetting] or sanitizes them. +// +// Parameters: +// - pluginManifests: Plugin manifests for sanitizing plugin settings. +// - opts: Options for controlling sanitization behavior. If nil, defaults are used. See [SanitizeOptions]. +func (o *Config) Sanitize(pluginManifests []*Manifest, opts *SanitizeOptions) { + if opts == nil { + opts = &SanitizeOptions{} + } + + var driverName string + if o.SqlSettings.DriverName != nil { + driverName = *o.SqlSettings.DriverName + } + sanitizeDataSourceField := func(dataSource string, fieldName string) string { + if opts.PartiallyRedactDataSources && driverName != "" { + sanitized, err := SanitizeDataSource(driverName, dataSource) + if err != nil { + mlog.Warn("Failed to sanitize "+fieldName+". Falling back to fully sanitizing the setting.", mlog.Err(err)) + return FakeSetting + } + return sanitized + } + return FakeSetting + } if o.LdapSettings.BindPassword != nil && *o.LdapSettings.BindPassword != "" { *o.LdapSettings.BindPassword = FakeSetting } @@ -4723,7 +4761,7 @@ func (o *Config) Sanitize(pluginManifests []*Manifest) { } if o.SqlSettings.DataSource != nil { - *o.SqlSettings.DataSource = FakeSetting + *o.SqlSettings.DataSource = sanitizeDataSourceField(*o.SqlSettings.DataSource, "SqlSettings.DataSource") } if o.SqlSettings.AtRestEncryptKey != nil { @@ -4735,15 +4773,18 @@ func (o *Config) Sanitize(pluginManifests []*Manifest) { } for i := range o.SqlSettings.DataSourceReplicas { - o.SqlSettings.DataSourceReplicas[i] = FakeSetting + o.SqlSettings.DataSourceReplicas[i] = sanitizeDataSourceField(o.SqlSettings.DataSourceReplicas[i], "SqlSettings.DataSourceReplicas") } for i := range o.SqlSettings.DataSourceSearchReplicas { - o.SqlSettings.DataSourceSearchReplicas[i] = FakeSetting + o.SqlSettings.DataSourceSearchReplicas[i] = sanitizeDataSourceField(o.SqlSettings.DataSourceSearchReplicas[i], "SqlSettings.DataSourceSearchReplicas") } for i := range o.SqlSettings.ReplicaLagSettings { - o.SqlSettings.ReplicaLagSettings[i].DataSource = NewPointer(FakeSetting) + if o.SqlSettings.ReplicaLagSettings[i].DataSource != nil { + sanitized := sanitizeDataSourceField(*o.SqlSettings.ReplicaLagSettings[i].DataSource, "SqlSettings.ReplicaLagSettings") + o.SqlSettings.ReplicaLagSettings[i].DataSource = NewPointer(sanitized) + } } if o.MessageExportSettings.GlobalRelaySettings != nil && @@ -4763,6 +4804,59 @@ func (o *Config) Sanitize(pluginManifests []*Manifest) { o.PluginSettings.Sanitize(pluginManifests) } +// SanitizeDataSource redacts sensitive information (username and password) from a database +// connection string while preserving other connection parameters. +// +// Parameters: +// - driverName: The database driver name (postgres or mysql) +// - dataSource: The database connection string to sanitize +// +// Returns: +// - The sanitized connection string with username/password replaced by SanitizedPassword +// - An error if the driverName is not supported or if parsing fails +// +// Examples: +// - PostgreSQL: "postgres://user:pass@host:5432/db" -> "postgres://****:****@host:5432/db" +// - MySQL: "user:pass@tcp(host:3306)/db" -> "****:****@tcp(host:3306)/db" +func SanitizeDataSource(driverName, dataSource string) (string, error) { + // Handle empty data source + if dataSource == "" { + return "", nil + } + + switch driverName { + case DatabaseDriverPostgres: + u, err := url.Parse(dataSource) + if err != nil { + return "", err + } + u.User = url.UserPassword(SanitizedPassword, SanitizedPassword) + + // Remove username and password from query string + params := u.Query() + params.Del("user") + params.Del("password") + u.RawQuery = params.Encode() + + // Unescape the URL to make it human-readable + out, err := url.QueryUnescape(u.String()) + if err != nil { + return "", err + } + return out, nil + case DatabaseDriverMysql: + cfg, err := mysql.ParseDSN(dataSource) + if err != nil { + return "", err + } + cfg.User = SanitizedPassword + cfg.Passwd = SanitizedPassword + return cfg.FormatDSN(), nil + default: + return "", errors.New("invalid drivername. Not postgres or mysql.") + } +} + type FilterTag struct { TagType string TagName string diff --git a/server/public/model/config_test.go b/server/public/model/config_test.go index 1edd4e5759..b85fea15bc 100644 --- a/server/public/model/config_test.go +++ b/server/public/model/config_test.go @@ -1521,7 +1521,7 @@ func TestConfigSanitize(t *testing.T) { QueryTimeLag: NewPointer("QueryTimeLag"), }} - c.Sanitize(nil) + c.Sanitize(nil, nil) assert.Equal(t, FakeSetting, *c.LdapSettings.BindPassword) assert.Equal(t, FakeSetting, *c.FileSettings.PublicLinkSalt) @@ -1543,10 +1543,20 @@ func TestConfigSanitize(t *testing.T) { t.Run("with default config", func(t *testing.T) { c := Config{} c.SetDefaults() - c.Sanitize(nil) + c.Sanitize(nil, nil) assert.Len(t, c.SqlSettings.ReplicaLagSettings, 0) }) + + t.Run("partially sanitize DataSource", func(t *testing.T) { + c := Config{} + c.SetDefaults() + *c.SqlSettings.DataSource = "postgres://mmuser:mostest@localhost:5432/mattermost_test?sslmode=disable" + c.Sanitize(nil, &SanitizeOptions{PartiallyRedactDataSources: true}) + + expectedURL := "postgres://" + SanitizedPassword + ":" + SanitizedPassword + "@localhost:5432/mattermost_test?sslmode=disable" + assert.Equal(t, expectedURL, *c.SqlSettings.DataSource) + }) } func TestPluginSettingsSanitize(t *testing.T) { @@ -1689,6 +1699,56 @@ func TestPluginSettingsSanitize(t *testing.T) { } } +func TestSanitizeDataSource(t *testing.T) { + t.Run(DatabaseDriverPostgres, func(t *testing.T) { + testCases := []struct { + Original string + Sanitized string + }{ + { + "", + "", + }, + { + "postgres://mmuser:mostest@localhost", + "postgres://" + SanitizedPassword + ":" + SanitizedPassword + "@localhost", + }, + { + "postgres://mmuser:mostest@localhost/dummy?sslmode=disable", + "postgres://" + SanitizedPassword + ":" + SanitizedPassword + "@localhost/dummy?sslmode=disable", + }, + { + "postgres://localhost/dummy?sslmode=disable&user=mmuser&password=mostest", + "postgres://" + SanitizedPassword + ":" + SanitizedPassword + "@localhost/dummy?sslmode=disable", + }, + } + driver := DatabaseDriverPostgres + for _, tc := range testCases { + out, err := SanitizeDataSource(driver, tc.Original) + require.NoError(t, err) + assert.Equal(t, tc.Sanitized, out) + } + }) + + t.Run(DatabaseDriverMysql, func(t *testing.T) { + testCases := []struct { + Original string + Sanitized string + }{ + { + "mmuser:mostest@tcp(localhost:3306)/mattermost_test?charset=utf8mb4,utf8&readTimeout=30s&writeTimeout=30s", + SanitizedPassword + ":" + SanitizedPassword + "@tcp(localhost:3306)/mattermost_test?charset=utf8mb4,utf8&readTimeout=30s&writeTimeout=30s", + }, + } + driver := DatabaseDriverMysql + for _, tc := range testCases { + out, err := SanitizeDataSource(driver, tc.Original) + require.NoError(t, err) + assert.Equal(t, tc.Sanitized, out) + } + }) +} + func TestConfigFilteredByTag(t *testing.T) { c := Config{} c.SetDefaults() @@ -2146,7 +2206,7 @@ func TestFilterConfig(t *testing.T) { require.NotEmpty(t, m) require.Equal(t, dsn, m["SqlSettings"].(map[string]any)["DataSource"]) - cfg.Sanitize(nil) + cfg.Sanitize(nil, nil) m, err = FilterConfig(cfg, ConfigFilterOptions{ GetConfigOptions: GetConfigOptions{ RemoveDefaults: true, @@ -2156,7 +2216,7 @@ func TestFilterConfig(t *testing.T) { require.NotEmpty(t, m) require.Equal(t, FakeSetting, m["SqlSettings"].(map[string]any)["DataSource"]) - cfg.Sanitize(nil) + cfg.Sanitize(nil, nil) m, err = FilterConfig(cfg, ConfigFilterOptions{ GetConfigOptions: GetConfigOptions{ RemoveDefaults: true, diff --git a/server/public/utils/sql/sql_utils.go b/server/public/utils/sql/sql_utils.go index 87d86e36d6..c1a24dd685 100644 --- a/server/public/utils/sql/sql_utils.go +++ b/server/public/utils/sql/sql_utils.go @@ -6,7 +6,6 @@ package sql import ( "context" dbsql "database/sql" - "net/url" "strings" "time" @@ -57,7 +56,7 @@ func SetupConnection(logger mlog.LoggerIFace, connType string, dataSource string } // At this point, we have passed sql.Open, so we deliberately ignore any errors. - sanitized, _ := SanitizeDataSource(*settings.DriverName, dataSource) + sanitized, _ := model.SanitizeDataSource(*settings.DriverName, dataSource) logger = logger.With( mlog.String("database", connType), @@ -101,37 +100,3 @@ func SetupConnection(logger mlog.LoggerIFace, connType string, dataSource string return db, nil } - -func SanitizeDataSource(driverName, dataSource string) (string, error) { - switch driverName { - case model.DatabaseDriverPostgres: - u, err := url.Parse(dataSource) - if err != nil { - return "", err - } - u.User = url.UserPassword("****", "****") - - // Remove username and password from query string - params := u.Query() - params.Del("user") - params.Del("password") - u.RawQuery = params.Encode() - - // Unescape the URL to make it human-readable - out, err := url.QueryUnescape(u.String()) - if err != nil { - return "", err - } - return out, nil - case model.DatabaseDriverMysql: - cfg, err := mysql.ParseDSN(dataSource) - if err != nil { - return "", err - } - cfg.User = "****" - cfg.Passwd = "****" - return cfg.FormatDSN(), nil - default: - return "", errors.New("invalid drivername. Not postgres or mysql.") - } -} diff --git a/server/public/utils/sql/sql_utils_test.go b/server/public/utils/sql/sql_utils_test.go index dbb678e6c5..e5f2872223 100644 --- a/server/public/utils/sql/sql_utils_test.go +++ b/server/public/utils/sql/sql_utils_test.go @@ -6,8 +6,6 @@ package sql import ( "testing" - "github.com/mattermost/mattermost/server/public/model" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -65,53 +63,3 @@ func TestResetReadTimeout(t *testing.T) { }) } } - -func TestSanitizeDataSource(t *testing.T) { - t.Run(model.DatabaseDriverPostgres, func(t *testing.T) { - testCases := []struct { - Original string - Sanitized string - }{ - { - "", - "//****:****@", - }, - { - "postgres://mmuser:mostest@localhost", - "postgres://****:****@localhost", - }, - { - "postgres://mmuser:mostest@localhost/dummy?sslmode=disable", - "postgres://****:****@localhost/dummy?sslmode=disable", - }, - { - "postgres://localhost/dummy?sslmode=disable&user=mmuser&password=mostest", - "postgres://****:****@localhost/dummy?sslmode=disable", - }, - } - driver := model.DatabaseDriverPostgres - for _, tc := range testCases { - out, err := SanitizeDataSource(driver, tc.Original) - require.NoError(t, err) - assert.Equal(t, tc.Sanitized, out) - } - }) - - t.Run(model.DatabaseDriverMysql, func(t *testing.T) { - testCases := []struct { - Original string - Sanitized string - }{ - { - "mmuser:mostest@tcp(localhost:3306)/mattermost_test?charset=utf8mb4,utf8&readTimeout=30s&writeTimeout=30s", - "****:****@tcp(localhost:3306)/mattermost_test?charset=utf8mb4,utf8&readTimeout=30s&writeTimeout=30s", - }, - } - driver := model.DatabaseDriverMysql - for _, tc := range testCases { - out, err := SanitizeDataSource(driver, tc.Original) - require.NoError(t, err) - assert.Equal(t, tc.Sanitized, out) - } - }) -}