nixos: improve predictability and consistency in directory creation and permissions-setting

tomeon commented 2 years ago

Purpose

This PR is intended to make Impermanence's directory creation and permissions-setting logic more predictable and consistent. In brief, the main change consists in topologically sorting directory specifications according to reasonably straightforward and (hopefully) sensible rules, such that users can have a pretty good idea of the order in which Impermanence will create the directories and be able to predict what permissions Impermanence will assign them.

Summary of changes

Topological sorting rules

Please see the comments to the dirBefore function for a description of the rules used for ordering directory and parentDirectory specifications.

The main rule is that parent paths come before child paths. This applies to both "source" paths (i.e., child paths of persistent storage directories) and "destination" paths (the paths where things will be linked or bind-mounted).

The `root` option

This PR adds a root attribute to directory and file specifications that indicates the base output path. By default, this is /. This change is not essential to the rest of this PR and could be handled in a separate PR if you would prefer that.

Directory creation and permissions-setting

Implicit versus explicit directories

This changeset introduces the notion of an "implicit" directory -- a kind of hidden entry in the environment.persistence.${persistentStoragePath}.directories list. Such hidden entries represent the parent directories (and grandparent directories, etc., all the way back to the persistentStoragePath itself) of explicitly-specified files and directories.

Implicit directories differ from explicit directories in that their associated mode, user, and group settings are advisory, in the following senses:

If the source path (path under persistent storage) of an implicit directory needs to be created, and the destination path already exists, the source path will be created using the mode, user and owner of the destination path, and
If an explicitly-specified directory and an implicit directory have the same destination path or source path, the settings associated with the explicitly-specified directory take precedence with respect to the identical path or paths.

Among its other effects, the logic described above addresses the fact that, previously, configurations like:

{
  environment.persistence.users.me = {
    home = "/home/me";
    files = [ ".somerc" ];
  };
}

resulted in setting the mode on /home/me to 0755.

Atomicity

Given the configuration:

{
  environment.persistence."/state".directories = [ "/foo/bar/baz" ];
}

Each component of the source and destination paths (relative to the persistent storage directory and root directory, respectively) is now created atomically.

That is, /state/foo, /state/foo/bar, and /state/foo/bar/baz are created atomically, as are /foo, /foo/bar, and /foo/bar/baz (note that /state is not necessarily created atomically).

These directories are also effectively chowned and chmodded atomically, using the trick of creating a temporary directory at a sibling path of the "final" directory, chowning and chmodding that directory, then rename-ing it to the final path.

Additional validations and assertions

Inability to topologically sort directories

If sort order is undecidable, the NixOS module will raise an error mentioning the problem and common causes thereof.

"Recursive" bind mounts and symlinks

The NixOS module now forbids having files and directories bind mounted or symlinked into persistent storage directories. Please see this section of the README for details.

Path traversal

As described in the README, the NixOS module will raise an error if provided any path specifications that contain .. elements that cannot be resolved/eliminated lexically. For instance, foo/../bar is fine (it can be simplified to just bar without looking at the filesystem and without "escaping" the persistent storage path), but ../foo/bar is not.

Inconsistent permissions settings

This changeset updates the NixOS module to forbid inconsistent permissions settings; please see this bit of the README.

Backward (in)compatibility

Loud breakage

This changeset narrows the space of valid Impermanence configurations. It's likely that the new validations and assertions described above will break someone's NixOS system config. I've tried to make newly-introduced error messages informative enough that users should be able to track down problems in short-ish order, but naturally that's not as nice as being backward-compatible (even if the compatibility is with arguably-subtly-broken configs).

Quiet breakage

It's possible that some users rely on Impermanence's current de facto directory sorting logic, or permissions-setting scheme. The changes in this PR could break such users' setups in ways that may not be immediately apparent. However, because this changeset retains the "don't touch permissions on existing source directories" and "always set the permissions on destination directories to the permissions on the corresponding source directories" logic, my hope is that the surface area for this kind of breakage is fairly small -- for instance, it may only be an issue if standing up a new system where persistent storage directories don't already exist.

Test results

Failing GitHub Actions build with new flake checks, but without changes to the nixos.nix module or create-directories.bash script: here.

Passing GitHub Actions build with changes to the nixos.nix module and create-directories.bash script: here.

Other considerations

Licensing

The topological sorting implementation is inspired and informed by fsBefore from <nixpkgs>/nixos/lib/utils.nix, which is used for topologically sorting config.fileSystems. Though this changeset does not copy code from fsBefore, the domains and even implementations are similar enough that it may be prudent to cite fsBefore and embed a copy of the MIT license. WDYT?

Possible improvements

Add an explicit `createDirectories` list

This would allow callers to control directory permissions at any and all levels of a persistent storage hierarchy. For instance:

{
  environment.persistence."/abc" = {
    directories = [
      { directory = "/foo/bar"; mode = "0750"; }
    ];

    files = [
      { file = "/bleep/bloop/.blarprc"; parentDirectory.mode = "0700"; }
    ];

    createDirectories = [
      { directory = "/foo"; mode = "0755"; }
      { directory = "/bleep"; mode = "0750"; }
    ];
  };
}

Every entry in directories would have a corresponding (implicit) entry in createDirectories for the directory in question, and every entry files would have a corresponding (implicit) entry in createDirectories for the parent directory of the file in question.

Thus the final directory scheme for the above would be:

[drwxr-xr-x]  /abc
├── [drwxr-x---]  bleep
│   └── [drwx------]  bloop
└── [drwxr-xr-x]  foo
    └── [drwxr-x---]  bar

Allow callers to directly define sorting priority

Perhaps it would make sense to add a priority attribute to directory specifications that allows callers to tell Impermenance whether a given directory should be created earlier or later than others, along the lines of lib.mkForce, lib.mkDefault, etc.?

For instance:

{
  environment.persistence."/abc".directories = [
    { directory = "/foo"; priority = 1000; }
    { directory = "/bar"; priority = 50; }
  ];
}

And perhaps, in addition, support lib.mkForce, etc., directly?:

{ lib, ... }:

{
  environment.persistence."/abc".directories = [
    (lib.mkDefault "/foo")
    (lib.mkForce "/bar")
  ];
}

This may be most useful as a tiebreaker in case the directory-sorting comparator cannot otherwise determine which operand comes "before" the other.

Final thoughts

I realize and regret that this PR is not really "first-time contribution to a project" material. It's a big bunch of changes that maintains syntactic compatibility with the existing NixOS module, but definitely not semantic compatibility.

On the other hand, I think the motivation for and results of these changes are sensible and defensible. The directory permissions that lead to testing errors here strike me as surprising and, in some cases, wrong -- particularly the user's home directory having mode 0755.

If you are open to working with me to get this over the finish line, I would welcome any and all feedback.

Thank you very much for your consideration.

talyz commented 1 year ago

Hi tomeon! Thank you for putting in all this work - it looks really impressive! It's also a huge changeset, almost all of it in one commit, which makes it harder to review and pinpoint bugs caused by it. Can you please split it up? I would expect one commit per change, e.g: sorting, root option, implicit directories, atomicity and each assertion which isn't directly related to a change.

rehno-lindeque commented 1 year ago

I came to this PR wondering about permissions myself.

Are folks convinced that

{ file = "..."; parentDirectory = { user = "..."; group = "..."; }; } # implicitly bubble up permissions to parent paths

and

{ directory = "..."; user = "..."; group = "..."; }; # implicitly bubble up permissions to parent paths

Is actually a good API with the complexity involved in sorting and then bubbling up permissions behind the scenes? I'm not personally too convinced it's adding much to the ergonomics to be honest.

Instead of...

environment.persistence = {
    users.me = {
      directories = [

        # ~/.local is created with ownership implicitly set to me:users
        # ~/.local/share is created with ownership implicitly set to me:users
        { directory = ".local/share/nix"; user = "me"; group = "users; } 

        # ~/.local is not created because "nix" comes before "nvim"
        # ~/.local/share is not created because "nix" comes before "nvim"
        # (If nix and nvim were swapped, you need to learn about setting ordering priorities)
        { directory = ".local/share/nvim"; user = "nvim"; group = "nvim"; } 

      ];
    };
};

...maybe it would be more clear for everyone involved if the API did not bubble up permissions? (That is always be explicit, never implicit.)

environment.persistence = {
    users.me = {
      directories = [
        ".local/share/nix"

        # (optional) shorthand for setting permissions inline
        { directory = ".local/share/nvim"; user = "nvim"; group = "nvim"; }
      ];

      permissions = {

        # ~/.local is created with default ownership root:root (entirely unaffected by permissions of its children - so files and subdirectories)     
        # ~/.local/share is created with ownership me:users 
        ".local/share" = { user  = "me"; group = "users; };

      };
    };
};

This is perhaps similar to the createDirectories idea that @tomeon mentioned.

Alternative naming ideas for permissions: extraPermissions / overridePermissions / ephemeralPermissions

Additionally one could potentially have

environment.persistence = {
    users.me = {
      directories = [
        ".local/share/nix"

        # (optional) shorthand for setting permissions inline
        { directory = ".local/share/nvim"; user = "nvim"; group = "nvim"; }
      ];

      # default ownership for ~/.local/share, ~/.local/share/nix is me:users
      defaultPermissions = { user = "me"; group = "users"; }; 

      permissions = {

        # protect ~/.local (set ownership to root:root)
        ".local" = { user = "root"; group = "root"; }; 

      };
    };
};

This is just a thought though, don't let me hold up the PR which already sounds like a nice improvement.

rehno-lindeque commented 1 year ago

(Alternative naming ideas for createDirectories: paths / unmanagedPaths / ephemeralPaths / implicitDirectories / implicitPaths)

tomeon commented 1 year ago

@talyz -- I've split up the changes into what I hope is a reasonably granular sequence. Found and fixed a few typos and other issues on the way, too :). Thank you for prompting me to disaggregate the much-too-big initial commits.

There are some commits that might still encompass too much. Examples:

d0741c4 - this changeset includes the addition of files and directories (internal) submodule options like prefix, source, destination, etcetera. It also includes the switch from mkOption's apply to lib.types.coercedTo when converting stringy files and directories specifications into attrsets. The commit message attempts to explain why both changes are included (basically, continuing to use the apply approach would have meant manually filling out values for the prefix, source, destination, ..., values when converting a string to an attrset, whereas using lib.types.coercedTo makes the NixOS module system do that work for us).
fff1226: has a high incidental-changes quotient.

Please LMK if you'd like me to tackle further breaking up these or other commits.

Please also note that I've moved the NixOS VM test into a fairly early position in the commit list, but with the bits that check mode and ownership disabled. This is to help ensure that no commits in between the introduction of the test and the commit that enables mode and ownership assertions break directory creation (even though they may break or otherwise alter the directory-permissions-setting logic). The moral of the story: nix flake check now passes for for all commits in this branch (tested with git rebase -i exec 'nix flake check -L').

talyz commented 1 year ago

I also started working on this issue a little while back, since I found the issue interesting, not knowing you had picked it back up. I have now pushed my changes to https://github.com/nix-community/impermanence/tree/dir-creation-order. There is some overlap between our solutions, but the main issue is solved differently. I'm creating directory entries for all parent directories in nix, simplifying the shell code significantly.

I'm obviously biased here and would prefer my solution, but there are still things from this PR I would very much like to merge - the tests, extended assertions, fixes, etc, which all look amazing! Maybe we should also sync our efforts going forward - I've created a matrix channel for the project at https://matrix.to/#/#impermanence:nixos.org.

nix-community / impermanence