-
Notifications
You must be signed in to change notification settings - Fork 39
First commit #31
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
base: master
Are you sure you want to change the base?
First commit #31
Conversation
chibenwa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This work is wonderful, thanks for putting it together.
I have mostly nits and checkstyles.
However I do believe we need to invest in significant testing with at least a dozen messages and ensure a good overall test coverage,
also while dmarc makes sense as part of arc, but it also makes sense of its own. How about extracting DMARC tooling (record parsing & evaluation) in a standalong dmarc maven module we could depend on?
arc/src/main/java/org/apache/james/arc/DNSPublicKeyRecordRetrieverArc.java
Show resolved
Hide resolved
|
Thanks all for taking time to review and providing your feedback! Keep it coming! I will work on improving the code in the coming days. |
|
I hope I can convince you not to follow the existing DKIM api which is really not so great. please have a look at the proposed API in #27 I'm not saying my proposed API design is perfect (by no means) but it should be better than what we have today |
Co-authored-by: Benoit TELLIER <btellier@linagora.com>
Co-authored-by: Benoit TELLIER <btellier@linagora.com>
Co-authored-by: Benoit TELLIER <btellier@linagora.com>
Co-authored-by: Benoit TELLIER <btellier@linagora.com>
Thanks for the kind comments, Benoit! 100% agree on need in broad testing covering all possible edge cases that I might have missed. I wish Appendix B in RFC 8617 had more examples as well as signing/public key pairs used in them. Let me think on making a module out of |
…dback - Introduced ArcValidationResult enum - Made ArcSealVerifyData immutable pojo - Removed dependency on geronimo.javamail - Removed dependency on Freemarker and simplified ARC templates - Updated versions in pom to pull them from the parent pom - Fixed missing licensing headers - Applied suggested style fixes
Sure, I will take a look at the proposed API and see how we can change it. |
|
I checked it a new commit with updates. Outstanding on my list:
|
I'd see a point: discoverability. Just like OpenDmarc and OpenArc are 2 distict projects.
Maybe we could reach the DMARC work group that published the spec and ask if there is a TCK (Technology COmpliance Kit): dmarc@ietf.org ? I had a look at OpenARC tests but they are thin. |
|
What method can be used to know if the sealer can be trusted ? I didn't see any in the ARCVerifier. |
The logic to validate previous ARC sealers is included in the |
My usage of ARC was limited, but as far as I know, no ARC sealer can be trusted. I think it was designed to be an opt-in feature. Only accept ARC headers from previously trusted (and configured) servers. Like Microsoft does, you need to configure the sealer there. A sealer can be a malicious actor with a valid signature modifying messages, and your server will deliver an authenticated phishing or spam. These things must be treated with caution. The RFC 8617 is experimental. From rfc8617#section-7.1: "If ARC-enabled ADMDs are trusted, Authenticated Received Chains can The section rfc8617#section-9.4 (Message Sealer Suspicion), says sealers must be treated with suspicion. |
Yes, discoverability makes sense. I can move it into it's own module under james-jdkim. A few additional things to consider before we do that:
|
SPF is already a standalong library of its own. I'll scope it as "let's keep this a dependency of DMARC" CF what about SPF, does it make sense to put it in its own module too? I am not opposed to it and think logically it might make sense if we decide to separate DMARC.
If @epinter agrees I think a dmarc library in Java fully deserve to be in the Apache James umbrella. No objections to depend on it though. |
I agree.
I think the development of this PR doesn't need to stop because of dmarc. Now it is just a method call to check alignment, it will be easy to replace with a call to a future library. |
- Rewired DMARC into separate module - Added relaxed header alignment for DMARC using PSL list - Removed commons-codec dependency
|
Hey guys! I committed changes with DMARC in a separate module and also implemented both strict and relaxed header checking against PSL list. The Jenkins build fails though because |
|
I have limited bandwith because of over-committing on another project. I will take time for a complete review of this work this weekend. |
|
Thanks, Benoit! No rush.. |
chibenwa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think we could benefit from updating the documentation (src/site ?) for mentionning arc and dkim support + giving some little code examples.
I (tried to) refrain from doing too much code style related comments but I'd be happy, if you accept, to propose you a changeset to polish a bit this work (i'd PR your branch).
arc/src/main/java/org/apache/james/arc/ArcSignatureRecordImpl.java
Outdated
Show resolved
Hide resolved
|
|
||
| private PublicSuffixList() {} | ||
|
|
||
| public static boolean isPublicSuffix(String domain) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PSL can't be used just to know if contains a suffix, it's missing exceptions and wildcard. Many valid domains will not be found.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback. I will take a closer look at PSL exceptions and wildcards, I think I just learned something new about PSL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback. I will take a closer look at PSL exceptions and wildcards, I think I just learned something new about PSL.
If you need, here is my library:
https://central.sonatype.com/artifact/dev.pinter.psl/publicsuffixlist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again, for the pointer to your lib! Nice work! I think it does actually more than what I need for the relaxed domain checking. I took another try at rewriting my initial PSL code to add ability to handle wildcards and exceptions. Checked in.
dmarc/src/main/java/org/apache/james/dmarc/PublicSuffixList.java
Outdated
Show resolved
Hide resolved
| import static org.junit.Assert.assertTrue; | ||
|
|
||
| public class PublicSuffixListTest { | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a PSL implementation is to be included here, I think a lot more tests will be needed. My implementation has more than 130 tests. A fail in PSL lookup can make invalid dmarc to pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know you have PSL implementation with 130 tests already! I would be glad to integrate with your work and delegate the relaxed org domain determination to your code. Let me know! Essentially I would need to be able to supply FQDN and receive either (1) a relaxed version of it (i.e. PSL public suffix + 1 part to the left of it) or (2) same FQDN if nothing is found in PSL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work you have on PSL! I think your library does more than what I need for the purposes of relaxed domain alignment. As a first step I rewrote initial PSL code I had to handle wildcards and exceptions, also added more test cases.
- Added ARC validation outcome with details on the failure - Refactored logic such as `computeBTag` into separate methods - Added hard fail whenever multiple From headers detected - Rewrote tag extraction logic to do it all in one pass for efficiency - Simplified getTimeMeasure to use standard Java - Removed unnecessary Overrides
Draft for ARC-sealing for review/comments/feedback. Work in progress.