Add option to disallow unencrypted secrets

[Luzifer]

As a operator of a public instance of OTS I want to ensure I really don't get plain text secrets in my store someone could blame me to have read.

Therefore this PR introduces an option to reject plain-text secrets by checking whether there is a base64 encoded OpenSSL header in the submitted secret. The option by default is disabled in order to keep the previous behavior and can be enabled through the customizations.

[Luzifer]

@sorcix PTAL

[sorcix]

Looks good to me.

You could replace the secretContainsCryptoHeader function with bytes.HasPrefix(secret, []byte("U2FsdGVkX1")). This is a way faster check for Salted_, if you don't mind not checking for two underscores.

[sorcix]

People that use OpenSSL directly may not always have the Salted__ prefix according to this StackOverflow post. So maybe the documentation for rejectUnencryptedSecrets should mention that the check is only correct when using the web interface.

That being said.. encrypted data is supposed to look random, and so are many (generated) secrets. So detecting encryption (other than the one you control yourself on the web interface) is pretty much impossible. Having this option may give a false sense of security.

[Luzifer]

Hmmm interesting thoughts… 🤔

Given OTS always followed the path to be compatible with openssl enc -aes-256-cbc I'd say lets disregard other means of encryption: The frontend is not able to work with it.

Looking at the post the header is only missing when used with the -S flag in OpenSSL 3.0.x which is IMHO a very unsafe way to use it as a deterministic salt is used. The frontend can't deal with that too. So rejecting that wouldn't that bad too…

Regarding the possibility a plain text secret could start with U2FsdGVkX1 I'd say the chance is low enough to let it slip through… …I mean, we're talking about a-zA-Z0-9 charset, so a chance of 1:839299365868340224 if I didn't miscalculate. (Of course if someone intentionally creates a secret starting that way they just can't be helped…)

In the end what I'm trying to do is to make a "best effort" to keep the admins of public instances safe of accidentally reading a plain text secret while doing maintenance on the data store. When talking about the safety of the data the user submitting the data should have a big interest not to send unencrypted data (which they don't but that's another story)…

Will change the check.

[tchbla]

IMHO the name of this option should be changed. The name "rejectUnencryptedSecrets" might lead an uninformed reader (e.g. a ciso...) to believe that it somehow technically prevents the storage of unencrypted secrets. However, that is not the case, and as far as I know, there is no way to ensure that without decryption, which would require submitting the passphrase - a step we strongly want to avoid. Therefore, in consideration of secure design, I would recommend naming that option something like "requireCryptoHeader."

[sorcix]

I'm still not 100% sure about what problem this MR fixes.

I want to ensure I really don't get plain text secrets in my store someone could blame me to have read.

In this case, the person that blames you should have created an encrypted secret in the first place.

If you want to fix the "I was debugging Redis and saw an unencrypted secret by accident" or "someone stole my redis dump and saw an unencrypted secret" you could add another layer of encryption server-side. Even a harcoded server-side encryption key would solve the "oops, I saw this by accident" case.

Adding server-side encryption does not prevent the story in your original post, though. An administrator would know that server-side encryption key and could decrypt the "plaintext" secrets. However, the encryption detection in this MR also doesn't work when the administrator adds a proxy in front of OTS and intercepts the secrets there. OTS would not store the secret, but at that point it has already leaked. So if one really wants to read a secret, this is only a very small hurdle.

It all comes down to: the user should always encrypt their secrets. If they don't, there is no way to protect against a malicious system adminsitrator. If you use your own curl/bash/whatever client, it's your own responsibility to encrypt the secrets. If someone sends an unencrypted secret to OTS, they only have themselves to blame.

But then again, a malicious system administrator could also modify the web interface to not encrypt secrets. :shrug: They could also remove the check this MR adds. I'm not sure this adds any value, security wise. Might even make it worse: a system administrator could say "look, I enabled this security check" to shift the blame and steal the secret with a proxy anyway.

[Luzifer]

Hmm. Yeah, you got very good points there.

In the end OTS is built on the "don't trust the server" principle. If people use their own clients to push "secrets" into the instance without encryption they already gave away their secrets "for free". As long as they are sending encrypted secrets neither a leak of the storage nor an admin looking at stuff is an issue… …if they don't… …well, its on them.

Fully agree. This PR is pointless.