From f621989c5818f4ddb5018cb42c6b4ca57cfef535 Mon Sep 17 00:00:00 2001 From: Claudio Costa Date: Thu, 28 Oct 2021 14:07:35 +0200 Subject: [PATCH] [MM-39597] Implement ConfigDiffs.Sanitize() (#18855) * Implement ConfigDiffs.Sanitize() * Remove possibly sensitive info from logs --- api4/config.go | 6 +- api4/config_local.go | 4 +- config/diff.go | 53 +++++ config/diff_test.go | 480 ++++++++++++++++++++++++++++++++++++++++++- model/config.go | 29 ++- 5 files changed, 552 insertions(+), 20 deletions(-) diff --git a/api4/config.go b/api4/config.go index 1168c3fc90..5609691bea 100644 --- a/api4/config.go +++ b/api4/config.go @@ -165,7 +165,7 @@ func updateConfig(c *Context, w http.ResponseWriter, r *http.Request) { c.Err = model.NewAppError("updateConfig", "api.config.update_config.diff.app_error", nil, diffErr.Error(), http.StatusInternalServerError) return } - auditRec.AddMeta("diff", diffs) + auditRec.AddMeta("diff", diffs.Sanitize()) newCfg.Sanitize() @@ -292,7 +292,7 @@ func patchConfig(c *Context, w http.ResponseWriter, r *http.Request) { c.Err = model.NewAppError("patchConfig", "api.config.patch_config.diff.app_error", nil, diffErr.Error(), http.StatusInternalServerError) return } - auditRec.AddMeta("diff", diffs) + auditRec.AddMeta("diff", diffs.Sanitize()) newCfg.Sanitize() @@ -400,8 +400,6 @@ func migrateConfig(c *Context, w http.ResponseWriter, r *http.Request) { } auditRec := c.MakeAuditRecord("migrateConfig", audit.Fail) - auditRec.AddMeta("from", from) - auditRec.AddMeta("to", to) defer c.LogAuditRec(auditRec) if !c.App.SessionHasPermissionTo(*c.AppContext.Session(), model.PermissionManageSystem) { diff --git a/api4/config_local.go b/api4/config_local.go index 91ef138e43..dff010978e 100644 --- a/api4/config_local.go +++ b/api4/config_local.go @@ -73,7 +73,7 @@ func localUpdateConfig(c *Context, w http.ResponseWriter, r *http.Request) { c.Err = model.NewAppError("updateConfig", "api.config.update_config.diff.app_error", nil, diffErr.Error(), http.StatusInternalServerError) return } - auditRec.AddMeta("diff", diffs) + auditRec.AddMeta("diff", diffs.Sanitize()) newCfg.Sanitize() @@ -131,7 +131,7 @@ func localPatchConfig(c *Context, w http.ResponseWriter, r *http.Request) { c.Err = model.NewAppError("patchConfig", "api.config.patch_config.diff.app_error", nil, diffErr.Error(), http.StatusInternalServerError) return } - auditRec.AddMeta("diff", diffs) + auditRec.AddMeta("diff", diffs.Sanitize()) auditRec.Success() diff --git a/config/diff.go b/config/diff.go index 299472cab7..1390918445 100644 --- a/config/diff.go +++ b/config/diff.go @@ -18,6 +18,59 @@ type ConfigDiff struct { ActualVal interface{} `json:"actual_val"` } +var configSensitivePaths = map[string]bool{ + "LdapSettings.BindPassword": true, + "FileSettings.PublicLinkSalt": true, + "FileSettings.AmazonS3SecretAccessKey": true, + "SqlSettings.DataSource": true, + "SqlSettings.AtRestEncryptKey": true, + "SqlSettings.DataSourceReplicas": true, + "SqlSettings.DataSourceSearchReplicas": true, + "EmailSettings.SMTPPassword": true, + "GitLabSettings.Secret": true, + "GoogleSettings.Secret": true, + "Office365Settings.Secret": true, + "OpenIdSettings.Secret": true, + "ElasticsearchSettings.Password": true, + "MessageExportSettings.GlobalRelaySettings.SMTPUsername": true, + "MessageExportSettings.GlobalRelaySettings.SMTPPassword": true, + "MessageExportSettings.GlobalRelaySettings.EmailAddress": true, + "ServiceSettings.GfycatAPISecret": true, + "ServiceSettings.SplitKey": true, + "PluginSettings.Plugins": true, +} + +// Sanitize replaces sensitive config values in the diff with asterisks filled strings. +func (cd ConfigDiffs) Sanitize() ConfigDiffs { + if len(cd) == 1 { + cfgPtr, ok := cd[0].BaseVal.(*model.Config) + if ok { + cfgPtr.Sanitize() + } + cfgPtr, ok = cd[0].ActualVal.(*model.Config) + if ok { + cfgPtr.Sanitize() + } + cfgVal, ok := cd[0].BaseVal.(model.Config) + if ok { + cfgVal.Sanitize() + } + cfgVal, ok = cd[0].ActualVal.(model.Config) + if ok { + cfgVal.Sanitize() + } + } + + for i := range cd { + if configSensitivePaths[cd[i].Path] { + cd[i].BaseVal = model.FakeSetting + cd[i].ActualVal = model.FakeSetting + } + } + + return cd +} + func diff(base, actual reflect.Value, label string) ([]ConfigDiff, error) { var diffs []ConfigDiff diff --git a/config/diff_test.go b/config/diff_test.go index 86e3f4dd5f..1b493908cc 100644 --- a/config/diff_test.go +++ b/config/diff_test.go @@ -75,6 +75,474 @@ func BenchmarkDiff(b *testing.B) { }) } +func TestDiffSanitized(t *testing.T) { + tcs := []struct { + name string + base *model.Config + actual *model.Config + diffs ConfigDiffs + err string + }{ + { + "nil", + nil, + nil, + nil, + "input configs should not be nil", + }, + { + "empty", + &model.Config{}, + &model.Config{}, + nil, + "", + }, + { + "defaults", + defaultConfigGen(), + defaultConfigGen(), + nil, + "", + }, + { + "default base, actual empty", + defaultConfigGen(), + &model.Config{}, + ConfigDiffs{ + { + Path: "", + BaseVal: func() model.Config { + cfg := defaultConfigGen() + cfg.Sanitize() + return *cfg + }(), + ActualVal: model.Config{}, + }, + }, + "", + }, + { + "empty base, actual default", + &model.Config{}, + defaultConfigGen(), + ConfigDiffs{ + { + Path: "", + BaseVal: model.Config{}, + ActualVal: func() model.Config { + cfg := defaultConfigGen() + cfg.Sanitize() + return *cfg + }(), + }, + }, + "", + }, + { + "sensitive LdapSettings.BindPassword", + func() *model.Config { + cfg := defaultConfigGen() + cfg.LdapSettings.BindPassword = model.NewString("base") + return cfg + }(), + func() *model.Config { + cfg := defaultConfigGen() + cfg.LdapSettings.BindPassword = model.NewString("actual") + return cfg + }(), + ConfigDiffs{ + { + Path: "LdapSettings.BindPassword", + BaseVal: model.FakeSetting, + ActualVal: model.FakeSetting, + }, + }, + "", + }, + { + "sensitive FileSettings.PublicLinkSalt", + func() *model.Config { + cfg := defaultConfigGen() + cfg.FileSettings.PublicLinkSalt = model.NewString("base") + return cfg + }(), + func() *model.Config { + cfg := defaultConfigGen() + cfg.FileSettings.PublicLinkSalt = model.NewString("actual") + return cfg + }(), + ConfigDiffs{ + { + Path: "FileSettings.PublicLinkSalt", + BaseVal: model.FakeSetting, + ActualVal: model.FakeSetting, + }, + }, + "", + }, + { + "sensitive FileSettings.AmazonS3SecretAccessKey", + func() *model.Config { + cfg := defaultConfigGen() + cfg.FileSettings.AmazonS3SecretAccessKey = model.NewString("base") + return cfg + }(), + func() *model.Config { + cfg := defaultConfigGen() + cfg.FileSettings.AmazonS3SecretAccessKey = model.NewString("actual") + return cfg + }(), + ConfigDiffs{ + { + Path: "FileSettings.AmazonS3SecretAccessKey", + BaseVal: model.FakeSetting, + ActualVal: model.FakeSetting, + }, + }, + "", + }, + { + "sensitive SqlSettings.DataSource", + func() *model.Config { + cfg := defaultConfigGen() + cfg.SqlSettings.DataSource = model.NewString("base") + return cfg + }(), + func() *model.Config { + cfg := defaultConfigGen() + cfg.SqlSettings.DataSource = model.NewString("actual") + return cfg + }(), + ConfigDiffs{ + { + Path: "SqlSettings.DataSource", + BaseVal: model.FakeSetting, + ActualVal: model.FakeSetting, + }, + }, + "", + }, + { + "sensitive SqlSettings.AtRestEncryptKey", + func() *model.Config { + cfg := defaultConfigGen() + cfg.SqlSettings.AtRestEncryptKey = model.NewString("base") + return cfg + }(), + func() *model.Config { + cfg := defaultConfigGen() + cfg.SqlSettings.AtRestEncryptKey = model.NewString("actual") + return cfg + }(), + ConfigDiffs{ + { + Path: "SqlSettings.AtRestEncryptKey", + BaseVal: model.FakeSetting, + ActualVal: model.FakeSetting, + }, + }, + "", + }, + { + "sensitive SqlSettings.DataSourceReplicas", + func() *model.Config { + cfg := defaultConfigGen() + cfg.SqlSettings.DataSourceReplicas = []string{ + "ds0", + "ds1", + } + return cfg + }(), + func() *model.Config { + cfg := defaultConfigGen() + cfg.SqlSettings.DataSourceReplicas = []string{ + "ds0", + "ds1", + "ds2", + } + return cfg + }(), + ConfigDiffs{ + { + Path: "SqlSettings.DataSourceReplicas", + BaseVal: model.FakeSetting, + ActualVal: model.FakeSetting, + }, + }, + "", + }, + { + "sensitive SqlSettings.DataSourceSearchReplicas", + func() *model.Config { + cfg := defaultConfigGen() + cfg.SqlSettings.DataSourceSearchReplicas = []string{ + "ds0", + "ds1", + } + return cfg + }(), + func() *model.Config { + cfg := defaultConfigGen() + cfg.SqlSettings.DataSourceSearchReplicas = []string{ + "ds0", + "ds1", + "ds2", + } + return cfg + }(), + ConfigDiffs{ + { + Path: "SqlSettings.DataSourceSearchReplicas", + BaseVal: model.FakeSetting, + ActualVal: model.FakeSetting, + }, + }, + "", + }, + { + "sensitive EmailSettings.SMTPPassword", + func() *model.Config { + cfg := defaultConfigGen() + cfg.EmailSettings.SMTPPassword = model.NewString("base") + return cfg + }(), + func() *model.Config { + cfg := defaultConfigGen() + cfg.EmailSettings.SMTPPassword = model.NewString("actual") + return cfg + }(), + ConfigDiffs{ + { + Path: "EmailSettings.SMTPPassword", + BaseVal: model.FakeSetting, + ActualVal: model.FakeSetting, + }, + }, + "", + }, + { + "sensitive GitLabSettings.Secret", + func() *model.Config { + cfg := defaultConfigGen() + cfg.GitLabSettings.Secret = model.NewString("base") + return cfg + }(), + func() *model.Config { + cfg := defaultConfigGen() + cfg.GitLabSettings.Secret = model.NewString("actual") + return cfg + }(), + ConfigDiffs{ + { + Path: "GitLabSettings.Secret", + BaseVal: model.FakeSetting, + ActualVal: model.FakeSetting, + }, + }, + "", + }, + { + "sensitive GoogleSettings.Secret", + func() *model.Config { + cfg := defaultConfigGen() + cfg.GoogleSettings.Secret = model.NewString("base") + return cfg + }(), + func() *model.Config { + cfg := defaultConfigGen() + cfg.GoogleSettings.Secret = model.NewString("actual") + return cfg + }(), + ConfigDiffs{ + { + Path: "GoogleSettings.Secret", + BaseVal: model.FakeSetting, + ActualVal: model.FakeSetting, + }, + }, + "", + }, + { + "sensitive Office365Settings.Secret", + func() *model.Config { + cfg := defaultConfigGen() + cfg.Office365Settings.Secret = model.NewString("base") + return cfg + }(), + func() *model.Config { + cfg := defaultConfigGen() + cfg.Office365Settings.Secret = model.NewString("actual") + return cfg + }(), + ConfigDiffs{ + { + Path: "Office365Settings.Secret", + BaseVal: model.FakeSetting, + ActualVal: model.FakeSetting, + }, + }, + "", + }, + { + "sensitive OpenIdSettings.Secret", + func() *model.Config { + cfg := defaultConfigGen() + cfg.OpenIdSettings.Secret = model.NewString("base") + return cfg + }(), + func() *model.Config { + cfg := defaultConfigGen() + cfg.OpenIdSettings.Secret = model.NewString("actual") + return cfg + }(), + ConfigDiffs{ + { + Path: "OpenIdSettings.Secret", + BaseVal: model.FakeSetting, + ActualVal: model.FakeSetting, + }, + }, + "", + }, + { + "sensitive ElasticsearchSettings.Password", + func() *model.Config { + cfg := defaultConfigGen() + cfg.ElasticsearchSettings.Password = model.NewString("base") + return cfg + }(), + func() *model.Config { + cfg := defaultConfigGen() + cfg.ElasticsearchSettings.Password = model.NewString("actual") + return cfg + }(), + ConfigDiffs{ + { + Path: "ElasticsearchSettings.Password", + BaseVal: model.FakeSetting, + ActualVal: model.FakeSetting, + }, + }, + "", + }, + { + "sensitive MessageExportSettings.GlobalRelaySettings", + func() *model.Config { + cfg := defaultConfigGen() + cfg.MessageExportSettings.GlobalRelaySettings = &model.GlobalRelayMessageExportSettings{ + SMTPUsername: model.NewString("base"), + SMTPPassword: model.NewString("base"), + EmailAddress: model.NewString("base"), + } + return cfg + }(), + func() *model.Config { + cfg := defaultConfigGen() + cfg.MessageExportSettings.GlobalRelaySettings = &model.GlobalRelayMessageExportSettings{ + SMTPUsername: model.NewString("actual"), + SMTPPassword: model.NewString("actual"), + EmailAddress: model.NewString("actual"), + } + return cfg + }(), + ConfigDiffs{ + { + Path: "MessageExportSettings.GlobalRelaySettings.SMTPUsername", + BaseVal: model.FakeSetting, + ActualVal: model.FakeSetting, + }, + { + Path: "MessageExportSettings.GlobalRelaySettings.SMTPPassword", + BaseVal: model.FakeSetting, + ActualVal: model.FakeSetting, + }, + { + Path: "MessageExportSettings.GlobalRelaySettings.EmailAddress", + BaseVal: model.FakeSetting, + ActualVal: model.FakeSetting, + }, + }, + "", + }, + { + "sensitive ServiceSettings.GfycatAPISecret", + func() *model.Config { + cfg := defaultConfigGen() + cfg.ServiceSettings.GfycatAPISecret = model.NewString("base") + return cfg + }(), + func() *model.Config { + cfg := defaultConfigGen() + cfg.ServiceSettings.GfycatAPISecret = model.NewString("actual") + return cfg + }(), + ConfigDiffs{ + { + Path: "ServiceSettings.GfycatAPISecret", + BaseVal: model.FakeSetting, + ActualVal: model.FakeSetting, + }, + }, + "", + }, + { + "sensitive ServiceSettings.SplitKey", + func() *model.Config { + cfg := defaultConfigGen() + cfg.ServiceSettings.SplitKey = model.NewString("base") + return cfg + }(), + func() *model.Config { + cfg := defaultConfigGen() + cfg.ServiceSettings.SplitKey = model.NewString("actual") + return cfg + }(), + ConfigDiffs{ + { + Path: "ServiceSettings.SplitKey", + BaseVal: model.FakeSetting, + ActualVal: model.FakeSetting, + }, + }, + "", + }, + { + "plugin config", + defaultConfigGen(), + func() *model.Config { + cfg := defaultConfigGen() + cfg.PluginSettings.Plugins = map[string]map[string]interface{}{ + "com.mattermost.newplugin": { + "key": true, + }, + } + return cfg + }(), + ConfigDiffs{ + { + Path: "PluginSettings.Plugins", + BaseVal: model.FakeSetting, + ActualVal: model.FakeSetting, + }, + }, + "", + }, + } + + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + diffs, err := Diff(tc.base, tc.actual) + if tc.err != "" { + require.EqualError(t, err, tc.err) + require.Nil(t, diffs) + } else { + require.NoError(t, err) + } + require.Equal(t, tc.diffs, diffs.Sanitize()) + }) + } +} + func TestDiff(t *testing.T) { tcs := []struct { name string @@ -110,9 +578,9 @@ func TestDiff(t *testing.T) { &model.Config{}, ConfigDiffs{ { - "", - *defaultConfigGen(), - model.Config{}, + Path: "", + BaseVal: *defaultConfigGen(), + ActualVal: model.Config{}, }, }, "", @@ -123,9 +591,9 @@ func TestDiff(t *testing.T) { defaultConfigGen(), ConfigDiffs{ { - "", - model.Config{}, - *defaultConfigGen(), + Path: "", + BaseVal: model.Config{}, + ActualVal: *defaultConfigGen(), }, }, "", diff --git a/model/config.go b/model/config.go index 5699e068d3..a2eda9ff30 100644 --- a/model/config.go +++ b/model/config.go @@ -3746,9 +3746,11 @@ func (o *Config) Sanitize() { *o.LdapSettings.BindPassword = FakeSetting } - *o.FileSettings.PublicLinkSalt = FakeSetting + if o.FileSettings.PublicLinkSalt != nil { + *o.FileSettings.PublicLinkSalt = FakeSetting + } - if *o.FileSettings.AmazonS3SecretAccessKey != "" { + if o.FileSettings.AmazonS3SecretAccessKey != nil && *o.FileSettings.AmazonS3SecretAccessKey != "" { *o.FileSettings.AmazonS3SecretAccessKey = FakeSetting } @@ -3756,7 +3758,7 @@ func (o *Config) Sanitize() { *o.EmailSettings.SMTPPassword = FakeSetting } - if *o.GitLabSettings.Secret != "" { + if o.GitLabSettings.Secret != nil && *o.GitLabSettings.Secret != "" { *o.GitLabSettings.Secret = FakeSetting } @@ -3772,10 +3774,17 @@ func (o *Config) Sanitize() { *o.OpenIdSettings.Secret = FakeSetting } - *o.SqlSettings.DataSource = FakeSetting - *o.SqlSettings.AtRestEncryptKey = FakeSetting + if o.SqlSettings.DataSource != nil { + *o.SqlSettings.DataSource = FakeSetting + } - *o.ElasticsearchSettings.Password = FakeSetting + if o.SqlSettings.AtRestEncryptKey != nil { + *o.SqlSettings.AtRestEncryptKey = FakeSetting + } + + if o.ElasticsearchSettings.Password != nil { + *o.ElasticsearchSettings.Password = FakeSetting + } for i := range o.SqlSettings.DataSourceReplicas { o.SqlSettings.DataSourceReplicas[i] = FakeSetting @@ -3785,7 +3794,9 @@ func (o *Config) Sanitize() { o.SqlSettings.DataSourceSearchReplicas[i] = FakeSetting } - if o.MessageExportSettings.GlobalRelaySettings.SMTPPassword != nil && *o.MessageExportSettings.GlobalRelaySettings.SMTPPassword != "" { + if o.MessageExportSettings.GlobalRelaySettings != nil && + o.MessageExportSettings.GlobalRelaySettings.SMTPPassword != nil && + *o.MessageExportSettings.GlobalRelaySettings.SMTPPassword != "" { *o.MessageExportSettings.GlobalRelaySettings.SMTPPassword = FakeSetting } @@ -3793,7 +3804,9 @@ func (o *Config) Sanitize() { *o.ServiceSettings.GfycatAPISecret = FakeSetting } - *o.ServiceSettings.SplitKey = FakeSetting + if o.ServiceSettings.SplitKey != nil { + *o.ServiceSettings.SplitKey = FakeSetting + } } // structToMapFilteredByTag converts a struct into a map removing those fields that has the tag passed