Introduce a type for options that contain the path to a secret file #174978
Conversation
symphorien
requested review from
mweinelt,
edolstra,
nbp and
infinisil
as code owners
github-actions
bot
added
6.topic: nixos
ofborg
bot
added
10.rebuild-darwin: 0
|
Check out #78640 as well |
|
Ahah it's embarrassing: I linked to this PR about two years ago, but then forgot about it and reimplemented it yesterday... Anyway, what blocks it? I see a rfc mentioned but it's closed. |
lib/types.nix
Outdated
| type = "derivation"; | ||
| name = "secretFileMadeWorldReadable"; | ||
| outPath = "${path}"; | ||
| outputs = [ "out" ]; | ||
| out = res; | ||
| outputName = "out"; | ||
| canBePublic = true; |
There was a problem hiding this comment.
how about something simpler:
| type = "derivation"; | |
| name = "secretFileMadeWorldReadable"; | |
| outPath = "${path}"; | |
| outputs = [ "out" ]; | |
| out = res; | |
| outputName = "out"; | |
| canBePublic = true; | |
| inherit path; | |
| __toString = r: r.path; | |
| canBePublic = true; |
There was a problem hiding this comment.
I initially tried toString but then "${config.the.option}" does not work anymore, you explicitly need to use toString. This way makes the migration fully transparent.
There was a problem hiding this comment.
odd, it seems to work fine here.
There was a problem hiding this comment.
indeed it works, I must have tested wrong.
like types.path but rejects bare paths which would have been copied to the store. (and actually everything that is in the store).
| # deliberately refuse paths, because toString x and "${x}" behave | ||
| # differently. Some modules might handle paths correctly, others will | ||
| # not, and anyway users can either quote the path or use makeWorldReadable. | ||
| check = x: ((isCoercibleToString x && (x.canBePublic or false)) || (isString x && !(hasPrefix "/nix/store" x))) && (hasPrefix "/" "${x}"); |
There was a problem hiding this comment.
The only thing that you can guarantee is that you can assert that the secret has already been leaked, but not prevent it from leaking.
My understanding of why this is not possible without a special builtin which does not evaluate its operand, or which provide another evaluation loop which is guaranteed to not push any secret is the following:
isString x Return true if x evaluates to a string, and false otherwise.
The evaluation of a path, is similar to a derivation, and as far as I recall submit derivation directly to the Nix daemon.
There was a problem hiding this comment.
not quite: path copying is really lazy. paths shouldn't be copied to the store unless they are interpolated into strings, constructed through builtins.path, or copyingly coerced by plugins. (can be checked by taking a path to a large file and munging it a bit. only when it is coerced to string, eg by interpolation or taking substrings, does the copy happen)
this check looks safe as long as we can guarantee that no interpolations are evaluated before the check is, but that too might be hard to do with perfect accuracy
There was a problem hiding this comment.
Huh indeed the copy happens even when only evaluating builtins like builtins.hasContext... I was very surprised.
$ uuidgen > /tmp/secret
$ nix repl "<nixpkgs>"
Welcome to Nix 2.8.1. Type :? for help.
Loading '<nixpkgs>'...
Added 16461 variables.
nix-repl> a = runCommand "foo" {} "cat ${./secret} sdf"
nix-repl> builtins.hasContext "${a}"
true
$ rg $(cat /tmp/secret ) /nix/store --glob "*secret"
/nix/store/8gnhfxbv1zzar3pwxlmsxx8lr7876cp4-secret
1:97c31780-96ec-41b1-b8bf-a0bbbe113cb5
The check has the following property: a configuration that evaluates does not leak secrets. (well it's not true because you can trick it with "/${./secret}" but you get my point). Maybe we could tweak the error message to say that if you see the error message the secret already leaked?
|
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 |
wegank
added
2.status: stale
stale
bot
removed
the
2.status: stale
wegank
added
the
2.status: stale
Description of changes
This should avoid mistakes like
passwordFile = /etc/secret;where that path is unquoted so nix copies it to the nix store and makes it public.I converted three modules to illustrate, but if this is merged we could add many others. It will be a breaking change but for security's sake.
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)nixos/doc/manual/md-to-db.shto update generated release notes