Authenticated Received Chain - ARC sealing

[agrinchenko]

Replacement branch for PR #31, based on the current apache/master to resolve merge conflicts on PR #31.

[chibenwa]

Hello @agrinchenko
I had one more pass of review on this work, oriented "reliability".
I deliberatly ingored "cosmetic remarks".
That work is really nice, can't wait to have it in and write Arc mailet / matchers for ASF James.
If you want a hand for those small fixes dont' hesitate!
Regards,
Benoit

[agrinchenko]

Thanks again, @chibenwa for thorough reviews! Feel free to suggest cosmetic remarks or if you see something else.

[chibenwa]

I think slightly more work would be needed in order to make the interface friendlier to use. I did make two comments in that direction, and paid attention to ask for minimal changes that should not impact much the code that was written but relieve library users... If you like the suggestion then it is great, otherwise I'm fine contributing those enhancements.

Also, this is a major contribution, we are forced to ask you to sign an ICLA https://www.apache.org/licenses/icla.pdf and send it to secretary@apache.org cf apache.org/licenses/contributor-agreements.html please send the email address used to sign the ICLA to private@james.apache.org. Do not Cc the list on your ICLA. We may track this onhttps://whimsy.apache.org/roster/icla/ with this info. With such a qualitative and impactful contribution this is a necessary measure.

Thanks again for your involvement

Best regards,

Benoit

[chibenwa]

That is a lot of string arguments that I would be annoyend to distinguish from each other and to actually come up with. Suggestion: syntactic sugar

// I know no record but for interface writing the corresponding POJO is worth it.
enum DkimStatus  {FAIL, PASS, NEUTRAL} // better discoverability that strings ! We can have a convenience parse method to bridge the gap, and getters.
record DmarcDataInput(Message message,  String spfHeaderText, String  spfDomain, String dkimResult, String dkimDomain);

// Then we could play with builders

dmarcVerifier.verify(DmarcDataInput.builder()
   .message(message)
   .spfHeaderValue("pass client-ip=192.0.2.1; envelope-from=sender@example.org")
   .ehlo("exemple.org")
   .dkimResult(PASS)
   .dkimDToken("example.org")
   .build());

Would already be a nice enhancement.

We could have a typed alternative for SPF. Also We likely should offer a wauy to work directly with DKIM signature records.

dmarcVerifier.verify(DmarcDataInput.builder()
   .message(message)
   .spfResult(spfResult)
   .ehlo("exemple.org") // This eases the user in understanding how to get that horrible SPF string and not get it himself
   .dkimSignatures(signatureRecords) // Again we would offweight the user from dealing himself with interpreting the DKIM results -> DMARC job. Type: List<SignatureRecord>
   .build());

To be honnest I do not care (here) about the internal, just about the outer layers. IMO we can easily do the last kilometer to make it easy to use.

Does this make sense?

[chibenwa]

Here again some interface cosmetic fixes would go a long way. Same principal: builders, and POJO extracted...

Suggested change

	keyRecordRetriever);
	ArcSetBuilder arcSetBuilder = new ArcSetBuilder(
	privateKey,
	keyRecordRetriever,
	amsTemplate,
	sealTemplate,
	"mx.example.org",
	System.currentTimeMillis() / 1000);
	PublicKeyRetrieverArc keyRecordRetriever = null;
	record ArcSigningInput(Message message, String helo, String mailFrom, String ip);
	Map<String, String> arcSet = arcSetBuilder.buildArcSet(ArcSigningInput.builder()
	.message(message)
	.ehlo("mail.example.org")
	.mailFrom("sender@example.org")
	.ip("192.0.2.1")
	.build());

1. IMO keyRecordRetriever is better handled as a class field as there is as far as I
2. Builder.

[chibenwa]

(Ps: those two comment address and I approuve)