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
networking.defaultMailServer.authPassFile: allow types.path #29298
Conversation
@Ma27, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @abbradar and @basvandijk to be potential reviewers. |
nixos/modules/programs/ssmtp.nix
Outdated
default = null; | ||
example = "/run/keys/ssmtp-authpass"; | ||
example = /run/keys/ssmtp-authpass; |
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.
Won't this result in /run/keys/ssmtp-autpass being copied to /nix/store/... at build time? If I understand that option correctly, that would be very undesirable. (You want the file not to be copied to the a world readable Nix store path.)
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 types.path
allows relative paths and does some validation to ensure that the path actually exists while evaluating the expressions.
Furthermore the configuration file accepts a file path which comes directly from the configuration if authPassFile
is given.
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 believe what @bjornfor means is that nix might copy the named file into the store while evaluating the example value, whereas when it is quoted it is treated as a string literal.
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.
Exactly what I meant. Also, if this reasoning is correct, using types.str ensures that one cannot define the option with a value that allows nix to copy the file into the store at eval/build time. So it acts as a safeguard against accidentally leaking the file into the store. (Please correct me if I'm wrong.)
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.
Per https://github.com/NixOS/nixpkgs/blob/master/lib/types.nix#L167, types.path
is just a str
that must begin with a forward slash.
I think it's generally hard to enforce stuff not being copied into the store via module options because nix probably will have evaluated the value (and hence copied stuff into the store) before it gets a chance to do the type-check. So if we added, say, types.keyPath
with the proviso that the value could not point into the store, nix might still copy the file into the Store despite module eval ultimately failing type-check.
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.
Then again, perhaps I'm thinking about this the wrong way ...
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.
you're right, it seems as Nix the origin of a path file into the nix store which means that this change might result in a security vulnerability -> closing for now...
6f34d77
to
78ec175
Compare
78ec175
to
da15b5f
Compare
Motivation for this change
types.path
is semantically correct when providing file path values.However
networking.defaultMailServer.authPassFile
only allows strings and null ATM.Things done
build-use-sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)