DKIM signing not working in bionic

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?

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

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.

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) {

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
>

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

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).

Fedora 28 integrated Giovanni's old patch.

Thanks for the summary, @sdeziel

Any change we can have this patch in Ubuntu 18.04?

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

@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.

Linked to https://bugs.launchpad.net/ubuntu/+source/amavisd-new/+bug/1778968
-> Patch in comment.

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

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.

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

Any news for 18.04?

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;

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

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

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.

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?

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!

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...

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.

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

Thanks a lot!

It is possible to see for all previous Ubuntu yet supported too?
The bug was not new.

Not possible for old Ubuntu too?

@brian-murray, @bllfr0g, @paelzer, @xiven, @vorlon, @ahasenack

Hi Neustradamus,
the initial bug was reported as "Upon upgrading to bionic, amavisd-new DKIM signing no longer works." so no one thought about former releases.
This also matches that there were no bugs about that issue, prior to Bionic being released.

Also there is an increased risk the further changes are backported.
In this case all >=Bionic was on 2.11, but Xenial is 2.10 and Trusty even on 2.7.
So even if it happens in older versions the tradeoff between regressions for current users vs the benefit of the fix for others might be different.
Especially with a somewhat dead upstream and a patch that existed in three variants - also the test instructions are ok'ish but not totally complete to try it in different variants. Therefore the risk is too high to apply it further back (IMHO).

The old patch applies, but with a 788 line offset in a 34k line perl file - oO

For now - for me personally - this is Won't Fix, but I'm open to be convinced if we get enough confirmation and confidence in it. So if you really think the very same issue affects Xenial, please share some details about why you think so. Do the testing steps above trigger it for you, if you can add extra details about testing if needed for Xenial.

On Mon, Dec 27, 2021 at 1:15 PM Krzysztof Gajda
<email address hidden> wrote:
>
> Hi.
>
> This is real issue, because DKIM signing doesn't work without this fix
> (and amavisd-new marks incorrectly outbound emails as RelayedOpenRelay).
>
> Any chance to include this patch in LTS18 current updates?
>
> PS: I've patched amavisd-new by myself, because the latest 18 LTS (with
> all current updated) doesn't have this fix included.
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1770532

Disclaimer: There might be something wrong with the launchpad notifications.
I got this mail in my inbox, and I found it at [1] which seems to be a
comment to the duplicate.
But I do not see this update on either of the bugs as a comment nor a
bug linked to your user at [2].
[1]: https://<email address hidden>/msg5985342.html
[2]: https://bugs.launchpad.net/~kgajdastl

Hi Krzysztof,
This is supposed to be fixed in Bionic from 1:2.11.0-1ubuntu1.1 and
tested successfully by various users.
If there is a new problem that would most likely be a new bug to
discuss, debug and fix.
If you file that bug please ensure to outline what config you have to
trigger the new issue (or this one again?).