mattermost / mattermost-plugin-legal-hold

Plugin to create and manage legal holds in Mattermost
Other
5 stars 2 forks source link

S3 bucket creds should be encrypted in transport to client #54

Open mickmister opened 1 month ago

mickmister commented 1 month ago

Referencing https://github.com/mattermost/mattermost-plugin-legal-hold/pull/46#discussion_r1621041207

The way the server config settings work, there's no way to let the plugin decide how the settings are stored, so they will always be stored and transported as plaintext. Ideally, we can have the plugin send the secret key to the client encrypted.

mickmister commented 1 month ago

@wiggin77 @fmartingr The way the server is handling this is a manual sanitization process before the config is returned to the client https://github.com/mattermost/mattermost/blob/b86ba51efd4f945f52a50d92e669e3d67a9bf872/server/public/model/config.go#L4341. Plugins don't have the same capability of sanitizing the config before it is sent to the client.

In similar situations ProdSec has suggested we can add a capability to declare a given plugin setting as "secret", which would allow the server to sanitize this upon fetching the config. That seems like the "correct" solution here, though that would require a server upgrade for the customer to the latest version. Also, this S3 bucket setting is currently part of a custom setting atm, with its own custom React component, to support testing the connection from the system console. I'm not sure that would fit well within this "secret" setting model. We would likely have to put the AWS secret key in its own plugin setting for that.


An alternative would be to use the KV store in some capacity, since the plugin has control over when those values are used/exposed. If we want to go down that route, I'm thinking it could work like this:

Then when the config is fetched by the client, the secret is not exposed to the client. Thoughts on this?

wiggin77 commented 1 month ago

@mickmister why can't the plugin API call sanitize before sending the config to clients?

mickmister commented 1 month ago

@wiggin77 The plugin isn't given the chance to do so, since the plugin is not contacted by the server for any reason when the client fetches the config. The server only does its own sanitization.

When the admin visits the system console, the entire (sanitized) server config is sent to the client. Additionally, when a given system console config page form (e.g. the plugin config form) is submitted, the entire server config is sent to be saved. Just pointing this out to provide overall context on how the config fetch/submit works.

In general, plugin config settings are unconditionally delivered to the client "as is", and plugins have no way of changing what values are sent to the client at that time. Any censoring/sanitizing of plugin settings needs to be done at-rest before the config is fetched from the client.

wiggin77 commented 1 month ago
  • which requires server version 8.0 (Any ideas if this version is applicable for this customer?)

There are multiple customers showing interest with different versions. That said, it's fine if custom S3 bucket is only available in 8+.

fmartingr commented 4 weeks ago

With custom settings being more and more prominent among plugins, is there an issue tracking this plugin API improvement of setting a configuration as "sensible" to be sanitized by the server automatically? Or should we provide helpers in the Plugin API directly so developers use so in each plugin? (I'd prefer the former, but unsure about the effort)