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

null mx record check #22

Merged
merged 1 commit into from Oct 16, 2022
Merged

Conversation

bmcculley
Copy link
Contributor

To address issue #10 this adds the ability to check for a null MX record.
It also adds a new error code NULL_MX_RECORD.

@bmcculley bmcculley mentioned this pull request Oct 14, 2022
Copy link
Owner

@michaelherold michaelherold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We definitely need test coverage for this. I'm pretty sure the implementation is incomplete as well.

Please take a look at my comments below.

# value. If it's value is 0 it should return null MX.
if len(records) > 0:
for rdata in records:
if rdata.preference == 0:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: Don't we need to check for a zero length (i.e. .) record in addition to the zero preference? My reading of Section 3 of the RFC says yes. Particularly combined with the SMTP fallback behavior of, "If an empty list of MXs is returned, the address is treated as if it was associated with an implicit MX RR, with a preference of 0, pointing to that host," and the examples at the end of RFC 974, it seems like zero does not imply by itself that it's a null MX record.

A test of the behavior would be good to show that it works as intended.

@bmcculley
Copy link
Contributor Author

Thank you @michaelherold after reading the RFC I agree with your comments above. Further it is now my understanding that a Null MX record should also only consist of a single MX record with 0 preference and an empty label.

I have updated the code accordingly and added some tests to verify that it works as intended. Please review and let me know your thoughts.

Copy link
Owner

@michaelherold michaelherold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good work, thanks! CI is failing due to an old version of Black not having the proper constraints. I will rebase and get this merged and released!

@@ -21,4 +21,5 @@ class DNSDiagnosis(BaseDiagnosis):
"Couldn't find an MX record for this domain " "but an A record does exist."
),
"NO_RECORD": "Couldn't find an MX record or A record for this domain.",
"NULL_RECORD": "Domain does not support email service."
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a small issue here that I will fix when rebasing. This needs to match the key above.

To address issue michaelherold#10 this adds the ability to check for a null MX
record, as defined by RFC 7505 section 3.

Also added tests to verify that it is working correctly.
@michaelherold michaelherold linked an issue Oct 16, 2022 that may be closed by this pull request
@michaelherold michaelherold merged commit 1de6b7c into michaelherold:master Oct 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Null MX validation
2 participants