[MM-39597] Implement ConfigDiffs.Sanitize() (#18855)

* Implement ConfigDiffs.Sanitize() * Remove possibly sensitive info from logs
2021-10-28 14:07:35 +02:00
--- 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) {
--- 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()

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

--- 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(),
 				},
 			},
 			"",
--- 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