add dkim client support

[gpatel-fr]

Related to closed issue #21

[icgood]

I reopened the issue.

fix #21

[icgood]

Looks good overall. Thanks for doing this! I had a few changes for you then it should be good to merge.

[icgood]

I'd like to keep this dependency as optional. A few things:

Please move this to an extras_require parameter:

extras_require={'dkim': ['dkimpy >= 0.9.0']},

And then move your new policy class to its own module (maybe slimta.policy.dkimheader). The test can stay put, just add dkimpy >= 0.9.0 as a new line in test/requirements.txt.

[icgood]

Please rename this to AddDKIMHeader.

[icgood]

Once you separate this class into its own file, this import can go up to the top.

[icgood]

This should be:

envelope.headers['DKIM-Signature'] = data

Prepending is typically only desired for the Received header.

[icgood]

Can you add a few assertIn checks for some of the expected tag=value pairs in the header?

[gpatel-fr]

2 points

• I am baffled by the separate file for dkim header; it's a header after all. Well if you used to collect data about animals and were storing the info in a drawer labelled 'animals' with sections for dogs, cats, horses and I wanted to add a section about guinea pigs, I'd expect to add a new section for guinea pigs in the drawer animals, not create a new drawer especially for guinea pigs. I'd say well, right, it's your filing system but sure I wonder greatly why you want it this way :-). IMO looking at the policy directory it would look strange having a 'headers.py' and a 'headerDKIM.py' or something like that.

• about the prepending of the DKIM header, it was my belief that it was the standard way of doing it:
  https://www.ietf.org/rfc/rfc6376.txt
  see 5.6, how do you understand it ?

[icgood]

I am baffled by the separate file for dkim header; it's a header after all. Well if you used to collect data about animals and were storing the info in a drawer labelled 'animals' with sections for dogs, cats, horses and I wanted to add a section about guinea pigs, I'd expect to add a new section for guinea pigs in the drawer animals, not create a new drawer especially for guinea pigs. I'd say well, right, it's your filing system but sure I wonder greatly why you want it this way :-). IMO looking at the policy directory it would look strange having a 'headers.py' and a 'headerDKIM.py' or something like that.

It has to do with keeping the dkimpy dependency optional. Your implementation accomplishes that by doing the import inside the apply method, but that's against PEP-8 and something I've tried to avoid thus far.

I do see your point, however, of organizing the files in a way that makes sense. How about this, move headers.py as-is to headers/__init__.py. Then, add your new policy in headers/dkim.py, with the dkimpy import at the top. Thoughts?

about the prepending of the DKIM header, it was my belief that it was the standard way of doing it:
https://www.ietf.org/rfc/rfc6376.txt
see 5.6, how do you understand it ?

Sorry about that, I hadn't looked at that yet, thanks for the link! You were correct to prepend.

[gpatel-fr]

About PEP-8 I'd say it is exceedingly onerous to try to conform to it everywhere, especially for import. I have looked a bit and autopep8 do not try to fix non module level import, pep8 itself

https://pep8.readthedocs.io/en/1.4.4/_modules/pep8.html

do not even conform stricly, and python-slimta either:

https://github.com/slimta/python-slimta/blob/master/slimta/queue/__init__.py

putting the headers.py code in init.py is just hiding away the inconsistency - I will refrain to pursue my drawer metaphor here -.

I think a better compromise is to just add after the headers import something along the lines of

try:
  import dkimpy
except ImportError:
  pass # noqua

I don't know if a 'pass' instruction is subjected to the honours of code quality control (I'll check it)

And by the way, the import of dkimpy in the Apply method was a remnant of my first implementation, that was indeed setting dkimpy as an optional dependency. I just forgot to move it to the file beginning.
It's a bit of an unwieldy case, it could be a separate product python-slimta-dkim except that headers are already included in the main product. And standard is a relative idea here: dkim is not managed by the standard Python library, but at the internet level (IETF) it is no less standard than the classical headers (received, from,...). Moving headers to a separate product python-slimta-headers would be meaningless, how to manage mail without headers. There is already a third party library in python-slimta (pycares), but although dns and dkim are internet standards dns is vastly more important than dkim. All considered I still think that the except at the beginning of headers.py conforms best to the spirit of PEP8 while not changing what is IMO a clear and rational organization that don't need fixing.

[icgood]

I'd be happy with this pattern that I've seen often, e.g. in the Python codebase:

try:
    import dkim
except ImportError:
    dkim = None

# inside AddDKIMHeader.__init__()
if dkim is None:
    raise ImportError('dkimpy is not installed')

Raising the exception in the policy constructor should bring the error to attention during initialization rather than once an email goes through, so I'd place it there.

[gpatel-fr]

Thanks, now I just have to do the rest of the job :-)

[gpatel-fr]

err, hello again.
I wonder what I have done wrong with this out of date with the base branch. Oh I see. You did a change a few days ago and I forgot to check for this today. Is this a problem since it's a totally unrelated change ?
anyway, I added a very small change, a comment for the prepend header in headers.py. Other than that (I hope you will not object) I think I have done what I could, I have not found any way to keep 100% coverage for headers.py without breaking PEP-8 more in test_slimta_policy_headers.py.