-
Notifications
You must be signed in to change notification settings - Fork 38
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
Add checks for RFC8460, SMTP-TLS reporting (TLSRPT) #881 #1300
base: main
Are you sure you want to change the base?
Conversation
…rrect translation markers, add tlsrpt callback
…licy records with parser
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 this PR! I left a few inline comments as well, general notes:
- We try to add typing to new code, where reasonably possible. Particularly method arguments, as our call trees can be a bit opaque.
- Let's make a new package
tasks/mail/
, take the new code totasks/mail.py
intotasks/mail/tlsrpt.py
and also move the new parser into that package. This is different from existing layout, but I want to keep things a bit more separate, smaller and clearer files, and am doing something similar for TLS.
For our team:
- @bwbroersma you've been talking about ABNF-based parsers, perhaps this is a situation where we could experiment with that rather than the parser that's written now. Thoughts?
- Merging this makes it a recommended standard by us. Is that what we want? Are there processes pending for this?
@@ -2182,8 +2183,8 @@ def __init__(self): | |||
label="detail mail auth spf label", | |||
explanation="detail mail auth spf exp", | |||
tech_string="detail mail auth spf tech table", | |||
worst_status=scoring.MAIL_AUTH_SPF_WORST_STATUS, | |||
full_score=scoring.MAIL_AUTH_SPF_PASS, | |||
worst_status=scoring.MAIL_AUTH_TLSRPT_WORST_STATUS, |
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.
Seems like this should stay at SPF, and TLSRPT should be used in MailAuthTlsRptExists
self._status(STATUS_NOTICE) | ||
self.verdict = "detail mail auth tlsrpt verdict bad" | ||
if tech_data: | ||
# More than one spf record. Show the records. |
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 don't think this comment fits the rest of the code? Might be wrong even for SPF.
MAIL_AUTH_TLSRPT_PASS = NO_POINTS | ||
MAIL_AUTH_TLSRPT_FAIL = NO_POINTS # TLS-RPT fail does not give a points penalty | ||
MAIL_AUTH_TLSRPT_ERROR = NO_POINTS | ||
MAIL_AUTH_TLSRPT_WORST_STATUS = STATUS_FAIL |
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.
Per the scoring documentation this should probably also use FULL/NO/NO_POINTS for PASS/FAIL/ERROR, with the WORST_STATUS as STATUS_INFO (or STATUS_NOTICE if we want to give it more weight). That setting for WORST_STATUS should give it no score impact.
mtauth.tlsrpt_available = tlsrpt_available | ||
mtauth.tlsrpt_record = tlsrpt_record | ||
mtauth.tlsrpt_score = tlsrpt_score | ||
log.debug(f"subtests: {subtests.keys()}") |
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 we want to keep this log, it should be a bit more clear.
@@ -824,3 +849,76 @@ def dmarc_get_public_suffix_list(): | |||
public_suffix_list = dmarc_get_public_suffix_list() | |||
|
|||
return public_suffix_list | |||
|
|||
|
|||
def tlsrpt_callback(data, status, r): |
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.
Can we add types for these parameters? We try to do it for all new code. I have no idea what "status" means here, or why it should be 0.
|
This PR adds support for checking the policy record (e.g. _smtp._tls.example.com). The check parses the record and results in a "WARNING" state when the TLSRPT policy record is either nonexistant or malformed. To check if the TLSRPT record is well formed, we include a TLSRPT policy record parser in this PR.
The respective descriptions have been updated for both "en" and "nl" translations – though the "nl" translations needs to be looked over by a native speaker.
The test results UI has been updated to include the TLSRPT check on the results page (as part of 'Authenticity marks against phishing' section).
TLSRPT record broken or non-existing:
TLSRPT record exists and is well-formed: