Contract for files containing secrets#495303
Conversation
500a75b to
6d5493d
Compare
| if lib.isPath cfg.jwtSecretKey then cfg.jwtSecretKey else cfg.jwtSecretKey.output.path | ||
| }) \ | ||
| sessionStoreKeyFile=$(< ${ | ||
| if lib.isPath cfg.sessionStoreKey then cfg.sessionStoreKey else cfg.sessionStoreKey.output.path |
There was a problem hiding this comment.
i wonder if backward compatibility with path would be more desirable than just with plain string, given path is insecure
There was a problem hiding this comment.
I agree in principle but I'm not sure I get what you mean. Isn't this the case here? Also note here I tried to be backwards compatible with the stash module in particular.
|
|
||
| secretOptionType = | ||
| let | ||
| contractSecretsType = types.submodule { |
There was a problem hiding this comment.
from what i can tell the submodules look similar, with this one also providing default values.
would it cause recursion problems if such a module were just exposed in some mutually accessible location, be it at e.g. contracts.secrets?
options.contracts could be an option taking type attrsOf deferredModule or the like, so that on the config side we could then plug in your shared submodule for contract.secrets, then access that from both places that got it now.
There was a problem hiding this comment.
I fully agree but isn't this one more implementation competing with the other two big PRs? Not saying we shouldn't think about it but TBH I purposely kept that out of this PR to focus on the contract for secrets. Unless a good argument for, I'd rather not introduce this complexity in the PR.
My hope by doing it like this is to create a handful of contracts, use them in a handful of places and then we can think of an underlying implementation with concrete use cases.
Note also that on one side, we set default values on the consumer side of the contract and on the other on the provider side of the contract. So both sides are not exactly the same although the overall shape is.
|
right now this looks like a secret-only version of the earlier PRs with the (module system type based) secrets contract logic inlined. otherwise i mostly still see questions from before:
i gotta say i would regard secrets as maybe not the easiest topic out there in that sense. code right now looks not too bad tho i think, (as far as i can tell fixable) duplication aside. |
Did I do something wrong here? Shouldn't those tests run? |
Just to reiterate my other comment, that was on purpose to focus on the contract itself.
If I'm not mistaken, if the service/module uses LoadCredentials, then this contract "simplifies" by setting owner and group to
I do believe contracts enable this. I'm not sure how to best pass information across nodes and I don't think contracts will solve that, but what contracts do is allow to create providers as one see fit without needing upstream modifications.
Similarly, I don't think contracts will solve that directly. Something that would help is a way to catalog all consumers of a particular contract. This would be very nice for documentation too. I'm not sure how to do that cleverly without recursing in all options.
I agree but as long as the PR/contract is one step in the correct direction and does not hinder any future goals, I'd say it's good enough. |
i guess my question would be if we could tell that facilitating those three use-cases might not make for breaking changes if users would come to rely on e.g. this implementation |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
6d5493d to
897e5b5
Compare
|
Tbh, I think basing this on how secrets are currently handled by NixOS/sops-nix/agenix is a significant limitation, though I understand why you do. But to make my case, the current mechanism is already inadequate for more sophisticated use cases. Most services that use Any sane implementation using I also don't see too clearly how this would integrate with services like e.g. vault, or more complex things like secrets that are encrypted-at-rest, while integrating cleanly with more granular service management than the activation script. It'd be nice to see at least a sketch of how more sophisticated secrets management than shoving around files would be implemented. It's probably possible to adhere to this contract, but I think it would be limiting and encourage bad practices (like what we do today with creating secrets in multiple locations just so that systemd-creds can read them from somewhere). In my eyes, ideally a secrets management contract for NixOS wouldn't worry about file ownership as a first-class citizen. That seems like a detail of methods of storing secrets, rather than communicating how secrets should be found and accessed in general. I don't mean to make the perfect the enemy of the good, but I feel like this is the type of thing that will be hard to change, and I already see issues. We'd probably do better by looking at the schemas for things like systemd-creds or vault and coming up with a way to implement local-file based secrets using those schemas, rather than the other way around. That'd likely also result in a nicer interface for secrets generation. I think it'd be very nice if contracts could help finally get secrets management to be a first-class concept in the NixOS world, but that probably takes a bit more design work. So I agree with @KiaraGrouwstra that this might not be the best use case to choose for a first attempt. |
|
Thanks @TLATER this is exactly the kind of feedback I wanted. I want to recognize that I know there is a lot of baggage surrounding secrets. But I think it is unfair to block any work on secrets by requiring them to solve every problems. Contracts originated from a few considerations:
All that to say I don't think it should be contracts' role to impose one way of working. Actually quite the opposite, I see contracts as a way to decouple parts of the whole NixOS system and allow for experimentation. Similarly, I wouldn't see it as necessarily an issue to have multiple contracts with different options for a common concept. For example for backups, backing up files and databases are using totally different and incompatible mechanisms. In that light, I see the effort of correctly using
I don't see how this is bad because it's not leaking secrets. It could be argued that it's a clumsy API that could be hidden by a NixOS systemd specific option (which I did undertake in a previous PR but that's another story).
That is completely fair and a misuse of systemd configuration. I'd argue this should be solved by at least better documentation which is completely lacking in the NixOS manual, nixpkgs manual or NixOS wiki (in order of preference for this to exist in). We should be able to review PRs that handle secrets with systemd and redirect to a page explaining the established best practices with motivation. But again, this should be an orthogonal effort.
I don't see how. I envisioned the contract to work this way with # Consumer
{ config, ... }:
let
cfg = config.myservice;
in
{
options = {
myservice.mysecret.input = {
# Pseudo code here, think of these surrounded by mkOption and setting the default.
owner = "root";
group = "root";
mode = "0440";
};
};
config = {
systemd.services.myservice = {
serviceConfig = {
DynamicUser = true;
LoadCredential = [ "MYSECRET:${cfg.mysecret.output.path}" ];
};
environment.MYSECRET = "%d/MYSECRET";
# Maybe a new option could be introduced which sets both environment and LoadCredntial
# secrets.MYSECRET = cfg.mysecret.output.path
};
};
}So having the contract doesn't hinder using
AFAIK services consume secrets in 3 fashion:
If any, the quantity of services that handle getting secrets from vault is very small in NixOS. So if one uses Vault, they will anyway need to create some adapter to fit how upstream services handle secrets. I don't see how to get away without this. I think it will be the same for other such more complex secrets providers. For encrypted-at-rest, do you mean secrets that would be decrypted as the systemd service starts? The only module using For better or worse, the current state in NixOS is using a file for the common denominator to share secrets. That's where the contract meets users by making the situation a bit better still by removing this possible permission mismatch and runtime error.
About finding secrets, you can't get around the fact that you need to distinguish secrets. We need to name them and have a mapping between the filesystem where files are stored encrypted and that name. Currently this mapping is manual and sometimes tedious. I assume you're saying it would be great if this mapping was done automatically? Like the Vars initiative? A service could then request for a secret and it is automatically created at deploy time for example. In any case, I still think this contract has value. Because at some point that automated system will need to make this secret available to the service in the way the service expects it. And this is what the contract encodes. We can always create a second more overarching contract which handles nicely such automated systems when we get there.
I see what you mean by hard to change but I see this instead as a nice stepping stone to build these automated systems with. Thanks to this contract you get a clean separation between the consumers and the providers. This lets one experiment however they like on the provider side. Now, with the current state of NixOS and its ubiquitous reliance on systemd, the only common denominator for sharing secrets I see in the short and medium term is through files. Btw that system that translates vault secrets to files for systemd seem to exist: https://github.com/numtide/systemd-vaultd |
if that works it should address the on another note, i no longer see the round-trip mechanism in this version. that makes me wonder if my earlier classification of whether cases needed such a round-trip is still relevant. if not, perhaps one of those other use-cases for contracts might raise less questions? |
|
Yep, thanks @ibizaman, I think this effort is great, I'm just a bit fearful of working ourselves into a corner we can never get out of again; Breaking changes to contracts will likely not be easily accepted, as they'll always involve changes to every single module that uses them.
... which is what I'm worried about. I don't think contracts need to be blocked on secrets, and I think rushing improvements to secrets through contracts will hinder improvements there. Specifically:
A future initiative to enforce using systemd-creds (or vault, or other services) "correctly" would involve changing the secrets contract. AIUI, it becomes impossible to use I think the case of vault exposes that problem a bit better:
Exactly! I think it's possible to design something here, it might be e.g. some kind of adapter that reads metadata from a contract. But if we codify everything as using filepaths today, it will be hard to introduce a solution like that. Worse, existing services that do support vault will not be able to do so at all, since the contract specifies that secrets management must be done through the filesystem. So, again, I completely understand why you're proposing this as-is; It's very pragmatic to just encode what is done in NixOS today, and just accept that that's how secrets will be handled in this ecosystem. I'm really worried it'll get us stuck there, though. That's not to say I don't like the idea, I'm here precisely because I've stumbled upon my own desire for a solution like what vars/selfhostblocks/etc. are proposing, and am finally catching up with the work y'all have been doing. The terminology is great and we need a contract like this for secrets. If I were to counter-propose something today, I'd suggest using simply an attrset that defines a set of secrets without any other metadata, and to document that this makes the secrets available in the systemd credentials store. Any implementation of the contract could then use systemd's
Adapting your pseudocode would look like this: # Consumer
{ config, ... }: {
config = {
secrets.mysecret = { };
systemd.services.myservice = {
serviceConfig = {
DynamicUser = true;
ImportCredential = [
config.secrets.mysecret.output # evaluates simply to: "mysecret"
];
};
environment.MYSECRET = "%d/mysecret";
};
};
}Hooking this up to e.g. sops-nix becomes trivial, since we no longer have to worry about file ownerships: {
sops.defaultSopsFile = "/etc/sops/secrets.yaml";
# Behind the scenes, sops simply grabs `config.secrets.*` that have matching names in
# `secrets.yaml` and places them in `/run/credstore/`
}But this is probably too married to systemd; It is limited as far as user services go, and some vault-using services will likely want to be able to access secrets at runtime via an API or something. I also imagine that the anti-systemd crowd would find this solution very unappealing. |
right -- thanks!
yeah, to be more precise i meant that the consumer would grab some return value from the provider, which seemed not to happen in the back-up ones. |
You're right, that made the implementation in the other PRs simpler but for the consumer option and provider option generator. (Not sure how to correctly call that part, I'm talking about https://github.com/NixOS/nixpkgs/pull/485453/changes#diff-6f2c3b22633946b34d7d8179bb3bb3dbb5767164cd4d491cc2c84ab03c730b1f and https://github.com/NixOS/nixpkgs/pull/432529/changes#diff-52f2397a5f73a560e904e384c5d1c554c844c46db1d372afb7758d53b9de1a7e). With this PR I wanted to show that contracts are usable without that code which in the end complicates things. We can always go back to those PRs when a few contracts are established. This should make the discussions more concrete. |
|
@TLATER I understand your arguments and fears but I don't agree with the conclusion. I still see this contract as a good (small) step towards better secret management. From a technical standpoint, I really don't see how codifying the permissions on the file path can ever hinder future initiatives to make secrets management better. We already rely on paths and codifying the permissions helps make the user experience better as explained in my previous comment. With a huge project like nixpkgs, not all parts of the system can or should evolve in lockstep. It is necessary to accept a transition period where modules/services are updated one after the other. Agreeing on a common goal or implementation is nice though. So say we find a better solution some day, we'll create a PR like this one which updates one module and start from there. I don't see a way around this. This PR proposes something concrete we can use now while the better solution is still up in the air. It's a bit unfair to block it for fear of something without a concrete counterargument to it. (I'm not saying that in a dismissive manner, I'm glad you commented on this). Similarly, not all parts of nixpkgs are of the same quality and that's to be expected. Note that the goal of contracts is to allow more interoperability between providers. I believe allowing the end user to choose between providers is very important. So should you be a proponent of systemd or not, I see a solution that interoperability between both as a good thing. Maybe this contract should be renamed to "fileSecretsContract" or something like that. I never had the ambition to solve all issues related to secrets management with this. I don't see it as a bad thing to have an hypothetical second competing contract related to secrets for services that work in a different way if we see the necessity for one. There is already something alike for backup contracts which distinguish backing up files and streams (e.g. databases). I think our dependence on paths to store secrets is pretty much immuable. Isn't systemd also using paths at some point even when using
It's paths all the way down. One other issue to consider is if the upstream service relies on paths for secret, what can we do? Going through all such upstream services and create PRs to make them support another mechanism is a daunting task. I don't see systemd change anytime soon and we shouldn't wait on all upstream services to update before making progress on our side. I think what you want is to create a layer above all these paths shenanigans which is fair and would be nice. My claim is what you're talking about will be a new layer on top of the current situation. And because it is above, this new layer will require to abstract over paths so having the path permission be encoded will help. But again I have no idea what this new layer should look like. Especially if you consider that new layer should be a contract which allow interoperability with multiple providers. The way Vault and Systemd credentials work is very different and from a cursory read of the documentation I only see paths as the common denominator, for better or worse. |
|
i now understand what you mean about the machinery being intended to propagate defaults from the consuming modules, and how that intent has led to the three existing implementations. edit: since one service could have many secrets, maybe this PR would end up maybe a bit less elegant than the second one to scale it
having the name mention files i would consider as helping clarify scope, yeah. |
|
Cool I'm glad the picture is getting clearer. For me too TBH.
I want to make clear that this PR implements just the contract. The consumer and provider agree on the options so they fit together but nothing forces this to be true apart from humans reviewing the code. I say that but the generic test ensures the provider has the correct options and using the What the other 2 PRs do is propose an implementation for a system which do force the consumer and provider to have the same options. The first PR tried that by (ab?)using the module system and the second by resorting to traditional functions. But this system which forces the option is not necessary per say for contracts to exist, as we can see in this PR. Seeing how the tests here do still make sure the options are correct, it seems the 2 other PRs are less necessary than I initially thought. I mean what those 2 PRs really add is a way to not need to define those options everywhere since they are the same across all consumers and providers. But will this really be necessary in practice? I don't know.
I did just update the PR title and will update the code shortly. |
i think one use-case for deduplication is easing refactors that might change some details |
897e5b5 to
bebe764
Compare
bebe764 to
6a4d4ef
Compare
|
if propagation seems mostly about defaults, i tried checking if we could just have like a |
|
This PR is replaced by #522054 See that PR’s description for why it is better than this one. |

Yet another PR for contracts, yes I know. But this one is different, hear me out!
Starting with presenting an underlying implementation was I realize a mistake.
The concept of contracts is still too abstract, for me too, to be able to choose the best implementation.
I noticed this because none of the comments in the previous PRs talked about the actual contracts.
So this PR focuses on one contract. The contract for secrets that are exchanged through files. With no fluff, no clever implementation. Just the plain old options which work because the consumer and provider sides agree on which options should exist. We can always reconvene later on when and if we find it important to have some sort of underlying implementation to contracts.
Motivation
The contract for file secrets intends to improve the user experience when a user must provide a secret to a module and its underlying service(s) through a file path.
Currently, most modules declare one or several options of type
stringwhich expect to receive the path to a file.This file is expected to exist at runtime, on the target host, before the service that requires them are started.
No constrain is set on how that file is generated nor when, as long as it exists when it is required.
An issue arise when the permissions of that file is incorrect.
In that case, the service that will try to read it will fail to do so and will not be able to start.
In other words, although the nix code correctly evaluates, a runtime error will happen which is bad user experience.
At best, some services do document what permissions they expect. But this is quite weak.
Contract Definition
This contract encodes what permissions the file should have.
A module or service that requires such a file is called a consumer.
One that will generate the file is called a provider. Sops-nix and Agenix are two such providers.
mode(typestring): the expected mode of the file.owner(typestring): the expected owner of the file.group(typestring): the expected group of the file.path(typestring): the path at which the file will exist on the target host.We could maybe remove the
modeoption if we made themodebe0400ifgroup == "root"and0440otherwise. I'm not sure if this cover all cases.The consumer and provider options are fully compatible with Sops-nix and Agenix and even share the same names.
Hardcoded Secrets Provider
This PR introduces a new provider.
The intention is to replace current usage of
pkgs.writeTextin the tests with this contract provider.Like
pkgs.writeText, thehardcodedsecretsmodule does store the secret in the nix store.But although
pkgs.writeTextcreates a file accessible by all users,the
hardcodedsecretsmodule correctly set the mode, owner and group of the file as expected by the consumer.This is useful to make sure the consumer sets the options correctly.
Generic test
A generic test is added to make sure any provider claiming to implement this contract behaves as expected.
The test makes sure the file generated by a provider has the correct content, mode, owner and group.
The
hardcodedsecretsprovider uses this test.Consumer Module: Stash
The Stash module's options has been changed to implement the consumer contract but still be backwards compatible.
Next Steps
I'd like reviews to address at least the options of the contract. Are they enough? Too much?
Also I’d love to hear thoughts on the genetic test.
I did myself successfully use this exact set of options in about 15 modules in my project and find them good as is and cover all use cases.
What Comes Next
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.EDIT: Update to make scope clearer - this contract is for files.