[pmg-devel] [PATCH pmg-api 05/12] add PMG::DKIMSign module

[Dominik Csapak]

took now a closer look, some comments inline

On 10/7/19 9:28 PM, Stoiko Ivanov wrote:
> the module serves 2 purposes:
> * it extends Mail::DKIM::Signer:
>    * it provides a glue layer between MIME::Entity's output method (which
>      expects print and uses \n as line terminator) and Mail::DKIM::Signer's
>      PRINT method (which expects \r\n)
>    * it infers the domain which should be used for signing based on the
>      sender's e-mail address (and the DKIM-settings in PMG-configuration)
> 
> * it provides helper methods for generating a new private key, outputting a
>    DKIM TXT record and for finding the domain which should be used for signing
>    a mail
> 
> certain headers get oversigned (in order to prevent adding a previously
> non-existing header (e.g. Reply-To) and retaining a valid signature).
> the list of headers which to oversign is inspired by rspamd's choice [0].
> for rationale see [1,2,3]
> 
> [0] https://rspamd.com/doc/modules/dkim_signing.html#sign-headers
> [1] https://noxxi.de/research/breaking-dkim-on-purpose-and-by-chance.html
> [2] https://github.com/rspamd/rspamd/issues/2136
> 
> Signed-off-by: Stoiko Ivanov <s.ivanov at proxmox.com>
> ---
>   src/Makefile        |   1 +
>   src/PMG/DKIMSign.pm | 150 ++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 151 insertions(+)
>   create mode 100644 src/PMG/DKIMSign.pm
> 
> diff --git a/src/Makefile b/src/Makefile
> index c864ae8..b03a27b 100644
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -73,6 +73,7 @@ LIBSOURCES =				\
>   	PMG/LDAPSet.pm			\
>   	PMG/LDAPCache.pm		\
>   	PMG/DBTools.pm			\
> +	PMG/DKIMSign.pm			\
>   	PMG/Quarantine.pm		\
>   	PMG/Report.pm			\
>   	PMG/RuleDB/Group.pm		\
> diff --git a/src/PMG/DKIMSign.pm b/src/PMG/DKIMSign.pm
> new file mode 100644
> index 0000000..fa61101
> --- /dev/null
> +++ b/src/PMG/DKIMSign.pm
> @@ -0,0 +1,150 @@
> +package PMG::DKIMSign;
> +
> +use strict;
> +use warnings;
> +use Mail::DKIM::Signer;
> +use Mail::DKIM::TextWrap;
> +use Crypt::OpenSSL::RSA;
> +
> +use PVE::Tools;
> +use PVE::INotify;
> +use PVE::SafeSyslog;
> +
> +use PMG::Utils;
> +use PMG::Config;
> +use base qw(Mail::DKIM::Signer);
> +
> +sub new {
> +    my ($class, $selector, $sign_all) = @_;
> +
> +    die "no selector provided\n" if ! $selector;
> +
> +    my %opts = (
> +	Algorithm => 'rsa-sha256',
> +	Method => 'relaxed/relaxed',
> +	Selector => $selector,
> +	KeyFile => "/etc/pmg/dkim/$selector.private",
> +    );
> +
> +    my $self = $class->SUPER::new(%opts);
> +
> +    # oversign certain headers
> +    $self->extended_headers({
> +	From => '+',
> +	To => '+',
> +	CC => '+',
> +	'Reply-To' => '+',
> +	Subject => '+',
> +    });
> +
> +    $self->{sign_all} = $sign_all;
> +
> +    return $self;
> +}
> +
> +# MIME::Entity can output to all objects responding to 'read' (and does so in
> +# chunks) Mail::DKIM::Signer has a 'READ' method and expects each line
> +# terminated with "\r\n"
> +
> +
> +sub print {
> +    my ($self, $line) = @_;
> +    $line =~ s/\012/\015\012/g;
> +    $self->PRINT($line);
> +}
> +
> +sub create_signature {
> +    my ($self) = @_;
> +
> +    $self->CLOSE();
> +    return $self->signature->as_string();
> +}
> +
> +#determines which domain should be used for signing based on the e-mailaddress
> +sub signing_domain {
> +    my ($self, $sender_email) = @_;
> +
> +    my $input_domain;
> +    if ( $sender_email =~ m/@([a-zA-Z0-9\-]+(?:\.[a-zA-Z0-9\-]+)*)$/ ) {
> +	$input_domain = $1;
> +    }

why not simply split at '@' and use the last part?

also, what happens if it does not match? we use $input_domain below 
rather unconditionally, which is probably not correct and produces some 
warnings

can we die/warn and early return if we do not have a valid input_domain?

> +
> +    if ($self->{sign_all}) {
> +	    $self->domain($input_domain) if $self->{sign_all};
> +	    return;
> +    }
> +
> +    # check that input_domain is in/a subdomain of in the
> +    # dkimdomains, falling back to the relay domains.
> +    my $dkimdomains = PVE::INotify::read_file('dkimdomains');
> +    $dkimdomains = PVE::INotify::read_file('domains') if !scalar(%$dkimdomains);
> +
> +    foreach my $domain (sort keys %$dkimdomains) {
> +	if ( $input_domain =~ /\Q$domain\E$/ ) {
> +	    $self->domain($domain);
> +	    return;
> +	}
> +    }
> +
> +    syslog('info', "not DKIM signing mail from $sender_email");
> +
> +    return;
> +}
> +
> +# key-handling and utility methods
> +sub get_selector_info {
> +    my ($selector) = @_;
> +
> +    die "no selector provided\n" if !defined($selector);
> +    my ($pubkey, $size);
> +    eval {
> +	my $privkeytext = PVE::Tools::file_get_contents("/etc/pmg/dkim/$selector.private");
> +	my $privkey =  Crypt::OpenSSL::RSA->new_private_key($privkeytext);
> +	$size = $privkey->size() * 8;
> +
> +	$pubkey = $privkey->get_public_key_x509_string();
> +    };
> +    die "$@\n" if $@;
> +
> +    $pubkey =~ s/-----(?:BEGIN|END) PUBLIC KEY-----//g;
> +    $pubkey =~ s/\v//mg;
> +
> +    # split record into 250 byte chunks for DNS-server compatibility
> +    # see opendkim-genkey
> +    my $record = qq{$selector._domainkey\tIN\tTXT\t( "v=DKIM1; h=sha256; k=rsa; "\n\t  "p=};
> +    my $len = length($pubkey);
> +    my $cur = 0;
> +    while ($len > 0) {
> +	if ($len < 250) {
> +	    $record .= substr($pubkey, $cur);
> +	    $len = 0;
> +	} else {
> +	    $record .= substr($pubkey, $cur, 250) . qq{"\n\t  "};
> +	    $cur += 250;
> +	    $len -= 250;
> +	}
> +    }
> +    $record .= qq{" )  ; ----- DKIM key $selector};
> +
> +    return ($record, $size);
> +}
> +
> +sub set_selector {
> +    my ($selector, $keysize) = @_;
> +
> +    die "no selector provided\n" if !defined($selector);
> +    die "no keysize provided\n" if !defined($keysize);
> +    die "invalid keysize\n" if ($keysize != 1024 && $keysize!= 2048 && $keysize != 4096);;

any reason why only those 3 keysizes? (i understand why >= 1024) maybe 
mulitples of 1024?

also, a superfluous ;

> +    my $privkey_file = "/etc/pmg/dkim/$selector.private";

we should probably ship that dir in the package (with 0600)

> +
> +    eval {
> +	my $cmd = ['openssl', 'genrsa', '-out', $privkey_file, $keysize];
> +	PMG::Utils::run_silent_cmd($cmd);
> +	my $cfg = PMG::Config->new();
> +	$cfg->set('admin', 'dkim_selector', $selector);
> +	$cfg->write();

i guess we should lock the config here with PMG::Config::lock_config

> +	PMG::Utils::reload_smtp_filter();
> +    };
> +    die "unable to generate dkim key for $selector ($keysize bits): $@\n" if $@;
> +}
> +1;
>