Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix DKIM's error when there is S/MIME-sign #2340

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

anhtunguyen
Copy link

@anhtunguyen anhtunguyen commented May 17, 2021

In $parts[0] there is MIME-Version and Content-Type delimited by "\n".
Need to separate them, otherwise error with DKIM: "dkim=neutral (body hash did not verify)" (The reason is in "h=Date:To:From:Reply-To:Subject:Message-ID:X-Mailer;" without MIME-Version and Content-Type)

Test environment : PHP 8.0.6, PHPmail, isSMTP, DKIM-sign, S/MIME

@Synchro
Copy link
Member

Synchro commented May 17, 2021

Thanks for this – I don't think anyone has actually used this combination before, so I'm happy to receive fixes like this!
Do you know for sure that openssl always uses \n line breaks on Windows, or should your substitution normalise line breaks first?

@anhtunguyen
Copy link
Author

@Synchro Openssl uses \n\n to separate headers and content. This is what everyone knows. I'm not sure if openssl uses the unique \n to split the lines of headers. Maybe \r\n too? But \n is present in all cases. That's why I split the lines with \n, then use the function TRIM for those lines. This way, if \r is present, it will be removed as well.

@Synchro
Copy link
Member

Synchro commented May 17, 2021

Thanks for clarifying. Do you have tests to cover this?

@anhtunguyen
Copy link
Author

@Synchro Yes, I've tested it many times already. And this solves the problem of incorrectly displaying the message with this configuration: PHP 8.x, PHPmail, S/MIME without DKIM (the same reason: MIME-Version and Content-Type not found)

@Synchro
Copy link
Member

Synchro commented May 17, 2021

No, I mean could you add tests to the PHPMailer test suite to ensure it works and prevent regression in future.

@anhtunguyen
Copy link
Author

anhtunguyen commented May 17, 2021

@Synchro Same test (environment: Linux/windows, php 8.x, PHP mail, S/MIME) :
https://github.com/PHPMailer/PHPMailer/blob/master/examples/smime_signed_mail.phps

@Synchro
Copy link
Member

Synchro commented May 17, 2021

I mean the automated unit tests in here. What you pointed at is example code which has to be adapted and run manually. Ideally there would be something similar that generates a message, but then verifies it as part of the test suite, which is run and checked every time a commit it made to the repo. Unfortunately there is no PHP-based test suite for DKIM (though I'm working on it), but S/MIME should be easier to write a test for – generate a signed message, then check if its signature matches the key.

$subparts = array_map('trim', explode("\n", $parts[0]));
foreach($subparts as $subpart) {
$this->MIMEHeader .= $subpart . static::$LE;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, this is great here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is already a function to normalise breaks in static::normalizeBreaks() which would make for a cleaner implementation here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants