Authenticated Received Chain - ARC-sealing by agrinchenko · Pull Request #31 · apache/james-jdkim

agrinchenko · 2025-10-07T19:35:02Z

Draft for ARC-sealing for review/comments/feedback. Work in progress.

Fix FWS is not trimmed correctly

- Use goals `single` instead of `attached` by the recommendation at http://maven.apache.org/plugins-archives/maven-assembly-plugin-2.5.5/attached-mojo.html

Following alphabetical order with groupId > artifactId

The JDKIM mailet has been moved into James server project

After removing the mailet (been moved to James), those libs are not necessary anymore.

…es project

- Add ValiMail ARC validation coverage for no-chain base messages, single-hop pass fixtures, missing and malformed ARC public keys, and AMS/AS selector-domain independence. - Add AMS bh= validation coverage for whitespace-tolerant body hashes, simple and relaxed body canonicalization, missing/empty/non-base64 body hash tags, modified body hashes, and modified message bodies. - Verify AMS body hashes before accepting ARC-Message-Signature by decoding bh=, applying the AMS body canonicalization mode, and comparing the computed SHA-256 digest. Reject malformed or unsupported body hash inputs cleanly while preserving existing multipart fixture behavior when the Mime4j DOM parser cannot reconstruct a raw multipart body.

- Updated ARCVerifier to parse AMS canonicalization as header/body modes. - Added support for AMS simple header canonicalization in addition to relaxed. - Rejects empty/invalid c= values before accepting AMS.

- d= is required, non-empty, and must look like a valid domain. - s= is required, non-empty, and must look like a valid selector. - t= remains optional, but if present must be numeric.

- Add ValiMail fixture-backed tests for the remaining AMS h= signed-header cases, covering empty h=, whitespace and folded separators, case-insensitive header names, duplicate header selection, missing headers, omitted h=, header order failures, and ARC header inclusion behavior. - Reject ARC-Seal in the AMS h= signed-header list so messages that include arc-seal in AMS signing data fail validation as expected by the upstream test suite, while preserving the allowed arc-message-signature case. - Add explicit first-hop signing coverage for messages without an incoming ARC chain, asserting that generated ARC sets start at i=1 and seal with cv=none.

agrinchenko · 2026-04-18T22:48:00Z

I added the remaining ARC test coverage based on RFC 8617 and the public ValiMail ARC test suite and am moving this PR out of draft, ready for review.

The added tests cover the ARC behavior identified in the sources, including AMS h= handling, ARC-Seal inclusion behavior, and first-hop ARC set generation for messages without an existing ARC chain. We are at 185 tests now.

agrinchenko · 2026-04-18T22:53:37Z

@chibenwa - PR is ready for the final review when you have a chance.

chibenwa

Thanks a lot for perseverance and putting this together!

I spent most of my friday night piecing this together... THere remains a few sensitive points that I believe would need to be adressed before that work is integrated.

Happy to contribute the fixes if needed.

Cheers!

Benoit

chibenwa · 2026-04-25T20:14:33Z

+ * <p>
+ * This class is not instantiable and all methods are static.
+ */
+public class ARCVerifier {


There is no x= / t= timestamp check in verifyAms — ArcSignatureRecordImpl.validate() checks expiry, but verifyAms never instantiates that path. An expired AMS passes silently.

ARC-Seal h= content isnot validated — RFC 8617 §4.1.1 specifies that the Seal's h= must cover exactly {aar, ams, as} for all prior hops in order. The code doesn't validate this constraint at all.

Good catch on AMS timestamps. I am adding 'x=' validation in 'verifyAms' and expired AMS signatures now fail, and 'x=' must be greater than 't='
when both are present.

Regarding the second point, I am checking the RFC 8617 and my reading is that 4.1.1 describes ARC-Authentication-Results only; ARC-Seal is defined in
4.1.3. My understanding is that ARC-Seal does not carry an 'h=' tag. The ARC headers covered by AS are defined by the sealing/validation algorithm, not by an AS 'h=' list. I will tighten validation to reject ARC-Seal headers that include 'h=' and keep the existing AS signing/verification path responsible for covering the ARC set headers in order.

I fixed the first point in the commit agrinchenko@2f8955f

The second point is addressed in a way to reject ARC-Seal headers that include 'h='. Again, this is based on my earlier comment above regarding the second point.

agrinchenko · 2026-04-25T20:26:34Z

@chibenwa, thanks for the review and feedback! Going through the points you raised.

- Validate every ARC-Message-Signature in multi-hop chains and use the ARC-Seal key from the instance being verified. - Parse ARC i= tags numerically, enforce the valid 1..50 range, and avoid i=1 matching i=10. - Harden AMS verification for unsupported algorithms, x=/t= expiry, no-body bh= checks, and reject ARC-Seal h= tags. - Fix multipart body reconstruction to emit CRLF after the closing boundary. - Return DMARC permerror for malformed From headers instead of throwing. - Replace the bundled public suffix list with Guava InternetDomainName and add Guava as a managed DMARC dependency. - Remove the SPF thread context classloader workaround. - Add ARC and DMARC regression tests for the validation, canonicalization, and malformed input cases above. - Refresh README ARC/DMARC documentation while preserving upstream DKIM content.

agrinchenko · 2026-04-26T16:28:17Z

@chibenwa I pushed the updates addressing the latest review feedback and the PR is ready for another pass. Thanks again for the detailed review!

chibenwa · 2026-04-26T17:46:05Z

+        for (int i = 1; i <= numArcInstances; i++) {
+            Set<Field> arcSet = arcVerifier.extractArcSet(messageHeaders, i);
+            if (arcSet == null || !checkArcAms(arcSet, message, arcVerifier)) {
+                return new ArcValidationOutcome(ArcValidationResult.FAIL, "Previous ARC hops validation failed");


Maybe we could get 'i' in the error message in order to know which one?

Good idea! Updating the ARC previous-hop validation failure message to include the failing ARC instance, like this -- "Previous ARC hop validation failed at i=...".

chibenwa · 2026-04-26T17:49:11Z

+        if (timestamp != null && expirationEpoch <= Long.parseLong(timestamp)) {
+            throw new ArcException("AMS x= expiration must be greater than t= timestamp");
+        }
+        long now = System.currentTimeMillis() / 1000;


It would be wonderful to rely on a Clock that can be overriden by the library user, defaulting to Clock.systemUTC if none: this would ease testability.

Interesting idea and it does make sense! Updating "ARCVerifier" to accept an injectable "Clock" override, with the existing constructor defaulting to "Clock.systemUTC()". The AMS expiration check now uses that clock. I am also updating AMS regression test to use a fixed clock so it is deterministic.

chibenwa · 2026-04-26T17:54:54Z

+    private void validateAmsTimestamp(Map<String, String> tags) {
+        String timestamp = tags.get("t");
+        String expiration = tags.get("x");
+        if (expiration == null) {


If expiration is unspecified, we need to check that t is not in the future.

Updating AMS timestamp validation to reject a t= value that is in the future and adding a new test case for that.

chibenwa · 2026-04-26T17:56:39Z

            if (bodyBytes.length == 0 && message.getBody() instanceof Multipart) {
                return true;
            }


verifyAmsBodyHash: empty-Multipart early return true bypasses the bh= check.

Remove it and let Arrays.equals handle it ?

Agreed. I am removing the empty-multipart early return so bh= is always compared normally. Also, I am adding a test for an empty multipart with an invalid body hash.

chibenwa · 2026-04-26T17:58:48Z

-    }
+import com.google.common.net.InternetDomainName;

+public class PublicSuffixList {


Please have it as aan interface and allow people to use there own

Depending on Guava (and a specific one) is not cool for a lib, we need to have a built in alternative for people: exclude guava dependency, write their own with their own version instead...

Yes, changing "PublicSuffixList" into an interface and making DMARCVerifier accept it. The default constructor will use a built-in implementation, and GuavaPublicSuffixList will remain available as an optional implementation. Also marking Guava optional in the DMARC module and adding a test covering custom PSL injection.

chibenwa · 2026-04-26T18:11:11Z

        return true;
    }

+    private boolean checkArcSealTags(List<Field> arcSet) {


checkArcSealTags only rejects h=

I think we miss validation of required tags (i=, a=, cv=, d=, s=, b=).

Maybe we also need to check cv value to be one of none pass fail.

Maybe we also need to valide d and s on Arc-Seal too (it's done for AMS as far as I understand)

Will tighten checkArcSealTags so ARC-Seal structure validation will require non-empty i, a, cv, d, s, and b tags and will validate cv against none/pass/fail. I will also validate d and s using the same domain/selector rules as AMS, still rejecting h=.

chibenwa · 2026-04-26T19:21:59Z

Also we might want to solve the merge conflict...

- Include the failing ARC instance in previous-hop AMS validation errors. - Add injectable Clock support to ARCVerifier and use it for AMS timestamp checks. - Reject AMS t= timestamps in the future, even when x= is absent. - Remove empty multipart bh= bypass and always compare computed body hashes. - Fix parsed multipart reconstruction by reading the boundary from parent Content-Type. - Tighten ARC-Seal tag validation for required i/a/cv/d/s/b tags, cv values, d/s syntax, and h= rejection. - Convert DMARC PublicSuffixList to an injectable interface. - Add built-in default PSL resolver and optional Guava-backed implementation. - Mark Guava optional and add custom PSL injection coverage. - Add regression tests for diagnostics, timestamp validation, multipart bh= handling, and custom PSL wiring.

agrinchenko · 2026-04-27T21:21:50Z

I checked after pushing the latest updates, and GitHub currently says "No conflicts with base branch" and "Changes can be cleanly merged." I do not see an active merge conflict at the moment, but I’ll keep an eye on it if the base branch moves again.

Arsnael · 2026-04-28T02:10:09Z

@agrinchenko maybe you should rebase this work on latest master if not done already? I see conflicts as well, and more than a 100 commits that do not belong with this work :)

chibenwa · 2026-04-28T14:55:40Z

Seen the new PR
I'll try to review it in the coming days
I propose to close this one for the time being.
Thanks for your involvment @agrinchenko !

agrinchenko · 2026-04-28T14:58:16Z

@chibenwa, @Arsnael I created a new PR-33 that is based off the current apache-master to address the merge issues.

agrinchenko · 2026-04-28T15:02:01Z

Closing this PR in favor of the clean replacement #33

agrinchenko · 2026-04-28T15:02:34Z

This PR is being replaced by #33

mbaechler and others added 30 commits July 5, 2019 09:25

Merge pull request apache#8 from Weiling-Liao/master

22266e5

Fix FWS is not trimmed correctly

JDKIM-42 Upgrade apache parent pom version 18 -> 21

e8a21b0

JDKIM-42 Upgrade maven-assembly-plugin 3.0.0 -> 3.1.1

ab18800

- Use goals `single` instead of `attached` by the recommendation at http://maven.apache.org/plugins-archives/maven-assembly-plugin-2.5.5/attached-mojo.html

JDKIM-42 Upgrade junit 4.10 -> 4.12

bc8ace8

JDKIM-42 Upgrade commons-codec 1.7 -> 1.13

fa03947

JDKIM-42 Upgrade commons-logging 1.1.1 -> 1.2

62408fd

JDKIM-42 Upgrade dnsjava 2.1.1 -> 2.1.9

eabf827

JDKIM-42 Upgrade log4j 1.2.16 -> log4j-core 2.12.1

e3f49cb

JDKIM-42 Upgrade maven-site-plugin 3.3 -> 3.7.1

2ee6bc6

JDKIM-42 Upgrade wagon-ssh 2.0 -> 3.3.3

b4f162c

JDKIM-42 Upgrade maven-bundle-plugin 2.3.7 -> 4.2.1

2d6177a

JDKIM-42 Upgrade maven-compiler-plugin 3.0 -> 3.8.1

c8bc3db

JDKIM-42 Upgrade apache-rat-plugin 0.8 -> 0.13

7b263de

JDKIM-42 Upgrade versions-maven-plugin 2.0 -> 2.7

3d05194

JDKIM-42 Upgrade apache-mime4j 0.8.1 -> 0.8.3

b558816

JDKIM-42 Reorder dependencies in pom files

8a92e65

Following alphabetical order with groupId > artifactId

JDKIM-42 Remove apache-jdkim-mailets

c896a16

The JDKIM mailet has been moved into James server project

JDKIM-42 Remove geronimo libs

82c43f5

After removing the mailet (been moved to James), those libs are not necessary anymore.

JDKIM-42 Update README to state that the mailet has been moved to Jam…

6ce11b2

…es project

JDKIM-44 Use JDK 8 as a target JDK

a3269a8

[JAMES-3226] #comment Added antora docs stub

1005b1c

JDKIM-46 Set up Jenkins build system

ac56007

[Documentation] Links should enforce HTTPS in menus

a5a91b2

[Documentation] Retire HUPA

cb715a8

Upgrade DNS Java 2.1.9 -> 3.4.1

1b04fb8

Upgrade commons-codec 1.13 -> 1.15

087df46

Upgrade junit 4.12 -> 4.13.2

3fa5e75

Upgrade log4j 2.12.1 -> 2.14.1

95862c5

Upgrade wagon-ssh 3.3.3 -> 3.4.3

80a6909

Remove unused log4j

4f4d6ef

agrinchenko added 5 commits April 18, 2026 17:50

Added ValiMail coverage tests in ARCTest for "c=" edge cases:

25a1356

- Updated ARCVerifier to parse AMS canonicalization as header/body modes. - Added support for AMS simple header canonicalization in addition to relaxed. - Rejects empty/invalid c= values before accepting AMS.

Added AMS tag validation in ARCVerifier:

ff1dcbb

- d= is required, non-empty, and must look like a valid domain. - s= is required, non-empty, and must look like a valid selector. - t= remains optional, but if present must be numeric.

Updated README with a Test Coverage section

5f04ea9

agrinchenko marked this pull request as ready for review April 18, 2026 22:48

agrinchenko changed the title ~~[Draft] Authenticated Received Chain - ARC-sealing~~ Authenticated Received Chain - ARC-sealing Apr 21, 2026

chibenwa reviewed Apr 25, 2026

View reviewed changes

Comment thread dmarc/src/main/resources/public_suffix_list.dat Outdated

chibenwa reviewed Apr 26, 2026

View reviewed changes

agrinchenko mentioned this pull request Apr 28, 2026

Authenticated Received Chain - ARC sealing #33

Open

agrinchenko closed this Apr 28, 2026

Conversation

agrinchenko commented Oct 7, 2025

Uh oh!

agrinchenko commented Apr 18, 2026

Uh oh!

agrinchenko commented Apr 18, 2026

Uh oh!

chibenwa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

agrinchenko commented Apr 25, 2026

Uh oh!

agrinchenko commented Apr 26, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chibenwa commented Apr 26, 2026

Uh oh!

agrinchenko commented Apr 27, 2026

Uh oh!

Arsnael commented Apr 28, 2026

Uh oh!

chibenwa commented Apr 28, 2026

Uh oh!

agrinchenko commented Apr 28, 2026

Uh oh!

agrinchenko commented Apr 28, 2026

Uh oh!

agrinchenko commented Apr 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants