bitwarden / sm-kubernetes

Kubernetes Operator for Bitwarden Secrets Manager.
Other
11 stars 3 forks source link

Creating a BitwardenSecret loads all secrets in project into the destination secret #60

Open deefdragon opened 2 months ago

deefdragon commented 2 months ago

I have created a BitwardenSecret with one item in the map, and the end result secret has literally every secret from my secret manager in it (the key being the secret's UUID), which is a non-trivial number of items.

I have certain tooling that expects exact key format for the secret, and having a whole bunch of UUID keys in the secret will be problimatic for that tooling, let alone the security implications of having all those extra secret values lying around.

This line is one problem line, adding ALL secrets tothe output secret. This would be fine on its own were it cleaned up, but ApplySecretMap run directly after it on the secret, only updates the k8s secret to have a key/value pair with a sane name instead of the UUID.

A more sane approach would be to pass the secrets fetched directly to ApplySecretMap instead of setting it on the k8s secret in UpdateSecretValues.

deefdragon commented 2 months ago

After making the change mentioned, the tests failed. Fixing the tests, I have come to the conclusion that this is intended behavior? If so, I think this is not a good decision, and there should at minimum be an option to disable this functionality. in a manner of least privilage, Secrets should be exposed as little as possible, and this was exposing EVERY secret to a given namespace.

evanjarrett commented 1 month ago

Can confirm this is an issue, or rather, I believe this is unexpected behavior. If I'm filtering values, I would expect only the bwSecretId I specify to be exposed to the secret. Why would I want all secrets to be exposed to an application if it doesn't need them?

A quick way to fix this would be to move this delete call one line down outside of the enclosing if, so that when a map is specified, only the secrets with bwSecretId maps remain.

The only other work around would be to create a new machine account for each kubes secret you want to create, which I think is going to make this project not very scalable... I would already be past the free tier, and for even modest deployments I could see this going past the 20 account limit.

M4RC02U1F4A4 commented 3 weeks ago

Is there any news about a fix for this issue?

deefdragon commented 3 weeks ago

I've been running a custom variant of this that solves this problem and a few others, (and has proven stable), but it is brute force about the lot, so not ideal and I'm not ready to make a PR for it.

That and I need permission from my employer to contribute to open source projects due to certain contract shenanigans and I cant get that until I'm off leave.

evanjarrett commented 2 weeks ago

I put together my own fork with this feature too, it just deletes out everything else in the list. you can see the diff here (has some GHA changes so that I'm able to build it) https://github.com/evanjarrett/sm-kubernetes/commit/8481fd3a3c1143ed9eeea6fd711b956f0fade8ea

This does not seem like the original intent of this project, but I don't understand how you would want all your secrets stored in a single Secret object in Kubes.

M4RC02U1F4A4 commented 2 weeks ago

I agree that having a single secret makes no sense, I also don't understand why this choice

If a user needs more than one secret, it seems that currently the only option is to create a machine account for each sync (then going to give permissions to individual values rather than to a project), which is not viable, since even with the Enterprise plan the limit is 50 machine accounts

AndreaF17 commented 4 days ago

I agree as well, If we look at the security point of view....can any maintainer provide a feedback about it?