rmilter: Fix a couple of bugs #21866

pjones · 2017-01-13T22:28:14Z

Motivation for this change

The module uses stringSplit but it should be splitString
rmilter doesn't actually support binding to multiple sockets.
Therefore, bind to the last one specified if socketActivation is
false.

I also believe there is a bug in this module related to systemd
ListenStream. If socketActivation is true, Postfix gets
connection timeouts trying to connect to one of the ListenStream
inet addresses. I don't know enough about ListenStream passing
connections on to fd:3 to understand what's going on.

These changes are in production (with socketActivation = false) via NixOps.

Things done

Tested using sandboxing
(nix.useSandbox on NixOS,
or option build-use-sandbox in nix.conf
on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- Linux
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

mention-bot · 2017-01-13T22:28:15Z

@pjones, thanks for your PR! By analyzing the history of the files in this pull request, we identified @avnik, @fpletz and @joachifm to be potential reviewers.

avnik · 2017-01-13T23:14:01Z

LGTM!
I will address question about socked activation to upstream author.

Mic92 · 2017-01-13T23:24:35Z

When multiple sockets are specified systemd socket activation will pass them as file descriptors from fd:3 upwards (fd:3, fd:4, ..., fd:n)

pjones · 2017-01-13T23:50:26Z

When multiple sockets are specified systemd socket activation will pass them as file descriptors from fd:3 upwards (fd:3, fd:4, ..., fd:n)

Then that also causes a problem since rmilter can only bind/listen to one address.

Mic92 · 2017-01-14T08:33:21Z

If it only support one socket address at the time, the options should not allow to specify multiple. Picking the last socket does not make sense to me.

avnik · 2017-01-14T10:40:04Z

actually socket activation needs only because libmilter can't setup proper permissions on unix socket, but systemd can.

avnik · 2017-01-14T12:08:38Z

rmilter unable to listen more than one socket/port, due libmilter desing flaw

Mic92 · 2017-01-14T15:00:16Z

socket activation is also useful to prevent race conditions between connecting smtp daemons and rmilter on startup, because systemd create listening sockets before any service is started.

fpletz

👍 on the changes, but this will break current setups using unix sockets and socketActivation disabled. The sockets will have 777 permissions.

Could you please add one or two sentences to the release notes for 17.03? Thanks!

avnik · 2017-01-20T11:56:20Z

@fpletz It why I implement socket activation in module (and also why upstream author add it), libmilter unable to set proper permissions on socket (as well, as listen to more than one socket)

pjones · 2017-01-21T21:26:29Z

@avnik If rmilter can't listen on more than one socket the Nix module shouldn't allow someone to configure more than one socket.

avnik · 2017-01-23T06:59:56Z

On Sat, Jan 21, 2017 at 01:26:31PM -0800, Peter J. Jones wrote: @avnik If rmilter can't listen on more than one socket the Nix module shouldn't allow someone to configure more than one socket.

Correct. Will try to improve it from my side (unless you want to do it as part of this PR)

pjones · 2017-01-24T17:45:27Z

@avnik If you have time please go ahead and work on it. I've got a lot on my plate right now so this is going to have to sit on the back burner for now.

pjones · 2017-01-24T17:49:09Z

@fpletz Maybe we should hold off on this PR until @avnik can change the interface for configuring the listening socket. Changing the interface is going to be better than what I'm doing here.

Also, @avnik, if you come up with something let me know and I can test it.

fpletz · 2017-01-28T01:51:18Z

I've implemented support for only one socket and deprecated the two old options. Also rebased to current master.

@avnik @pjones What do you think?

avnik · 2017-01-28T13:33:26Z

@fpletz thank you. Your changes looks good.

pjones · 2017-01-28T20:21:28Z

nixos/modules/services/mail/rmilter.nix


  rmilterConf = ''
    pidfile = /run/rmilter/rmilter.pid;
-    bind_socket = ${if cfg.socketActivation then "fd:3" else last inetSockets};
+    bind_socket = ${if cfg.socketActivation then "fd:3" else rspamdSocket};


Shouldn't rspamdSocket be rmilterSocket instead?

Oh, right! I've actually never tested this with a ~~rspamd~~ postfix but will before merging of course. Thanks for reading my changes carefully! 👍

pjones · 2017-01-28T20:23:18Z

I added one comment, and I also think the description for the socketActivation option needs to be updated since it doesn't actually force rmilter to use an inet socket now.

* The module uses `stringSplit` but it should be `splitString` * `rmilter` doesn't actually support binding to multiple sockets. Therefore, bind to the last one specified if `socketActivation` is `false`. I also believe there is a bug in this module related to systemd `ListenStream`. If `socketActivation` is true, Postfix gets connection timeouts trying to connect to one of the `ListenStream` inet addresses. I don't know enough about `ListenStream` passing connections on to `fd:3` to understand what's going on. These changes are in production (with `socketActivation = false`) via NixOps.

fpletz · 2017-03-17T22:04:24Z

Rebased, fixed the issues @pjones mentioned and also tightened the permissions so only users in the rmilter or rspamd groups can access the unix sockets.

fpletz · 2017-03-17T22:10:43Z

nixos/modules/services/mail/rmilter.nix

-milter_protocol = 6
-milter_mail_macros = i {mail_addr} {client_addr} {client_name} {auth_authen}
-# skip mail without checks if milter will die
-milter_default_action = accept


One more thing: I removed milter_default_action here because it would get appended last in my main.cf so I couldn't overwrite it anymore. This should be set in services.postfix.extraConfig manually if required IMHO. The postfix default is tempfail which should be fine.

For what it's worth, I don't use this feature of the package. I add the milter stuff to postfix by hand because my needs are a bit different.

fpletz requested changes Jan 14, 2017

View reviewed changes

fpletz added 6.topic: nixos 8.has: module (update) 9.needs: changelog labels Jan 14, 2017

fpletz added this to the 17.03 milestone Jan 14, 2017

globin assigned fpletz Jan 22, 2017

fpletz force-pushed the pjones/rmilter branch from 7fd9728 to 4ac0aee Compare January 28, 2017 01:49

fpletz approved these changes Jan 28, 2017

View reviewed changes

fpletz removed the 9.needs: changelog label Jan 28, 2017

pjones commented Jan 28, 2017

View reviewed changes

pjones and others added 3 commits March 17, 2017 20:15

rmilter service: support only one socket

8ab2d2e

rmilter/rspamd service: tighten unix socket permissions

00239ce

fpletz force-pushed the pjones/rmilter branch from 4ac0aee to 00239ce Compare March 17, 2017 22:03

fpletz reviewed Mar 17, 2017

View reviewed changes

fpletz merged commit 295a824 into NixOS:master Mar 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rmilter: Fix a couple of bugs #21866

rmilter: Fix a couple of bugs #21866

pjones commented Jan 13, 2017

mention-bot commented Jan 13, 2017

avnik commented Jan 13, 2017

Mic92 commented Jan 13, 2017

pjones commented Jan 13, 2017

Mic92 commented Jan 14, 2017

avnik commented Jan 14, 2017

avnik commented Jan 14, 2017

Mic92 commented Jan 14, 2017 •

edited

fpletz left a comment

avnik commented Jan 20, 2017

pjones commented Jan 21, 2017

avnik commented Jan 23, 2017 via email

pjones commented Jan 24, 2017

pjones commented Jan 24, 2017

fpletz commented Jan 28, 2017

avnik commented Jan 28, 2017

pjones Jan 28, 2017

fpletz Jan 29, 2017 •

edited

pjones commented Jan 28, 2017

fpletz commented Mar 17, 2017

fpletz Mar 17, 2017

pjones Mar 18, 2017

rmilter: Fix a couple of bugs #21866

rmilter: Fix a couple of bugs #21866

Conversation

pjones commented Jan 13, 2017

Motivation for this change

Things done

mention-bot commented Jan 13, 2017

avnik commented Jan 13, 2017

Mic92 commented Jan 13, 2017

pjones commented Jan 13, 2017

Mic92 commented Jan 14, 2017

avnik commented Jan 14, 2017

avnik commented Jan 14, 2017

Mic92 commented Jan 14, 2017 • edited

fpletz left a comment

Choose a reason for hiding this comment

avnik commented Jan 20, 2017

pjones commented Jan 21, 2017

avnik commented Jan 23, 2017 via email

pjones commented Jan 24, 2017

pjones commented Jan 24, 2017

fpletz commented Jan 28, 2017

avnik commented Jan 28, 2017

pjones Jan 28, 2017

Choose a reason for hiding this comment

fpletz Jan 29, 2017 • edited

Choose a reason for hiding this comment

pjones commented Jan 28, 2017

fpletz commented Mar 17, 2017

fpletz Mar 17, 2017

Choose a reason for hiding this comment

pjones Mar 18, 2017

Choose a reason for hiding this comment

Mic92 commented Jan 14, 2017 •

edited

fpletz Jan 29, 2017 •

edited