Improve folding signature algorithm

[ludusrusso]

Following #27, I've rewritten the folding algorithm in order to better fold headers and avoid introducing new lines inside headers.

However, it seems it breaks the skim sign.

In my understanding, this should have been call after the signature was generated, but it seems that the new algorithm changes the generates signature itself.

Any help?

[ludusrusso]

Example output:

        DKIM-Signature: a=ed25519-sha256;
         bh=2jUSOH9NhtVGCQWNr9BrIAPreKQjO6Sn7XIkfJVOzv8=; c=simple/simple;
         d=football.example.com; h=From:To:Subject:Date:Message-ID; s=brisbane;
         t=424242; v=1;
         b=CtvfpO96cCh7+PJLDna/bOJ1WdQ2qt7Nfrq6FzJqjYQv+TjOUc+rWk81TQJnmvdZsCDbmpr2
         hj7RDH4JDGa0CA==

[foxcpp]

In my understanding, this should have been call after the signature was generated, but it seems that the new algorithm changes the generates signature itself.

The likely issue is addition of non-empty b= that changes how folding behaves. I tried to integrate go-message/textproto algorithm into go-msgauth and hit the exactly same issue. You can observe it by adding fmt.Println for sigField here: https://github.com/emersion/go-msgauth/blob/e4c87369d72f4a8d834a872ff0c43258936fd32d/dkim/sign.go#L261 and for formatSignature(s.sigParams) here: https://github.com/emersion/go-msgauth/blob/e4c87369d72f4a8d834a872ff0c43258936fd32d/dkim/sign.go#L276

It is important to make sure both values are equal (with the exception of b= vs b=BASE64).

[foxcpp]

In your case, it shows:

DKIM-Signature: a=rsa-sha256;
 bh=2jUSOH9NhtVGCQWNr9BrIAPreKQjO6Sn7XIkfJVOzv8=; c=simple/simple;
 d=example.org; h=From:To:Subject:Date:Message-ID; s=brisbane; t=424242;
 v=1;

DKIM-Signature: a=rsa-sha256;
 bh=2jUSOH9NhtVGCQWNr9BrIAPreKQjO6Sn7XIkfJVOzv8=; c=simple/simple;
 d=example.org; h=From:To:Subject:Date:Message-ID; s=brisbane; t=424242;
 v=1;
 b=KXuOIdcHesiuc+CNVJC0ooTO9LvodnM01dstXNTA7elICw7r9LKD5zrPnGwtP5Ye+sOndhJO
 23pJIY3LP5oXzCnV3nkZiEwIb0rKIgYxgxx8JfCWbZgurhN35rQjnwCtmcGvN1aJ7ba1x1x63Vf
 xp0zwssLR2w8R+PkR4r1ROlM=

Uh, missing b=?

[codecov-commenter]

Codecov Report

Merging #29 into master will increase coverage by 0.31%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #29      +/-   ##
==========================================
+ Coverage   66.30%   66.62%   +0.31%     
==========================================
  Files           7        7              
  Lines         837      845       +8     
==========================================
+ Hits          555      563       +8     
  Misses        213      213              
  Partials       69       69              

Impacted Files	Coverage Δ
dkim/header.go	93.68% <100.00%> (+0.58%)	:arrow_up:
dkim/sign.go	68.58% <100.00%> (ø)

---

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 e4c8736...a231a2f. Read the comment docs.

[ludusrusso]

Additionally, I would appreciate if you add more test cases to ensure it generates valid signatures in edge cases related to folding.

Yea! I'll add some additional test cases! To be honest, I did not added more test before because they are covered by TestSign

[ludusrusso]

Now, if we have some part of the header longher than 75cahrs, the new algorithm will break the line length limit. This should not be an issues since this limit is only a recommendation! What do you think? https://github.com/emersion/go-msgauth/issues/18#issuecomment-597186179

Anyway this should fix #18

[foxcpp]

I agree that 75 chars limitation can be ignored. Following rationale in RFC, it is not really relevant today as a rare person looks at message header.

[ludusrusso]

@foxcpp thanks! I've also added a more test that checks only if the headers names in the "h" values are properly folded in the generated signature!

[foxcpp]

FWIW, That's the h= value my mail server used for a typical plaintext message:

Subject:Subject:Sender:To:To:Cc:From:From:Date:Date:MIME-Version:Content-Type:Content-Type:Content-Transfer-Encoding:Reply-To:In-Reply-To:Message-Id:Message-Id:References:Autocrypt:Openpgp

Interesting is that old folding algorithm broke it in the middle of a field name yet OpenDKIM verified the signature successfully: https://paste.dn42.us/paste/#/8T1GgumCllH0ZGDbwvOI8dvNYA8!TqXFCKZWbnYkBUP4_rBv1Fd3e-OVScQBZDav2mXSMw4

[foxcpp]

@emersion, what do you think about this PR?

[ludusrusso]

FWIW, That's the h= value my mail server used for a typical plaintext message:

Subject:Subject:Sender:To:To:Cc:From:From:Date:Date:MIME-Version:Content-Type:Content-Type:Content-Transfer-Encoding:Reply-To:In-Reply-To:Message-Id:Message-Id:References:Autocrypt:Openpgp

Interesting is that old folding algorithm broke it in the middle of a field name yet OpenDKIM verified the signature successfully: https://paste.dn42.us/paste/#/8T1GgumCllH0ZGDbwvOI8dvNYA8!TqXFCKZWbnYkBUP4_rBv1Fd3e-OVScQBZDav2mXSMw4

I also notice this! I've tried with different mail provider receivers and in some case (like hotmail/outlook) the dkim fails while other (like gmail) it passes.

[ludusrusso]

Hi @emersion, thanks for the feedback! I was wondering: if we do not want to limit the line lenght to 75 chars, we could make the folding algorithm simpler by just putting the "b" value at the end of the header without folding it! What do you think about this?

[emersion]

Would the b= value ever be longer than 1000 chars? (I guess that's an issue with other header fields too, like h=)

[ludusrusso]

Would the b= value ever be longer than 1000 chars? (I guess that's an issue with other header fields too, like h=)

Not super sure about b, but since it's an hash it should be fixed in length, I'm correct?

Regarding h, you're right, it could be more then 1000 chars if you include a lot of headers! I've to think about a way to fold it :D

[ludusrusso]

@emersion what do you think about b folding?

[foxcpp]

b= is a signature value. I do not expect it to hit 998 characters limitation even with RSA-4096 (though one might consider it to be dangerously close, ~682 characters in base64)