Merge ~dick-mrns/dkimpy:linesep into dkimpy:master
- Git
- lp:~dick-mrns/dkimpy
- linesep
- Merge into master
Proposed by
meeuw
Status: | Merged |
---|---|
Approved by: | Scott Kitterman |
Approved revision: | 6bc38aaf9b1ed7cae32d21a93f5084c3b3f8a5da |
Merge reported by: | Scott Kitterman |
Merged at revision: | 6bc38aaf9b1ed7cae32d21a93f5084c3b3f8a5da |
Proposed branch: | ~dick-mrns/dkimpy:linesep |
Merge into: | dkimpy:master |
Diff against target: |
251 lines (+69/-15) 6 files modified
dkim/__init__.py (+19/-12) dkim/arcsign.py (+1/-1) dkim/dkimsign.py (+2/-2) dkim/tests/test_dkim.py (+16/-0) dkim/tests/test_util.py (+25/-0) dkim/util.py (+6/-0) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Scott Kitterman | Approve | ||
Review via email: mp+361264@code.launchpad.net |
Commit message
Description of the change
To post a comment you must log in.
Revision history for this message
meeuw (dick-mrns) wrote : | # |
Revision history for this message
Scott Kitterman (kitterman) wrote : | # |
Responding needs fixing until the tests, etc I mentioned in the bug are included.
review:
Needs Fixing
Revision history for this message
meeuw (dick-mrns) wrote : | # |
Okay, I think I've fixed all your suggestions, could you please have a look?
Revision history for this message
Scott Kitterman (kitterman) wrote : | # |
Looks good. I'll see about merging this.
review:
Approve
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | diff --git a/dkim/__init__.py b/dkim/__init__.py | |||
2 | index a8d3bbe..b0b5721 100644 | |||
3 | --- a/dkim/__init__.py | |||
4 | +++ b/dkim/__init__.py | |||
5 | @@ -365,7 +365,7 @@ def text(s): | |||
6 | 365 | return s.encode('ascii') | 365 | return s.encode('ascii') |
7 | 366 | 366 | ||
8 | 367 | 367 | ||
10 | 368 | def fold(header, namelen=0): | 368 | def fold(header, namelen=0, linesep=b'\r\n'): |
11 | 369 | """Fold a header line into multiple crlf-separated lines at column 72. | 369 | """Fold a header line into multiple crlf-separated lines at column 72. |
12 | 370 | 370 | ||
13 | 371 | >>> text(fold(b'foo')) | 371 | >>> text(fold(b'foo')) |
14 | @@ -399,10 +399,10 @@ def fold(header, namelen=0): | |||
15 | 399 | i = header[:maxleng].rfind(b" ") | 399 | i = header[:maxleng].rfind(b" ") |
16 | 400 | if i == -1: | 400 | if i == -1: |
17 | 401 | j = maxleng | 401 | j = maxleng |
19 | 402 | pre += header[:j] + b"\r\n " | 402 | pre += header[:j] + linesep + b" " |
20 | 403 | else: | 403 | else: |
21 | 404 | j = i + 1 | 404 | j = i + 1 |
23 | 405 | pre += header[:i] + b"\r\n " | 405 | pre += header[:i] + linesep + b" " |
24 | 406 | header = header[j:] | 406 | header = header[j:] |
25 | 407 | maxleng = 72 | 407 | maxleng = 72 |
26 | 408 | if len(header) > 2: | 408 | if len(header) > 2: |
27 | @@ -460,8 +460,9 @@ class DomainSigner(object): | |||
28 | 460 | #: (with either \\n or \\r\\n line endings) | 460 | #: (with either \\n or \\r\\n line endings) |
29 | 461 | #: @param logger: a logger to which debug info will be written (default None) | 461 | #: @param logger: a logger to which debug info will be written (default None) |
30 | 462 | #: @param signature_algorithm: the signing algorithm to use when signing | 462 | #: @param signature_algorithm: the signing algorithm to use when signing |
31 | 463 | #: @param linesep: use this line seperator for folding the headers | ||
32 | 463 | def __init__(self,message=None,logger=None,signature_algorithm=b'rsa-sha256', | 464 | def __init__(self,message=None,logger=None,signature_algorithm=b'rsa-sha256', |
34 | 464 | minkey=1024): | 465 | minkey=1024, linesep=b'\r\n'): |
35 | 465 | self.set_message(message) | 466 | self.set_message(message) |
36 | 466 | if logger is None: | 467 | if logger is None: |
37 | 467 | logger = get_default_logger() | 468 | logger = get_default_logger() |
38 | @@ -482,6 +483,9 @@ class DomainSigner(object): | |||
39 | 482 | #: Minimum public key size. Shorter keys raise KeyFormatError. The | 483 | #: Minimum public key size. Shorter keys raise KeyFormatError. The |
40 | 483 | #: default is 1024 | 484 | #: default is 1024 |
41 | 484 | self.minkey = minkey | 485 | self.minkey = minkey |
42 | 486 | # use this line seperator for output | ||
43 | 487 | self.linesep = linesep | ||
44 | 488 | |||
45 | 485 | 489 | ||
46 | 486 | #: Header fields to protect from additions by default. | 490 | #: Header fields to protect from additions by default. |
47 | 487 | #: | 491 | #: |
48 | @@ -608,7 +612,7 @@ class DomainSigner(object): | |||
49 | 608 | 612 | ||
50 | 609 | header_value = b"; ".join(b"=".join(x) for x in fields) | 613 | header_value = b"; ".join(b"=".join(x) for x in fields) |
51 | 610 | if not standardize: | 614 | if not standardize: |
53 | 611 | header_value = fold(header_value, namelen=len(header_name)) | 615 | header_value = fold(header_value, namelen=len(header_name), linesep=b'\r\n') |
54 | 612 | header_value = RE_BTAG.sub(b'\\1',header_value) | 616 | header_value = RE_BTAG.sub(b'\\1',header_value) |
55 | 613 | header = (header_name, b' ' + header_value) | 617 | header = (header_name, b' ' + header_value) |
56 | 614 | h = HashThrough(self.hasher()) | 618 | h = HashThrough(self.hasher()) |
57 | @@ -634,10 +638,10 @@ class DomainSigner(object): | |||
58 | 634 | # relaxed/simple (for broken receivers), and fold now. | 638 | # relaxed/simple (for broken receivers), and fold now. |
59 | 635 | idx = [i for i in range(len(fields)) if fields[i][0] == b'b'][0] | 639 | idx = [i for i in range(len(fields)) if fields[i][0] == b'b'][0] |
60 | 636 | fields[idx] = (b'b', base64.b64encode(bytes(sig2))) | 640 | fields[idx] = (b'b', base64.b64encode(bytes(sig2))) |
62 | 637 | header_value = b"; ".join(b"=".join(x) for x in fields) + b"\r\n" | 641 | header_value = b"; ".join(b"=".join(x) for x in fields) + self.linesep |
63 | 638 | 642 | ||
64 | 639 | if not standardize: | 643 | if not standardize: |
66 | 640 | header_value = fold(header_value, namelen=len(header_name)) | 644 | header_value = fold(header_value, namelen=len(header_name), linesep=self.linesep) |
67 | 641 | 645 | ||
68 | 642 | return header_value | 646 | return header_value |
69 | 643 | 647 | ||
70 | @@ -947,7 +951,7 @@ class ARC(DomainSigner): | |||
71 | 947 | results_lists = [raw.replace(srv_id + b';', b'').strip() for (raw, parsed) in auth_headers] | 951 | results_lists = [raw.replace(srv_id + b';', b'').strip() for (raw, parsed) in auth_headers] |
72 | 948 | results_lists = [tags.split(b';') for tags in results_lists] | 952 | results_lists = [tags.split(b';') for tags in results_lists] |
73 | 949 | results = [tag.strip() for sublist in results_lists for tag in sublist] | 953 | results = [tag.strip() for sublist in results_lists for tag in sublist] |
75 | 950 | auth_results = srv_id + b'; ' + b';\r\n '.join(results) | 954 | auth_results = srv_id + b'; ' + (b';' + self.linesep + b' ').join(results) |
76 | 951 | 955 | ||
77 | 952 | # extract cv | 956 | # extract cv |
78 | 953 | parsed_auth_results = AuthenticationResultsHeader.parse('Authentication-Results: ' + auth_results.decode('utf-8')) | 957 | parsed_auth_results = AuthenticationResultsHeader.parse('Authentication-Results: ' + auth_results.decode('utf-8')) |
79 | @@ -1216,7 +1220,8 @@ class ARC(DomainSigner): | |||
80 | 1216 | def sign(message, selector, domain, privkey, identity=None, | 1220 | def sign(message, selector, domain, privkey, identity=None, |
81 | 1217 | canonicalize=(b'relaxed', b'simple'), | 1221 | canonicalize=(b'relaxed', b'simple'), |
82 | 1218 | signature_algorithm=b'rsa-sha256', | 1222 | signature_algorithm=b'rsa-sha256', |
84 | 1219 | include_headers=None, length=False, logger=None): | 1223 | include_headers=None, length=False, logger=None, |
85 | 1224 | linesep=b'\r\n'): | ||
86 | 1220 | # type: (bytes, bytes, bytes, bytes, bytes, tuple, bytes, list, bool, any) -> bytes | 1225 | # type: (bytes, bytes, bytes, bytes, bytes, tuple, bytes, list, bool, any) -> bytes |
87 | 1221 | """Sign an RFC822 message and return the DKIM-Signature header line. | 1226 | """Sign an RFC822 message and return the DKIM-Signature header line. |
88 | 1222 | @param message: an RFC822 formatted message (with either \\n or \\r\\n line endings) | 1227 | @param message: an RFC822 formatted message (with either \\n or \\r\\n line endings) |
89 | @@ -1229,11 +1234,12 @@ def sign(message, selector, domain, privkey, identity=None, | |||
90 | 1229 | @param include_headers: a list of strings indicating which headers are to be signed (default all headers not listed as SHOULD NOT sign) | 1234 | @param include_headers: a list of strings indicating which headers are to be signed (default all headers not listed as SHOULD NOT sign) |
91 | 1230 | @param length: true if the l= tag should be included to indicate body length (default False) | 1235 | @param length: true if the l= tag should be included to indicate body length (default False) |
92 | 1231 | @param logger: a logger to which debug info will be written (default None) | 1236 | @param logger: a logger to which debug info will be written (default None) |
93 | 1237 | @param linesep: use this line seperator for folding the headers | ||
94 | 1232 | @return: DKIM-Signature header field terminated by \\r\\n | 1238 | @return: DKIM-Signature header field terminated by \\r\\n |
95 | 1233 | @raise DKIMException: when the message, include_headers, or key are badly formed. | 1239 | @raise DKIMException: when the message, include_headers, or key are badly formed. |
96 | 1234 | """ | 1240 | """ |
97 | 1235 | 1241 | ||
99 | 1236 | d = DKIM(message,logger=logger,signature_algorithm=signature_algorithm) | 1242 | d = DKIM(message,logger=logger,signature_algorithm=signature_algorithm,linesep=linesep) |
100 | 1237 | return d.sign(selector, domain, privkey, identity=identity, canonicalize=canonicalize, include_headers=include_headers, length=length) | 1243 | return d.sign(selector, domain, privkey, identity=identity, canonicalize=canonicalize, include_headers=include_headers, length=length) |
101 | 1238 | 1244 | ||
102 | 1239 | 1245 | ||
103 | @@ -1260,7 +1266,7 @@ dkim_verify = verify | |||
104 | 1260 | def arc_sign(message, selector, domain, privkey, | 1266 | def arc_sign(message, selector, domain, privkey, |
105 | 1261 | srv_id, signature_algorithm=b'rsa-sha256', | 1267 | srv_id, signature_algorithm=b'rsa-sha256', |
106 | 1262 | include_headers=None, timestamp=None, | 1268 | include_headers=None, timestamp=None, |
108 | 1263 | logger=None, standardize=False): | 1269 | logger=None, standardize=False, linesep=b'\r\n'): |
109 | 1264 | # type: (bytes, bytes, bytes, bytes, bytes, bytes, list, any, any, bool) -> list | 1270 | # type: (bytes, bytes, bytes, bytes, bytes, bytes, list, any, any, bool) -> list |
110 | 1265 | """Sign an RFC822 message and return the ARC set header lines for the next instance | 1271 | """Sign an RFC822 message and return the ARC set header lines for the next instance |
111 | 1266 | @param message: an RFC822 formatted message (with either \\n or \\r\\n line endings) | 1272 | @param message: an RFC822 formatted message (with either \\n or \\r\\n line endings) |
112 | @@ -1272,11 +1278,12 @@ def arc_sign(message, selector, domain, privkey, | |||
113 | 1272 | @param include_headers: a list of strings indicating which headers are to be signed (default all headers not listed as SHOULD NOT sign) | 1278 | @param include_headers: a list of strings indicating which headers are to be signed (default all headers not listed as SHOULD NOT sign) |
114 | 1273 | @param timestamp: the time in integer seconds when the message is sealed (default is int(time.time) based on platform, can be string or int) | 1279 | @param timestamp: the time in integer seconds when the message is sealed (default is int(time.time) based on platform, can be string or int) |
115 | 1274 | @param logger: a logger to which debug info will be written (default None) | 1280 | @param logger: a logger to which debug info will be written (default None) |
116 | 1281 | @param linesep: use this line seperator for folding the headers | ||
117 | 1275 | @return: A list containing the ARC set of header fields for the next instance | 1282 | @return: A list containing the ARC set of header fields for the next instance |
118 | 1276 | @raise DKIMException: when the message, include_headers, or key are badly formed. | 1283 | @raise DKIMException: when the message, include_headers, or key are badly formed. |
119 | 1277 | """ | 1284 | """ |
120 | 1278 | 1285 | ||
122 | 1279 | a = ARC(message,logger=logger,signature_algorithm=b'rsa-sha256') | 1286 | a = ARC(message,logger=logger,signature_algorithm=b'rsa-sha256',linesep=linesep) |
123 | 1280 | if not include_headers: | 1287 | if not include_headers: |
124 | 1281 | include_headers = a.default_sign_headers() | 1288 | include_headers = a.default_sign_headers() |
125 | 1282 | return a.sign(selector, domain, privkey, srv_id, include_headers=include_headers, | 1289 | return a.sign(selector, domain, privkey, srv_id, include_headers=include_headers, |
126 | diff --git a/dkim/arcsign.py b/dkim/arcsign.py | |||
127 | index 2d668b9..ff5a263 100644 | |||
128 | --- a/dkim/arcsign.py | |||
129 | +++ b/dkim/arcsign.py | |||
130 | @@ -59,7 +59,7 @@ def main(): | |||
131 | 59 | 59 | ||
132 | 60 | #try: | 60 | #try: |
133 | 61 | sig = dkim.arc_sign(message, selector, domain, open(privatekeyfile, "rb").read(), | 61 | sig = dkim.arc_sign(message, selector, domain, open(privatekeyfile, "rb").read(), |
135 | 62 | domain + ": none", cv) | 62 | domain + ": none", cv, dkim.util.get_linesep(message)) |
136 | 63 | for line in sig: | 63 | for line in sig: |
137 | 64 | sys.stdout.write(line) | 64 | sys.stdout.write(line) |
138 | 65 | sys.stdout.write(message) | 65 | sys.stdout.write(message) |
139 | diff --git a/dkim/dkimsign.py b/dkim/dkimsign.py | |||
140 | index 73f9de4..227b8eb 100644 | |||
141 | --- a/dkim/dkimsign.py | |||
142 | +++ b/dkim/dkimsign.py | |||
143 | @@ -70,8 +70,8 @@ def main(): | |||
144 | 70 | 70 | ||
145 | 71 | message = sys.stdin.read() | 71 | message = sys.stdin.read() |
146 | 72 | try: | 72 | try: |
149 | 73 | d = dkim.DKIM(message,logger=logger, | 73 | d = dkim.DKIM(message,logger=logger, signature_algorithm=args.signalg, |
150 | 74 | signature_algorithm=args.signalg) | 74 | linesep=dkim.util.get_linesep(message)) |
151 | 75 | sig = d.sign(args.selector, args.domain, open( | 75 | sig = d.sign(args.selector, args.domain, open( |
152 | 76 | args.privatekeyfile, "rb").read(), identity = args.identity, | 76 | args.privatekeyfile, "rb").read(), identity = args.identity, |
153 | 77 | canonicalize=canonicalize, include_headers=include_headers, | 77 | canonicalize=canonicalize, include_headers=include_headers, |
154 | diff --git a/dkim/tests/test_dkim.py b/dkim/tests/test_dkim.py | |||
155 | index fbe241b..a94937c 100644 | |||
156 | --- a/dkim/tests/test_dkim.py | |||
157 | +++ b/dkim/tests/test_dkim.py | |||
158 | @@ -46,6 +46,11 @@ class TestFold(unittest.TestCase): | |||
159 | 46 | self.assertEqual( | 46 | self.assertEqual( |
160 | 47 | b"foo" * 24 + b"\r\n foo", dkim.fold(b"foo" * 25)) | 47 | b"foo" * 24 + b"\r\n foo", dkim.fold(b"foo" * 25)) |
161 | 48 | 48 | ||
162 | 49 | def test_linesep(self): | ||
163 | 50 | self.assertEqual( | ||
164 | 51 | b"foo" * 24 + b"\n foo", dkim.fold(b"foo" * 25, linesep=b"\n")) | ||
165 | 52 | |||
166 | 53 | |||
167 | 49 | 54 | ||
168 | 50 | class TestSignAndVerify(unittest.TestCase): | 55 | class TestSignAndVerify(unittest.TestCase): |
169 | 51 | """End-to-end signature and verification tests.""" | 56 | """End-to-end signature and verification tests.""" |
170 | @@ -203,6 +208,17 @@ p=11qYAYKxCrfVS/7TyWQHOg7hcvPapiMlrwIaaPcHURo=""" | |||
171 | 203 | res = d.verify(dnsfunc=self.dnsfunc5) | 208 | res = d.verify(dnsfunc=self.dnsfunc5) |
172 | 204 | self.assertTrue(res) | 209 | self.assertTrue(res) |
173 | 205 | 210 | ||
174 | 211 | def test_verifies_lflinesep(self): | ||
175 | 212 | # A message verifies after being signed. | ||
176 | 213 | for header_algo in (b"simple", b"relaxed"): | ||
177 | 214 | for body_algo in (b"simple", b"relaxed"): | ||
178 | 215 | sig = dkim.sign( | ||
179 | 216 | self.message, b"test", b"example.com", self.key, | ||
180 | 217 | canonicalize=(header_algo, body_algo), linesep=b"\n") | ||
181 | 218 | res = dkim.verify(sig + self.message, dnsfunc=self.dnsfunc) | ||
182 | 219 | self.assertFalse(b'\r\n' in sig) | ||
183 | 220 | self.assertTrue(res) | ||
184 | 221 | |||
185 | 206 | def test_implicit_k(self): | 222 | def test_implicit_k(self): |
186 | 207 | # A message verifies after being signed when k= tag is not provided. | 223 | # A message verifies after being signed when k= tag is not provided. |
187 | 208 | for header_algo in (b"simple", b"relaxed"): | 224 | for header_algo in (b"simple", b"relaxed"): |
188 | diff --git a/dkim/tests/test_util.py b/dkim/tests/test_util.py | |||
189 | index e01b3b5..bf39566 100644 | |||
190 | --- a/dkim/tests/test_util.py | |||
191 | +++ b/dkim/tests/test_util.py | |||
192 | @@ -22,6 +22,7 @@ from dkim.util import ( | |||
193 | 22 | DuplicateTag, | 22 | DuplicateTag, |
194 | 23 | InvalidTagSpec, | 23 | InvalidTagSpec, |
195 | 24 | parse_tag_value, | 24 | parse_tag_value, |
196 | 25 | get_linesep, | ||
197 | 25 | ) | 26 | ) |
198 | 26 | 27 | ||
199 | 27 | 28 | ||
200 | @@ -75,6 +76,30 @@ class TestParseTagValue(unittest.TestCase): | |||
201 | 75 | self.assertEqual(len(sig),11) | 76 | self.assertEqual(len(sig),11) |
202 | 76 | 77 | ||
203 | 77 | 78 | ||
204 | 79 | class TestGetLineSep(unittest.TestCase): | ||
205 | 80 | """Line seperator probing tests.""" | ||
206 | 81 | |||
207 | 82 | def test_default(self): | ||
208 | 83 | self.assertEqual( | ||
209 | 84 | b'\r\n', | ||
210 | 85 | get_linesep(b'abc')) | ||
211 | 86 | |||
212 | 87 | def test_withcrlf(self): | ||
213 | 88 | self.assertEqual( | ||
214 | 89 | b'\r\n', | ||
215 | 90 | get_linesep(b'abc\r\n')) | ||
216 | 91 | |||
217 | 92 | def test_withlf(self): | ||
218 | 93 | self.assertEqual( | ||
219 | 94 | b'\n', | ||
220 | 95 | get_linesep(b'abc\n')) | ||
221 | 96 | |||
222 | 97 | def test_toosmall(self): | ||
223 | 98 | self.assertEqual( | ||
224 | 99 | b'\r\n', | ||
225 | 100 | get_linesep(b'a')) | ||
226 | 101 | |||
227 | 102 | |||
228 | 78 | def test_suite(): | 103 | def test_suite(): |
229 | 79 | from unittest import TestLoader | 104 | from unittest import TestLoader |
230 | 80 | return TestLoader().loadTestsFromName(__name__) | 105 | return TestLoader().loadTestsFromName(__name__) |
231 | diff --git a/dkim/util.py b/dkim/util.py | |||
232 | index 3d1f722..97b59c9 100644 | |||
233 | --- a/dkim/util.py | |||
234 | +++ b/dkim/util.py | |||
235 | @@ -33,6 +33,7 @@ __all__ = [ | |||
236 | 33 | 'InvalidTagSpec', | 33 | 'InvalidTagSpec', |
237 | 34 | 'InvalidTagValueList', | 34 | 'InvalidTagValueList', |
238 | 35 | 'parse_tag_value', | 35 | 'parse_tag_value', |
239 | 36 | 'get_linesep', | ||
240 | 36 | ] | 37 | ] |
241 | 37 | 38 | ||
242 | 38 | 39 | ||
243 | @@ -80,3 +81,8 @@ def get_default_logger(): | |||
244 | 80 | if not logger.handlers: | 81 | if not logger.handlers: |
245 | 81 | logger.addHandler(NullHandler()) | 82 | logger.addHandler(NullHandler()) |
246 | 82 | return logger | 83 | return logger |
247 | 84 | |||
248 | 85 | def get_linesep(msg): | ||
249 | 86 | if msg[-2:] != b'\r\n' and msg[-1:] == b'\n': | ||
250 | 87 | return b'\n' | ||
251 | 88 | return b'\r\n' |
On Mon, Dec 17, 2018 at 04:04:13AM -0000, Scott Kitterman wrote: object) , it should be added to the @param list (I test_dkim. py, so we know this
> I'm fine with the concept, but I think it needs more work.
>
> As far as the change in dkimsign.py, I'd rather see '\r\n' the default of the
> check. As an example, a single long line with no newline at the end should be
> folded the same as before the change.
>
> Arcsign.py will need the same changes.
>
> For the internal dkim/__init__.py changes, I'm fine with the API extension,
> but for class DomainSigner(
> know there are others missing too, but let's not make it worse).
>
> Finally, it needs a test case in dkim/tests/
> works. It needs to pass on at least python2.7, python3.6, and python3.7.
> Testing python3.3 - 3.5 would be nice.
Will do!