From 70fe2abea67e05f124201533945396289025c32e Mon Sep 17 00:00:00 2001 From: Ben Schumacher Date: Thu, 12 Sep 2024 19:23:57 +0200 Subject: [PATCH] [MM-57194] Allow plugins to mark setting fields as secret (#27986) Co-authored-by: Claudio Costa --- server/channels/api4/config.go | 4 +- server/channels/api4/config_local.go | 2 +- server/channels/app/app_iface.go | 2 + server/channels/app/config.go | 16 +- .../app/opentracing/opentracing_layer.go | 15 ++ server/channels/app/plugin.go | 19 ++ server/channels/app/plugin_api_test.go | 33 +-- .../main.go | 17 ++ server/config/diff.go | 11 +- server/config/diff_test.go | 4 +- server/public/model/config.go | 36 +++- server/public/model/config_test.go | 193 +++++++++++++++++- server/public/model/manifest.go | 4 + 13 files changed, 311 insertions(+), 45 deletions(-) diff --git a/server/channels/api4/config.go b/server/channels/api4/config.go index 1c0707f8c50..2056ef4a4ae 100644 --- a/server/channels/api4/config.go +++ b/server/channels/api4/config.go @@ -205,7 +205,7 @@ func updateConfig(c *Context, w http.ResponseWriter, r *http.Request) { } auditRec.AddEventPriorState(&diffs) - newCfg.Sanitize() + c.App.SanitizedConfig(newCfg) cfg, err = config.Merge(&model.Config{}, newCfg, &utils.MergeConfig{ StructFieldFilter: func(structField reflect.StructField, base, patch reflect.Value) bool { @@ -355,7 +355,7 @@ func patchConfig(c *Context, w http.ResponseWriter, r *http.Request) { auditRec.AddEventPriorState(&diffs) - newCfg.Sanitize() + c.App.SanitizedConfig(newCfg) auditRec.Success() diff --git a/server/channels/api4/config_local.go b/server/channels/api4/config_local.go index d2e688cb181..c3959023945 100644 --- a/server/channels/api4/config_local.go +++ b/server/channels/api4/config_local.go @@ -77,7 +77,7 @@ func localUpdateConfig(c *Context, w http.ResponseWriter, r *http.Request) { } auditRec.AddEventPriorState(&diffs) - newCfg.Sanitize() + c.App.SanitizedConfig(newCfg) auditRec.Success() c.LogAudit("updateConfig") diff --git a/server/channels/app/app_iface.go b/server/channels/app/app_iface.go index df8afc7fb2f..f350d5383e9 100644 --- a/server/channels/app/app_iface.go +++ b/server/channels/app/app_iface.go @@ -320,6 +320,8 @@ type AppIface interface { // RevokeSessionsFromAllUsers will go through all the sessions active // in the server and revoke them RevokeSessionsFromAllUsers() *model.AppError + // SanitizedConfig sanitizes a given configuration for a system admin without any secrets. + SanitizedConfig(cfg *model.Config) // SaveConfig replaces the active configuration, optionally notifying cluster peers. SaveConfig(newCfg *model.Config, sendConfigChangeClusterMessage bool) (*model.Config, *model.Config, *model.AppError) // SearchAllChannels returns a list of channels, the total count of the results of the search (if the paginate search option is true), and an error. diff --git a/server/channels/app/config.go b/server/channels/app/config.go index d079f152194..07ea00cf56e 100644 --- a/server/channels/app/config.go +++ b/server/channels/app/config.go @@ -214,11 +214,25 @@ func (a *App) GetConfigFile(name string) ([]byte, error) { // GetSanitizedConfig gets the configuration for a system admin without any secrets. func (a *App) GetSanitizedConfig() *model.Config { cfg := a.Config().Clone() - cfg.Sanitize() + + a.SanitizedConfig(cfg) return cfg } +// SanitizedConfig sanitizes a given configuration for a system admin without any secrets. +func (a *App) SanitizedConfig(cfg *model.Config) { + manifests, err := a.getPluginManifests() + if err != nil { + // GetPluginManifests might error, e.g. when plugins are disabled. + // Sanitize all plugin settings in this case. + cfg.Sanitize(nil) + return + } + + cfg.Sanitize(manifests) +} + // GetEnvironmentConfig returns a map of configuration keys whose values have been overridden by an environment variable. // If filter is not nil and returns false for a struct field, that field will be omitted. func (a *App) GetEnvironmentConfig(filter func(reflect.StructField) bool) map[string]any { diff --git a/server/channels/app/opentracing/opentracing_layer.go b/server/channels/app/opentracing/opentracing_layer.go index e1ac4e11ba5..dd2a8759fae 100644 --- a/server/channels/app/opentracing/opentracing_layer.go +++ b/server/channels/app/opentracing/opentracing_layer.go @@ -15239,6 +15239,21 @@ func (a *OpenTracingAppLayer) SanitizeTeams(session model.Session, teams []*mode return resultVar0 } +func (a *OpenTracingAppLayer) SanitizedConfig(cfg *model.Config) { + origCtx := a.ctx + span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.SanitizedConfig") + + a.ctx = newCtx + a.app.Srv().Store().SetContext(newCtx) + defer func() { + a.app.Srv().Store().SetContext(origCtx) + a.ctx = origCtx + }() + + defer span.Finish() + a.app.SanitizedConfig(cfg) +} + func (a *OpenTracingAppLayer) SaveAcknowledgementForPost(c request.CTX, postID string, userID string) (*model.PostAcknowledgement, *model.AppError) { origCtx := a.ctx span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.SaveAcknowledgementForPost") diff --git a/server/channels/app/plugin.go b/server/channels/app/plugin.go index 952cb5b5394..d8c0c263568 100644 --- a/server/channels/app/plugin.go +++ b/server/channels/app/plugin.go @@ -378,6 +378,25 @@ func (ch *Channels) ShutDownPlugins() { } } +func (a *App) getPluginManifests() ([]*model.Manifest, error) { + pluginsEnvironment := a.GetPluginsEnvironment() + if pluginsEnvironment == nil { + return nil, model.NewAppError("GetPluginManifests", "app.plugin.disabled.app_error", nil, "", http.StatusNotImplemented) + } + + plugins, err := pluginsEnvironment.Available() + if err != nil { + return nil, errors.Wrap(err, "failed to get list of available plugins") + } + + manifests := make([]*model.Manifest, len(plugins)) + for i := range plugins { + manifests[i] = plugins[i].Manifest + } + + return manifests, nil +} + func (a *App) GetActivePluginManifests() ([]*model.Manifest, *model.AppError) { pluginsEnvironment := a.GetPluginsEnvironment() if pluginsEnvironment == nil { diff --git a/server/channels/app/plugin_api_test.go b/server/channels/app/plugin_api_test.go index 28f016e66f5..f8ab754c29c 100644 --- a/server/channels/app/plugin_api_test.go +++ b/server/channels/app/plugin_api_test.go @@ -813,43 +813,12 @@ func TestPluginAPISavePluginConfig(t *testing.T) { assert.Equal(t, expectedConfiguration, savedConfiguration) } -func TestPluginAPIGetPluginConfig(t *testing.T) { - th := Setup(t) - defer th.TearDown() - - manifest := &model.Manifest{ - Id: "pluginid", - SettingsSchema: &model.PluginSettingsSchema{ - Settings: []*model.PluginSetting{ - {Key: "MyStringSetting", Type: "text"}, - {Key: "MyIntSetting", Type: "text"}, - {Key: "MyBoolSetting", Type: "bool"}, - }, - }, - } - - api := NewPluginAPI(th.App, th.Context, manifest) - - pluginConfigJsonString := `{"mystringsetting": "str", "myintsetting": 32, "myboolsetting": true}` - var pluginConfig map[string]any - - err := json.Unmarshal([]byte(pluginConfigJsonString), &pluginConfig) - require.NoError(t, err) - - th.App.UpdateConfig(func(cfg *model.Config) { - cfg.PluginSettings.Plugins["pluginid"] = pluginConfig - }) - - savedPluginConfig := api.GetPluginConfig() - assert.Equal(t, pluginConfig, savedPluginConfig) -} - func TestPluginAPILoadPluginConfiguration(t *testing.T) { th := Setup(t) defer th.TearDown() var pluginJson map[string]any - err := json.Unmarshal([]byte(`{"mystringsetting": "str", "MyIntSetting": 32, "myboolsetting": true}`), &pluginJson) + err := json.Unmarshal([]byte(`{"mystringsetting": "str", "MyIntSetting": 32, "myBoolsetting": true}`), &pluginJson) require.NoError(t, err) th.App.UpdateConfig(func(cfg *model.Config) { diff --git a/server/channels/app/plugin_api_tests/manual.test_load_configuration_plugin/main.go b/server/channels/app/plugin_api_tests/manual.test_load_configuration_plugin/main.go index 6f2c1a2a551..4b706d5b117 100644 --- a/server/channels/app/plugin_api_tests/manual.test_load_configuration_plugin/main.go +++ b/server/channels/app/plugin_api_tests/manual.test_load_configuration_plugin/main.go @@ -34,6 +34,7 @@ func (p *MyPlugin) OnConfigurationChange() error { } func (p *MyPlugin) MessageWillBePosted(_ *plugin.Context, _ *model.Post) (*model.Post, string) { + // Test API.LoadPluginConfiguration if p.configuration.MyStringSetting != "str" { return nil, "MyStringSetting has invalid value" } @@ -43,6 +44,22 @@ func (p *MyPlugin) MessageWillBePosted(_ *plugin.Context, _ *model.Post) (*model if !p.configuration.MyBoolSetting { return nil, "MyBoolSetting has invalid value" } + + // Test API.GetPluginConfig + pc := p.API.GetPluginConfig() + if pc == nil { + return nil, "GetPluginConfig returned nil" + } + if pc["mystringsetting"] != "str" { + return nil, fmt.Sprintf("MyStringSetting has invalid value: %v", pc["mystringsetting"]) + } + if pc["MyIntSetting"] != float64(32) { + return nil, fmt.Sprintf("MyIntSetting has invalid value: %v", pc["MyIntSetting"]) + } + if pc["myBoolsetting"] != true { + return nil, fmt.Sprintf("MyBoolSetting has invalid value: %v", pc["myBoolsetting"]) + } + return nil, "OK" } diff --git a/server/config/diff.go b/server/config/diff.go index 08535eac535..cdee5d6d52e 100644 --- a/server/config/diff.go +++ b/server/config/diff.go @@ -60,21 +60,24 @@ var configSensitivePaths = map[string]bool{ // Sanitize replaces sensitive config values in the diff with asterisks filled strings. func (cd ConfigDiffs) Sanitize() ConfigDiffs { if len(cd) == 1 { + // PluginSettings.Plugins gets sanitized anyway, so there is no need to use the plugin manifests here. + var pluginManifests []*model.Manifest + cfgPtr, ok := cd[0].BaseVal.(*model.Config) if ok { - cfgPtr.Sanitize() + cfgPtr.Sanitize(pluginManifests) } cfgPtr, ok = cd[0].ActualVal.(*model.Config) if ok { - cfgPtr.Sanitize() + cfgPtr.Sanitize(pluginManifests) } cfgVal, ok := cd[0].BaseVal.(model.Config) if ok { - cfgVal.Sanitize() + cfgVal.Sanitize(pluginManifests) } cfgVal, ok = cd[0].ActualVal.(model.Config) if ok { - cfgVal.Sanitize() + cfgVal.Sanitize(pluginManifests) } } diff --git a/server/config/diff_test.go b/server/config/diff_test.go index 73300799d34..c794ba0765b 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() + cfg.Sanitize(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() + cfg.Sanitize(nil) return *cfg }(), }, diff --git a/server/public/model/config.go b/server/public/model/config.go index 6d64caa9a9e..e6791798262 100644 --- a/server/public/model/config.go +++ b/server/public/model/config.go @@ -3197,6 +3197,38 @@ func (s *PluginSettings) SetDefaults(ls LogSettings) { } } +// Sanitize cleans up the plugin settings by removing any sensitive information. +// It does so by checking if the setting is marked as secret in the plugin manifest. +// If it is, the setting is replaced with a fake value. +// If a plugin is no longer installed, all settings of it's are sanitized. +// If the list of manifests in nil, i.e. plugins are disabled, all settings are sanitized. +func (s *PluginSettings) Sanitize(pluginManifests []*Manifest) { + manifestMap := make(map[string]*Manifest, len(pluginManifests)) + + for _, manifest := range pluginManifests { + manifestMap[manifest.Id] = manifest + } + + for id, settings := range s.Plugins { + manifest := manifestMap[id] + + for key := range settings { + if manifest == nil { + // Sanitize plugin settings for plugins that are not installed + settings[key] = FakeSetting + continue + } + + for _, definedSetting := range manifest.SettingsSchema.Settings { + if definedSetting.Secret && strings.EqualFold(definedSetting.Key, key) { + settings[key] = FakeSetting + break + } + } + } + } +} + type WranglerSettings struct { PermittedWranglerRoles []string AllowedEmailDomain []string @@ -4396,7 +4428,7 @@ func (o *Config) GetSanitizeOptions() map[string]bool { return options } -func (o *Config) Sanitize() { +func (o *Config) Sanitize(pluginManifests []*Manifest) { if o.LdapSettings.BindPassword != nil && *o.LdapSettings.BindPassword != "" { *o.LdapSettings.BindPassword = FakeSetting } @@ -4462,6 +4494,8 @@ func (o *Config) Sanitize() { if o.ServiceSettings.SplitKey != nil { *o.ServiceSettings.SplitKey = FakeSetting } + + o.PluginSettings.Sanitize(pluginManifests) } // structToMapFilteredByTag converts a struct into a map removing those fields that has the tag passed diff --git a/server/public/model/config_test.go b/server/public/model/config_test.go index 3d7b7775c78..a1cdef22477 100644 --- a/server/public/model/config_test.go +++ b/server/public/model/config_test.go @@ -1387,7 +1387,7 @@ func TestConfigSanitize(t *testing.T) { QueryTimeLag: NewPointer("QueryTimeLag"), }} - c.Sanitize() + c.Sanitize(nil) assert.Equal(t, FakeSetting, *c.LdapSettings.BindPassword) assert.Equal(t, FakeSetting, *c.FileSettings.PublicLinkSalt) @@ -1409,12 +1409,201 @@ func TestConfigSanitize(t *testing.T) { t.Run("with default config", func(t *testing.T) { c := Config{} c.SetDefaults() - c.Sanitize() + c.Sanitize(nil) assert.Len(t, c.SqlSettings.ReplicaLagSettings, 0) }) } +func TestPluginSettingsSanitize(t *testing.T) { + plugins := map[string]map[string]any{ + "plugin.id": { + "somesetting": "some value", + "secrettext": "a secret", + "secretnumber": 123, + }, + "another.plugin": { + "somesetting": 456, + }, + } + + for name, tc := range map[string]struct { + manifests []*Manifest + expected map[string]map[string]any + }{ + "nil list of manifests": { + manifests: nil, + expected: map[string]map[string]any{ + "plugin.id": { + "somesetting": FakeSetting, + "secrettext": FakeSetting, + "secretnumber": FakeSetting, + }, + "another.plugin": { + "somesetting": FakeSetting, + }, + }, + }, + "empty list of manifests": { + manifests: []*Manifest{}, + expected: map[string]map[string]any{ + "plugin.id": { + "somesetting": FakeSetting, + "secrettext": FakeSetting, + "secretnumber": FakeSetting, + }, + "another.plugin": { + "somesetting": FakeSetting, + }, + }, + }, + "one plugin installed": { + manifests: []*Manifest{ + { + Id: "plugin.id", + SettingsSchema: &PluginSettingsSchema{ + Settings: []*PluginSetting{ + { + Key: "somesetting", + Type: "text", + Secret: false, + }, + { + Key: "secrettext", + Type: "text", + Secret: true, + }, + { + Key: "secretnumber", + Type: "number", + Secret: true, + }, + }, + }, + }, + }, + expected: map[string]map[string]any{ + "plugin.id": { + "somesetting": "some value", + "secrettext": FakeSetting, + "secretnumber": FakeSetting, + }, + "another.plugin": { + "somesetting": FakeSetting, + }, + }, + }, + "two plugins installed": { + manifests: []*Manifest{ + { + Id: "plugin.id", + SettingsSchema: &PluginSettingsSchema{ + Settings: []*PluginSetting{ + { + Key: "somesetting", + Type: "text", + Secret: false, + }, + { + Key: "secrettext", + Type: "text", + Secret: true, + }, + { + Key: "secretnumber", + Type: "number", + Secret: true, + }, + }, + }, + }, + { + Id: "another.plugin", + SettingsSchema: &PluginSettingsSchema{ + Settings: []*PluginSetting{ + { + Key: "somesetting", + Type: "number", + Secret: false, + }, + }, + }, + }, + }, + expected: map[string]map[string]any{ + "plugin.id": { + "somesetting": "some value", + "secrettext": FakeSetting, + "secretnumber": FakeSetting, + }, + "another.plugin": { + "somesetting": 456, + }, + }, + }, + } { + t.Run(name, func(t *testing.T) { + name := name // TODO: Remove once go1.22 is used + tc := tc // TODO: Remove once go1.22 is used + + if name != "one plugin installed" { + return + } + + c := PluginSettings{} + c.SetDefaults(*NewLogSettings()) + c.Plugins = plugins + + c.Sanitize(tc.manifests) + + assert.Equal(t, tc.expected, c.Plugins, name) + }) + } + + t.Run("one plugin installed, two in the config", func(t *testing.T) { + c := PluginSettings{} + c.SetDefaults(*NewLogSettings()) + c.Plugins = plugins + + c.Sanitize([]*Manifest{ + { + Id: "plugin.id", + SettingsSchema: &PluginSettingsSchema{ + Settings: []*PluginSetting{ + { + Key: "somesetting", + Type: "text", + Secret: false, + }, + { + Key: "secrettext", + Type: "text", + Secret: true, + }, + { + Key: "secretnumber", + Type: "number", + Secret: true, + }, + }, + }, + }, + }) + + expected := map[string]map[string]any{ + "plugin.id": { + "somesetting": "some value", + "secrettext": FakeSetting, + "secretnumber": FakeSetting, + }, + "another.plugin": { + "somesetting": FakeSetting, + }, + } + assert.Equal(t, expected, c.Plugins) + }) +} + func TestConfigFilteredByTag(t *testing.T) { c := Config{} c.SetDefaults() diff --git a/server/public/model/manifest.go b/server/public/model/manifest.go index 4d337f1d480..87443603fbf 100644 --- a/server/public/model/manifest.go +++ b/server/public/model/manifest.go @@ -89,6 +89,10 @@ type PluginSetting struct { // and the opposite environment is running the plugin, the setting will be hidden in the admin console UI. // Note that this functionality is entirely client-side, so the plugin needs to handle the case of invalid submissions. Hosting string `json:"hosting"` + + // If true, the setting is sanitized before showing it in the System Console or returning it via the API. + // This is useful for settings that contain sensitive information. + Secret bool `json:"secret"` } type PluginSettingsSection struct {