Repository navigation
0.4.0: implement signing, namespace the library, fix verification bugs - #15
Merged
Merged
Conversation
Breaking release. Classes move to PSR-4 under angrychimp\DKIM\ (Sign, Verify,
Exception); the old global names remain as Composer-loaded aliases.
Signing is implemented, open since 2012. It reuses the canonicalization the
verifier already had, and supports relaxed/simple, sha1/sha256, configurable
signed-header sets, and RFC 6376 5.4.2 bottom-to-top ordering for repeated
headers.
Two security fixes in verification:
* openssl_verify() returns -1 on internal error, which is truthy, so an
errored verification was reported as a pass. It now requires an explicit
success.
* a= was never validated, so a signature naming any algorithm (e.g.
ed25519-sha256) was still verified as RSA. Restricted to rsa-sha1 and
rsa-sha256, checked before the DNS lookup so a bogus a= costs no traffic.
Correctness fixes, each of which silently produced or rejected signatures no
other implementation agreed with:
* header lookup matched on a bare prefix, so Message-ID also matched
Message-ID-Hash (added by Mailman 3)
* relaxed canonicalization of an empty body emitted CRLF instead of a null
input, affecting verification as well as signing
* h= was deduplicated, so a message repeating a header could never verify
* x= expiration was never enforced, and reading t= unguarded errored when
x= appeared alone
* c= now defaults to simple/simple and tolerates a lone algorithm
Hardened parsing against malformed input: tags and key records with no "=" or
a trailing ";", q= with no "/", headers with no colon, failed DNS lookups, and
messages with no blank line, whose body was read from the middle of a header.
Adds three test suites and GitHub Actions running them on PHP 7.4-8.4. Because
the signer and verifier share canonicalization code, agreeing with each other
proves little -- two of the bugs above survived a green round-trip run -- so CI
also cross-checks against dkimpy in both directions.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
actions/checkout v4 and actions/setup-python v5 target Node 20, which the runners now force onto Node 24. v7 of each targets Node 24 natively. shivammathur/setup-php stays on v2 -- that is still its current major. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Breaking release. Classes move to PSR-4 under
angrychimp\DKIM\; signing is implemented; several verification bugs are fixed.The old global names (
DKIM_Sign,DKIM_Verify,DKIM_Exception) still work as Composer-loaded aliases, so existing code keeps running.Signing
Open since 2012. Reuses the canonicalization the verifier already had, so
Signis small. Supports relaxed/simple, sha1/sha256, configurable signed-header sets, and RFC 6376 5.4.2 bottom-to-top ordering for repeated headers.Security fixes
openssl_verify()returns-1on internal error, which is truthy.if (!$vResult)therefore reported an errored verification as a pass. Now requires an explicit success.a=was never validated. A signature naming any algorithm (e.g.ed25519-sha256) was still verified as RSA. Restricted torsa-sha1/rsa-sha256, and checked before the DNS lookup so a bogusa=costs no network traffic.Correctness fixes
Each of these silently produced, or rejected, signatures no other implementation agreed with:
Message-IDalso matchedMessage-ID-Hash— the header Mailman 3 adds to every list message.CRLFinstead of a null input. In the shared parent class, so it broke verification too.h=was deduplicated, so a message repeating a header could never verify.x=expiration was never enforced, and readingt=unguarded errored whenx=appeared alone.c=now defaults tosimple/simpleand tolerates a lone algorithm.Plus hardening against malformed input: tags and key records with no
=or a trailing;(how real DKIM TXT records are written),q=with no/, headers with no colon, failed DNS lookups, and messages with no blank line — whose body was previously read from the middle of a header.Testing
Three suites (
tests/expiry.php,tests/sign.php,tests/verify.php), no framework, no fixtures, no DNS — keypairs are generated per run.CI runs them on PHP 7.4–8.4, and cross-checks against dkimpy in both directions. That second job matters: the signer and verifier share
_canonicalizeBody(), so this library agreeing with itself proves little. Two of the bugs above survived a fully green round-trip run and were only caught by an independent implementation. The empty-body andh=tests are therefore asserted against the RFC directly rather than round-tripped.Every fix in this PR was mutation-tested — reverting it must fail the suite.
Not included
l=(deliberately — it lets an attacker append content), ed25519 (RFC 8463), and the phpseclib 1.x code path, which is unreachable with phpseclib 2.x+, untested, and passes a bare base64 key toloadKey(). Tracked in TODO.md; I'd suggest deleting it.🤖 Generated with Claude Code