launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06865
[Merge] lp:~mbp/launchpad/925597-dkim-from into lp:launchpad
Martin Pool has proposed merging lp:~mbp/launchpad/925597-dkim-from into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #925597 in Launchpad itself: "From address is ignored if DKIM is supported"
https://bugs.launchpad.net/launchpad/+bug/925597
For more details, see:
https://code.launchpad.net/~mbp/launchpad/925597-dkim-from/+merge/98976
This fixes bug <https://bugs.launchpad.net/launchpad/+bug/925597> which is hit if you are sending mail from gmail, but your gmail address is not registered with lp.
I wonder if I also need to test the case that your gmail address is known but not verified? I probably should.
--
https://code.launchpad.net/~mbp/launchpad/925597-dkim-from/+merge/98976
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~mbp/launchpad/925597-dkim-from into lp:launchpad.
=== modified file 'lib/lp/services/mail/incoming.py'
--- lib/lp/services/mail/incoming.py 2012-01-04 03:23:19 +0000
+++ lib/lp/services/mail/incoming.py 2012-03-23 07:14:26 +0000
@@ -103,8 +103,8 @@
return domain in _trusted_dkim_domains
-def _authenticateDkim(signed_message):
- """Attempt DKIM authentication of email.
+def _verifyDkimOrigin(signed_message):
+ """Find a From or Sender address for which there's a DKIM signature, if any.
:returns: A string email address for the trusted sender, if there is one,
otherwise None.
@@ -190,6 +190,34 @@
return None
+def _getPrincipalByDkim(mail):
+ """Determine the security principal from DKIM, if possible.
+
+ To qualify:
+ * there must be a dkim signature from a trusted domain
+ * the From or Sender must be in that domain
+ * the address in this header must be verified for a person
+
+ :returns: (None, None), or (principal, trusted_addr).
+ """
+ log = logging.getLogger('mail-authenticate-dkim')
+ authutil = getUtility(IPlacelessAuthUtility)
+
+ dkim_trusted_address = _verifyDkimOrigin(mail)
+ if dkim_trusted_address is None:
+ return None, None
+
+ log.debug('authenticated DKIM mail origin %s' % dkim_trusted_address)
+ dkim_principal = authutil.getPrincipalByLogin(dkim_trusted_address)
+ if dkim_principal is None:
+ log.debug("valid dkim signature, but not from a known email address, "
+ "therefore disregarding it")
+ return None, None
+ else:
+ return (dkim_principal, dkim_trusted_address)
+
+
+
def authenticateEmail(mail,
signature_timestamp_checker=None):
"""Authenticates an email by verifying the PGP signature.
@@ -207,20 +235,13 @@
"""
log = logging.getLogger('process-mail')
-
- dkim_trusted_addr = _authenticateDkim(mail)
- if dkim_trusted_addr is not None:
- # The Sender field, if signed by a trusted domain, is the strong
- # authenticator for this mail.
- log.debug('trusted DKIM mail from %s' % dkim_trusted_addr)
- email_addr = dkim_trusted_addr
- else:
- email_addr = parseaddr(mail['From'])[1]
-
authutil = getUtility(IPlacelessAuthUtility)
- principal = authutil.getPrincipalByLogin(email_addr)
-
- # Check that sender is registered in Launchpad and the email is signed.
+
+ principal, dkim_trusted_address = _getPrincipalByDkim(mail)
+ if dkim_trusted_address is None:
+ from_addr = parseaddr(mail['From'])[1]
+ principal = authutil.getPrincipalByLogin(from_addr)
+
if principal is None:
setupInteraction(authutil.unauthenticatedPrincipal())
return None
@@ -230,9 +251,9 @@
raise InactiveAccount(
"Mail from a user with an inactive account.")
- if dkim_trusted_addr is not None:
+ if dkim_trusted_address:
log.debug('accepting dkim strongly authenticated mail')
- setupInteraction(principal, dkim_trusted_addr)
+ setupInteraction(principal, dkim_trusted_address)
return principal
else:
log.debug("attempt gpg authentication for %r" % person)
=== modified file 'lib/lp/services/mail/tests/test_dkim.py'
--- lib/lp/services/mail/tests/test_dkim.py 2012-01-01 02:58:52 +0000
+++ lib/lp/services/mail/tests/test_dkim.py 2012-03-23 07:14:26 +0000
@@ -46,16 +46,6 @@
b/mPfjC0QJTocVBq6Za/PlzfV+Py92VaCak19F4WrbVTK5Gg5tW220MCAwEAAQ=="""
-plain_content = """\
-From: Foo Bar <foo.bar@xxxxxxxxxxxxx>
-Date: Fri, 1 Apr 2010 00:00:00 +1000
-Subject: yet another comment
-To: 1@xxxxxxxxxxxxxxxxxxxxxxxxxx
-
- importance critical
-
-Why isn't this fixed yet?"""
-
class TestDKIM(TestCaseWithFactory):
"""Messages can be strongly authenticated by DKIM."""
@@ -119,13 +109,29 @@
if l.find(substring) == -1:
self.fail("didn't find %r in log: %s" % (substring, l))
+ def makeMessageText(self, sender=None, from_address=None):
+ if from_address is None:
+ from_address = "Foo Bar <foo.bar@xxxxxxxxxxxxx>"
+ text = ("From: " + from_address + "\n" + """\
+Date: Fri, 1 Apr 2010 00:00:00 +1000
+Subject: yet another comment
+To: 1@xxxxxxxxxxxxxxxxxxxxxxxxxx
+
+ importance critical
+
+Why isn't this fixed yet?""")
+ if sender is not None:
+ text = "Sender: " + sender + "\n" + text
+ return text
+
+
def test_dkim_broken_pubkey(self):
"""Handle a subtly-broken pubkey like qq.com, see bug 881237.
The message is not trusted but inbound message processing does not
abort either.
"""
- signed_message = self.fake_signing(plain_content)
+ signed_message = self.fake_signing(self.makeMessageText())
self._dns_responses['example._domainkey.canonical.com.'] = \
sample_dns.replace(';', '')
principal = authenticateEmail(
@@ -136,7 +142,7 @@
self.assertDkimLogContains('unexpected error in DKIM verification')
def test_dkim_garbage_pubkey(self):
- signed_message = self.fake_signing(plain_content)
+ signed_message = self.fake_signing(self.makeMessageText())
self._dns_responses['example._domainkey.canonical.com.'] = \
'aothuaonu'
principal = authenticateEmail(
@@ -155,7 +161,7 @@
self.test_dkim_valid_strict)
def test_dkim_valid_strict(self):
- signed_message = self.fake_signing(plain_content,
+ signed_message = self.fake_signing(self.makeMessageText(),
canonicalize=(dkim.Simple, dkim.Simple))
self._dns_responses['example._domainkey.canonical.com.'] = \
sample_dns
@@ -166,7 +172,7 @@
'foo.bar@xxxxxxxxxxxxx')
def test_dkim_valid(self):
- signed_message = self.fake_signing(plain_content)
+ signed_message = self.fake_signing(self.makeMessageText())
self._dns_responses['example._domainkey.canonical.com.'] = \
sample_dns
principal = authenticateEmail(
@@ -177,7 +183,7 @@
def test_dkim_untrusted_signer(self):
# Valid signature from an untrusted domain -> untrusted
- signed_message = self.fake_signing(plain_content)
+ signed_message = self.fake_signing(self.makeMessageText())
self._dns_responses['example._domainkey.canonical.com.'] = \
sample_dns
saved_domains = incoming._trusted_dkim_domains[:]
@@ -198,8 +204,8 @@
# that of the From-sender, if that domain is relaying the message.
# However, we shouldn't then trust the purported sender, because they
# might have just made it up rather than relayed it.
- tweaked_message = plain_content.replace('foo.bar@xxxxxxxxxxxxx',
- 'steve.alexander@xxxxxxxxxxxxxxx')
+ tweaked_message = self.makeMessageText(
+ from_address='steve.alexander@xxxxxxxxxxxxxxx')
signed_message = self.fake_signing(tweaked_message)
self._dns_responses['example._domainkey.canonical.com.'] = \
sample_dns
@@ -215,7 +221,7 @@
# We still treat this as weakly authenticated by the purported
# From-header sender, though perhaps in future we would prefer
# to reject these messages.
- signed_message = self.fake_signing(plain_content)
+ signed_message = self.fake_signing(self.makeMessageText())
self._dns_responses['example._domainkey.canonical.com.'] = \
sample_dns
fiddled_message = signed_message.replace(
@@ -230,7 +236,7 @@
def test_dkim_changed_from_realname(self):
# If the real name part of the message has changed, it's detected.
- signed_message = self.fake_signing(plain_content)
+ signed_message = self.fake_signing(self.makeMessageText())
self._dns_responses['example._domainkey.canonical.com.'] = \
sample_dns
fiddled_message = signed_message.replace(
@@ -246,7 +252,7 @@
def test_dkim_nxdomain(self):
# If there's no DNS entry for the pubkey it should be handled
# decently.
- signed_message = self.fake_signing(plain_content)
+ signed_message = self.fake_signing(self.makeMessageText())
principal = authenticateEmail(
signed_message_from_string(signed_message))
self.assertWeaklyAuthenticated(principal, signed_message)
@@ -258,8 +264,8 @@
# treated as weakly authenticated.
# The library doesn't log anything if there's no header at all.
principal = authenticateEmail(
- signed_message_from_string(plain_content))
- self.assertWeaklyAuthenticated(principal, plain_content)
+ signed_message_from_string(self.makeMessageText()))
+ self.assertWeaklyAuthenticated(principal, self.makeMessageText())
self.assertEqual(principal.person.preferredemail.email,
'foo.bar@xxxxxxxxxxxxx')
@@ -267,7 +273,7 @@
# The message has a syntactically valid DKIM signature that
# doesn't actually correspond to what was signed. We log
# something about this but we don't want to drop the message.
- signed_message = self.fake_signing(plain_content)
+ signed_message = self.fake_signing(self.makeMessageText())
signed_message += 'blah blah'
self._dns_responses['example._domainkey.canonical.com.'] = \
sample_dns
@@ -292,11 +298,32 @@
person=person,
address='dkimtest@xxxxxxxxxxx')
self._dns_responses['example._domainkey.canonical.com.'] = sample_dns
- tweaked_message = (
- "Sender: dkimtest@xxxxxxxxxxxxx\n" + plain_content.replace(
- "From: Foo Bar <foo.bar@xxxxxxxxxxxxx>",
- "From: DKIM Test <dkimtest@xxxxxxxxxxx>"))
+ tweaked_message = self.makeMessageText(
+ sender="dkimtest@xxxxxxxxxxxxx",
+ from_address="DKIM Test <dkimtest@xxxxxxxxxxx>")
signed_message = self.fake_signing(tweaked_message)
principal = authenticateEmail(
signed_message_from_string(signed_message))
self.assertStronglyAuthenticated(principal, signed_message)
+
+ def test_dkim_signed_but_from_unknown_address(self):
+ """Sent from trusted dkim address, but only the From address is known.
+
+ See https://bugs.launchpad.net/launchpad/+bug/925597
+ """
+ person = self.factory.makePerson(
+ email='dkimtest@xxxxxxxxxxx',
+ name='dkimtest',
+ displayname='DKIM Test')
+ self._dns_responses['example._domainkey.canonical.com.'] = sample_dns
+ tweaked_message = self.makeMessageText(
+ sender="dkimtest@xxxxxxxxxxxxx",
+ from_address="DKIM Test <dkimtest@xxxxxxxxxxx>")
+ signed_message = self.fake_signing(tweaked_message)
+ principal = authenticateEmail(
+ signed_message_from_string(signed_message))
+ self.assertEqual(principal.person.preferredemail.email,
+ 'dkimtest@xxxxxxxxxxx')
+ self.assertWeaklyAuthenticated(principal, signed_message)
+ self.assertDkimLogContains(
+ 'valid dkim signature, but not from a known email address')
