From 00ee47c7fcc6e0a9f78474194d8028babbe972b3 Mon Sep 17 00:00:00 2001 From: Jean Helou Date: Wed, 11 Jun 2025 09:05:52 +0200 Subject: [PATCH] RFC - Proposal for an updated JDKIM API In the proposed changes, DKIMSigner API is made more functional, both signature template and private key become a parameter of the signing operation. Additionally the signature algorithm is also made a parameter This enables per signature template and private key variation and more importantly allows signing with multiple keys which is necessary to support migration to new signing algoritms as proposed in https://datatracker.ietf.org/doc/html/rfc8463#section-3 The other signature template fields would probably also benefit from having a structured API switching from string parsing to a record to make it easier for consumer to handle different signature requirements for different domains for example. It would also make the API safer to use by better documenting which fields are mandatory and which are optional with default values. The change to DKIMVerifier still needs work but it returns a record instead of a list the record encodes the standard pass criteria, it contains a list of signature verifications that record the exact state of verifying each record. The introduction of the SignatureVerification allows to uncouple the structre of a template and the structure of a verification that are currently welded together through the SignatureRecord interface despite having different mandatory fields or behaviors. This part may still need work as I am not very familiar with the verification constraints and may have missed operational concerns that are not in the specification. --- .../java/org/apache/james/jdkim/DKIMTest.java | 197 ++++++++++++++++-- 1 file changed, 185 insertions(+), 12 deletions(-) diff --git a/main/src/test/java/org/apache/james/jdkim/DKIMTest.java b/main/src/test/java/org/apache/james/jdkim/DKIMTest.java index 681406a..fae2f01 100644 --- a/main/src/test/java/org/apache/james/jdkim/DKIMTest.java +++ b/main/src/test/java/org/apache/james/jdkim/DKIMTest.java @@ -19,6 +19,7 @@ package org.apache.james.jdkim; +import static java.util.stream.Collectors.joining; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.Assert.assertEquals; @@ -34,6 +35,9 @@ import java.nio.charset.StandardCharsets; import java.util.Arrays; import java.util.List; +import java.util.Objects; +import java.util.Optional; +import java.util.stream.Stream; import org.apache.commons.codec.binary.Base64; import org.apache.james.jdkim.MockPublicKeyRecordRetriever.Record; @@ -41,6 +45,7 @@ import org.apache.james.jdkim.api.Result; import org.apache.james.jdkim.api.SignatureRecord; import org.apache.james.jdkim.impl.Message; +import org.apache.james.jdkim.tagvalue.SignatureRecordTemplate; import org.junit.Test; public class DKIMTest { @@ -62,33 +67,37 @@ public class DKIMTest { * - "bh=" and "b=" placeholder are required for now because the same implementation is used for * signing and verifying. The fields are mandatory for verifying. */ - private static final String SIGNATURE_TEMPLATE = "v=1; a=rsa-sha256; c=simple; d=messiah.edu; h=date:from:subject; q=dns/txt; s=selector2;"; + private static final String SIGNATURE_TEMPLATE = "v=1; c=simple; d=messiah.edu; h=date:from:subject; q=dns/txt; s=selector2;"; private static final String SIGNATURE_TEMPLATE_2 = "v=1; a=rsa-sha1; c=simple; d=messiah.edu; h=date:from:subject; q=dns/txt; s=selector2;"; private static final String SIGNATURE_TEMPLATE_3 = "v=1; a=rsa-sha256; c=simple; d=messiah.edu; h=date:from:subject; q=dns/txt; s=selector3;"; - private final DKIMSigner dkimSigner = new DKIMSigner(SIGNATURE_TEMPLATE, TestKeys.privateKey); + private final DKIMSigner dkimSigner = new DKIMSigner(); private final DKIMVerifier verifier = new DKIMVerifier(keyRecordRetriever); + @Test public void should_verify_generated_signature_single_key() throws Exception { - String expectedSignature = "DKIM-Signature: a=rsa-sha256; q=dns/txt; b=Axa8s/gTnnJ8em45KV/AQw33hQ4uYtBKiQp3dLq7oRFt+WmDZ5ZErPq4lBVXfP+IAvP+Au91J8270ivn1J/6E0YqKntn4s1hjcNBPPRVohvmlcQ1mEMd6DuYDtWjDFwG2GWZwtilaPY2afhlTuAbHkn8nHm7MVtAGETO8QQ2zfD1NSGzKbNYP9I+hrDJq5ajka6PZn1d+mDhUH5Px8yScYqo5i8Z8GXaejSIu7RsLDuxtOO2cuClRi8MKGxc7MiGndMufXB8xbS1L80IFlyunOVY5eBaqnnhF2YrDDQfZ6DTqorzX6D5dNjpjOG6AbsqkW83Drx0TTV/5M0raU1SIw==; c=simple; s=selector2; d=messiah.edu; v=1; bh=6pQY5V6Dw8mCYWq017gfbpv+x2X4GvOhIIZtKw6iU6g=; h=date:from:subject;"; + DkimSignatureHeader expectedSignature = new DkimSignatureHeader("a=rsa-sha256; q=dns/txt; b=Axa8s/gTnnJ8em45KV/AQw33hQ4uYtBKiQp3dLq7oRFt+WmDZ5ZErPq4lBVXfP+IAvP+Au91J8270ivn1J/6E0YqKntn4s1hjcNBPPRVohvmlcQ1mEMd6DuYDtWjDFwG2GWZwtilaPY2afhlTuAbHkn8nHm7MVtAGETO8QQ2zfD1NSGzKbNYP9I+hrDJq5ajka6PZn1d+mDhUH5Px8yScYqo5i8Z8GXaejSIu7RsLDuxtOO2cuClRi8MKGxc7MiGndMufXB8xbS1L80IFlyunOVY5eBaqnnhF2YrDDQfZ6DTqorzX6D5dNjpjOG6AbsqkW83Drx0TTV/5M0raU1SIw==; c=simple; s=selector2; d=messiah.edu; v=1; bh=6pQY5V6Dw8mCYWq017gfbpv+x2X4GvOhIIZtKw6iU6g=; h=date:from:subject;"); + SignatureRecordTemplate recordTemplate = new SignatureRecordTemplate(SIGNATURE_TEMPLATE); ByteArrayInputStream inputStream = readFileToByteArrayInputStream("/org/apache/james/jdkim/Mail-DKIM/corpus/multiple_2.txt"); - String actualSignature = dkimSigner.sign(inputStream); + + DkimSignatureHeader actualSignature = dkimSigner.sign(inputStream, recordTemplate, TestKeys.privateKey); // optional HashMethod parameter defaults to SHA-256 // sanity check as it is very easy to change the bodyhash behaviour and thus break the signature assertEquals(expectedSignature, actualSignature); InputStream originalInputStream = readFileToByteArrayInputStream("/org/apache/james/jdkim/Mail-DKIM/corpus/multiple_2.txt"); - InputStream signatureInputStream = new ByteArrayInputStream((actualSignature + "\r\n").getBytes(StandardCharsets.UTF_8)); + InputStream signatureInputStream = new ByteArrayInputStream((actualSignature.asMimeHeader() + "\r\n").getBytes(StandardCharsets.UTF_8)); SequenceInputStream verifyInputStream = new SequenceInputStream(signatureInputStream, originalInputStream); - List verifiedSignatures = verifier.verify(verifyInputStream); + DkimVerification dkimVerification = verifier.verify(verifyInputStream); - assertThat(verifiedSignatures) + assertThat(dkimVerification.isSuccess).isTrue(); + assertThat(dkimVerification.signatureVerifications) .hasSize(1) .allSatisfy(it -> - assertThat("DKIM-Signature:" + it.toString()).isEqualTo(expectedSignature) + assertThat("DKIM-Signature:" + it.asHeaderString()).isEqualTo(expectedSignature) ); } @@ -98,9 +107,9 @@ public void should_verify_generated_signature_multiple_keys() throws Exception { // You might want to sign with multiple keys to support several hashmethods on the same selector // or several signing algorithm on the same selector ( ECDSA is a proposed alternative to rsa ) // or when upgrading keys for any reason to add several signatures using different selectors. - String expectedSignature = "DKIM-Signature: a=rsa-sha256; q=dns/txt; b=Axa8s/gTnnJ8em45KV/AQw33hQ4uYtBKiQp3dLq7oRFt+WmDZ5ZErPq4lBVXfP+IAvP+Au91J8270ivn1J/6E0YqKntn4s1hjcNBPPRVohvmlcQ1mEMd6DuYDtWjDFwG2GWZwtilaPY2afhlTuAbHkn8nHm7MVtAGETO8QQ2zfD1NSGzKbNYP9I+hrDJq5ajka6PZn1d+mDhUH5Px8yScYqo5i8Z8GXaejSIu7RsLDuxtOO2cuClRi8MKGxc7MiGndMufXB8xbS1L80IFlyunOVY5eBaqnnhF2YrDDQfZ6DTqorzX6D5dNjpjOG6AbsqkW83Drx0TTV/5M0raU1SIw==; c=simple; s=selector2; d=messiah.edu; v=1; bh=6pQY5V6Dw8mCYWq017gfbpv+x2X4GvOhIIZtKw6iU6g=; h=date:from:subject;"; - String expectedSignature2 = "DKIM-Signature: a=rsa-sha1; q=dns/txt; b=YiwcfjqM7myZ/OENExlyGVzy+rg/779R6pF7bPl79aL6e7yGYeN0XdLRcJEqhg+/uNFwcC7zrbWUwPBVFpFN8pKdQT7TgTr+ydoN65QiBa/rXH4m8Ga+oKx8652dXAHm9oMvG166VdMRsEKTJq2bFpM9RR4mW0KtHPte2JWiOtCYO6MPTlWA2JnIgQp+3+03rnOcKdQ+sn/bi9OwanwE4jgIBcPeHkHVr1fVsV53nvDlbk1DiDX+uOXvuk6bjPVBN4srZiSIvFKsmco0tGZx8cgs5OKyjmtIWVOvjxgupXWvEJJ1nMi1UQ1AXh6jDqWrMDCioRCMG9TeGy8fjcjcfw==; c=simple; s=selector2; d=messiah.edu; v=1; bh=q6DWKdHUzNbVPt6YBbD1KOai/b8=; h=date:from:subject;"; - String expectedSignature3 = "DKIM-Signature: a=rsa-sha256; q=dns/txt; b=crkBsqVuTJJmjZyNuJtmGXBsHIT7tJq0ONWLvNfO29sl1kNm9UzTZ4mOYR+akNJqonkaFFaVM9MZ/6QUd5NbGaIytxXnxv+NPNu6ZzUunlcRyOPhEQ/znemg3WjibRs24gWubBZZXApkqQ9kFh/atatoaJhTls/lnbP8ZV3XlWVN12UuESU3qdieRvrhKWX5/Od7LqZS04ZTeToAabOtDmm6hYl2R6wxizdHrOkGiNERfbB8Iaws5f3Qnt0S94wQ5FVTPzgRiO9OW8hYbAijS4Bh8/NXV5xauMXjCETfxX3pQYUuxc4QVhnoMbmuqgEulzuJUzapjotLLFxQRaSsjw==; c=simple; s=selector3; d=messiah.edu; v=1; bh=6pQY5V6Dw8mCYWq017gfbpv+x2X4GvOhIIZtKw6iU6g=; h=date:from:subject;"; + DkimSignatureHeader expectedSignature = new DkimSignatureHeader("a=rsa-sha256; q=dns/txt; b=Axa8s/gTnnJ8em45KV/AQw33hQ4uYtBKiQp3dLq7oRFt+WmDZ5ZErPq4lBVXfP+IAvP+Au91J8270ivn1J/6E0YqKntn4s1hjcNBPPRVohvmlcQ1mEMd6DuYDtWjDFwG2GWZwtilaPY2afhlTuAbHkn8nHm7MVtAGETO8QQ2zfD1NSGzKbNYP9I+hrDJq5ajka6PZn1d+mDhUH5Px8yScYqo5i8Z8GXaejSIu7RsLDuxtOO2cuClRi8MKGxc7MiGndMufXB8xbS1L80IFlyunOVY5eBaqnnhF2YrDDQfZ6DTqorzX6D5dNjpjOG6AbsqkW83Drx0TTV/5M0raU1SIw==; c=simple; s=selector2; d=messiah.edu; v=1; bh=6pQY5V6Dw8mCYWq017gfbpv+x2X4GvOhIIZtKw6iU6g=; h=date:from:subject;"); + DkimSignatureHeader expectedSignature2 = new DkimSignatureHeader("a=rsa-sha1; q=dns/txt; b=YiwcfjqM7myZ/OENExlyGVzy+rg/779R6pF7bPl79aL6e7yGYeN0XdLRcJEqhg+/uNFwcC7zrbWUwPBVFpFN8pKdQT7TgTr+ydoN65QiBa/rXH4m8Ga+oKx8652dXAHm9oMvG166VdMRsEKTJq2bFpM9RR4mW0KtHPte2JWiOtCYO6MPTlWA2JnIgQp+3+03rnOcKdQ+sn/bi9OwanwE4jgIBcPeHkHVr1fVsV53nvDlbk1DiDX+uOXvuk6bjPVBN4srZiSIvFKsmco0tGZx8cgs5OKyjmtIWVOvjxgupXWvEJJ1nMi1UQ1AXh6jDqWrMDCioRCMG9TeGy8fjcjcfw==; c=simple; s=selector2; d=messiah.edu; v=1; bh=q6DWKdHUzNbVPt6YBbD1KOai/b8=; h=date:from:subject;"); + DkimSignatureHeader expectedSignature3 = new DkimSignatureHeader("DKIM-Signature: a=rsa-sha256; q=dns/txt; b=crkBsqVuTJJmjZyNuJtmGXBsHIT7tJq0ONWLvNfO29sl1kNm9UzTZ4mOYR+akNJqonkaFFaVM9MZ/6QUd5NbGaIytxXnxv+NPNu6ZzUunlcRyOPhEQ/znemg3WjibRs24gWubBZZXApkqQ9kFh/atatoaJhTls/lnbP8ZV3XlWVN12UuESU3qdieRvrhKWX5/Od7LqZS04ZTeToAabOtDmm6hYl2R6wxizdHrOkGiNERfbB8Iaws5f3Qnt0S94wQ5FVTPzgRiO9OW8hYbAijS4Bh8/NXV5xauMXjCETfxX3pQYUuxc4QVhnoMbmuqgEulzuJUzapjotLLFxQRaSsjw==; c=simple; s=selector3; d=messiah.edu; v=1; bh=6pQY5V6Dw8mCYWq017gfbpv+x2X4GvOhIIZtKw6iU6g=; h=date:from:subject;"); ByteArrayInputStream inputStream1 = readFileToByteArrayInputStream("/org/apache/james/jdkim/Mail-DKIM/corpus/multiple_2.txt"); String actualSignature1 = dkimSigner.sign(inputStream1); @@ -119,7 +128,7 @@ public void should_verify_generated_signature_multiple_keys() throws Exception { // prepend signatures to the message as per https://datatracker.ietf.org/doc/html/rfc6376#section-3.5 InputStream originalInputStream = readFileToByteArrayInputStream("/org/apache/james/jdkim/Mail-DKIM/corpus/multiple_2.txt"); String signatures = String.join("\r\n", Arrays.asList(actualSignature1, actualSignature2, actualSignature3)); - InputStream signatureInputStream = new ByteArrayInputStream((signatures+"\r\n").getBytes(StandardCharsets.UTF_8)); + InputStream signatureInputStream = new ByteArrayInputStream((signatures + "\r\n").getBytes(StandardCharsets.UTF_8)); SequenceInputStream verifyInputStream = new SequenceInputStream(signatureInputStream, originalInputStream); @@ -183,4 +192,168 @@ public InputStream getBodyInputStream() { return headers.getBodyInputStream(); } } + + // ======== IGNORE CODE BELOW THIS FOR STARTERS ====== // + + private static class DkimSignatureHeader { + private final String name = "DKIM-Signature"; + private final String value; + + private DkimSignatureHeader(String value) { + this.value = value; + } + + public String asMimeHeader() { + return name + ": " + value; + } + + @Override + public boolean equals(Object o) { + if (o == null || getClass() != o.getClass()) return false; + DkimSignatureHeader that = (DkimSignatureHeader) o; + return Objects.equals(name, that.name) && Objects.equals(value, that.value); + } + + @Override + public int hashCode() { + return Objects.hash(name, value); + } + } + + /** + * source https://datatracker.ietf.org/doc/html/rfc8601#section-2.7.1 + */ + private static enum SignatureVerificationResult { + // NONE means The message was not signed so it cannot be a signature result. + /** + * The message was signed, the signature or signatures were acceptable + * to the ADMD, and the signature(s) passed verification tests. + */ + PASS, + /** + * The message was signed and the signature or signatures were + * acceptable to the ADMD, but they failed the verification test(s). + */ + FAIL, + /** + * The message was signed, but some aspect of the signature or + * signatures was not acceptable to the ADMD. + */ + POLICY, + /** + * The message was signed, but the signature or signatures contained + * syntax errors or were not otherwise able to be processed. + * This result is also used for other failures not covered elsewhere + * in this list. + */ + NEUTRAL, + /** + * The message could not be verified due to some error th at is likely + * transient in nature, such as a temporary inability to retrieve a + * public key. + * A later attempt may produce a final result. + */ + TEMPFAIL, + /** + * The message could not be verified due to some error that is + * unrecoverable, such as a required header field being absent. + * A later attempt is unlikely to produce a final result. + */ + PERMFAIL + } + + private enum FailedHeaderTagName { + SIGNING_DOMAIN("header.d"), + IDENTITY("header.i"), + ALGORITHM("header.a"), + SELECTOR("header.s"); + + private final String value; + + FailedHeaderTagName(String value) { + this.value = value; + } + } + + private static class FailedHeaderTag { + private final FailedHeaderTagName tagName; + private final String value; + + private FailedHeaderTag(FailedHeaderTagName tagName, String value) { + this.tagName = tagName; + this.value = value; + } + + public String asHeaderString() { + return tagName.value + "=" + value + ";"; + } + } + + private static class SignatureVerification { + private final SignatureVerificationResult result; + private final Optional reason; + private final Optional tag; + + private SignatureVerification( + SignatureVerificationResult result, + Optional reason, + Optional tag + ) { + this.result = result; + this.reason = reason; + this.tag = tag; + } + + String asHeaderString() { + String reason = this.reason.map(it -> "reason=" + it).orElse(""); + String tag = this.tag.map(FailedHeaderTag::asHeaderString).orElse(""); + + switch (result) { + case PASS: + return "dkim=pass (good signature);"; + case FAIL: + return Stream.of("dkim=fail", reason, tag) + .filter(it->!it.isEmpty()) + .collect(joining("", " ", ";")); + case POLICY: + return Stream.of("dkim=policy", reason, tag) + .filter(it->!it.isEmpty()) + .collect(joining("", " ", ";")); + case NEUTRAL: + return Stream.of("dkim=neutral", reason, tag) + .filter(it->!it.isEmpty()) + .collect(joining("", " ", ";")); + case TEMPFAIL: + return Stream.of("dkim=temperror", reason, tag) + .filter(it->!it.isEmpty()) + .collect(joining("", " ", ";")); + case PERMFAIL: + return Stream.of("dkim=permerror", reason, tag) + .filter(it->!it.isEmpty()) + .collect(joining("", " ", ";")); + default: + throw new IllegalStateException("unreachable code, which will disappear in later java versions with exhaustivity checking"); + } + } + } + + private static class DkimVerification { + private final List signatureVerifications; + boolean isSuccess; + + private DkimVerification(List signatureVerifications) { + this.signatureVerifications = signatureVerifications; + isSuccess = signatureVerifications.stream() + .anyMatch(it -> + it.result == SignatureVerificationResult.PASS + ); + } + + String asHeaderString() { + return signatureVerifications.stream() + .map(it -> + it.asHeaderString()).collect(joining(";\r\n") + ); + } + } } \ No newline at end of file