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

nixos-generate-config: Add system.copySystemConfiguration = true #67475

Open
wants to merge 1 commit into
base: master
from

Conversation

@Infinisil
Copy link
Member

commented Aug 26, 2019

Motivation for this change

This will be useful for the many people who accidentally delete their
configuration.nix file.

The alternative of changing the default of this option to true won't
work well because for some NixOS build, we can't assume for the
configuration file to be at the standard location. E.g. in nixops,
will point to something completely different than what
the nixops evaluation thinks, therefore leaking the nixops deploy users
config into the deployment machines. See also #16922

This continues #24707

In the future we might want to add an option like system.copySystemConfigurationDir = ./. which would copy the whole directory the configuration is in.

Ping @danbst @techhazard @bobvanderlinden @ryantrinkle @vcunat

Things done

  • Tested that it works with
diff --git a/nixos/tests/installer.nix b/nixos/tests/installer.nix
index a136678c6ef..4599d01e1ef 100644
--- a/nixos/tests/installer.nix
+++ b/nixos/tests/installer.nix
@@ -754,4 +754,13 @@ in {
     '';
   };
 
+  copySystemConfig = makeInstallerTest "copySystemConfig" (simple-test-config // {
+    extraConfig = ''
+      system.copySystemConfiguration = true;
+    '';
+    preBootCommands = ''
+      $machine->succeed("test -e /run/current-system/configuration.nix");
+    '';
+  });
+
 }

I don't think it's worth including this as another NixOS test though

This will be useful for the many people who accidentally delete their
configuration.nix file.

The alternative of changing the default of this option to true won't
work well because for some NixOS build, we can't assume for the
configuration file to be at the standard location. E.g. in nixops,
<nixos-config> will point to something completely different than what
the nixops evaluation thinks, therefore leaking the nixops deploy users
config into the deployment machines.
# still recover it from
# - /run/current-system/configuration.nix (last one)
# - /nix/var/nix/profiles/system-*/configuration.nix (old versions)
system.copySystemConfiguration = true;

This comment has been minimized.

Copy link
@danbst

danbst Aug 26, 2019

Contributor

Should this be false by default?

This comment has been minimized.

Copy link
@mmahut

mmahut Aug 26, 2019

Member

The option is false by default, but I think the point of this pull request is have it enabled for NixOS.

This comment has been minimized.

Copy link
@danbst

danbst Aug 26, 2019

Contributor

I'm saying this, because originally I've suggested "false" as default.

#24707 (comment)

My reasoning is like this:

  • default config should be close to real NixOS defaults.
  • ideally, we should take an idea from postgresql default config - list an option, but leave it commented, together with default value. If you uncomment - it is still default, you have both to uncomment and change to make thing happen.
  • configuration can have secrets embedded, in which case it shouldn't be copied to store by default. Or at least warned about that. We care about security.

This comment has been minimized.

Copy link
@mmahut

mmahut Aug 26, 2019

Member
  • configuration can have secrets embedded, in which case it shouldn't be copied to store by default. Or at least warned about that. We care about security.

If the secret is embedded and used, it ends up in the nix store anyway, no?

This comment has been minimized.

Copy link
@danbst

danbst Aug 26, 2019

Contributor

@mmahut 🤔 indeed. Okay, then I'd be fine with commented-by-default, as are other options.

This comment has been minimized.

Copy link
@Infinisil

Infinisil Aug 26, 2019

Author Member

I'd like for this to be true and uncommented because there's no harm in doing so, the worst thing it can do is increase the closure size a bit, but the benefit it provides will be appreciated by a lot. If problems like the one with nixops wouldn't exist I'd make this option's default = true;; changing the default here is the next best thing we can do.

This comment has been minimized.

Copy link
@danbst

danbst Aug 26, 2019

Contributor

I won't take a hard stance and counter this. I just want to keep consistency in the sample config file. The only uncommented options are vital ones - hardware config, boot loader config and state version. Everything else is "recommendation" and thus, commented out.

From pragmatic viewpoint, yes, it would be great to make it enabled for new users.

@mmahut
mmahut approved these changes Aug 26, 2019
@vcunat

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

Well, hopefully this comment makes it clear to most people that only this single file is copied. Otherwise I'd fear that we'd exchange small early loss for a larger later loss (due to not noticing soon, as it's natural to assume that all configuration is copied). I personally still think it might be better to instead somehow encourage version-controlling the config, but I'm not against this change.

@grahamc

This comment has been minimized.

Copy link
Member

commented Aug 31, 2019

My objection to this PR is the name of the option is a bit of a lie: It does not copy the system configuration, it copies maybe-some-of-it. Enabling a sort-of-broken feature by default turns on a footgun people might think they can rely on.

@mmahut

This comment has been minimized.

Copy link
Member

commented Aug 31, 2019

@grahamc that is a good point. I guess would not be very easy to eval the imports and copy them as well? Hmm.

@Infinisil

This comment has been minimized.

Copy link
Member Author

commented Aug 31, 2019

Well I think the message in this PR is clear enough that only that file is backed up. In addition I think a lot of people, especially beginners, only have a single configuration.nix anyways.

But yeah, maybe an option like system.copySystemConfigurationDir = ./. would be good to implement (as already suggested in earlier issues), which would copy all of /etc/nixos with that value, covering a much wider range of config setups.

@disassembler

This comment has been minimized.

Copy link
Member

commented Sep 1, 2019

I'm not a fan of protecting users from themselves. We have rollbacks, people can use version control for their nixos configs. I don't see much benefit in adding this option, and as @grahamc said above, it creates a false sense of security that their configs are backed up when really it's just configuration.nix, which I try to encourage people to keep as minimal as possible (ideally just imports) e.g. https://github.com/disassembler/network/blob/master/machines/irkutsk.nix is the configuration.nix I symlink for my main laptop.

@Infinisil

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2019

@disassembler I think we should protect users from themselves! Users (including me) do stupid things, and unless we have protective measures against this, bad stuff happens too easily. A good example is how rm received the --no-preserve-root flag.

While people can rollback, that doesn't allow recovering a Nix config. And while people can use version control, many do not, I presume many aren't even familiar with git.

The "false sense of security" argument is a good reason though, people with only a single configuration.nix might become accustomed to being able to recover it, but once they split it into multiple files they could get surprised that the others haven't been backed up the same way.

So I think a system.copySystemConfigurationDir = ./. would be a good way to solve this.

@danbst

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

@disassembler in my beginner times, I've lost configuration.nix once.

TBH I'd better prefer that /etc/nixos would be git-controlled (git add . && git commit on rebuild switch). It actually isn't something complicated to do - just a custom activationScript.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.