Skip to content
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

Open
wants to merge 33 commits into
base: main
Choose a base branch
from

Conversation

uwekamper
Copy link
Contributor

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-broken
tlsrpt-nonexistant

TLSRPT record exists and is well-formed:

tlsrpt-good

…rrect translation markers, add tlsrpt callback
Copy link
Collaborator

@mxsasha mxsasha left a 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 to tasks/mail.py into tasks/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,
Copy link
Collaborator

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.
Copy link
Collaborator

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
Copy link
Collaborator

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()}")
Copy link
Collaborator

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):
Copy link
Collaborator

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.

@bwbroersma
Copy link
Collaborator

@mxsasha:

  • Currently the parsing is using pyparsing:

    WSP = White(ws=" ", exact=1).suppress() # Whitespace
    field_delim = ZeroOrMore(WSP) + Literal(";") + ZeroOrMore(WSP) # Fields are semicolon-delimited

    We know at least using pyparsing like this does not work:

    WSP = Optional(White(ws=" ")).suppress()
    sep = (WSP + CaselessLiteral(";") + WSP).suppress()
    equal = WSP + Literal("=") + WSP

    because of DMARC whitespace #1114, so either this also does not work, or the DMARC code should change Optional to ZeroOrMore.
    BTW I also noted this regular expressions for parsing email addresses:

    # RegEx for parsing email.
    regex_tld = r"(?:[a-zA-Z]{2,63}|xn--[a-zA-Z0-9]+)"
    regex_mailaddr = (
    r"(?P<mailaddr>([a-zA-Z0-9]{0,61}@)?([a-zA-Z0-9]([a-zA-Z0-9\-]{0,61}[a-zA-Z0-9])?\.)+" r"" + regex_tld + ")"
    )

    The weird thing is, this does not require a @, while I think is too restrictive in the mailbox part.
    Note the ABNF parsers are suggested in security.txt ABNF states signed messages must end with only one newline #1475.

  • Would it be an option to also have a feature flag for a sub test, and maybe temporarily hide it by default?
    Currently that's only there for complete checks (not sub tests):

    # Features
    # # Checks
    # # The following flags enable and disable various parts of the checks. This allows for faster, more targeted scanning.
    INTERNET_NL_CHECK_SUPPORT_IPV6 = get_boolean_env("INTERNET_NL_CHECK_SUPPORT_IPV6", True)
    INTERNET_NL_CHECK_SUPPORT_DNSSEC = get_boolean_env("INTERNET_NL_CHECK_SUPPORT_DNSSEC", True)
    INTERNET_NL_CHECK_SUPPORT_MAIL = get_boolean_env("INTERNET_NL_CHECK_SUPPORT_MAIL", True)
    INTERNET_NL_CHECK_SUPPORT_TLS = get_boolean_env("INTERNET_NL_CHECK_SUPPORT_TLS", True)
    INTERNET_NL_CHECK_SUPPORT_APPSECPRIV = get_boolean_env("INTERNET_NL_CHECK_SUPPORT_APPSECPRIV", True)
    INTERNET_NL_CHECK_SUPPORT_RPKI = get_boolean_env("INTERNET_NL_CHECK_SUPPORT_RPKI", True)

    I know there was interest from the Netherlands governmental e-mail community to sign up TLSRPT for the comply-or-explain list. I'm unsure if this is done and what is the current state, at least it's not taken into the procedure yet (because I would be involved with that).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants