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.

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

@aanderse Indeed it does!

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

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

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 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.

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.

also see NixOS/rfcs#59

@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.

@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.

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.

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

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