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/mailman: lots of big improvements #77444

Merged
merged 12 commits into from Jan 30, 2020
Merged

nixos/mailman: lots of big improvements #77444

merged 12 commits into from Jan 30, 2020

Conversation

@alyssais
Copy link
Member

@alyssais alyssais commented Jan 10, 2020

Motivation for this change

Our current Mailman module is quite lacking in a number of ways, including security issues:

  • Rather than being pulled in as a normal package, files from mailman-web were copied into Nixpkgs and modified. So updating mailman-web would be extremely difficult.
  • It was not possible to override mailman-web settings, which I suspect would be required for almost all deployments.
  • Security-sensitive things like SECRET_KEY were hard coded in these config files, meaning that not only were they stored in the Nix store, they were the same for all NixOS users.
  • The Hyperkitty API key was set as a NixOS option and stored in the Nix store.
  • Mailman web could only be run behind Apache httpd, rather than other popular web servers like uwsgi.
  • The Mailman service would not be restarted if its configuration file changed.
  • The mailman and mailman-web packages were not usable as normal packages, especially to non-NixOS users — they were only useful as installed and overridden by the module.
  • The module directly set order-dependent array values for Postfix configuration, meaning a user could not reliably further override those values in other modules without mkForce.
  • Custom archivers were basically impossible to use.

With these changes, all of these problems are resolved, and the Mailman module is much more useful. I’ve been using it pretty much like this for months, and it has worked perfectly.

Where necessary, I’ve explained the rationale for each change in more detail in the commit message. I strongly recommend reviewing commit by commit to understand why each change was made.

I’ve retained backwards compatibility as much as was practical, but some small changes will be required by users. The hyperkittyApiKey setting no longer makes sense, for example, and users will be required to set some Postfix configuration for reasons described above. All of this will be guided by error and warning messages.

Fixes #77274.

Things done
  • 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.
pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
pkgs/servers/mail/mailman/wrapped.nix Outdated Show resolved Hide resolved
@ofborg ofborg bot requested a review from globin Jan 10, 2020
@alyssais alyssais force-pushed the alyssais:mailman branch from 11ec743 to 64ff93b Jan 10, 2020
@alyssais
Copy link
Member Author

@alyssais alyssais commented Jan 10, 2020

@FRidh all done :)

Copy link
Contributor

@jonringer jonringer left a comment

related commits should probably be squashed, not really sure where to draw the line on the module commits, but 17 commits seems excessive if this is likely to be backported

pkgs/servers/mail/mailman/web.nix Show resolved Hide resolved
pkgs/top-level/python-packages.nix Outdated Show resolved Hide resolved
pkgs/servers/mail/mailman/web.nix Outdated Show resolved Hide resolved
pkgs/servers/mail/mailman/web.nix Show resolved Hide resolved
@alyssais
Copy link
Member Author

@alyssais alyssais commented Jan 10, 2020

related commits should probably be squashed, not really sure where to draw the line on the module commits, but 17 commits seems excessive if this is likely to be backported

-1 to squashing any of it. Would make it much more difficult to see what had changed.

I think it’s probably not worth backporting. There are security implications, but this is a big, breaking change, and it’s been like this for months…

@jcumming
Copy link

@jcumming jcumming commented Jan 10, 2020

This is a big improvement, thanks @alyssais !

@alyssais alyssais force-pushed the alyssais:mailman branch from 64ff93b to e6e6344 Jan 10, 2020
@alyssais
Copy link
Member Author

@alyssais alyssais commented Jan 10, 2020

@jonringer done

@tazjin
tazjin approved these changes Jan 11, 2020
Copy link
Member

@tazjin tazjin left a comment

LGTM, minor non-functional nitpicks.

pkgs/servers/mail/mailman/default.nix Outdated Show resolved Hide resolved
nixos/modules/services/mail/mailman.nix Show resolved Hide resolved
@globin
Copy link
Member

@globin globin commented Jan 13, 2020

You might want to have a look at https://github.com/mayflower/nixexprs/blob/master/modules/mailman3.nix, which predates this module, to see if you want to steal something. I deemed it too hacky to try and push it upstream at the time but it might be interesting to compare if there's anything there missing here. When this PR is merged I'll probably just dump it anyway in favour of this.

@alyssais alyssais force-pushed the alyssais:mailman branch from e6e6344 to 7639a22 Jan 13, 2020
@alyssais alyssais force-pushed the alyssais:mailman branch from 7639a22 to ef4dfeb Jan 14, 2020
@alyssais
Copy link
Member Author

@alyssais alyssais commented Jan 14, 2020

I’ve dropped e6f970d because of https://gitlab.com/mailman/mailman/merge_requests/594. I could have applied the patch to the mailman package, but since it was an entirely cosmetic change I think it’s better to just wait for it to be fixed in upstream, and then we can unify the config file locations.

@alyssais
Copy link
Member Author

@alyssais alyssais commented Jan 14, 2020

Mailman won’t actually be usable on NixOS until #75782 and #77686 (which has to make it through staging) are in master, fwiw.

Copy link
Member

@Infinisil Infinisil left a comment

Some suggestions based on NixOS/rfcs#42

COMPRESS_OFFLINE = true;
STATIC_ROOT = "/var/lib/mailman-web/static";
MEDIA_ROOT = "/var/lib/mailman-web/media";
} // cfg.webSettings;

This comment has been minimized.

@Infinisil

Infinisil Jan 16, 2020
Member

You can assign these defaults in the config section of the module with

{
  config.services.mailman.webSettings = {
    DEFAULT_FROM_EMAIL = mkDefault cfg.siteOwner;
    SERVER_EMAIL = mkDefault cfg.siteOwner;
    # ...
  };
}

Then you don't need to do this overriding here and makes the defaults inspectable.

This comment has been minimized.

@alyssais

alyssais Jan 30, 2020
Author Member

Not possible until #75584, and I don’t think this should have to wait until then.

nixos/modules/services/mail/mailman.nix Outdated Show resolved Hide resolved
alyssais added 3 commits Oct 20, 2019
Not everybody is using Apache.
A default of example.com is useful to nobody.  The correct value of
this depends on the system.
This replaces all Mailman secrets with ones that are generated the
first time the service is run.  This replaces the hyperkittyApiKey
option, which would lead to a secret in the world-readable store.
Even worse were the secrets hard-coded into mailman-web, which are not
just world-readable, but identical for all users!

services.mailman.hyperkittyApiKey has been removed, and so can no
longer be used to determine whether to enable Hyperkitty.  In its
place, there is a new option, services.mailman.hyperkitty.enable.  For
consistency, services.mailman.hyperkittyBaseUrl has been renamed to
services.mailman.hyperkitty.baseUrl.
alyssais added 3 commits Oct 22, 2019
@alyssais alyssais force-pushed the alyssais:mailman branch 2 times, most recently from 59644fc to d1f4e18 Jan 30, 2020
alyssais added 5 commits Jan 9, 2020
It's likely that a user might want to set multiple values for
relay_domains, transport_maps, and local_recipient_maps, and the order
is significant.  This means that there's no good way to set these
across multiple NixOS modules, and they should probably all be set
together in the user's Postfix configuration.

So, rather than setting these in the Mailman module, just make the
Mailman module check that the values it needs to occur somewhere, and
advise the user on what to set if not.
We already had python3Packages.mailman, but that's only really usable
as a library.  The only other option was to create a whole Python
environment, which was undesirable to install as a system-wide
package.
Previously, some files were copied into the Nixpkgs tree, which meant
we wouldn't easily be able to update them, and was also just messy.

The reason it was done that way before was so that a few NixOS
options could be substituted in.  Some problems with doing it this way
were that the _package_ changed depending on the values of the
settings, which is pretty strange, and also that it only allowed those
few settings to be set.

In the new model, mailman-web is a usable package without needing to
override, and I've implemented the NixOS options in a much more
flexible way.  NixOS' mailman-web config file first reads the
mailman-web settings to use as defaults, but then it loads another
configuration file generated from the new services.mailman.webSettings
option, so _any_ mailman-web Django setting can be customised by the
user, rather than just the three that were supported before.  I've
kept the old options, but there might not really be any good reason to
keep them.
This will allow users to provide other archiver plugins than the
default mailman-hyperkitty.
@alyssais alyssais force-pushed the alyssais:mailman branch from d1f4e18 to f2340cd Jan 30, 2020
@alyssais
Copy link
Member Author

@alyssais alyssais commented Jan 30, 2020

I plan on merging this today so it gets into 20.03. The only significant change I’ve made recently is adding the release notes. If anybody is up for reviewing the release notes, that would be much appreciated.

@alyssais alyssais force-pushed the alyssais:mailman branch from f2340cd to 360c252 Jan 30, 2020
@alyssais alyssais merged commit 6ea79d2 into NixOS:master Jan 30, 2020
16 checks passed
16 checks passed
mailman, mailman-web on x86_64-darwin No attempt
Details
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
mailman, mailman-web on aarch64-linux Success
Details
mailman, mailman-web on x86_64-linux Success
Details
@alyssais alyssais deleted the alyssais:mailman branch Jan 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.