emersion / go-msgauth

🔏 A Go library and tools for DKIM, DMARC and Authentication-Results
MIT License
162 stars 51 forks source link

Add option to allow SHA1 verification for back compatible #33

Closed huicao closed 4 years ago

codecov-commenter commented 4 years ago

Codecov Report

Merging #33 into master will increase coverage by 1.06%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #33      +/-   ##
==========================================
+ Coverage   66.62%   67.68%   +1.06%     
==========================================
  Files           7        7              
  Lines         845      848       +3     
==========================================
+ Hits          563      574      +11     
+ Misses        213      206       -7     
+ Partials       69       68       -1     
Impacted Files Coverage Δ
dkim/verify.go 65.19% <100.00%> (+4.03%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update fb07ec9...c5eb55c. Read the comment docs.

emersion commented 4 years ago

Please rebase against our master branch, to only keep the last commit.

https://git-rebase.io/

huicao commented 4 years ago

rebased, thanks!

emersion commented 4 years ago

Thanks for rebasing.

What's the use-case for this patch? The RFC says it "MUST NOT" be used, so I'm not sure it's a good idea to add an option to use it.

DKIM signatures aren't designed to be verified a long time after the email is received. Instead, the receiving mail server is supposed to check the signatures and add an Authentication-Results header.

huicao commented 4 years ago

The traffic received by our email server still has lot of SHA1 used. Google or others still validate them. I understand those servers should not send this, but this is the way how DKIM is adopted.

emersion commented 4 years ago

Hmm. Actually, scratch that. The RFC states: "rsa-sha1 MUST NOT be used for signing or verifying."

I would be fine to add this as an option if the RFC said sha1 MUST NOT be used for signing only. But the RFC explicitly says it MUST NOT be used for verifying too.

I'm afraid I can't accept this PR, sorry. I'd rather make DKIM signature verification fail in this case. Feel free to keep this patch in a fork.

emersion commented 4 years ago

Note, the real solution is to contact the email providers still using sha1 and ask them to upgrade.

huicao commented 4 years ago

I don't think SHA1 will disappear any time soon, so many APIs still uses SHA1 or MD5 to query. I don't have the time or resource to tell all those providers to fix them. Actually, having SHA1 DKIM is better than no DKIM at all.

foxcpp commented 4 years ago

DKIM is used as an input for policy decisions, lack of SHA1 support incentivizes adoption of better algorithms. If everybody continues to support SHA1 - it will indeed not "disappear any time soon".