internetstandards / Internet.nl

Internet standards compliance test suite
https://internet.nl
171 stars 36 forks source link

Improve null MX recommendation in verdict based on SPF value #989

Open mxsasha opened 1 year ago

mxsasha commented 1 year ago

Building #905, the verdict "you have no mail servers, you should probably have null MX" (info) should only occur when SPF is -all (and no others), i.e. only if SPF confirms no mail is sent from this domain. For any other SPF, the result should be greyed out and verdict should not mention null MX

mxsasha commented 4 months ago

I don't remember the discussion, but while this makes sense to me, it does mean some changes in our DNS lookups:

The status is set in three places, as this affects three test types: https://github.com/internetstandards/Internet.nl/blob/ebfebc5217ecb5df3f7a11c22427442f25415db1/checks/tasks/dnssec.py#L186-L187 https://github.com/internetstandards/Internet.nl/blob/ebfebc5217ecb5df3f7a11c22427442f25415db1/checks/tasks/ipv6.py#L237-L238 https://github.com/internetstandards/Internet.nl/blob/ebfebc5217ecb5df3f7a11c22427442f25415db1/checks/tasks/tls.py#L1100-L1101

The decision is made here, which I think happens once, before the different celery tasks are spawned. https://github.com/internetstandards/Internet.nl/blob/ebfebc5217ecb5df3f7a11c22427442f25415db1/checks/tasks/shared.py#L135

At the decision time, we don't have the SPF records. That's done much later, in a specific celery task. We don't have communication between our checks, so the only way is to make the SPF lookup, parsing and evaluation part of do_mail_get_servers. It's a bit sloppy, because some of that work is repeated later, but it's the only way to support this.

mxsasha commented 4 months ago

This affects only domains with zero MX records. The old behaviour is:

The new behaviour is:

Where no_mx is a neutral opinion, and no_null_mx is a "you should probably be setting null mx" opinion. The content for these labels will probably need to be updated, note that they exist in multiple tests.

mxsasha commented 4 months ago

Change is merged, content update pending, see comment above