Release 1.0.2

(LP: #1844189)

.gitignore

@@ -1,4 +1,3 @@
 dist
 dkimpy_milter.egg-info
 *.pyc
-*~

CHANGES

@@ -1,3 +1,13 @@
+1.0.2 2019-10-07
+ - Fix startup logging so it provides information at a useful time
+ - Fix message extraction so that signing in the same pass through the milter
+   as verifying works correctly
+ - Fix variable initialization so mailformed mails missing body From do not
+   cause a traceback (LP: #1844161)
+ - Catch more ascii encoding errors to improve resilience against bad data
+   (LP: #1844189)
+ - Fix sysv init so it works (LP: #1839487)
+
 1.0.1 2019-02-11
  * Reorder milter start and dropping privileges so permissions on Unix socket
    are correct (LP: 1797720)

README

@@ -37,7 +37,7 @@ an C compiler.  Alternately, install these dependencies from dsitribution/OS
 packages and then pip install dkimpy_milter.
 
 The milter will work with either pydns (DNS) or dnspython (dns), preferring
-dnspython is both are available.  The dkimpy DKIM module also works with
+dnspython if both are available.  The dkimpy DKIM module also works with
 either.
 
 
@@ -84,9 +84,8 @@ MTA INTEGRATION
 
 Both a systemd unit file and a sysv init file are provided.  Both make
 assumptions about defaults being used, e.g. if a non-standard pidfile name is
-used, they will need to be updated.  The sysv init file is Debian specific and
+used, they will need to be updated.  The sysv init file uses start-stop-deamon
-untested, since the developers are not using sysv init.  Feedback/patches
+from Debian.  It is not portable to systems without that available.
-welcome.
 
 The dkimpy-milter drops priviledges after setup to the user/group specified in
 UserID.  During initial setup, this system user needs to be manually created.

dkimpy_milter/__init__.py

@@ -54,6 +54,7 @@ class dkimMilter(Milter.Base):
         self.privatersa = privateRSA
         self.privateed25519 = privateEd25519
         self.fp = None
+        self.fdomain = ''
 
     @Milter.noreply
     def connect(self, hostname, unused, hostaddr):
@@ -61,9 +62,7 @@ class dkimMilter(Milter.Base):
         self.external_connection = False
         self.hello_name = None
         # sometimes people put extra space in sendmail config, so we strip
-        self.receiver = self.getsymval('j')
+        self.receiver = self.getsymval('j').strip()
-        if self.receiver is not None:
-            self.receiver = self.receiver.strip()
         try:
             self.AuthservID = milterconfig['AuthservID']
         except:
@@ -135,14 +134,18 @@ class dkimMilter(Milter.Base):
             try:
                 self.fdomain = self.author.split('@')[1].lower()
             except IndexError as er:
-                self.fdomain = ''  # self.author was not a proper email address
+                pass # self.author was not a proper email address
             if (milterconfig.get('Syslog') and
                     milterconfig.get('debugLevel') >= 1):
                 syslog.syslog("{0}: {1}".format(name, val))
         elif lname == 'authentication-results':
             self.arheaders.append(val)
         if self.fp:
+            try:
                 self.fp.write("%s: %s\n" % (name, val))
+            except:
+                # Don't choke on header fields with garbage in them.
+                pass
         return Milter.CONTINUE
 
     @Milter.noreply
@@ -176,20 +179,19 @@ class dkimMilter(Milter.Base):
             except:
                 # Don't error out on unparseable AR header fiels
                 pass
-        # Check or sign DKIM
+        # Check and/or sign DKIM
         self.fp.seek(0)
+        txt = self.fp.read()
         if milterconfig.get('Domain'):
             domain = milterconfig.get('Domain')
         else:
             domain = ''
         if ((self.fdomain in domain) and not milterconfig.get('Mode') == 'v'
                 and not self.external_connection):
-            txt = self.fp.read()
             self.sign_dkim(txt)
         if ((self.has_dkim) and (not self.internal_connection) and
             (milterconfig.get('Mode') == 'v' or
              milterconfig.get('Mode') == 'sv')):
-            txt = self.fp.read()
             self.check_dkim(txt)
         if self.arresults:
             h = authres.AuthenticationResultsHeader(authserv_id=
@@ -257,14 +259,10 @@ class dkimMilter(Milter.Base):
 
     def check_dkim(self, txt):
         res = False
+        self.header_a = None
         for y in range(self.has_dkim):  # Verify _ALL_ the signatures
             d = dkim.DKIM(txt)
             try:
-                dnsoverride = milterconfig.get('DNSOverride')
-                if isinstance(dnsoverride, str):
-                    syslog.syslog("DNSOverride: {0}".format(dnsoverride))
-                    res = d.verify(idx=y, dnsfunc=lambda _x: dnsoverride)
-                else:
                 res = d.verify(idx=y)
                 if res:
                     if d.signature_fields.get(b'a') == 'ed25519-sha256':
@@ -288,9 +286,20 @@ class dkimMilter(Milter.Base):
                 self.dkim_comment = str(x)
                 if milterconfig.get('Syslog'):
                     syslog.syslog("check_dkim: {0}".format(x))
+            try:
                 self.header_i = d.signature_fields.get(b'i')
+            except TypeError as x:
+                self.header_i = None
+            try:
                 self.header_d = d.signature_fields.get(b'd')
                 self.header_a = d.signature_fields.get(b'a')
+            except Exception as x:
+                self.dkim_comment = str(x)
+                if milterconfig.get('Syslog'):
+                    syslog.syslog("check_dkim: {0}".format(x))
+                self.header_d = None
+            if not self.header_a:
+                self.header_a = 'rsa-sha256'
             if res:
                 if (milterconfig.get('Syslog') and
                         (milterconfig.get('SyslogSuccess') or
@@ -310,12 +319,18 @@ class dkimMilter(Milter.Base):
                         syslog.syslog('DKIM: Fail (saved as {0})'
                                       .format(fname))
                 else:
-                    syslog.syslog('DKIM: Fail ({0})'.format(d.domain.lower()))
+                    if milterconfig.get('Syslog'):
+                        if d.domain:
+                            syslog.syslog('DKIM: Fail ({0})'
+                                          .format(d.domain.lower()))
+                        else:
+                            syslog.syslog('DKIM: Fail, unextractable domain')
             if res:
                 result = 'pass'
             else:
                 result = 'fail'
             res = False
+            if self.header_d:
                 self.arresults.append(
                     authres.DKIMAuthenticationResult(result=result,
                                                  header_i=self.header_i,
@@ -324,6 +339,7 @@ class dkimMilter(Milter.Base):
                                                  result_comment=
                                                  self.dkim_comment)
             )
+            self.header_a = None
         return
 
 
@@ -358,10 +374,10 @@ def main():
     own_socketfile(milterconfig)
     drop_privileges(milterconfig)
     sys.stdout.flush()
-    Milter.runmilter(miltername, socketname, 240)
     if milterconfig.get('Syslog'):
-        syslog.syslog('dkimpy-milter started:{0} user:{1}'
+        syslog.syslog('dkimpy-milter starting:{0} user:{1}'
                       .format(pid, milterconfig.get('UserID')))
+    Milter.runmilter(miltername, socketname, 240)
 
 if __name__ == "__main__":
     main()

dkimpy_milter/__main__.py

@@ -1,6 +0,0 @@
-#!/usr/bin/python2
-
-from dkimpy_milter import main
-
-if __name__ == "__main__":
-    main()

dkimpy_milter/config.py

@@ -48,7 +48,6 @@ defaultConfigData = {
     'DiagnosticDirectory': '',
     'MacroList': '',
     'MacroListVerify': '',
-    'DNSOverride': None,
     'debugLevel': 0  # Undocumented config item for developer use
     }
 
@@ -335,7 +334,6 @@ def _readConfigFile(path, configData=None, configGlobal={}):
         'DiagnosticDirectory': 'str',
         'MacroList': 'dataset',
         'MacroListVerify': 'dataset',
-        'DNSOverride': 'str',
         'debugLevel': 'int'
         }
 
@@ -390,9 +388,6 @@ def _readConfigFile(path, configData=None, configGlobal={}):
         if conversion == 'bool':
             configData[name] = _find_boolean(value)
         elif conversion == 'str':
-            if isinstance(value, list):
-                configData[name] = line.split(None, 1)[1]
-            else:
             configData[name] = str(value)
         elif conversion == 'int':
             configData[name] = int(value)
@@ -404,7 +399,7 @@ def _readConfigFile(path, configData=None, configGlobal={}):
             configData[name] = conversion(value)
     fp.close()
     try:
-        configData['AuthservID'] = _make_authserv_id(configData.get('AuthservID', 'HOSTNAME'))
+        configData['AuthservID'] = _make_authserv_id(configData['AuthservID'])
         configData['IntHosts'] = HostsDataset(configData['InternalHosts'])
     except:
         pass

dkimpy_milter/util.py

@@ -150,18 +150,10 @@ def own_socketfile(milterconfig):
     """If socket is Unix socket, chown to UserID before dropping privileges"""
     import os
     user, group = user_group(milterconfig.get('UserID'))
-    offset = None
+    if milterconfig.get('Socket')[:1] == '/':
-    sockname = milterconfig.get('Socket')
+        os.chown(milterconfig.get('Socket')[1:], user, group)
-    if sockname[:1] == '/':
+    if milterconfig.get('Socket')[:6] == "local:":
-        offset = 0
+        os.chown(milterconfig.get('Socket')[6:], user, group)
-    elif sockname[:6] == "local:":
-        offset = 6
-    elif sockname[:5] == "unix:":
-        offset = 5
-
-    if offset is not None:
-        if os.path.exists(sockname[offset:]):
-            os.chown(sockname[offset:], user, group)
 
 
 def read_keyfile(milterconfig, keytype):

man/dkimpy-milter.conf.5

@@ -311,13 +311,6 @@ be set:
 (b) KeyTable, SigningTable, no Domain, no KeyFile, no Selector;
 [fooTable options NOT IMPLEMENTED]
 
-.TP 
-.I DNSOverride (string)
-Provide a text string that a verifying milter should use instead of
-consulting the DNS on each message.  This is useful primarily for
-testing purposes in environments where it is awkward to modify the
-system DNS resolution.  It should not be used in production.
-
 .TP
 .I PeerList (dataset)
 Identifies a set of "peers" that identifies clients whose connections

setup.py

@@ -23,14 +23,14 @@ description = "Domain Keys Identified Mail (DKIM) signing/verifying milter for P
 
 kw = {}  # Work-around for lack of 'or' requires in setuptools.
 try:
-    import dns
+    import DNS
-    kw['install_requires'] = ['dkimpy>=0.7', 'pymilter', 'authres>=1.1.0', 'PyNaCl', 'ipaddress', 'dnspython']
-except ImportError:  # If PyDNS is not installed, prefer dnspython
     kw['install_requires'] = ['dkimpy>=0.7', 'pymilter', 'authres>=1.1.0', 'PyNaCl', 'ipaddress', 'PyDNS']
+except ImportError:  # If PyDNS is not installed, prefer dnspython
+    kw['install_requires'] = ['dkimpy>=0.7', 'pymilter', 'authres>=1.1.0', 'PyNaCl', 'ipaddress', 'dnspython']
 
 setup(
     name='dkimpy-milter',
-    version='1.0.1',
+    version='1.0.2',
     author='Scott Kitterman',
     author_email='scott@kitterman.com',
     url='https://launchpad.net/dkimpy-milter',

system/dkimpy-milter

@@ -20,9 +20,9 @@
 ### END INIT INFO
 prefix="/usr/local"
 exec_prefix=${prefix}
-sysconfdir="/etc/dkimpy-milter"
+sysconfdir="/usr/local/etc"
 bindir="${exec_prefix}/bin/"
-RUNDIR="/var/run/dkimpy-milter"
+RUNDIR="/run/dkimpy-milter"
 DAEMON=${bindir}/dkimpy-milter
 PATH=/sbin:/bin:/usr/sbin:/usr/bin:/usr/local/bin:
 NAME=dkimpy-milter
@@ -67,14 +67,14 @@ case "$1" in
 			fi
 		fi
 	fi
-
+        start-stop-daemon --start --background --quiet --pidfile \
-        start-stop-daemon --start --quiet --pidfile $RUNDIR/$NAME.pid --startas \
+                $RUNDIR/$NAME.pid --exec $DAEMON $sysconfdir/$NAME.conf
-                $DAEMON $sysconfdir/$NAME.conf --name $NAME --test > /dev/null \
 	echo "$NAME."
 	;;
   stop)
 	echo -n "Stopping $DESC: "
 	if [ -f $RUNDIR/$NAME.pid ]; then
+		chown root:root $RUNDIR/$NAME.pid
 		start-stop-daemon --stop --pidfile $RUNDIR/$NAME.pid 
 		rm $RUNDIR/$NAME.pid
 		#echo $SOCKET
@@ -87,6 +87,7 @@ case "$1" in
   force-reload)
         echo -n "Force reloading $DESC: "
         if [ -f $RUNDIR/$NAME.pid ]; then
+                chown root:root $RUNDIR/$NAME.pid
                 start-stop-daemon --stop --pidfile $RUNDIR/$NAME.pid
                 rm $RUNDIR/$NAME.pid
                 #echo $SOCKET
@@ -95,7 +96,7 @@ case "$1" in
                 fi
         fi
         sleep 1
-        start-stop-daemon --start --chuid $USER --background --quiet --pidfile \
+        start-stop-daemon --start --background --quiet --pidfile \
                 $RUNDIR/$NAME.pid --exec $DAEMON $sysconfdir/$NAME.conf
         echo "$NAME."
         ;;
@@ -103,6 +104,7 @@ case "$1" in
         echo "Restarting $DESC: "
         echo -n "Stopping $DESC: "
         if [ -f $RUNDIR/$NAME.pid ]; then
+                chown root:root $RUNDIR/$NAME.pid
                 start-stop-daemon --stop --pidfile $RUNDIR/$NAME.pid
                 rm $RUNDIR/$NAME.pid
                 #echo $SOCKET
@@ -113,7 +115,7 @@ case "$1" in
         echo "$NAME."
 	sleep 1
         echo -n "Starting $DESC: "
-        start-stop-daemon --start --chuid $USER --background --quiet --pidfile \
+        start-stop-daemon --start --background --quiet --pidfile \
                 $RUNDIR/$NAME.pid --exec $DAEMON $sysconfdir/$NAME.conf
         echo "$NAME."
 	;;

tests/00_minimal.miltertest

@@ -1,12 +0,0 @@
--- -*- lua -*-
-for _, keytype in ipairs({"ed25519", "rsa"}) do
-   for _, func in ipairs({"signing", "verify"}) do
-      mt.echo("testing "..keytype.." "..func)
-      conn = mt.connect("unix:"..keytype.."."..func..".sock")
-      if conn == nil then
-         error("mt.connect() failed "..keytype.." "..func)
-      end
-      mt.disconnect(conn)
-      mt.echo(keytype.." "..func.." complete")
-   end
-end

tests/01_connect.miltertest

@@ -1,40 +0,0 @@
--- -*- lua -*-
-for _, keytype in ipairs({"ed25519", "rsa"}) do
-   for _, func in ipairs({"signing", "verify"}) do
-      mt.echo("testing "..keytype.." "..func)
-      conn = mt.connect("unix:"..keytype.."."..func..".sock")
-      if conn == nil then
-         error("mt.connect() failed "..keytype.." "..func)
-      end
-      if mt.conninfo(conn, "localhost", "127.0.0.1") ~= nil then
-         error("mt.conninfo() failed "..keytype.." "..func)
-      end
-      if mt.getreply(conn) ~= SMFIR_CONTINUE then
-         error("mt.conninfo() unexpected reply "..keytype.." "..func)
-      end
-
-      if mt.test_action(conn, SMFIF_ADDHDRS) then
-         print("could add headers "..keytype.." "..func)
-      else
-         error("mt.test_action() says could not add headers "..keytype.." "..func)
-      end
-
-      if mt.test_action(conn, SMFIF_CHGHDRS) then
-         print("could change headers "..keytype.." "..func)
-      else
-         error("mt.test_action() says could not change headers "..keytype.." "..func)
-      end
-
--- -- FIXME: this part of the test fails, as apparently the
--- -- dkimpy-milter claims the right to change the body of a message,
--- -- even though it shouldn't.  How can we fix the negotiation?
---      if mt.test_action(conn, SMFIF_CHGBODY) then
---         error("mt.test_action() says could change body "..keytype.." "..func)
---      else
---         print("could not change body "..keytype.." "..func)
---      end
-
-      mt.disconnect(conn)
-      mt.echo(keytype.." "..func.." test complete")
-   end
-end

tests/02_sign_message.miltertest

@@ -1,100 +0,0 @@
--- -*- lua -*-
-
-msg = {
-   ['headers'] = {
-      ['From'] = 'Alice <alice@example.net>',
-      ['Message-Id'] = '<dkimpy-milter-test-02@example.net>',
-      ['To'] = 'Bob <bob@example.biz>',
-      ['Date'] = 'Mon, 18 Feb 2019 08:32:50 -0500',
-      ['Subject'] = 'Signing test',
-      ['Content-Type'] = 'text/plain',
-   },
-   ['body'] = "This is a test!\r\n",
-}
-
--- returns miltertest connection object
-function connect_and_send (sockname, headers, body)
-   conn = mt.connect(sockname)
-   if conn == nil then
-      error "mt.connect() failed"
-   end
-   if mt.conninfo(conn, "localhost", "127.0.0.1") ~= nil then
-      error "mt.conninfo() failed"
-   end
-   if mt.getreply(conn) ~= SMFIR_CONTINUE then
-      error "mt.conninfo() unexpected reply"
-   end
-
-   -- mt.macro(conn, SMFIC_MAIL, "i", "simple-message")
-   if mt.mailfrom(conn, "<alice@example.net>") ~= nil then
-      error "mt.mailfrom() failed"
-   end
-   if mt.getreply(conn) ~= SMFIR_CONTINUE then
-      error "mt.mailfrom() unexpected reply"
-   end
-   -- mt.rcptto() is called implicitly
-
-   -- send headers
-   for key,value in pairs(headers) do
-      if mt.header(conn, key, value) ~= nil then
-         error("mt.header(" .. key .. ") failed")
-      end
-      if mt.getreply(conn) ~= SMFIR_CONTINUE then
-         error("mt.header(" .. key .. ") unexpected reply")
-      end
-   end
-   -- send EOH
-   if mt.eoh(conn) ~= nil then
-      error "mt.eoh() failed"
-   end
-   if mt.getreply(conn) ~= SMFIR_CONTINUE then
-      error "mt.eoh() unexpected reply"
-   end
-
-   -- send body
-   if mt.bodystring(conn, body) ~= nil then
-      error "mt.bodystring() failed"
-   end
-   if mt.getreply(conn) ~= SMFIR_CONTINUE then
-      error "mt.bodystring() unexpected reply"
-   end
-   -- end of message; let the filter react
-   if mt.eom(conn) ~= nil then
-      error "mt.eom() failed"
-   end
-   reply = mt.getreply(conn)
-   if reply ~= SMFIR_CONTINUE then
-      error ("mt.eom() unexpected reply: " .. reply)
-   end
-   return conn
-end
-
-for _, keytype in ipairs({"ed25519", "rsa"}) do
-   mt.echo("testing "..keytype)
-   signing = connect_and_send("unix:"..keytype..".signing.sock", msg.headers, msg.body)
-   -- verify that a test header field got added
-   if not mt.eom_check(signing, MT_HDRINSERT) then
-      error "no header added by signer"
-   end
-
-   signature = mt.getheader(signing, "DKIM-Signature", 0)
-
-   mt.disconnect(signing)
-
-   mt.echo("DKIM-Signature: " .. signature)
-
-   msg.headers['DKIM-Signature'] = signature
-
-   verify = connect_and_send("unix:"..keytype..".verify.sock", msg.headers, msg.body)
-
-   if not mt.eom_check(verify, MT_HDRINSERT) then
-      error "no header added in verify"
-   end
-
-   authres = mt.getheader(verify, "Authentication-Results", 0)
-   mt.echo("Authentication-Results: "..authres)
-
-   mt.disconnect(verify)
-
-   mt.echo(keytype.." complete")
-end

tests/dkimpy-milter

@@ -1,2 +0,0 @@
-#!/bin/sh
-python2 -m dkimpy_milter "$@"

tests/runtests

@@ -1,84 +0,0 @@
-#!/bin/bash
-
-set -e
-WORKDIR=$(mktemp -d)
-TESTDIR=$(realpath "$(dirname "$0")")
-DKIMPY_MILTER=${DKIMPY_MILTER:-"$TESTDIR/dkimpy-milter"}
-KEY_TYPES=(ed25519 rsa)
-
-cd "$WORKDIR"
-
-printf "Testing %s from directory %s\n" "$DKIMPY_MILTER" "$WORKDIR"
-
-for keytype in "${KEY_TYPES[@]}"; do
-    dknewkey --ktype "$keytype" "testkey.$keytype"
-    if [ "$keytype" = ed25519 ]; then
-        keyfile=KeyFileEd25519
-        selector=SelectorEd25519
-    else
-        keyfile=KeyFile
-        selector=Selector
-    fi
-    cat > "$keytype.signing.conf" <<EOF
-Domain example.net
-$keyfile testkey.$keytype.key
-$selector testkey
-Socket unix:$keytype.signing.sock
-PidFile $keytype.signing.pid
-Mode s
-UserID $(id --name --user):$(id --name --group)
-EOF
-
-    cat > "$keytype.verify.conf" <<EOF
-Socket unix:$keytype.verify.sock
-PidFile $keytype.verify.pid
-Mode v
-DNSOverride $(cat testkey.$keytype.dns)
-UserID $(id --name --user):$(id --name --group)
-EOF
-done
-
-cleanup() {
-    echo cleaning up jobs:
-    jobs
-    for keytype in "${KEY_TYPES[@]}"; do
-        for func in signing verify; do
-            if [ -s "$keytype.$func.pid" ] && kill -0 "$(cat "$keytype.$func.pid")"; then
-                kill "$(cat $keytype.$func.pid)"
-            fi
-        done
-    done
-    wait
-    for keytype in "${KEY_TYPES[@]}"; do
-        for func in signing verify; do
-            errdata="$keytype.$func.stderr"
-            if [ -s "$errdata" ]; then
-                printf -- "-> %s:\n" "$errdata"
-                cat "$errdata"
-                printf -- "-> end %s\n" "$errdata"
-            fi
-        done
-    done
-    rm -rf "$WORKDIR"
-}
-
-for keytype in "${KEY_TYPES[@]}"; do
-    for func in signing verify; do
-        PYTHONPATH="$(dirname "$TESTDIR")" "$DKIMPY_MILTER" "$keytype.$func.conf" 2>"$keytype.$func.stderr" &
-    done
-done
-trap cleanup EXIT
-
-# ugly ugly (how are we supposed to know that the milters are all ready?):
-sleep 2
-
-# uses miltertest from opendkim:
-for x in ${TESTS:-"$TESTDIR"/*.miltertest}; do
-    if ! [ -e "$x" ]; then
-        if [ -e "$TESTDIR/$x" ]; then
-            x="$TESTDIR/$x"
-        fi
-    fi
-    printf -- "-> running %s...\n" "$x"
-    miltertest -s "$x"
-done