Add types.secretPath

[infinisil]

Motivation for this change

NixOS modules currently have no way to declare a path option as a secret. Using types.path in no way prevents people from putting secrets in the Nix store (you can prevent it only for path literals like /run/keys/foo by setting apply = toString, which I've been suggesting for some time).

This PR solves this problem by introducing types.secretPath which is a type that prevents assignments to values in the Nix store. Along with it comes the function lib.secretInNixStore, which explicitly imports a secret into the Nix store, and is the only way to get around the checks in secretPath. This is for scenarios where you don't care about a secret being in the Nix store.

On the side this also introduces lib.strings.validDerivationName for generating a valid derivation name from a potentially invalid one.

Things done

• Go through the code to do some cleaning up, making sure the comments are all up-to-date
• Added tests, ideally including ones that verify secrets aren't imported unless secretInNixStore is used
• Added documentation
• Go through NixOS modules to replace types.path with types.secretPath where appropriate
• Ideally make sure that the _secretInNixStore attribute doesn't end up in the nix store. This doesn't matter much since the secret is already in the store, but we shouldn't help an attacker by letting them know which files are secrets and which aren't

Ping @Profpatsch @rycee @roberth

[roberth]

Let's be very cautious about this kind of change.

1. We don't want to create a false sense of security.
2. This will have a lasting impact on the ecosystem, for better or for worse.

I find the fact that this PR comes virtually without documentation concerning. I know it's on the TODO list, but I think it should be done first. The documentation should expose every flaw in this partial security solution.

[aanderse]

Does this prevents me from shooting myself in the foot with services.foo.passwordFile = pkgs.writeText "bad-idea.txt" "notSoSecret";?

[infinisil]

@aanderse Indeed it does!

[aanderse]

Mostly a good thing then... though it was kinda nice to have that fallback as justification for dropping .password options entirely.

[infinisil]

@aanderse Oh you can still fallback to it being in the store with lib.secretInNixStore, that's what that's for

[roberth]

So I think the main concern is what could happen to the path before and after the check.

Idea: a distinct type

It seems possible to have a bit more control by introducing an actual new type.

This is actually a pattern that works for in-memory secrets too. (readFile keyPath etc.)
Having roughly the same interface for actual secrets and paths to secrets is probably a good thing. Although it may seem overengineered for paths, it has some practical benefits.

# todo: add non-store path validation code to p
protectPath = p: {
  _type = "secretPath";
  # to protect against generic attrset traversing code
  spillSecret = x: p;
  outPath = throw "Attempt to use a protected path. Protected paths must be explicitly unprotected to make security critical code easier to identify. See <link to manual section about protected paths>";
};
unprotectPath = pp: pp.spillSecret null;

That manual section will have guidelines, particularly

• marking with protectPath as soon as possible
• calling unprotectPath as late as possible

On the module side, unprotectPath will draw attention to code like this, which, particularly if spread out, is rather inconspicuous.

let keyPath = /. + unprotectPath cfg.keyPath;
     configText = "key = ${keyPath}";
in { ... config.environment.etc.... = configText ... }

On the user configuration side, we can require via types.secretPath that the user creates an actual secretPath and provide a useful error message with guidelines. Nix linters can require that all usages of protectPath are direct applications with literals. Additionally we can have a safe protectedSubpath function that operates directly on a secretPath, preventing a common mistake being "${keyDir}/file.key"

[roberth]

Some of us are conditioned to recognize that this is dangerous, but newcomers aren't.

Suggested change

	secretInNixStore = path:
	makeWorldReadable = path:

[infinisil]

I originally didn't choose that because world-readable sounds like literally everybody can read it, but really it's just the people that have access to the machine.

Also, secrets don't have to be present on the evaluation host, in which case the secret can't be made world readable (though it already is on the remote host).

I think it's also important to clearly indicate what a function does, and this function does "Allow a secret path to be in the nix store, potentially importing it first". Though I like the idea of better indicating that it's it's not secure in general.

How about:

• storeExposedSecret
• nixStoreExposedSecret
• unprotectedStoreSecret
• insecureStoreSecret
• secretInUnprotectedStore
• secretInUnprotectedNixStore
• exposedStoreSecret

[roberth]

in which case the secret can't be made world readable (though it already is on the remote host).

Judging from the examples in #78640 (comment), this seems to apply only to (iv). That one seems like a particularly inconvenient and insecure method.

A name like that seems ok. Which one is best may depend on whether or not the { _type = "secretPath"; } idea is implemented.

[Scrumplex]

I would like to see the word insecure here. insecureStoreSecret is good in my book

[danbst]

huh, these becomes a pattern https://github.com/NixOS/nixpkgs/blob/master/pkgs/build-support/kernel/make-initrd.nix#L24

[infinisil]

Nice, something to refactor with the new function introduced here!

[infinisil]

@roberth I like the idea of protectPath, for both being able to audit modules more easily but also for telling module authors "watch out, you're handling a path to a secret, don't expose it". I'll make sure to incorporate this idea.

Some comments on what you said:

• I don't think it makes sense to support in-memory secrets in any way, because the only way to extract information out of Nix (for a NixOS build) is via derivations, and those are in the Nix store, which is no good for secrets. Maybe in the future when there's something for Support private files in the Nix store nix#8, but not for now

• Note that these use cases should be supported:

  1. secretPath = secretInNixStore ./the-secret: A path literal to a file not in the store is imported into the store (the file has to exist in the eval host) and copied to the target host
  2. secretPath = secretInNixStore /nix/store/xxx-hello: A path literal to a file in the store already (could be a symlink pointing to a store file) is not imported into the store since it's already there (but the path has to exist in the eval host), but is copied to the target host
  3. secretPath = "/run/keys/the-secret": A string to a path is just passed along, expected to exist at runtime (not on the evaluation host). This does not need secretInNixStore.
  4. secretPath = secretInNixStore "/nix/store/xxx-hello": A string to a path in the Nix store on the target host. Could be useful if the secret is put into the store on the target host through other means.

  So using a literal means "Is going to be in the eval hosts store", while a string means "Is going to be in the target host". But note that 3. is the target use case, as it doesn't mean to have secrets in the store, which is why all the others require secretInNixStore.

[roberth]

because the only way to extract information out of Nix (for a NixOS build) is via derivations

Currently. It's basically one nix-instantiate call away. The rest is performance optimization.

NixOS/nix#8 is an interesting idea, but unlikely to become successful. I'd love to be proven wrong, but the status quo of an unencumbered nix store seems quite attractive.

To return to the PR topic :) It looks like it's going to be a good change.

[globin]

also see NixOS/rfcs#59

[zimbatm]

👍 for the idea, it's good that somebody is tackling this.

In terms of implementation, I think we can get away with much less machinery by introducing:

lib.secretPath = path: { type = "path to secret"; __toString = _: toString path; };

This is enough to encapsulate the original path so it doesn't get added to the store when interpolated in values no?

Then the module option would also wrap the value in a secretPath if it's not already a secretPath.

I might be missing a use-case. It would be interesting to lay the out with a few examples.

[zimbatm]

What do you think of linking the user to the documentation where the user would have access to all the context instead?

Right now I think the user would try to use the secret in the store, get the error message on nixos-rebuild and just apply lib.secretInNixStore as directed.

[infinisil]

Sounds good (once the docs exist)

[rnhmjoj]

Instead of secretInNixStore, which may still be interpreted as being secure/encrypted, I would call it something more ominous like unprotectedSecretPath.

[zimbatm]

Same

[infinisil]

@zimbatm Not entirely sure what you're saying, but no that's not enough, because people can still pass "${./path/to/secret}", which when used eventually is still imported into the store.

The goal with this is to prevent any secrets from accidentally ending up in the store. Unfortunately as I just discovered, there's a problem with Nix evaluation itself that prevents some safety for this, see NixOS/nix#3358. Unfortunately I think this is a blocking issue for this PR.

[lheckemann]

@infinisil IMHO the string context functions are edge cases that shouldn't block this. Even if this change doesn't support complete safety because of these cases, it's a significant improvement over the status quo (where paths where it's avoidable are copied in silently) and should be merged and used.

[infinisil]

Maybe you're right, I'll try to look into this again. Before I forget: This needs to be verified to work with flakes, which don't support builtins.storePath, which is used here.

[stale]

I marked this as stale due to inactivity. → More info

[roberth]

Seems like this was a precursor to sanitizeDerivationName :)

Times have changed!

nix-repl> stdenv.mkDerivation { name = "hi!@#$%^&*()"; }
«derivation /nix/store/180bgrm5h84yc9vcg8fpp9p7jks1n9yj-hi-.drv»

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/low-effort-and-high-impact-tasks/21095/15

[Atemu]

What's the status on this? Is there more a minimal version we could have that'd still be an improvement over the status quo?

[ambroisie]

@Atemu I've seen #328472 which is a recent PR trying to take a stab at a very similar problem.