Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Sep 13, 2017

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
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@mention-bot
Copy link

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

default = null;
example = "/run/keys/ssmtp-authpass";
example = /run/keys/ssmtp-authpass;
Copy link
Contributor

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

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Contributor

@joachifm joachifm Sep 13, 2017

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.

Copy link
Contributor

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

Copy link
Member Author

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

@Ma27 Ma27 force-pushed the ssmtp/allow-paths-for-auth-pass-file branch from 6f34d77 to 78ec175 Compare September 13, 2017 13:46
@Ma27 Ma27 force-pushed the ssmtp/allow-paths-for-auth-pass-file branch from 78ec175 to da15b5f Compare September 13, 2017 13:47
@Ma27 Ma27 closed this Sep 15, 2017
@Ma27 Ma27 deleted the ssmtp/allow-paths-for-auth-pass-file branch September 15, 2017 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants