Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[RFC 0059]: Systemd Service Secrets #59
[RFC 0059]: Systemd Service Secrets #59
Changes from 1 commit
00c0dd3
0942ab5
8cd4e84
4b658f8
8a0ed5b
ea81bf7
9067c85
00f96a9
69739ba
bbd8f65
6ded518
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear to me why the helper unit is needed. Can't the keys be fetched using an
ExecStartPre=+...
command?More generally, instead of creating our own mechanism for passing keys to services, maybe the kernel keyring mechanism can be used for this? Units would call
keyctl request/search/read/...
to fetch keys. These keys would either be preloaded into the keyring or produced on demand using therequest-key
program.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I briefly tried
ExecStartPre
, but unfortunately it does not seem to run within the mount namespace, so it doesn't have a access to thePrivateTmp
we want. I am not sure if this is intentional or not.Regarding kernel keyring mechanism - I agree, it might be a good default backend. The directory based thing was just the dumbest proof-of-concept case I came up with (given that similar approaches are used in nixops and krops/stockholm). Part of the intention is to have a somewhat agnostic interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kernel keyring mechanism sounds overkill to me. Its usecase is not to communicate files between userland processes, but between userland and kernel drivers. Files are a perfectly sufficient abstraction for passing secrets around in userland.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have mixed feelings about this.
Using files usually implies having to worry about ACLs and who's allowed to access them. We can cheat by mounting them in a private namespace, but it's still a bit cumbersome.
Sometimes you want to have "use once" properties and provide a new key on every read / issue tokens on access etc. We could cheat again by providing these key files by a fuse filesystem, but then it just gets more complicated.
The kernel keyring might be a good abstraction over all this, it's just not widely adapted currently and lacking real-world usage. Reading
keyrings (7)
looks promising. In addition to thread/process/session, there's also anupcall
feature, bouncing back to userspace to request secrets which could be a request to whatever credentials provider is used.I'd love to experiment with that a bit, or see some real-world examples. Anybody aware of these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kernel keyring is very much intended for userspace keys too. See the kerberos stuff that has been ported to use it for that, or systemd's cryptsetup.
I think the kernel keyring has deficiencies (upcalls, yuck! also no namespacing for containers, …), but it probably is the right approach in the long run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's what the
!
prefix is for:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems unnecessarily verbose. I would do something like this:
and then our systemd module can generate whatever helper units are necessary.
It also avoids imperative-sounding function names like
mkSecretsScope
which suggest that they allocate a unique new scope, but that's not the case, e.g. insecretsScope1
andsecretsScope2
are actually the same scope.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, it's more compact. Why I initially decided against it and for something that modifies the resulting structure separately was to reduce changes to the existing systemd modules. At the same time I thought it would be nice to be able to get the secrets as arguments, which I thought made it nicer to deal with in code. In the
needsSecrets
case, if I understand it correctly, the user would be requesting a secret by its name, like in the scope creation, but then would have to possibly deal with paths (as strings) to pass the secret as an argument to the service, or load it into an env-var, because there would not be an automatic mapping mechanism anymore. Unless we pack it up into a shell variable or so.I do not necessarily perceive
mk*
as an imperative terminology, but that depends on the reader. We have a lot of puremk*
functions that construct some structure. But I'm not at all attached to the naming here, so we can change it to whatever seems more suitable if they're still around further down the road.Edit: In fact, I'm not attached to most of the terms in the RFC, such as "sidecart" and similar. I merely picked them from what I thought would make it easily enough understood. So if there are suggestions to rename things, I'm up for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@edolstra: Did this sufficiently address your remarks? I'm not super familiar with the inner workings of the process, so for now I just left those as discussion comments here, but I'm willing to incorporate some of the things pointed out into "alternatives" or the core section, if there is some consensus around that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, getting back to this suggestion - if somebody could point me to the simplest way of implementing it with such an additional field on any systemd service without needing to change too many things in the guts of core modules I'm willing to try and adjust the poc to see how that works. One thing I do kinda like though is the ability to have some at least very basic checking of secrets used, which wouldn't work the same way with just strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does the secret store deal with system/main config upgrades? When I uninstall an old service and install a new one that asks for a secret of the same name, does the new service get access to the old secret?
When I uninstall a lot of services, are old secrets garbage collected? Do downgrades always work seamlessly?
Can you outline the possible pitfalls regarding that topic in an extra section?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the current shape, no such management would happen. The secrets would just remain in the secrets store and if a config author decides to re-use them, then so be it. I can include that. I was initially thinking of the drawbacks of "sth that would be worse when adding this", but I guess it depends on the reference point. Definitely good points to add, even if they would remain unhandled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it's maybe a strawman, but if you put a secret in
/nix/store
, it's going to vanish upon garbage collection when I uninstall the package. If I don't care whether it's readable by root or by all users (e.g. inside a container), this is a drawback now because no such garbage collection would take place.As in "something that would be worse when adding this", you could also say that people possibly expect that NixOS doesn't do any state management by default and that downgrading will bring their machine back into the exact state, but that's not the case anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The difference being that before this RFC, I'd manually handle the secret state and remember to handle it. After all, I'm deleting the line saying
service.foo.secretsFile = "/foo/bar";
, and when I delete the line, I'll delete the file. This is now not the case anymore. Instead, I need to remember how this semiautomatic secret handling works.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is exactly the purpose of this RFC not to have secrets in the nix store. This is possible without any problems and if they are referenced in the nixos config they would not be garbage collected as currently is the case. And this can still be achieved with this proposal as before.
Actually currently the nix store of (nixos) containers is shared with the host and all other containers, so that might be an issue nonetheless.
With regard to secret state handling, a lot of services have the possibility do use a
passwordFile
,secretFile
, etc. but there is no abstraction for handing out permissions to secrets correctly, which is something like the proposal in this RFC is needed. Currently you have to not only drop the files on the machine yourself in the correct destination (same with this RFC, as long as you are using a fetcher that requires that), but also set the correct permissions for the respective service, which might be a chicken-and-egg problem of adding users for service by enabling it and the service requiring the secret.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to transition some non-critical services first, to gain some experience without breaking anything. If this works for 10+ regularly used non-critical e.g. webapps, then transition everything to this approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will fix the wording on that one. Parts of this section are maybe still a bit too "note to self"-like. What I intended was not for "inclusion into upstream" but more to hypothetically verify this is covering all the most important aspects needed for the most critical services. Your version is definitely better for an actual slow inclusion into upstream step.