albertito / chasquid

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

make dkim-py example compatible with Alpine #54

Closed xrstf closed 7 months ago

xrstf commented 8 months ago

Busybox's diff doesn't have the extended CLI flags, but it seems that comm works fine enough with dkim-py.

I originally tried the Go versions of the DKIM tools, but they consistently produced invalid signatures. For testing I am sending myself a local email via

echo -e "Subject: this is a test email\nTo: me@mydomain.com\n\n hello there username." | msmtp -v me@mydomain.com

When using dkim-go, the signature is only accepted by dkim-go, dkim-py rejects it with

dkim.ValidationError: body hash mismatch (got b'nC3tBYkxSxy2Nw1wostkVwsDSyxn8WTIj5Uh9aI3U/c=', expected b'47DEQpj8HBSa+/TImW+5JCeuQeRkm5NMpJWZG3hSuFU=')

If I however use dkim-py, the signature is in turn only accepted by dkim-py. dkim-go rejects it likewise with

: Permanent failure: body hash does not match

Since mail-tester.com accepts dkim-py and rejects dkim-go, I for now went with the Python version and so needed Alpine compatibility :grin:

(My test email might be broken; I don't know, but with the Python tooling things just work :shrug: )

albertito commented 8 months ago

Thanks for sending this!

Busybox's diff doesn't have the extended CLI flags, but it seems that comm works fine enough with dkim-py.

The problem with changing the diff call to comm is that coreutil's comm relies on the input files being sorted, and fails if they are not.

So changing diff for comm breaks the example hook for coreutil's environments :(

I can't think of a reasonable workaround (other than reimplementing the diff in the hook script, which is not very reasonable :).

I think it would be ideal if dkimsign.py would have an option to just print the DKIM header. That way, we wouldn't need to do this comparison.

Looking at the code, that's already the way it works: it computes and prints the DKIM header, and then prints the original message, so maybe it wouldn't be that crazy to propose a change?

What do you think?

I originally tried the Go versions of the DKIM tools, but they consistently produced invalid signatures. For testing I am sending myself a local email via [...] Since mail-tester.com accepts dkim-py and rejects dkim-go, I for now went with the Python version and so needed Alpine compatibility 😁

(My test email might be broken; I don't know, but with the Python tooling things just work 🤷 )

For what is worth, I totally sympathize with this. Getting DKIM to work should not be this hard :(

xrstf commented 8 months ago

Just in general, how do you stand regarding integrating DKIM support into chasquid itself? It's not like a spam or anti virus check, which I would consider optional bonus features: DKIM however is basically mandatory for most email setup anyway.

xrstf commented 8 months ago

The problem with changing the diff call to comm is that coreutil's comm relies on the input files being sorted, and fails if they are not.

So changing diff for comm breaks the example hook for coreutil's environments :(

How do you mean "coreutil's environments"? I am using comm for my post-data script and it works just fine for me. Maybe I'm currently relying on an implementation detail in dkim-py, but for now it definitely works.

albertito commented 8 months ago

The problem with changing the diff call to comm is that coreutil's comm relies on the input files being sorted, and fails if they are not. So changing diff for comm breaks the example hook for coreutil's environments :(

How do you mean "coreutil's environments"? I am using comm for my post-data script and it works just fine for me. Maybe I'm currently relying on an implementation detail in dkim-py, but for now it definitely works.

Oh, sorry I wasn't clear!

You're relying in an implementation detail in comm.

printf '4\n3\n2\n1\n' > file1
printf '4\n3\nz\nx\n2\n1\n' > file2
comm -3 file1 file2
echo $?

On Alpine, that works, because /bin/comm comes from Busybox.

On Debian (for example), that does not work, because /bin/comm comes from Coreutils.

So your patch fixes the problem for Alpine, but breaks Debian/Ubuntu/Fedora/etc. :(

I'd love to find a way to make it work out of the box on both, but the only way I can think of is to re-implement the diffing logic in bash, and that is just too convoluted for something meant as an example.

But if dkimpy could output just the new headers, then the diff/comm would not be needed at all.

Just in general, how do you stand regarding integrating DKIM support into chasquid itself? It's not like a spam or anti virus check, which I would consider optional bonus features: DKIM however is basically mandatory for most email setup anyway.

I started writing DKIM integration some time ago, but haven't worked on it in a while. It's probably something I'll pick up again soon, because, as you point out, it is becoming more and more common, and integration via the hook is unfortunately fairly brittle.

I hope to have it ready one day but it wouldn't be wise to make any promises :)

xrstf commented 8 months ago

On Debian (for example), that does not work, because /bin/comm comes from Coreutils.

Oh, good to know. I just checked that comm existed in both Alpine and Debian and though it would be good enough.

I started writing DKIM integration some time ago, but haven't worked on it in a while. It's probably something I'll pick up again soon, because, as you point out, it is becoming more and more common, and integration via the hook is unfortunately fairly brittle.

Would you accept outside contributions for the DKIM support?

albertito commented 7 months ago

I started writing DKIM integration some time ago, but haven't worked on it in a while. It's probably something I'll pick up again soon, because, as you point out, it is becoming more and more common, and integration via the hook is unfortunately fairly brittle.

Would you accept outside contributions for the DKIM support?

Yes, but with something as big as DKIM, it is probably better to let me do the initial skeleton first, since I know I can be quite picky, and it would be quite a large change.

That said, if you are interested in helping with it, that would be great, maybe we can chat on irc if you want to discuss more details? It's probably more practical to have an interactive conversation about it at this point.

BTW, sorry for the delay, I didn't realize I had left this unanswered :S

Thank you!

albertito commented 7 months ago

I've just uploaded the first iteration of internal DKIM support to the next branch.

It's commit 404fc07040dc7e7db1969b7d3707daaafe90007a (but expect the id to change as I rebase things over time).

It'll probably take a while of polishing it before I merge in master, but I've been using it for a few days, and it passes all the checks I could find so far.

Hopefully the internal DKIM implementation will be good enough soon, and that we won't need to do any more tricks with diff.

Thanks again for sending this, even though it wasn't merged, I really appreciate it and I hope the new DKIM implementation makes your life easier too :)

xrstf commented 7 months ago

I haven't forgotten you, adding DKIM is still on my TODO list, but atm I have other projects to hack on 😁