Disclosure of S3 secret access key

Well, my preference would be to standardize on the usual behavior – it should not appear even to the administrator; there should be a field where the administrator sets it, and it should load /blank/ instead of filled. Honestly, the only time I ever see click-to-reveal is when you’re dealing with something designed to store sets of credentials, not a single one, but click-to-reveal would resolve that half of it, yes.

Either way, though, the log entries that show setting changes that echo the password back in plaintext are… not a common behavior.

You can set them in the app.yml file and have them not show up in settings. Docker deployment with custom site_settings.yml provides hints

4 Likes

Well, yeah, and I can use an EC2 instance-role too and honestly I probably will be switching to that.

Do you have any thoughts on how we could improve here @mpalmer? It is a Discourse core value™ to be safe by default, so it is worth thinking about any small things we can improve here as we go.

There’s two separate issues here, to my mind:

  1. Logging changes to sensitive data: I think that definitely needs to be fixed. We scrub passwords and API keys from logs, the same thing should definitely apply to S3 secret keys, IMO.
  2. Shoulder-surfing creds: Leaving the box blank, even though there’s a value set, seems like a UX nightmare – I predict “I entered the S3 key and now it’s gone!” support requests if we did that. It is handy to be able to check on an entered value to make sure it’s right – I’ve noticed a steady increase in the number of password entry boxes that have “click to show” ability. That seems like a reasonable middle ground to me – hide it behind dots or something by default, with a “click to show” option.
8 Likes

That’d settle it for me, yes.

I think we should do part 1 in the 2.0 release, @techapj can you assist? It seems relatively straightforward, if we are doing it elsewhere? Let me know if it is not.

3 Likes

While we are at it, should we scrub all these “secret” settings from staff logs?

  • google_oauth2_client_secret
  • twitter_consumer_secret
  • instagram_consumer_secret
  • facebook_app_secret
  • github_client_secret
  • s3_secret_access_key

The previous and new value will be replaced with [FILTERED], just like what we do in /logs. Example:

5 Likes

Yes probably, I think the same logic would apply.

3 Likes

Part 1 done in:

https://github.com/discourse/discourse/commit/abcb6af8f953101a1f20b5e231f52cc81867bd38

3 Likes

I guess the logic should be, screen any setting from logs that has the string “secret” in it? Or _secret if you want to be careful.

2 Likes

Got it. Will amend the logic to scrub any setting that has _secret in the name.

Since I’m sure more sensitive settings will appear in the future, and plugins may add some also, would it be particularly tricky to add the ability to mark “sensitive” site settings as such in site_settings.yml?

4 Likes

Done in:

https://github.com/discourse/discourse/commit/8d6a9eb51114b35cab006ed284240e8c84dd3276

As per the current logic, these 8 settings will be scrubbed:

  • google_oauth2_client_secret
  • twitter_consumer_secret
  • instagram_consumer_secret
  • facebook_app_secret
  • github_client_secret
  • s3_secret_access_key
  • push_api_secret_key
  • sso_secret

Added on my list.

6 Likes

Wow, thanks a lot. Just in time for me to… switch to instance-role credentials. :smiley:

But seriously, thanks for getting it done!

1 Like

I presume you’re familiar with the security considerations around instance role creds and SSRF vulnerabilities? It’s quite a pity AWS doesn’t protect the metadata store with a magic header like GCP does.

Yeah – I’ve got a thread elsewhere where I’m being persnickety about exactly what the permissions the S3 user should have. (Though now that I’m reminded this is a thing, there’s no reason I shouldn’t add an IP blocker to the role.) Far as security goes, anybody able to execute code as Discourse’s user can, well, execute code as Discourse’s user, so they’re as likely to be able to grab the credentials from Discourse as they are the instance metadata.

Exactly, and then @techAPJ you can use this site setting type to automatically set the type of the input to password to hide the content and add a Show value button that will toggle the type of the input :wink:

6 Likes

This is now done via:

https://github.com/discourse/discourse/commit/46fc57222f24add74d2d075eb36663565967622c

Demo:

11 Likes