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.

Sign up for GitHub

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

Introduce a type for options that contain the path to a secret file #174978

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Introduce a type for options that contain the path to a secret file #174978

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fa226aa
nixos: add types.secretFile
symphorien May 27, 2022
67e17c7
nixos/knot: use types.secretFile for secret files
symphorien May 27, 2022
b4df6da
nixos/acme: use types.secretFile for credential files
symphorien May 27, 2022
2caf44d
nixos/miniflux: use types.secretFile for admin credentials
symphorien May 27, 2022
775864b
nixos: add release notes for conversion to types.secretFile
symphorien May 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions lib/types.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ let
concatStringsSep
escapeNixString
isCoercibleToString
hasPrefix
;
inherit (lib.trivial)
boolToString
Expand Down Expand Up @@ -395,6 +396,30 @@ rec {
merge = mergeEqualOption;
};

secretFile = let
type = mkOptionType {
name = "secretFile";
description = "quoted path to a secret file";
# either the return value of makeWorldReadable or a proper string. We
# 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}");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

merge = mergeEqualOption;
};
makeWorldReadable = path:
# to make "${returnValue}" work, but still being able to store an attribute in it
if lib.isAttrs path then
path // { canBePublic = true; }
else
let res = {
toString = _: "${path}";
canBePublic = true;
};
in res;
in
type // { inherit makeWorldReadable; };

listOf = elemType: mkOptionType rec {
name = "listOf";
description = "list of ${elemType.description}";
Expand Down
17 changes: 17 additions & 0 deletions nixos/doc/manual/development/option-types.section.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,23 @@ merging is handled.
coerced to a string. Even if derivations can be considered as
paths, the more specific `types.package` should be preferred.

`types.secretFile`

: A string representing a filesystem path (starting with "/") to a file which
is supposed to remain secret.
Depending on how the module is implemented, setting an option of type
`types.path` to the literal value `/etc/secret_file` can make nix copy the
secret file at `nixos-rebuild` time to the store and thus make it world
readable. To avoid this behavior, it is enough to quote the path:
`"/etc/secret_file"`. This solution is simple, but it is easy to forget
about it. `types.secretFile` is design to prevent such mistakes.
Compared to `types.path`, this type adds further restrictions:
- only strings (as opposed to unquoted paths) are accepted as input.
- the string must not start by `/nix/store/`.
To bypass these restrictions you can pass the value to the function
`types.secretFile.makeWorldReadable`. This can be useful when writing NixOS
tests, as in this case the secret file is only an example value.

`types.package`

: A top-level store path. This can be an attribute set pointing
Expand Down
40 changes: 40 additions & 0 deletions nixos/doc/manual/from_md/development/option-types.section.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,46 @@
</para>
</listitem>
</varlistentry>
<varlistentry>
<term>
<literal>types.secretFile</literal>
</term>
<listitem>
<para>
A string representing a filesystem path (starting with
<quote>/</quote>) to a file which is supposed to remain
secret. Depending on how the module is implemented, setting
an option of type <literal>types.path</literal> to the
literal value <literal>/etc/secret_file</literal> can make
nix copy the secret file at <literal>nixos-rebuild</literal>
time to the store and thus make it world readable. To avoid
this behavior, it is enough to quote the path:
<literal>&quot;/etc/secret_file&quot;</literal>. This
solution is simple, but it is easy to forget about it.
<literal>types.secretFile</literal> is design to prevent
such mistakes. Compared to <literal>types.path</literal>,
this type adds further restrictions:
</para>
<itemizedlist spacing="compact">
<listitem>
<para>
only strings (as opposed to unquoted paths) are accepted
as input.
</para>
</listitem>
<listitem>
<para>
the string must not start by
<literal>/nix/store/</literal>. To bypass these
restrictions you can pass the value to the function
<literal>types.secretFile.makeWorldReadable</literal>.
This can be useful when writing NixOS tests, as in this
case the secret file is only an example value.
</para>
</listitem>
</itemizedlist>
</listitem>
</varlistentry>
<varlistentry>
<term>
<literal>types.package</literal>
Expand Down
30 changes: 30 additions & 0 deletions nixos/doc/manual/from_md/release-notes/rl-2211.section.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,36 @@
PHP now defaults to PHP 8.1, updated from 8.0.
</para>
</listitem>
<listitem>
<para>
The NixOS module system now has a type
<literal>secretFile</literal> for options which contain the
path to a file that should remain secret. This is to avoid
mistakes where one writes
<literal>services.miniflux.adminCredentialsFile = /etc/secret</literal>
instead of
<literal>services.miniflux.adminCredentialsFile = &quot;/etc/secret&quot;;</literal>
which makes nix copy <literal>/etc/secret</literal> to the
store where everyone could read it. A number of modules have
been converted to use it. This is a breaking change visible by
an error message like this:
</para>
<programlisting>
error: A definition for option `services.miniflux.adminCredentialsFile' is not of type `quoted path to a secret file'. Definition values:
- In `nixos/lib/build-vms.nix': /etc/secret
</programlisting>
<para>
To fix the issue you must assess whether leaking
<literal>/etc/secret</literal> to the store and thus all local
users is a problem. If it is the case, then add quotes:
<literal>services.miniflux.adminCredentialsFile = &quot;/etc/secrets&quot;</literal>.
If you notice this during upgrade, your secret probably has
already leaked and it may be a good time to change it. If
leaking this file is acceptable (for example in a NixOS test),
you can keep the old behavior like this:
<literal>services.miniflux.adminCredentialsFile = lib.types.secretFile.makeWorldReadable /etc/secret</literal>.
</para>
</listitem>
</itemizedlist>
</section>
<section xml:id="sec-release-22.11-new-services">
Expand Down
19 changes: 19 additions & 0 deletions nixos/doc/manual/release-notes/rl-2211.section.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,25 @@ In addition to numerous new and upgraded packages, this release has the followin

- PHP now defaults to PHP 8.1, updated from 8.0.

- The NixOS module system now has a type `secretFile` for options which contain
the path to a file that should remain secret. This is to avoid mistakes where
one writes `services.miniflux.adminCredentialsFile = /etc/secret` instead of
`services.miniflux.adminCredentialsFile = "/etc/secret";` which makes nix
copy `/etc/secret` to the store where everyone could read it. A number of
modules have been converted to use it. This is a breaking change visible by
an error message like this:
```
error: A definition for option `services.miniflux.adminCredentialsFile' is not of type `quoted path to a secret file'. Definition values:
- In `nixos/lib/build-vms.nix': /etc/secret
```
To fix the issue you must assess whether leaking `/etc/secret` to the store
and thus all local users is a problem. If it is the case, then add quotes:
`services.miniflux.adminCredentialsFile = "/etc/secrets"`. If you notice this
during upgrade, your secret probably has already leaked and it may be a good
time to change it. If leaking this file is acceptable (for example in a NixOS
test), you can keep the old behavior like this:
`services.miniflux.adminCredentialsFile = lib.types.secretFile.makeWorldReadable /etc/secret`.

<!-- To avoid merge conflicts, consider adding your item at an arbitrary place in the list instead. -->

## New Services {#sec-release-22.11-new-services}
Expand Down
2 changes: 1 addition & 1 deletion nixos/modules/security/acme/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -554,7 +554,7 @@ let
};

credentialsFile = mkOption {
type = types.path;
type = types.secretFile;
inherit (defaultAndText "credentialsFile" null) default defaultText;
description = ''
Path to an EnvironmentFile for the cert's service containing any required and
Expand Down
2 changes: 1 addition & 1 deletion nixos/modules/services/networking/knot.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ in {
};

keyFiles = mkOption {
type = types.listOf types.path;
type = types.listOf types.secretFile;
default = [];
description = ''
A list of files containing additional configuration
Expand Down
2 changes: 1 addition & 1 deletion nixos/modules/services/web-apps/miniflux.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ in
};

adminCredentialsFile = mkOption {
type = types.path;
type = types.secretFile;
description = ''
File containing the ADMIN_USERNAME and
ADMIN_PASSWORD (length >= 6) in the format of
Expand Down
4 changes: 2 additions & 2 deletions nixos/tests/acme.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,12 @@ import ./make-test-python.nix ({ pkgs, lib, ... }: let
dnsConfig = nodes: {
dnsProvider = "exec";
dnsPropagationCheck = false;
credentialsFile = pkgs.writeText "wildcard.env" ''
credentialsFile = lib.types.secretFile.makeWorldReadable (pkgs.writeText "wildcard.env" ''
EXEC_PATH=${dnsScript nodes}
EXEC_POLLING_INTERVAL=1
EXEC_PROPAGATION_TIMEOUT=1
EXEC_SEQUENCE_INTERVAL=1
'';
'');
};

documentRoot = pkgs.runCommand "docroot" {} ''
Expand Down
4 changes: 2 additions & 2 deletions nixos/tests/knot.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,12 @@ let
paths = [ exampleZone delegatedZone ];
};
# DO NOT USE pkgs.writeText IN PRODUCTION. This put secrets in the nix store!
tsigFile = pkgs.writeText "tsig.conf" ''
tsigFile = lib.types.secretFile.makeWorldReadable (pkgs.writeText "tsig.conf" ''
key:
- id: slave_key
algorithm: hmac-sha256
secret: zOYgOgnzx3TGe5J5I/0kxd7gTcxXhLYMEq3Ek3fY37s=
'';
'');
in {
name = "knot";
meta = with pkgs.lib.maintainers; {
Expand Down
8 changes: 4 additions & 4 deletions nixos/tests/miniflux.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,14 @@ let
defaultPort = 8080;
defaultUsername = "admin";
defaultPassword = "password";
adminCredentialsFile = pkgs.writeText "admin-credentials" ''
adminCredentialsFile = lib.types.secretFile.makeWorldReadable (pkgs.writeText "admin-credentials" ''
ADMIN_USERNAME=${defaultUsername}
ADMIN_PASSWORD=${defaultPassword}
'';
customAdminCredentialsFile = pkgs.writeText "admin-credentials" ''
'');
customAdminCredentialsFile = lib.types.secretFile.makeWorldReadable (pkgs.writeText "admin-credentials" ''
ADMIN_USERNAME=${username}
ADMIN_PASSWORD=${password}
'';
'');

in
with lib;
Expand Down