cynicsketch / nix-mineral

Conveniently and reasonably harden NixOS.
MIT License
150 stars 7 forks source link

Reformat nix-mineral.nix #1

Open cynicsketch opened 3 months ago

cynicsketch commented 3 months ago

The main module is currently somewhat disorganized, and should be reformatted in a logical way.

cynicsketch commented 3 months ago

An easy and effective way to make it more readable is to break it apart into many submodules like with the override. It will be slightly tedious but worth it in the long run for maintainability and modularity.

wyyllou commented 3 months ago

Wouldn't it be a bit better to change nix-mineral as a whole to one nixos module? As in merge nix-mineral and nm-overrides into a single importable module. It is my understanding this is a much more standard method of doing something like this.

An implementation like this might look like:

a nixos module called nix-mineral

options would be, enable, overrides.

example (assumed the user imported the module earlier):

nix-mineral = {
  enable = true;
  overrides = {
    desktop = {
      allow-multilib.enable = true;
      allow-unprivileged-userns.enable = true;
      home-exec.enable = true;
      tmp-exec.enable = true;
      var-lib-exec.enable = true;
      usbguard-disable.enable = true;
    };
    performance = {
      allow-smt.enable = true;
      no-pti.enable = true;
    };
    security = {
      tcp-timestamp-disable.enable = true;
    };
  };
};
wyyllou commented 3 months ago

In this implementation, i would suggest nix-mineral.enable to be by default true, to preserve the behavior of importing the nix mineral module and not changing or adding any overrides.

wyyllou commented 3 months ago

And possibly for each part of the config to be a default import, so if you dont want the filesystems hardening configuration at all, you can just turn it off and fileSystems would not be modified

cynicsketch commented 3 months ago

Yes, that would be ideal! Thanks for the idea. It should be doable with some copy pasting and precursory tests.

wyyllou commented 3 months ago

Yes, that would be ideal! Thanks for the idea. It should be doable with some copy pasting and precursory tests.

I would be willing to implement that at some point if you would like. I have some experience writing nixos modules :)

cynicsketch commented 3 months ago

I would be willing to implement that at some point if you would like. I have some experience writing nixos modules :)

I'd highly appreciate the help, since things are getting hectic for me soon.

wyyllou commented 3 months ago
  # ...
  overrides = {
    desktop = {
      allow-multilib.enable = true;
      allow-unprivileged-userns.enable = true;
      home-exec.enable = true;
      tmp-exec.enable = true;
      var-lib-exec.enable = true;
      usbguard-disable.enable = true;
    };
    performance = {
      allow-smt.enable = true;
      no-pti.enable = true;
    };
    security = {
      tcp-timestamp-disable.enable = true;
    };
    # ...

Maybe this code would be better understood if items were not all attrsets with .enable, and instead were all booleans, and overrides could be just replaced with settings. Also flip things like *-disable.enable. Something like:

nix-mineral = {
  enable = true;
  settings = {
    desktop = {
      allow-multilib = true;
      allow-unprivileged-userns = true;
      home-exec = true;
      tmp-exec = true;
      var-lib-exec = true;
      usbguard = false;
    };
    performance = {
      allow-smt = true;
      pti = false;
    };
    security = {
      tcp-timestamp = false;
    };
  };
};

Feels a lot more clean to me. One nit-pick i can foresee with removing the *-disable syntax is that the user may think enabling all performance options nets them more performance, while its disabling some that would, and enabling others, so maybe we could keep that just for the performance section?

cynicsketch commented 3 months ago

Feels a lot more clean to me. One nit-pick i can foresee with removing the *-disable syntax is that the user may think enabling all performance options nets them more performance, while its disabling some that would, and enabling others, so maybe we could keep that just for the performance section?

I think we can remove "enable" from most things, and just have it be implied that most options enable something. For others, it doesn't make sense to remove it entirely i.e tcp-timestamp forces tcp timestamps off when true ("disables" it).

As for users assuming things incorrectly, that's not supposed to happen with the basic documentation on the options, which if we merged nm-overrides with nix-mineral, would probably be best inserted into a separate .md file since the assumption would be that all users set the options elsewhere (e.g configuration.nix) and not directly edit any components of this module.

cynicsketch commented 2 months ago

https://github.com/cynicsketch/nix-mineral/pull/19

Starting on this. TODO: Transfer the rest of the overrides and break apart the defaults in to separate options so that nix-mineral.enable simply calls nix-mineral.default.filesystems = true; ... etc.