Discision on disabling DKIM test too strict

[dennisbaaten]

If the DMARC policy is set to "reject" and if SPF is set to "-all" we disable (grey out) the DKIM test. This is also mentioned in our test explanation.

However, today I saw this test results: https://internet.nl/mail/vraaghetdepolitie.nl/896198/#control-panel-9. Apparently, if the DMARC policy subtest has a warning because the authorization record was not set up correctly for the used ruf/rua domain, the DKIM test is not disabled. This seems a bit too strict.

I consider the absence of an authorization record to be a standalone problem, which should not affect our decision to disable the DKIM test.

[dennisbaaten]

See also: https://github.com/internetstandards/Internet.nl/issues/249

[bwbroersma]

Just noticing the same issue: https://internet.nl/mail/cert-wm.nl/964455/#mailauth which confused me (and another colleague) at first view.

@dennisbaaten: I propose renaming the issue: s/disabling/skipping/, because the naming in the code is 'skip'. BTW the test is always executed, it's only about skipping (graying out) the results if it did not pass.

The relevant code: https://github.com/internetstandards/Internet.nl/blob/3ddbe6aacec748ecc429fe9327b6a954c8686b0f/checks/tasks/mail.py#L117-L136 Is the fix just deleting line 126? https://github.com/internetstandards/Internet.nl/blob/3ddbe6aacec748ecc429fe9327b6a954c8686b0f/checks/tasks/mail.py#L126

BTW I also notices some other issues: These are regex checks, instead of proper parsing: https://github.com/internetstandards/Internet.nl/blob/3ddbe6aacec748ecc429fe9327b6a954c8686b0f/checks/__init__.py#L5-L7 They are wrong because update: ~they imply order of tags~ (there should be order, correctly noted by @gthess and RFC 7489 section 6.4) WSP = SP / HTAB (RFC 5234 - Core Rules) and *WSP in 6.4, but there could be an unnecessary pct=100, of course there could also be a pct=0, which is not checked and then the whole skipping would be invalid. Both issues are already addressed:

• 279

• 532

In trying to trigger an incorrect skip, I noticed it still detected DKIM because I had an * CNAME, which also let's the DKIM pass, filed in:

• 1025

[gthess]

Note for DMARC_NON_SENDING_POLICY_ORG, it is currently not wrong. The order for the DMARC record is v=DMARC1; p=... and then anything else. This is also used with a valid record (passed proper parsing) so the order is enforced at this point.

[bwbroersma]

@gthess thanks, you are correct, I updated my issue to reflect order is required, however regex parsing is tricky, I don't think whitespace is handled correctly, e.g.:

     dmarc-version   = "v" *WSP "=" *WSP %x44 %x4d %x41 %x52 %x43 %x31

     dmarc-sep       = *WSP %x3b *WSP

     dmarc-request   = "p" *WSP "=" *WSP
                       ( "none" / "quarantine" / "reject" )

So whitespace is allowed like v = DMARC1 ; p = reject, does not passes ^v=DMARC1;\ *p=reject;?, or is the dmarc_record the parsed data after whitespace suppression? It seems this whitespace suppress() is not fixing the parsing of my excessive whitespace use (and it seems a lot of DMARC parsers fail on whitespace around the =). https://github.com/internetstandards/Internet.nl/blob/71f97f7072ebeb458d9109b8bf3c8b5ca989c67b/checks/tasks/dmarc_parser.py#L45-L48 Opened a separate issue for this:

• 1114

[bwbroersma]

The DmarcPolicyStatus.valid should not be weakened with the current regular expressions, since otherwise invalidsp= will be matched by .*sp=, now this isn't the case since invalidsp will not result in a valid DmarcPolicyStatus.

[gthess]

I believe you can get the terms/directives from the parsed records and maybe replacing the regular expression with explicit code that looks at what is available will yield better results. Or insert the missing optional whitespaces in the current regular expressions. The latter is easier to do but the former would be easier to read at the end.