nixos/samba: migrate to structural settings (RFC42)

[anthonyroussel]

Description of changes

See RFC42

• Migrated services.samba.extraConfig to services.samba.settings
• Renamed services.samba.enableNmbd to nmbd.enable
• Renamed services.samba.enableWinbind to winbindd.enable
• Added services.samba.smbd.enable
  • This option will disable the smbd daemon
  • Required to continue the work on samba-ad-dc SystemD service
• Split SystemD configuration and sync it with upstream

Also see #302645

TODO / Work in progress:

• Improve new module documentation
• Fix SystemD services dependencies

Also planned later:

• Lint module with nixfmt-rfc-style (in another PR)
• Remove with lib; (in another PR)
• Migrate enable options to mkEnableOption, and maybe disable them by default (in another PR)
• Add samba-ad-dc support and ensureDomain provisioner (in another PR)

This PR is a requirement to start the work on Samba AD DC NixOS module.

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

---

Add a 👍 reaction to pull requests you find important.

[anthonyroussel]

@ofborg test samba

[anthonyroussel]

Fixed merge conflict

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/4040

[anthonyroussel]

Rebased against master branch and fixed merge conflict

[anthonyroussel]

Hello @trofi, I noticed that you've recently modified the Samba package.

So I thought you might be interested in reviewing this pull request to migrate the NixOS Samba module to RFC42 :)

[misuzu]

Looks like the global section must be always on top and after this change it can end up after shares declarations (if they have a name starting with uppercase letter) which breaks everything... It seems the global options are only applied for the shares after the global section, this might even be a security issue.

[anthonyroussel]

@misuzu Hello, thank you for pointing this out.

The smb.conf documentation does not mention anything about the section ordering, especially regarding the special global section.

But yes indeed, you're right, I was able to reproduce the issue you mentioned with the following configuration:

  services.samba = {
    enable = true;
    openFirewall = true;
    settings = {
      global = {
        "guest ok" = true;
      };
      "public" = {
        "path" = "/public";
        "browseable" = "yes";
        "comment" = "Public samba share.";
      };
      "Shared" = {
        "path" = "/shared";
        "browseable" = "yes";
        "comment" = "Shared samba share.";
      };
    };
  };
};

In this configuration, the public share will allow guest access, while the Shared share won't.

I see a few possible ways to address this issue:

• Split the settings into globalSettings and shareSettings, ensuring that globalSettings appears first in the configuration file and shareSettings after.
• Use a customized version of pkgs.formats.iniWithGlobalSection to automatically place the global section (and its [global] header) at the top of the configuration file.
• Revert the PR and restore the previous behavior (no RFC42).

Does anyone have any other ideas?

Unfortunately, I’m not available to work on this today.
Does anyone have any thoughts on whether we should consider reverting my PR, or we wait for me to work on a fix to be merged in a few days?

[bachp]

@anthonyroussel

There is no need to split the section from a user perspective. We can also do this via somthing along the lines of:

  globalSettings = { global = cfg.settings.global; };
  shareSettings = builtins.removeAttrs cfg.settings [ "global" ];

However I'm not sure how to tell formats.ini to put this two attrsets in the right order.

[bachp]

@misuzu @anthonyroussel

I think I found a solution in #341257

I just ran the samba module test but did not try yet with the above reproducer if it really fixes the issue. and tired with the above config and with the changes the order is restored.

[endocrimes]

Samba settings aren't strict INI so this breaks things - specifically configuring extensions like fruit:appl = yes for enabling the Darwin compatibility layer.

[anthonyroussel]

Hello! Thank you for the feedback.

Could you provide an example where this configuration isn’t working as expected, please?

To configure fruit:appl = yes for vfs_fruit, you can use this syntax:

services.samba.settings.global."fruit:appl" = "yes";

[brookst]

Hi. I've update the NixOS Wiki article: https://nixos.wiki/index.php?title=Samba&diff=13441&oldid=13357
I think that updates the snippets to be compatible with these changes. Perhaps someone with more familiarity ought to look over that article to see if any info has gone out-of-date.

Cheers!

[bachp]

Thank you @brookst.

There is also https://wiki.nixos.org/wiki/Samba which is linked from the official nixos.org page. Maybe it makes sense to update there too.