Investigating validation failure

[Synchro]

As your readme says, this code is failing to validate known-valid messages, so I'm looking into why that is. As a reference, I'm comparing with the output of the port25 validator, which validates the same messages successfully, and conveniently provides a canonicalised body to compare with.

The first thing I've found is that when message and body are separated, the body loses its first line, which in most cases will be the first MIME boundary string. I suspect an off-by-one error in _getBodyFromRaw().

A second problem: In validate() is this foreach:

foreach ($this->_publicKeys[$dkim['d']] as $num => $publicKey) {

Unfortunately $num is also used in the outer loop that iterates over the signatures:

foreach ($signatures as $num => $signature) {

and $num is used as an array index inside the second loop, so it's not going to be end well.

I'm using the teonsystems fork while investigating this, but the same things apply here too. (pinging @bostjan)

[angrychimp]

Thanks for the comments. I'll look into the off-by-one issue and see if I can make that work. In the meantime, I've corrected the indexing issue you've correctly identified. I appreciate the help!

[Synchro]

Thanks. It might help to know that apart from the missing first line, the canonicalised body comes out exactly the same as the port25 tester (minus port25's weird control char representation!), so that's good news!

I found another very small bug. It scans through the keys found in DNS records assuming that everything contains DKIM keys (which it should), but due to a local config issue, mine served up an SPF record instead, so it it failed to find a p element, and throws an undefined index error. Admittedly this was a problem of my own making, but if a record isn't a DKIM record it should be ignored (i.e. continue the loop), or the error should be trapped.

[angrychimp]

Great point. I've added a check for the presence of a (non-empty) public "p=" value.

[angrychimp]

Ok, I've pushed that line back into the body for hash validation. We'll see what kind of impact that has.

[Synchro]

Great, thanks. @bostjan, could you merge these changes into your fork?

[angrychimp]

After reviewing the changes in the @bostjan fork, they look acceptable. If I get a pull request I'll merge everything back into the master here. (I'd prefer that, rather than just manually moving things over and forcing him to merge.)

[bostjan]

Hello angrychimp (I like your handle:),

can I just say "just merge it" or do I have to create pull request? If so, do you need me to rebase it, or would just merging your master into mine before issuing a pull request be ok for you?

b.

[Synchro]

@bostjan, I already made a PR from your fork (#8) which @angrychimp is getting on with merging. You obviously made quite a lot of changes so it's not going to be a simple merge whichever way around it goes!

[bostjan]

I understand, but those changes were needed. I would much prefer to just "composer require" it :)

[Synchro]

Nothing stopping you - you can just merge the changes back into your fork, and you'll find that future upstream changes are easier to integrate because you have a common code base.

[bostjan]

Ah, work, it just never really goes away, does it? :)

Will merge, but if your code will work, I will switch back to it quite fast.

b.

[turboflash]

With regards to the off-by-one error in _getBodyFromRaw(), I encountered "Computed body hash does not match signature body hash" issue when I use the code to validate original message downloaded from Gmail. I have to comment out array_unshift($lines, $line); in order for it to work properly.

Also in RFC6376 Section 3.4.5, the empty line (which separates header and body) is not part of the body.