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

Add a new secrets driver with support for encryption using different encrypters #25118

Open
ivanfed0t0v opened this issue Jan 25, 2025 · 10 comments
Labels
kind/feature Categorizes issue or PR as related to a new feature.

Comments

@ivanfed0t0v
Copy link

Feature request description

I'd like to have a more generic driver to encrypt Podman secrets at rest with support for custom user-provided encrypters.
I'm also willing to work on the PR to add this driver but wanted to get your opinion on it first.

Suggest potential solution

The solution could be to create a file driver copy that, instead of storing secrets in plaintext, calls an encrypter (specified in the driver config) and stores the result in JSON. There could be some embedded encrypters, and it should be possible to add custom ones as a shell command.

I've made a proof-of-concept using the shell driver and systemd-creds as the encrypter. It works but there's too much overhead due to handling CLI options and file storage. With a generic driver, the systemd-creds command could be called directly without the need for that script.

Have you considered any alternatives?

Shell driver works, but requires too much boilerplate.

Additional context

Add any other context or screenshots about the feature request here.

@ivanfed0t0v ivanfed0t0v added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 25, 2025
@apollo13
Copy link
Contributor

Not a podman maintainer, but I have to say your solution looks nice and I think the overhead is kinda minimal if at all. FWIW your code might get easier if you where to store the secrets in a single file per secret, this might let get you away without locking. If you are afraid that the secret names are not file-system compatible you could base64 them etc…

@ivanfed0t0v
Copy link
Author

Yeah, I agree that overhead is small, but still it would be nice to not think about secrets storage and cli parsing. Also, having some built-in encrypters will make it easier for everyone to use.

As for the locking part, I went with it mostly for the sake of being compatible with the default file driver.

@Luap99
Copy link
Member

Luap99 commented Jan 27, 2025

See my other comment here #23624 (comment)

Unless we fix the step where we extract secretes in plain text into the container storage I am not sure how encrypted drivers help. That would just cause users to think that the secrets are protected even when they are not.

@ivanfed0t0v
Copy link
Author

With encryption at rest there's always going to be a problem that data could be read unencrypted while in use. No way around that. But it makes data unreadable when there are no containers running. And I think that's still better than nothing.

My use case is to run scheduled backups with containerized restic. It will run a backup for an hour every day. If password is stored with encrypted secrets, it means that password is protected 23 hours a day and gets exposed only for one hour.

Btw, is there a technical limitation why secrets are not stored in memory (tmpfs) or is it just something that noone implemented yet? If I use env variable, will it also be stored on disk at some point?

@apollo13
Copy link
Contributor

If I use env variable, will it also be stored on disk at some point?

Yes in the OCI config iirc

@Luap99
Copy link
Member

Luap99 commented Jan 27, 2025

With encryption at rest there's always going to be a problem that data could be read unencrypted while in use. No way around that. But it makes data unreadable when there are no containers running. And I think that's still better than nothing.

That is fair but I think saying secrets are encrypted as long as we put them on a persistent disk will just mislead users.

My use case is to run scheduled backups with containerized restic. It will run a backup for an hour every day. If password is stored with encrypted secrets, it means that password is protected 23 hours a day and gets exposed only for one hour.

Btw, is there a technical limitation why secrets are not stored in memory (tmpfs) or is it just something that noone implemented yet? If I use env variable, will it also be stored on disk at some point?

For file based secretes it should be simple the extract them onto tmpfs and use that file as base mount. The thing is though that is just not how the code was designed. It has been a while since I looked at it but my understanding is that we extract the secret in the persistent container store (i.e. under graphroot). Then they stay there until the container is removed even if the secret is removed.

Now it should be easy enough to load the secret on each start instead and put it on tmpfs but this would also be a significant behavior change that we cannot just do like that.

If I use env variable, will it also be stored on disk at some point?

Yes in the OCI config iirc

Yes I think this is correct. The oci spec is used in many places so I am not sure if moving it directly yo tmpfs would break things. But technically it should be at least possible to copy the final spec to tmpfs and only there add the secret env vars I assume.

@ivanfed0t0v
Copy link
Author

That is fair but I think saying secrets are encrypted as long as we put them on a persistent disk will just mislead users.

I totally agree that giving a false sense of security is a problem. But that problem is already present IMO because there's pass driver. So maybe it's possible to solve this problem with a clearer documentation and/or naming (like "at-rest-encrypter" instead of "encrypter")?

For file based secretes it should be simple the extract them onto tmpfs and use that file as base mount. The thing is though that is just not how the code was designed. It has been a while since I looked at it but my understanding is that we extract the secret in the persistent container store (i.e. under graphroot). Then they stay there until the container is removed even if the secret is removed.

Now it should be easy enough to load the secret on each start instead and put it on tmpfs but this would also be a significant behavior change that we cannot just do like that.

Ok. I still don't think it's a deal breaker for this feature, because most of the people who would enable secret encryption would also encrypt their drives. But it's definitely something worth adding to the documentation.

Yes I think this is correct. The oci spec is used in many places so I am not sure if moving it directly yo tmpfs would break things. But technically it should be at least possible to copy the final spec to tmpfs and only there add the secret env vars I assume.

Would you accept a PR for something like this?

@Luap99
Copy link
Member

Luap99 commented Jan 27, 2025

That is fair but I think saying secrets are encrypted as long as we put them on a persistent disk will just mislead users.

I totally agree that giving a false sense of security is a problem. But that problem is already present IMO because there's pass driver. So maybe it's possible to solve this problem with a clearer documentation and/or naming (like "at-rest-encrypter" instead of "encrypter")?

Yes this will require proper documentation on how this works.

For file based secretes it should be simple the extract them onto tmpfs and use that file as base mount. The thing is though that is just not how the code was designed. It has been a while since I looked at it but my understanding is that we extract the secret in the persistent container store (i.e. under graphroot). Then they stay there until the container is removed even if the secret is removed.
Now it should be easy enough to load the secret on each start instead and put it on tmpfs but this would also be a significant behavior change that we cannot just do like that.

Ok. I still don't think it's a deal breaker for this feature, because most of the people who would enable secret encryption would also encrypt their drives. But it's definitely something worth adding to the documentation.

Sure most people should use encrypted disk and this is indeed not really concern though having things only on tmpfs would still be nicer but certainly not a blocker.

Yes I think this is correct. The oci spec is used in many places so I am not sure if moving it directly yo tmpfs would break things. But technically it should be at least possible to copy the final spec to tmpfs and only there add the secret env vars I assume.

Would you accept a PR for something like this?

Well it would require some research on how we use the persistent config.json. I doubt we actually need that in podman.
Doing a copy by default would be a performance hit so that isn't a good either in practise unless there is no better way.
I think just changing the path location to tmpfs and testing thoroughly should be tried first.
cc @mheon

In general I would not object a PR if such work was done but if the argument for the file based driver is the disk is encrypted then the same argument should count for the oci spec as well so the location should not be that important.

@mheon
Copy link
Member

mheon commented Jan 27, 2025

I actually don't see a reason why the OCI spec can't be put in the c/storage per-container rundir - which (should be) a tmpfs - instead of where it is right now. It should not be persistent.

(Though I will note that Podman does read the final OCI spec during some operations - e.g. inspect - to give up-to-date information as to what the container is doing. Which means we will pull the version of the spec with the secrets added and print those in inspect. We've talked about fixing this at the inspect level by omitting all environment variables added by secrets, for all drivers - not just this hypothetical new driver - to not accidentally leak things)

@Luap99
Copy link
Member

Luap99 commented Jan 28, 2025

We've talked about fixing this at the inspect level by omitting all environment variables added by secrets, for all drivers

That is already done since a5e9b4d (v5.3.0)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

No branches or pull requests

4 participants