DKIM signing not working in bionic

Bug #1770532 reported by Adam Jacobs on 2018-05-11
52
This bug affects 6 people
Affects Status Importance Assigned to Milestone
amavisd-new (Debian)
Confirmed
Unknown
amavisd-new (Ubuntu)
High
Unassigned
Bionic
Undecided
 Christian Ehrhardt 
Cosmic
High
Unassigned

Bug Description

[Impact]

 * There is a known upstream issue in 2.0.11 breaking DKIM signing.
   - https://bugzilla.redhat.com/show_bug.cgi?id=1364730
   - https://lists.amavis.org/pipermail/amavis-users/2018-February/005292.html

 * given the activity on the report it seems plenty of people set this up
   pre-Bionic and are now running into these failures on upgrade to the
   current LTS.

 * Add a fix to avoid more people being hit by this on upgrade and forced
   to deploy workarounds (or drop the functionality)

[Test Case]

 * Setup amavisd for DKIM signing, see
   https://www.ijs.si/software/amavisd/amavisd-new-docs.html#dkim
   or any of
   https://www.faqforge.com/linux/how-to-enable-dkim-email-signatures-in-amavisd-new-and-ispconfig-3/
   https://nwgat.ninja/setting-up-dkim-and-spf-with-amavis-on-ubuntu-16-04-2/
   ...
   There seem to be a lot all doing the same essential steps.

   TL;DR would be:
   $ apt install amavisd-new
   $ mkdir -p /var/db/dkim/
   $ amavisd-new genrsa /var/db/dkim/example-foo.key.pem
   Add in /etc/amavis/conf.d/21-ubuntu_defaults
$enable_dkim_signing = 1;
dkim_key('example.com', 'foo', '/var/db/dkim/example-foo.key.pem');
@dkim_signature_options_bysender_maps = (
{ '.' => { ttl => 21*24*3600, c => 'relaxed/simple' } } );
@mynetworks = qw(0.0.0.0/8 127.0.0.0/8 10.0.0.0/8 172.16.0.0/12
192.168.0.0/16); # list your internal networks
- Now showkeys will report your key including the pblic key you'll need
 - amavisd-new showkeys
- add the public key (as displayed) to your DNS zone, increment SOA sequence number and reload DNS;
- then test signing and a published key
   - amavisd-new testkeys

Never the less you'd need to setup a lot of details and it feels unclear if you test the right thing, therefor my preference is with so many users reporting about the issue to rely on them to test their real setups.

[Regression Potential]

 * Lacking upstream being active there is always a chance things are
   missed, but multiple people came up with very similar solutions and
   multiple people tested these successfully.
   The actual change sets the originating flag where it is needed on the
   creation of dkim signatures.
   Due to that setups not triggering dkim_make_signatures should be not
   affected at all. And those that use dkim_make_signatures are those
   failing now due to the issue.

[Other Info]

 * Upstream seems essentially dead atm, so it is on the community (users
   reporting patches on the ML) and the Distributions (e.g. Fedora have
   taken a very similar change) alone for now.
 * For some extra confidence I'd ask for some extra time in proposed for
   this update.

----

Upon upgrading to bionic, amavisd-new DKIM signing no longer works.

A quick google search reveals that this is a known bug in amavisd 2.11.0:

https://bugzilla.redhat.com/show_bug.cgi?id=1364730
https://lists.amavis.org/pipermail/amavis-users/2018-February/005292.html

The redhat bug includes a proposed (one-line) patch. Fedora has already taken up this patch in their repo. I've applied the patch to my bionic server and it is a good fix there, too.

Requesting that ubuntu also includes this patch in its repo.

ProblemType: Bug
DistroRelease: Ubuntu 18.04
Package: amavisd-new 1:2.11.0-1ubuntu1 [modified: usr/sbin/amavisd-new]
ProcVersionSignature: Ubuntu 4.15.0-20.21-generic 4.15.17
Uname: Linux 4.15.0-20-generic x86_64
ApportVersion: 2.20.9-0ubuntu7
Architecture: amd64
Date: Thu May 10 18:57:32 2018
PackageArchitecture: all
ProcEnviron:
 TERM=xterm-256color
 PATH=(custom, no user)
 XDG_RUNTIME_DIR=<set>
 LANG=en_US.UTF-8
 SHELL=/bin/bash
SourcePackage: amavisd-new
UpgradeStatus: Upgraded to bionic on 2018-05-10 (0 days ago)
modified.conffile..etc.amavis.conf.d.15-content_filter_mode: [modified]
modified.conffile..etc.amavis.conf.d.50-user: [modified]
mtime.conffile..etc.amavis.conf.d.15-content_filter_mode: 2016-12-11T19:39:20.357027
mtime.conffile..etc.amavis.conf.d.50-user: 2017-06-19T06:44:56.517411

Related branches

Adam Jacobs (bllfr0g) wrote :
Joshua Powers (powersj) on 2018-05-14
Changed in amavisd-new (Ubuntu):
status: New → Confirmed
importance: Undecided → High
Andreas Hasenack (ahasenack) wrote :

There are two patches floating out there:

a) https://lists.amavis.org/pipermail/amavis-users/2016-July/004428.html (the one you applied I believe)
--- amavisd.orig Tue Apr 26 21:24:33 2016
+++ amavisd Fri Jul 1 01:03:15 2016
@@ -34338,6 +34329,7 @@ sub collect_some_dkim_info($) {
     $sig_ind++;
   }
   Amavis::load_policy_bank($_,$msginfo) for @bank_names;
+ $msginfo->originating(c('originating'));
   $msginfo->dkim_signatures_valid(\@signatures_valid) if @signatures_valid;
 # if (ll(5) && $sig_ind > 0) {
 # # show which header fields are covered by which signature

b) https://lists.amavis.org/pipermail/amavis-users/2018-February/005297.html
--- amavisd.orig Tue Apr 26 21:24:33 2016
+++ amavisd Fri Aug 5 12:32:39 2016
@@ -22806,6 +22806,7 @@ sub process_smtp_request($$$$) {
         }
         # load policy banks from the 'client_ipaddr_policy' lookup
         Amavis::load_policy_bank($_,$msginfo) for @bank_names_cl;
+ $msginfo->originating(c('originating'));

         $msginfo->client_addr($cl_ip); # ADDR
         $msginfo->client_port($cl_port); # PORT
@@ -34338,6 +34330,7 @@ sub collect_some_dkim_info($) {
     $sig_ind++;
   }
   Amavis::load_policy_bank($_,$msginfo) for @bank_names;
+ $msginfo->originating(c('originating'));
   $msginfo->dkim_signatures_valid(\@signatures_valid) if @signatures_valid; # if (ll(5) && $sig_ind > 0) {
 # # show which header fields are covered by which signature

It's unfortunate upstream is no longer responsive.

Would you perhaps be able to comment on or even test the second patch?

Andreas Hasenack (ahasenack) wrote :

Confirmed fedora is using the first (one-liner) patch:

$ cat amavisd-new-2.11.0-detect_originating_email.patch
From giovanni at paclan.it Fri Jul 1 09:35:49 2016
From: giovanni at paclan.it (Giovanni)
Date: Fri, 1 Jul 2016 07:35:49 +0000 (UTC)
Subject: dkim not working after upgrading to 2.11.0
Message-ID: <nl56gl$f40$<email address hidden>>

Hi,
after upgrading to 2.11.0, DKIM signing does not work anymore because
the email is no more detected as ORIGINATING.
This diff fixes the issue.
 Cheers
  Giovanni

--- amavisd.orig Tue Apr 26 21:24:33 2016
+++ amavisd Fri Jul 1 01:03:15 2016
@@ -34338,6 +34329,7 @@ sub collect_some_dkim_info($) {
     $sig_ind++;
   }
   Amavis::load_policy_bank($_,$msginfo) for @bank_names;
+ $msginfo->originating(c('originating'));
   $msginfo->dkim_signatures_valid(\@signatures_valid) if @signatures_valid;
 # if (ll(5) && $sig_ind > 0) {
 # # show which header fields are covered by which signature

Adam Jacobs (bllfr0g) wrote :

Correct, I am running patch (a), the same as included in Fedora, and it fixes the bug for me.

Before implementing the patch I spent a little time trying to understand what it does. Best I can tell, it is appropriate to call $msginfo->originating(c('originating')) after ANY call to load_policy_bank(). Indeed, that was the case in release 2.10.

Patch (b) (which subsumes patch (a)) looks good to me too, though I haven't tested it. Basically, there are a number of different ways in amavisd to apply a policy bank: the different places load_policy_bank() gets called correspond to different ways a policy bank can be triggered, and it just so happens that the way I'm applying the policy bank in my use case corresponds to the call from patch (a.)

All that said, I think both (a) and (b) are both poor fixes. It looks like upstream was TRYING, in 2.11, to remove the requirement to call $msginfo->originating(c('originating')) after each invocation of load_policy_bank() by moving a call to $msginfo->originating(c('originating')) to the end of the load_policy_bank() sub itself. Diffing 2.10 and 2.11, $msginfo->originating(c('originating')) is removed from after each call to load_policy_bank, and added to the very end of load_policy_bank itself. That said, the new call at the end of load_policy_bank() does not seem to be effective (this is the actual bug.) I tried to figure out why it isn't working; it seems to me that it should, since msginfo is an object reference (I think), so I'd think that calling a method on the object ought to be a persistent change that survives the return of the sub, but apparently not. My perl isn't good enough to know why this isn't working, and an hour or two of googling about perl objects didn't reveal the answer.

Someone who actually knows perl could probably propose the "right" fix in minutes, and I'd be happy to test it in that case.

Adam Jacobs (bllfr0g) wrote :

Okay, I took another look and now I understand why the author's change in 2.11 breaks DKIM signing in certain cases. The problem is that he added $msginfo->originating(c('originating')) to the end of load_policy_bank(), which seems like the right thing to do, except for certain kinds of policy bank, $msginfo isn't yet available when the policy bank is loaded, so the "originating" flag isn't even available yet to be set.

What that in mind, in lieu of patches (a) and (b), I've created a new patch, (c), which I believe is comprehensive and should be a good fix for all use cases. I'm running it now on my server and it's working perfectly:

--- amavisd-new.orig 2018-05-10 18:35:58.566308979 -0700
+++ amavisd-new 2018-05-15 18:09:57.749070821 -0700
@@ -33570,6 +33570,7 @@
   my $allowed_hdrs = cr('allowed_added_header_fields');
   my $from_str = join(', ', qquote_rfc2821_local(@rfc2822_from)); # logging
   substr($from_str,100) = '[...]' if length($from_str) > 100;
+ $msginfo->originating(c('originating'));
   if (!$allowed_hdrs || !$allowed_hdrs->{lc('DKIM-Signature')}) {
     do_log(5, "dkim: inserting a DKIM-Signature header field disabled");
   } elsif (!$msginfo->originating) {

Andreas Hasenack (ahasenack) wrote :

Would you be able to start a discussion about this new patch in the amavis-users mailing list?

https://lists.amavis.org/cgi-bin/mailman/listinfo/amavis-users

OK

https://lists.amavis.org/pipermail/amavis-users/2018-May/005364.html

On 05/25/2018 12:05 PM, Andreas Hasenack wrote:
> Would you be able to start a discussion about this new patch in the
> amavis-users mailing list?
>
> https://lists.amavis.org/cgi-bin/mailman/listinfo/amavis-users
>

Stephen Day (sd) wrote :

Upstream development doesn't seem active. We are probably all maintaining our own packages for this. Can we patch this in ubuntu?

Robie Basak (racb) on 2018-08-13
tags: added: server-next
Simon Déziel (sdeziel) wrote :

The 3 patches proposed on the mailing list:

Giovanni's old: https://lists.amavis.org/pipermail/amavis-users/2018-February/005292.html
Giovanni's new: https://lists.amavis.org/pipermail/amavis-users/2018-February/005297.html
Adam Jacobs': https://lists.amavis.org/pipermail/amavis-users/2018-May/005364.html

Giovanni's new patch was confirmed to work by 1 user and Adam's patch too (different testing users though).

Simon Déziel (sdeziel) wrote :

Fedora 28 integrated Giovanni's old patch.

Changed in amavisd-new (Debian):
status: Unknown → Confirmed
Andreas Hasenack (ahasenack) wrote :

Thanks for the summary, @sdeziel

Zhang Huangbin (michaelbibby) wrote :

Any change we can have this patch in Ubuntu 18.04?

Andreas Hasenack (ahasenack) wrote :

Which one, @michaelbibby? Have you tried one of the tree mentioned in comment #9?

Stephen Day (sd) wrote :

@ahasenack all of the patches seem to work. It would make sense to me to use the same as fedora but the final decision isn't mine. Any patch would be better than the current situation where we are all building our own.

Changed in amavisd-new (Ubuntu Bionic):
status: New → Confirmed
Changed in amavisd-new (Ubuntu Cosmic):
status: Confirmed → In Progress

I studied the three patches - thanks to all the people linking to them.

Yes Fedora took "only" the old version [1], but given how the mail threads read we should IMHO go go with one of the newer ones.

Sticking to the names coined by Simon Deziel:
Giovanni's old: working but a few edge cases left (before dkim_signatures_valid)
Adam Jacobs: picked a new place for the same change further improving the situation (in dkim_make_signatures)
Giovanni's new: added the same also at /^MAIL\z/ section of process_smtp_request
Adam Jacobs new: a new place at the beginning of dkim_make_signatures

I've read through the three of them and agree to Adam as being the most complrehensive that should cover all cases. Unless I failed to track the control flow in this 35kloc perl thing :-/!!!

The former patches might have changed other code paths, as usually "$msginfo->originating(c('originating')) if $msginfo;" would have been set by load_policy_bank depending on some attributes of the message. But if we are in dkim_make_signatures there is no way this should not be set (not trying to get mad at the control flow) it seems safe to set it if we are in that path not bothering too much how we got here - that would be for upstream which seems dead reading between the lines of the linked discussions.

I think any of these might even work "for the case reported here", and due to that this needs to test a bunch of non-dkim-signing setups if they are affected in any bad way more than the actual case test.

I'll prep an MP [2] with for review and an associated cosmic PPA to test [3].

[1]: https://src.fedoraproject.org/cgit/rpms/amavisd-new.git/tree/amavisd-new-2.11.0-detect_originating_email.patch
[2]: https://code.launchpad.net/~paelzer/ubuntu/+source/amavisd-new/+git/amavisd-new/+merge/355543
[3]: https://launchpad.net/~ci-train-ppa-service/+archive/ubuntu/3436

Changed in amavisd-new (Ubuntu Cosmic):
status: In Progress → Triaged
Simon Déziel (sdeziel) wrote :

Thanks Christian for the test PPA!

I'm unfortunately unable to run tests myself but maybe Adam and/or Stephen would be?

Since it is the same code that was tested int he upstream reports we should be fine and it is actually in cosmic-unapproved (waiting due to the beta freeze).

When we later on go for an Bionic SRU testing will be required and very helpful.

Launchpad Janitor (janitor) wrote :

This bug was fixed in the package amavisd-new - 1:2.11.0-1ubuntu2

---------------
amavisd-new (1:2.11.0-1ubuntu2) cosmic; urgency=medium

  * d/p/100_more_amavisd_helpers_fixes: Fix Debian/Ubuntu pathing in
    amavisd-release (LP: #1792293)
  * d/p/105_amavisd_fix_originating_dkim_signing.patch: Fix DKIM signing
    in 2.11.0 (LP: #1770532)

 -- Christian Ehrhardt <email address hidden> Mon, 24 Sep 2018 14:05:01 +0200

Changed in amavisd-new (Ubuntu Cosmic):
status: Triaged → Fix Released
Neustradamus (neustradamus) wrote :

Any news for 18.04?

Hasenpfeffer (hasenpfeffer) wrote :

FYI, for a backwards and forwards compatibility workaround until a fix is distributed, I was able to utilize the Amavis::Custom functionality to "patch" the core code. Put the following code at the end of a config file that amavisd-new loads, or otherwise load a file with the end code in it. For example:

50-user:

use strict;

#
# Place your configuration directives here. They will override those in
# earlier files.
#
# See /usr/share/doc/amavisd-new/ for documentation and examples of
# the directives you can use in this file
#

# Your configuration stuff here

#------------ Do not modify anything below this line -------------
1; # ensure a defined return

# Make sure the originating flag is up-to-date.
package Amavis::Custom;
use strict;

BEGIN {
  import Amavis::Conf qw(c);
}

sub new {
    my($class,$conn,$msginfo) = @_;
    $msginfo->originating(c('originating'));
}

1;

Hasenpfeffer (hasenpfeffer) wrote :

Note I found at least 1 use case of the originating flag combine with the MYUSERS policy before the Amavis::Custom->new workaround is executed, so a fix is still needed for that use case.

Added SRU template here in the bug
Opened MP for the fix to Bionic => https://code.launchpad.net/~paelzer/ubuntu/+source/amavisd-new/+git/amavisd-new/+merge/356402

description: updated

FYI - This is now in the SRU unapproved queue for Bionic

Changed in amavisd-new (Ubuntu Bionic):
status: Confirmed → Triaged
Changed in amavisd-new (Ubuntu Bionic):
assignee: nobody →  Christian Ehrhardt  (paelzer)
status: Triaged → In Progress

Hello Adam, or anyone else affected,

Accepted amavisd-new into bionic-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/amavisd-new/1:2.11.0-1ubuntu1.1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested and change the tag from verification-needed-bionic to verification-done-bionic. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-bionic. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

Changed in amavisd-new (Ubuntu Bionic):
status: In Progress → Fix Committed
tags: added: verification-needed verification-needed-bionic

Unfortunately I don't have a bionic server available any more, but I did
upgrade to cosmic and the fix is there, and has been working perfectly.

On 11/2/18 6:29 PM, Steve Langasek wrote:
> Hello Adam, or anyone else affected,
>
> Accepted amavisd-new into bionic-proposed. The package will build now
> and be available at https://launchpad.net/ubuntu/+source/amavisd-
> new/1:2.11.0-1ubuntu1.1 in a few hours, and then in the -proposed
> repository.
>
> Please help us by testing this new package. See
> https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how
> to enable and use -proposed. Your feedback will aid us getting this
> update out to other Ubuntu users.
>
> If this package fixes the bug for you, please add a comment to this bug,
> mentioning the version of the package you tested and change the tag from
> verification-needed-bionic to verification-done-bionic. If it does not
> fix the bug for you, please add a comment stating that, and change the
> tag to verification-failed-bionic. In either case, without details of
> your testing we will not be able to proceed.
>
> Further information regarding the verification process can be found at
> https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in
> advance for helping!
>
> N.B. The updated package will be released to -updates after the bug(s)
> fixed by this package have been verified and the package has been in
> -proposed for a minimum of 7 days.
>
> ** Changed in: amavisd-new (Ubuntu Bionic)
> Status: In Progress => Fix Committed
>
> ** Tags added: verification-needed verification-needed-bionic
>

Thanks Adam for the notice, I'd really prefer if someone with a real setup of this could verify it.
Anyone else of the people commenting on this issue so far able to verify this from Bionic-proposed?

Thomas Pike (xiven) wrote :

I was also having this issue and was using the config patch from comment #21 to work around it.

Have now tested with the package from bionic-proposed in #25 (and the #21 config patch disabled) and can confirm that it works as expected.

Thanks Thomas!

tags: added: verification-done verification-done-bionic
removed: verification-needed verification-needed-bionic
Adam Jacobs (bllfr0g) wrote :
Download full text (5.0 KiB)

Thanks++

> On Nov 5, 2018, at 02:06,  Christian Ehrhardt  <email address hidden> wrote:
>
> Thanks Thomas!
>
> ** Tags removed: verification-needed verification-needed-bionic
> ** Tags added: verification-done verification-done-bionic
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1770532
>
> Title:
> DKIM signing not working in bionic
>
> Status in amavisd-new package in Ubuntu:
> Fix Released
> Status in amavisd-new source package in Bionic:
> Fix Committed
> Status in amavisd-new source package in Cosmic:
> Fix Released
> Status in amavisd-new package in Debian:
> Confirmed
>
> Bug description:
> [Impact]
>
> * There is a known upstream issue in 2.0.11 breaking DKIM signing.
> - https://bugzilla.redhat.com/show_bug.cgi?id=1364730
> - https://lists.amavis.org/pipermail/amavis-users/2018-February/005292.html
>
> * given the activity on the report it seems plenty of people set this up
> pre-Bionic and are now running into these failures on upgrade to the
> current LTS.
>
> * Add a fix to avoid more people being hit by this on upgrade and forced
> to deploy workarounds (or drop the functionality)
>
> [Test Case]
>
> * Setup amavisd for DKIM signing, see
> https://www.ijs.si/software/amavisd/amavisd-new-docs.html#dkim
> or any of
> https://www.faqforge.com/linux/how-to-enable-dkim-email-signatures-in-amavisd-new-and-ispconfig-3/
> https://nwgat.ninja/setting-up-dkim-and-spf-with-amavis-on-ubuntu-16-04-2/
> ...
> There seem to be a lot all doing the same essential steps.
>
> TL;DR would be:
> $ apt install amavisd-new
> $ mkdir -p /var/db/dkim/
> $ amavisd-new genrsa /var/db/dkim/example-foo.key.pem
> Add in /etc/amavis/conf.d/21-ubuntu_defaults
> $enable_dkim_signing = 1;
> dkim_key('example.com', 'foo', '/var/db/dkim/example-foo.key.pem');
> @dkim_signature_options_bysender_maps = (
> { '.' => { ttl => 21*24*3600, c => 'relaxed/simple' } } );
> @mynetworks = qw(0.0.0.0/8 127.0.0.0/8 10.0.0.0/8 172.16.0.0/12
> 192.168.0.0/16); # list your internal networks
> - Now showkeys will report your key including the pblic key you'll need
> - amavisd-new showkeys
> - add the public key (as displayed) to your DNS zone, increment SOA sequence number and reload DNS;
> - then test signing and a published key
> - amavisd-new testkeys
>
> Never the less you'd need to setup a lot of details and it feels
> unclear if you test the right thing, therefor my preference is with so
> many users reporting about the issue to rely on them to test their
> real setups.
>
> [Regression Potential]
>
> * Lacking upstream being active there is always a chance things are
> missed, but multiple people came up with very similar solutions and
> multiple people tested these successfully.
> The actual change sets the originating flag where it is needed on the
> creation of dkim signatures.
> Due to that setups not triggering dkim_make_signatures should be not
> affected at all. And those that use dkim_make_signatures are those
> failing now due...

Read more...

The verification of the Stable Release Update for amavisd-new has completed successfully and the package has now been released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.

Launchpad Janitor (janitor) wrote :

This bug was fixed in the package amavisd-new - 1:2.11.0-1ubuntu1.1

---------------
amavisd-new (1:2.11.0-1ubuntu1.1) bionic; urgency=medium

  * d/p/100_more_amavisd_helpers_fixes: Fix Debian/Ubuntu pathing in
    amavisd-release (LP: #1792293)
  * d/p/105_amavisd_fix_originating_dkim_signing.patch: Fix DKIM signing
    in 2.11.0 (LP: #1770532)

 -- Christian Ehrhardt <email address hidden> Wed, 10 Oct 2018 13:36:43 +0200

Changed in amavisd-new (Ubuntu Bionic):
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Duplicates of this bug

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.