Merge lp:~diane-trout/dkimpy/python3 into lp:dkimpy

[Diane Trout]

Several of the unit tests failed under python3.3

Under python3 there were some inconsistencies in whether strings were bytes or strings. I tried to make it more consistent. With revno 103 the only unit tests that were failing under python 3.3 were a docstring test.

[Stuart Gathman (stuart-gathman)]

Most of the proposed changes add b to a string constant. Does re.compile really take byte strings in python3? Or does it depend on whether you are parsing bytes or strings?

[Diane Trout (diane-trout)]

On Wednesday, March 20, 2013 23:27:28 Stuart Gathman wrote:
> Most of the proposed changes add b to a string constant. Does re.compile
> really take byte strings in python3? Or does it depend on whether you are
> parsing bytes or strings?

Yep. assuming the order is right. I did run test.py with python 2.7.3 and
python 3.3.0. In theory it passed with both. (I'm not sure what would happen
with 2.6)

Python 3.2.3 (default, Feb 20 2013, 14:44:27)
[GCC 4.7.2] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import re
>>> re.compile(br'foo')
<_sre.SRE_Pattern object at 0x1bcc5a0>
>>>

The order of the r & b modifiers matters:
rb'foo' gives a syntax error.

The reason I needed to go tag things is I got error messages like the
following.

Python 3.3.0 (default, Mar 6 2013, 13:45:29)
[GCC 4.7.2] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import re
>>> a = b'123'
>>> re.search('2', a)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3.3/re.py", line 161, in search
    return _compile(pattern, flags).search(string)
TypeError: can't use a string pattern on a bytes-like object

Also I found another issue in dkimpy/__init__ where I had a message that had a
x tag but not the t tag. (rfc4871 says "if both are present" implying t= is
optional.)

I hadn't committed the following patch as I thought I should make a test case
first.

=== modified file 'dkim/__init__.py'
--- dkim/__init__.py 2013-03-20 16:01:55 +0000
+++ dkim/__init__.py 2013-03-20 16:12:41 +0000
@@ -177,7 +177,7 @@
         if re.match(br"\d+$", sig[b'x']) is None:
             raise ValidationError(
                 "x= value is not a decimal integer (%s)" % sig[b'x'])
- if int(sig[b'x']) < int(sig[b't']):
+ if b't' in sig and int(sig[b'x']) < int(sig[b't']):

[Diane Trout (diane-trout)]

On Wednesday, March 20, 2013 23:27:28 you wrote:
> Most of the proposed changes add b to a string constant. Does re.compile
> really take byte strings in python3? Or does it depend on whether you are
> parsing bytes or strings?

One other thing I found was the dns library I was using was returning python 3
strings, and most of dkim had been moved to using byte strings, which caused
indexing issues.

I'm not really sure if dns get_txt records should be returning bytes or
strings, and wanted to ask one of the python3 porting people but it looks like
they're still travelling back from pycon.

I did discover that looking for b'data' and 'data' in a python 3 dictionary is
different.

diane

[Scott Kitterman (kitterman)]

On Thursday, March 21, 2013 12:31:22 AM Diane Trout wrote:
> On Wednesday, March 20, 2013 23:27:28 you wrote:
> > Most of the proposed changes add b to a string constant. Does re.compile
> > really take byte strings in python3? Or does it depend on whether you are
> > parsing bytes or strings?
>
> One other thing I found was the dns library I was using was returning python
> 3 strings, and most of dkim had been moved to using byte strings, which
> caused indexing issues.
>
> I'm not really sure if dns get_txt records should be returning bytes or
> strings, and wanted to ask one of the python3 porting people but it looks
> like they're still travelling back from pycon.
>
> I did discover that looking for b'data' and 'data' in a python 3 dictionary
> is different.

I did the python3 port of pydns. It's quite possible I got it wrong. At the
time, there was no other python3 DNS module available, so I was sort of
feeling my way along in the dark. It wouldn't surprise me if I got stuff
wrong. Were you using pydns (module DNS) or dnspython (module dns)? Pydns
in particular is fun to deal with since it's very old. It was originally
written for python1.5 or 1.6 and has not had a lot more than bug fixing done to
it for a long time before I ported it.

[Diane Trout (diane-trout)]

Looks like the one I used was imported with:

import DNS

It appears the debian package is python3-dns with a homepage of
http://launchpad.net/py3dns

I'm guessing dns should probably return bytes by default?

I read through https://en.wikipedia.org/wiki/Internationalized_domain_name and
RFC 2181 which seem to indicate DNS can hold arbitrary binary strings and that
individual records can specify their own encoding -- like punycode
representation for non-ASCII domain names.

So the "ideal" dns library might have some helper functions to decode specific
resource records to unicode.

Diane

On Thursday, March 21, 2013 02:37:24 Scott Kitterman wrote:
> On Thursday, March 21, 2013 12:31:22 AM Diane Trout wrote:
> > On Wednesday, March 20, 2013 23:27:28 you wrote:
> > > Most of the proposed changes add b to a string constant. Does
> > > re.compile
> > > really take byte strings in python3? Or does it depend on whether you
> > > are
> > > parsing bytes or strings?
> >
> > One other thing I found was the dns library I was using was returning
> > python 3 strings, and most of dkim had been moved to using byte strings,
> > which caused indexing issues.
> >
> > I'm not really sure if dns get_txt records should be returning bytes or
> > strings, and wanted to ask one of the python3 porting people but it looks
> > like they're still travelling back from pycon.
> >
> > I did discover that looking for b'data' and 'data' in a python 3
> > dictionary
> > is different.
>
> I did the python3 port of pydns. It's quite possible I got it wrong. At
> the time, there was no other python3 DNS module available, so I was sort of
> feeling my way along in the dark. It wouldn't surprise me if I got stuff
> wrong. Were you using pydns (module DNS) or dnspython (module dns)?
> Pydns in particular is fun to deal with since it's very old. It was
> originally written for python1.5 or 1.6 and has not had a lot more than bug
> fixing done to it for a long time before I ported it.

[lp:~diane-trout/dkimpy/python3]

104. By Diane Trout <email address hidden> on 2013-03-21

The t tag may not be present when x is specified.

Also add in some tests for checking validate_signature_fields

[Scott Kitterman (kitterman)]

On Thursday, March 21, 2013 03:16:25 AM you wrote:
> Looks like the one I used was imported with:
>
> import DNS
>
> It appears the debian package is python3-dns with a homepage of
> http://launchpad.net/py3dns
>
> I'm guessing dns should probably return bytes by default?
>
> I read through https://en.wikipedia.org/wiki/Internationalized_domain_name
> and RFC 2181 which seem to indicate DNS can hold arbitrary binary strings
> and that individual records can specify their own encoding -- like punycode
> representation for non-ASCII domain names.
>
> So the "ideal" dns library might have some helper functions to decode
> specific resource records to unicode.

That's the one I ported. If you have some interest in it, I'm glad for help
with getting it straightened out (even just correcting/contributing tests
would help a lot).

[Diane Trout (diane-trout)]

I'll make a stab at it in the next day or two, as It'd be cleaner than the
function i hid in dkimpy to convert return values to byte strings.

Also in my python3 dkimpy branch I also committed some tests and the fix for a
missing t=, should I open a new pull request and/or file a bug?

Diane

On Thursday, March 21, 2013 04:25:23 you wrote:
> On Thursday, March 21, 2013 03:16:25 AM you wrote:
> > Looks like the one I used was imported with:
> >
> > import DNS
> >
> > It appears the debian package is python3-dns with a homepage of
> > http://launchpad.net/py3dns
> >
> > I'm guessing dns should probably return bytes by default?
> >
> > I read through https://en.wikipedia.org/wiki/Internationalized_domain_name
> > and RFC 2181 which seem to indicate DNS can hold arbitrary binary strings
> > and that individual records can specify their own encoding -- like
> > punycode
> > representation for non-ASCII domain names.
> >
> > So the "ideal" dns library might have some helper functions to decode
> > specific resource records to unicode.
>
> That's the one I ported. If you have some interest in it, I'm glad for help
> with getting it straightened out (even just correcting/contributing tests
> would help a lot).

[Scott Kitterman (kitterman)]

A new merge request is probably best.

[Stuart Gathman (stuart-gathman)]

Long ago, Nostradamus foresaw that on 03/20/2013 11:16 PM, Diane Trout
would write:
> Looks like the one I used was imported with:
>
> import DNS
>
> It appears the debian package is python3-dns with a homepage of
> http://launchpad.net/py3dns
>
> I'm guessing dns should probably return bytes by default?
>
> I read through https://en.wikipedia.org/wiki/Internationalized_domain_name and
> RFC 2181 which seem to indicate DNS can hold arbitrary binary strings and that
> individual records can specify their own encoding -- like punycode
> representation for non-ASCII domain names.
>
> So the "ideal" dns library might have some helper functions to decode specific
> resource records to unicode.
>
Yes, the unfinished goal for pydns is to have a "raw" value of bytes for
TXT, and "cooked" value of unicode. The default should be bytes (and I
screwed it up by adding some global options for decoding to unicode),
and we will be defining extensions for cooked values. Another screwy
thing is that A RRs are returned as dotted quad strings, whereas AAAA
RRs are returned as bytes. That is more difficult to rationalize
without breaking existing code. So that led to the idea of 3 modes:
raw, cooked, compat.

[Diane Trout (diane-trout)]

What do you think of the test layout in dkimpy? (test.py in root which is importing unittest style tests in <module>.tests?)

Once upon a time I tried to use unit2 & nose2 and had some state issues... (although that was a django app I was trying to test...)

The DNS.Request(...).req(resulttype='binary') from py3dns "3.0.3" worked 'better' than without the resulttype tag, however my program still crashed at some point in dkim because one of the data results wasn't a list.

I'm thinking a reasonable next step is to write some more test cases, I'm just not sure if you have a preference for a test runner for python3? Unittest is more verbose than some of the other choices, but at least it is in the standard library.

Diane

> On Thursday, March 21, 2013 03:16:25 AM you wrote:
> > Looks like the one I used was imported with:
> >
> > import DNS
> >
> > It appears the debian package is python3-dns with a homepage of
> > http://launchpad.net/py3dns
> >
> > I'm guessing dns should probably return bytes by default?
> >
> > I read through https://en.wikipedia.org/wiki/Internationalized_domain_name
> > and RFC 2181 which seem to indicate DNS can hold arbitrary binary strings
> > and that individual records can specify their own encoding -- like punycode
> > representation for non-ASCII domain names.
> >
> > So the "ideal" dns library might have some helper functions to decode
> > specific resource records to unicode.
>
> That's the one I ported. If you have some interest in it, I'm glad for help
> with getting it straightened out (even just correcting/contributing tests
> would help a lot).

[Stuart Gathman (stuart-gathman)]

Long ago, Nostradamus foresaw that on 03/22/2013 01:47 AM, Diane Trout
would write:
> What do you think of the test layout in dkimpy? (test.py in root which is importing unittest style tests in <module>.tests?)
>
> Once upon a time I tried to use unit2 & nose2 and had some state issues... (although that was a django app I was trying to test...)
The tests should be nose compatible, and include test.py in root so that
you can also run them without nose. At a simple level, nose is simply a
way of finding all your tests by naming convention (class names start
with Test... and methods start with test_...) so that you don't have to
write a test.py. Any deviations from a nose compatible class name
should be fixed.

[lp:~diane-trout/dkimpy/python3]

105. By Diane Trout <email address hidden> on 2013-03-22

py3dns returns strings, not bytes. so the join didn't work.

[Stuart Gathman (stuart-gathman)]

Revision 110 adds the changes from Diane that are still applicable. Both python2.7 and python3.4 pass unit tests now.

I am not happy that *all* of the API is bytes. For instance, the canonicalization options should be str, not bytes. The header names for frozen_sign, should_not_sign, etc, should be str. But, of course, email messages must be bytes.

I introduced a text(s) function that converts the bytes/str/unicode argument to str (as defined by python version). If text is a little more efficient (and I just remembered I can define one of 2 versions dynamically), we can use that to lookup header names in those config fields, and change them back to str instead of bytes.

[Stuart Gathman (stuart-gathman)]

I guess I was supposed to create another branch and resubmit the merge proposal, and not just do it on the trunk. But except for frozen_headers, etc, the API is no worse. And we can fix those.

[Stuart Gathman (stuart-gathman)]

So what is the proper way to prevent auto-merging this year old branch from Diane? I've already manually merged the changes that were still relevant.

[Diane Trout (diane-trout)]

On Sunday, February 22, 2015 01:37:56 Stuart Gathman wrote:
> So what is the proper way to prevent auto-merging this year old branch from
> Diane? I've already manually merged the changes that were still relevant.

I just got a notification from launchpad, would it help for me to try and pull
and update my branch?

Diane

[William Grant (wgrant)]

I'd just set the merge proposal status at the top to "Merged" manually. It won't automerge it.

=== modified file 'dkim/__init__.py'
--- dkim/__init__.py	2012-10-27 15:23:05 +0000
+++ dkim/__init__.py	2013-03-22 22:58:34 +0000
@@ -118,8 +118,8 @@
         lastindex[h] = i
     return sign_headers
 
-FWS = r'(?:\r?\n\s+)?'
-RE_BTAG = re.compile(r'([;\s]b'+FWS+r'=)(?:'+FWS+r'[a-zA-Z0-9+/=])*(?:\r?\n\Z)?')
+FWS = br'(?:\r?\n\s+)?'
+RE_BTAG = re.compile(br'([;\s]b'+FWS+br'=)(?:'+FWS+br'[a-zA-Z0-9+/=])*(?:\r?\n\Z)?')
 
 def hash_headers(hasher, canonicalize_headers, headers, include_headers,
                  sigheader, sig):
@@ -165,7 +165,7 @@
         raise ValidationError(
             "i= domain is not a subdomain of d= (i=%s d=%s)" %
             (sig[b'i'], sig[b'd']))
-    if b'l' in sig and re.match(br"\d{,76}$", sig['l']) is None:
+    if b'l' in sig and re.match(br"\d{,76}$", sig[b'l']) is None:
         raise ValidationError(
             "l= value is not a decimal integer (%s)" % sig[b'l'])
     if b'q' in sig and sig[b'q'] != b"dns/txt":
@@ -177,7 +177,7 @@
         if re.match(br"\d+$", sig[b'x']) is None:
             raise ValidationError(
                 "x= value is not a decimal integer (%s)" % sig[b'x'])
-        if int(sig[b'x']) < int(sig[b't']):
+        if b't' in sig and int(sig[b'x']) < int(sig[b't']):
             raise ValidationError(
                 "x= value is less than t= value (x=%s t=%s)" %
                 (sig[b'x'], sig[b't']))
@@ -417,7 +417,7 @@
         include_headers = self.default_sign_headers()
 
     # rfc4871 says FROM is required
-    if 'from' not in ( x.lower() for x in include_headers ):
+    if b'from' not in ( x.lower() for x in include_headers ):
         raise ParameterError("The From header field MUST be signed")
 
     # raise exception for any SHOULD_NOT headers, call can modify 
@@ -540,7 +540,7 @@
         raise KeyFormatError("missing public key: %s"%name)
     try:
         pub = parse_tag_value(s)
-    except InvalidTagValueList:
+    except InvalidTagValueList as e:
         raise KeyFormatError(e)
     try:
         pk = parse_public_key(base64.b64decode(pub[b'p']))
@@ -555,8 +555,8 @@
     # fields when verifying.  Since there should be only one From header,
     # this shouldn't break any legitimate messages.  This could be
     # generalized to check for extras of other singleton headers.
-    if 'from' in include_headers:
-      include_headers.append('from')      
+    if b'from' in include_headers:
+      include_headers.append(b'from')      
     h = hasher()
     self.signed_headers = hash_headers(
         h, canon_policy, headers, include_headers, sigheaders[idx], sig)

=== modified file 'dkim/dnsplug.py'
--- dkim/dnsplug.py	2012-06-13 03:09:15 +0000
+++ dkim/dnsplug.py	2013-03-22 22:58:34 +0000
@@ -44,7 +44,7 @@
     response = DNS.DnsRequest(name, qtype='txt').req()
     if not response.answers:
         return None
-    return b''.join(response.answers[0]['data'])
+    return ''.join(response.answers[0]['data'])
 
 def get_txt_Milter_dns(name):
     """Return a TXT record associated with a DNS name."""
@@ -80,6 +80,6 @@
     except UnicodeDecodeError:
         return None
     txt = _get_txt(unicode_name)
-    if txt:
+    if txt and isinstance(txt, str):
       txt = txt.encode('utf-8')
     return txt

=== modified file 'dkim/tests/test_dkim.py'
--- dkim/tests/test_dkim.py	2012-10-27 14:48:29 +0000
+++ dkim/tests/test_dkim.py	2013-03-22 22:58:34 +0000
@@ -92,14 +92,14 @@
       # <https://bugs.launchpad.net/dkimpy/+bug/939128>
       # Simple-mode signature header verification is wrong
       # (should ignore FWS anywhere in signature tag: b=)
-      sample_msg = """\
+      sample_msg = b"""\
 From: [e-mail]
 To: [e-mail]
 Subject: this is my
     test message
-""".replace('\n', '\r\n')
+""".replace(b'\n', b'\r\n')
 
-      sample_privkey = """\
+      sample_privkey = b"""\
 -----BEGIN RSA PRIVATE KEY-----
 MIIBOwIBAAJBANmBe10IgY+u7h3enWTukkqtUD5PR52Tb/mPfjC0QJTocVBq6Za/
 PlzfV+Py92VaCak19F4WrbVTK5Gg5tW220MCAwEAAQJAYFUKsD+uMlcFu1D3YNaR
@@ -111,22 +111,22 @@
 -----END RSA PRIVATE KEY-----
 """
 
-      sample_pubkey = """\
+      sample_pubkey = b"""\
 -----BEGIN PUBLIC KEY-----
 MFwwDQYJKoZIhvcNAQEBBQADSwAwSAJBANmBe10IgY+u7h3enWTukkqtUD5PR52T
 b/mPfjC0QJTocVBq6Za/PlzfV+Py92VaCak19F4WrbVTK5Gg5tW220MCAwEAAQ==
 -----END PUBLIC KEY-----
 """
 
-      sample_dns = """\
+      sample_dns = b"""\
 k=rsa; \
 p=MFwwDQYJKoZIhvcNAQEBBQADSwAwSAJBANmBe10IgY+u7h3enWTukkqtUD5PR52T\
 b/mPfjC0QJTocVBq6Za/PlzfV+Py92VaCak19F4WrbVTK5Gg5tW220MCAwEAAQ=="""
 
-      _dns_responses = {'example._domainkey.canonical.com.': sample_dns}
+      _dns_responses = {b'example._domainkey.canonical.com.': sample_dns}
       for header_mode in [dkim.Relaxed, dkim.Simple]:
 
-        dkim_header = dkim.sign(sample_msg, 'example', 'canonical.com',
+        dkim_header = dkim.sign(sample_msg, b'example', b'canonical.com',
             sample_privkey, canonicalize=(header_mode, dkim.Relaxed))
         # Folding dkim_header affects b= tag only, since dkim.sign folds
         # sig_value with empty b= before hashing, and then appends the
@@ -192,6 +192,33 @@
                 res = dkim.verify(sig+message, dnsfunc=self.dnsfunc)
                 self.assertFalse(res)
 
+    def test_validate_signature_fields(self):
+        sig = {b'v': b'1',
+               b'a': b'rsa-sha256',
+               b'b': b'K/UUOt8lCtgjp3kSTogqBm9lY1Yax/NwZ+bKm39/WKzo5KYe3L/6RoIA/0oiDX4kO\n    \t Qut49HCV6ZUe6dY9V5qWBwLanRs1sCnObaOGMpFfs8tU4TWpDSVXaNZAqn15XVW0WH\n    \t EzOzUfVuatpa1kF4voIgSbmZHR1vN3WpRtcTBe/I=',
+               b'bh': b'n0HUwGCP28PkesXBPH82Kboy8LhNFWU9zUISIpAez7M=',
+               b'c': b'simple/simple',
+               b'd': b'kitterman.com',
+               b'h': b'From:To:Subject:Date:Cc:MIME-Version:Content-Type:\n    \t Content-Transfer-Encoding:Message-Id',
+               b's': b'2007-00',
+               b't': b'1299525798'}
+        dkim.validate_signature_fields(sig)
+        # try new version
+        sigVer = sig.copy()
+        sigVer[b'v'] = 2
+        self.assertRaises(dkim.ValidationError, dkim.validate_signature_fields, sigVer)
+        # try with x
+        sigX = sig.copy()
+        sigX[b'x'] = b'1399525798'
+        dkim.validate_signature_fields(sig)
+        # try with late t
+        sigX[b't'] = b'1400000000'
+        self.assertRaises(dkim.ValidationError, dkim.validate_signature_fields, sigX)
+        # try without t
+        del sigX[b't']
+        dkim.validate_signature_fields(sigX)
+        
+        
 def test_suite():
     from unittest import TestLoader
     return TestLoader().loadTestsFromName(__name__)

=== modified file 'dkim/tests/test_util.py'
--- dkim/tests/test_util.py	2011-06-17 03:38:14 +0000
+++ dkim/tests/test_util.py	2013-03-22 22:58:34 +0000
@@ -62,7 +62,7 @@
             DuplicateTag, parse_tag_value, b'foo=bar;foo=baz')
 
     def test_trailing_whitespace(self):
-      hval = '''v=1; a=rsa-sha256; d=facebookmail.com; s=s1024-2011-q2; c=relaxed/simple;
+      hval = b'''v=1; a=rsa-sha256; d=facebookmail.com; s=s1024-2011-q2; c=relaxed/simple;
           q=dns/txt; i=@facebookmail.com; t=1308078492;
           h=From:Subject:Date:To:MIME-Version:Content-Type;
           bh=+qPyCOiDQkusTPstCoGjimgDgeZbUaJWIr1mdE6RFxk=;
@@ -71,7 +71,7 @@
           3KzW0yB9JHwiDCw1EioVkv+OMHhAYzoIypA0bQyi2bc=;
   '''
       sig = parse_tag_value(hval)
-      self.assertEquals(sig[b't'],'1308078492')
+      self.assertEquals(sig[b't'],b'1308078492')
       self.assertEquals(len(sig),11)