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

Some domains are listed as live even when DNS times out #91

Merged

Conversation

jsf9k
Copy link
Member

@jsf9k jsf9k commented Oct 17, 2018

@climber-girl noticed that the output of the following command reports nara-at-home.gov as "live" even though there are no MX records for that domain:

$ trustymail --json --debug --timeout=30 --smtp-timeout=5 --dns=8.8.4.4 nara-at-work.gov

This pull request fixes this issue. Domains for which no MX records exist will no longer be listed as "live".

This pull request is related to NCATS JIRA ticket CYHY-706.

@jsf9k jsf9k self-assigned this Oct 17, 2018
@jsf9k jsf9k requested review from h-m-f-t, KyleEvers, climber-girl and a team October 17, 2018 16:53
@jsf9k
Copy link
Member Author

jsf9k commented Oct 17, 2018

There are a couple of other places where we catch the same exceptions (dns.resolver.NoAnswer and dns.timeout.Exception), but I don't think we want to label the domain as "not live" in those cases. I would be interested to hear others' thoughts, though.

The parts of the code I am talking about are here and here.

Copy link
Member

@dav3r dav3r left a comment

Choose a reason for hiding this comment

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

@jsf9k, this makes sense to me.

@jsf9k
Copy link
Member Author

jsf9k commented Oct 19, 2018

I guess the real question is, what does "live" vs "not live" mean for trustymail? Does it mean we have or don't have an MX record, respectively?

I think it's worth looking at the relevant code in dhs-ncats/trustymail_reporter to see how live vs not live impacts the results that are reported.

I'm very interested to hear what @h-m-f-t thinks about this.

@jsf9k jsf9k merged commit 0bedde8 into develop Oct 22, 2018
@jsf9k jsf9k deleted the bugfix/domains_are_listed_as_live_even_when_dns_times_out branch October 22, 2018 13:01
Copy link
Member

@h-m-f-t h-m-f-t left a comment

Choose a reason for hiding this comment

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

(Recognizing that I missed the period when the PR was open...) I think this is the wrong decision because it obscures the need for DMARC-related action. A domain that exists but doesn't send mail can be trivially spoofed -- and that can be stopped with a strong DMARC policy.

The presence of an MX record means that the domain receives mail. While email communication is typically full-duplex, the absence of email records doesn't mean that legitimate email isn't being send on behalf of the domain.

To me, "live" in trustymail should mean "does the domain exist?". I think the "live" nomenclature was used because that's what it was in pshtt. Maybe exists is a better term?

@jsf9k
Copy link
Member Author

jsf9k commented Oct 22, 2018

I can always revert the PR.

What does "exists" mean here? That the domain is registered? That's not something we check for explicitly in trustymail, although in NCATS' scanning it is always the case because we use current-federal as our list of parent domains.

If this is the definition of exists, then I don't think it makes sense to include live in the trustymail data at all since we aren't checking if it is registered or not.

@h-m-f-t
Copy link
Member

h-m-f-t commented Oct 22, 2018

It's true we're not checking top-level domain whois', but I think that (and welcome further discussion on) a domain not NXDOMAIN'ing or SERVFAILing is a reasonable proxy for existence. It's functionally "live" if the domain resolves in public DNS, and therefore can benefit from DMARC action.

mcdonnnj added a commit that referenced this pull request Jan 23, 2023
mcdonnnj added a commit that referenced this pull request Jan 23, 2023
Lineage pull request for: skeleton
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants