search

albertito / chasquid

SMTP (email) server with a focus on simplicity, security, and ease of operation [mirror]
https://blitiri.com.ar/p/chasquid/
Other
868 stars 56 forks source link

Use dkimpy for generating DKIM signatures #19

Closed ghost closed 3 years ago

ghost commented 3 years ago

Compared to driusan/dkim, dkimpy is actively maintained and available on many distros (Ubuntu, Fedora, Debian, etc).

This PR would make enabling DKIM an easier procedure, without cloning driusan/dkim or compile & install.

albertito commented 3 years ago

Thanks for sending this pull request! And sorry for the delay in responding.

I think that in principle, supporting dkimpy by default is a good idea, although I rather add it as an option instead of replacing driusan/dkim entirely.

I'll go over the patches in more detail in the next few days and have more concrete feedback/suggestions then.

Thanks again!

albertito commented 3 years ago

I've been trying to get this to work, but it's not going well :(

I can make something functional, but it's somewhat hacky and (most importantly) untestable. You can find what I have in the add-dkimpy-support branch.

The problems are:

  1. I have to use the --help output to tell the tools apart. This is quite hacky, but I am willing to live with it.
  2. dkimpy's dkimsign can't be made to output new headers only. So I need to get the full output and then pass it through diff | grep | sed to extract new headers. Same as above, this is hacky but I guess it can be done.
  3. dkimpy's dkimverify can't be told to use a specific dns server, or to take specific DNS values for dkim lookups. This prevents it from being used in integration tests.

Problems 2 and 3 are specifically about dkimpy, not the dual support, and are an issue even if we go dkimpy-only.

Problem 3 is the main blocker, since I don't want to support something that can't be reasonably tested.

One option would be to use driusan/dkim tools for testing dkimpy integration. It would allow us to still support dkimpy for signing, and have tests for it. But it isn't great to have that level of cross-dependencies, and again overall it's not particularly clean.

I'll keep this open and keep trying to find a reasonable way to make it work for a bit longer, but so far it's looking more brittle that I would like.

Sorry about this, please let me know if you have any thoughts or suggestions!

ghost commented 3 years ago

Thanks for the comprehensive testing!

I was also using diff with

diff --changed-group-format='%>' --unchanged-group-format='' file1 file2

(from StackOverflow) before switching to comm.

Personally I'm not too concerned with dkimverify, as an email should not be rejected based on DKIM failure alone.

albertito commented 3 years ago

Commit 643d5ce implements dkimpy support. It's currently in next, I'll let it sit there for a bit to give it more exposure in case we need to make some adjustment.

If you try the default hook from that patch, please let us know how it goes!

I'll close this once I merge it into master in a few days, or comment further if something comes up.

Thanks again!

albertito commented 3 years ago

The patch adding dkimpy support is now in the master branch and was included in the new v1.8 release.

Thank you!