Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
mickmister opened this issue May 30, 2024 · 5 comments
Open

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

mickmister opened this issue May 30, 2024 · 5 comments

Comments

@mickmister
Copy link
Contributor

Referencing #46 (comment)

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
Copy link
Contributor Author

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

  • Plugin implements the ConfigurationWillBeSaved hook, which requires server version 8.0 (Any ideas if this version is applicable for this customer?)
  • If AWS secret exists in the plugin config uncensored during this hook:
    • Store this value in the kv store
    • Overwrite the value in the config to be censored/asterisks
  • When the plugin needs to use the custom S3 settings, it will fetch the secret key from the kv store instead of using the censored version in the config

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

@wiggin77
Copy link
Member

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

@mickmister
Copy link
Contributor Author

@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
Copy link
Member

  • 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
Copy link
Contributor

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)

@mickmister mickmister removed their assignment Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants