nixos/libvirtd: extraConfig goes first

[noonien]

Motivation for this change

In case of duplicate config names, libvirtd only uses the first value found.
This prevents users from defining auth_unix_ro and auth_unix_rw configs in
extraConfig.

Things done

The commit fixes this issue by putting extraConfig first.

• Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
• Built on platform(s)
  • NixOS
  • macOS
  • other Linux distributions
• 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 nixpkgs-review --run "nixpkgs-review wip"
• Tested execution of all binary files (usually in ./result/bin/)
• Determined the impact on package closure size (by running nix path-info -S before and after)
• Ensured that relevant documentation is up to date
• Fits CONTRIBUTING.md.

[aanderse]

It looks like replacing the extraConfig option with a settings option that takes an attribute set as a type might work here. What do you think about that?

[noonien]

That's probably a better options.

However, I'm not sure how removing the extraConfig option would affect existing configurations. And I didn't want to reduce the changes of this PR being accepted.

The settings option could probably be implemented in another PR?

[noonien]

Perhaps I should have mentioned it previously, but, the motivation for this change is to permit improving security for accessing the libvirt socket.

Since accessing the libvirt socket can basically give users root access, (like the docker socket does), and, because due to the way the module is currently written, the only way of accessing the socket as a non-root user, is to add it to the libvirt group, which basically makes sudo authentication useless.

The change I proposed, fixes this by allowing the necessary settings to be made so that libvirt can do SASL authentication, using PAM, which means adding a user to the libvirt group doesn't automatically give the user non-authenticated root access

[aanderse]

The settings option could probably be implemented in another PR?

Certainly could be if you're not interested in tackling that now.

I'm not overly familiar with this software so I can't speak to whether the original module author made a mistake or not. So I'll trust your judgement on whether a more flexible approach provides value or not:

60d9045

With that change you could do something like this:

# prepend the contents
virtualisation.libvirtd.extraConfig = lib.mkBefore ''
  auth_unix_ro = "something-else"
  auth_unix_rw = "something-else"
'';

# or replace the file entirely
virtualisation.libvirtd.extraConfig = lib.mkForce ''
  auth_unix_ro = "something-else"
  auth_unix_rw = "something-else"
'';

Again, I don't know the software so there might be no value in this... just thought I'd ask 🤷‍♂️

[noonien]

That works for me, however, what happens when existing configurations use this?

virtualisation.libvirtd.extraConfig = ''
    foo = "bar"
'';

Will the original content (setting the auth to polkit) be replaced? That means polkit will not be used any more, which is a breaking change, and probably a serious security flaw.

[aanderse]

With type = "lines"; the values will be merged and the current behavior will be preserved.

[noonien]

Perfect.

That implementation would be better then.

[aanderse]

Cool. Let me know what you want to do. Feel free to copy/paste the code I posted if you like.

[noonien]

I don't mind merging your commits. Or, do you want me to pull them into this PR?

[aanderse]

I thought you might just rewrite your commit to be like mine, but whatever you want to do. Please make sure you run a quick test... I have not!

[stale]

I marked this as stale due to inactivity. → More info