Added support for STARTTLS. #8

jsf9k · 2017-10-03T21:55:36Z

This PR pertains to issue #5.

The CSV output now includes ~~two~~several additional columns. "Mail Server Ports Tested" contains a comma-delimited list of the ports that were tested. "Mail Server Is Listening" states whether a given mail server (which comes from an MX record) and port combination is listening for TCP connections.
"Mail Server Supports SMTP", states whether a given mail server and port combination really appears to be an SMTP server. "Mail Server Supports STARTTLS" indicates whether the mail server and port combination supports STARTTLS. I've run most of current-federal.csv though the code, and it appears to be working.

The DNS timeout serves double duty as the SMTP connection timeout
value as well.

The output now includes two additional columns. "Sends Mail" states whether a given mail server (which comes from an MX record) and port combination really appears to be an SMTP server. "Supports STARTTLS" indicates whether the mail server and port combination supports STARTTLS. I've run current-federal.csv though the code, and it appears to be working. The DNS timeout serves double duty as the SMTP connection timeout value as well.

jsf9k · 2017-10-03T21:57:18Z

@h-m-f-t, I went ahead and created the PR. If we uncover any issues, desired changes, etc. we can continue committing them to my branch and they will be added to the PR.

h-m-f-t

Excellent work, @jsf9k!

I've added a few comments; happy to talk through these some more.

h-m-f-t · 2017-10-08T04:09:21Z

trustymail/trustymail.py

+            # Now try to say hello.  This will tell us if the thing
+            # that is listening is an SMTP server.
+            try:
+                smtp_connection.ehlo_or_helo_if_needed()


The docs for ehlo_or_helo_if_needed() indicate it calls ehlo()/helo(), which has name as a parameter. This parameter "defaults to the fully qualified domain name of the local host", which, in our and an arbitrary user's case, seems liable to be an .in-addr.arpa address or just the IP.

Per the 6th and 7th paragraphs of section 4.1.4. of RFC 5321, the current SMTP standard, this looks to be ok (if an address doesn't reverse to be an FQDN, SMTP will use the raw address). Perhaps we should run a few tests and see if hardcoding (or making configurable) an EHLO name (mail.something.gov) makes any difference in our results?

I will add a flag for setting the name. By default it will work as it does now, but if you specify the name it will be used instead.

h-m-f-t · 2017-10-08T04:24:35Z

trustymail/trustymail.py

+
+            # We were able to connect to the server and say hello, so
+            # it must send mail.
+            domain.sends_mail.append(server_and_port + "-" + str(True))


We may want to play with the verbiage here; "sends mail" may be fudgy. I don't necessarily think that technical correctness should go ahead of user friendliness, but just that "sends mail" may be too general.

Port 25 is used for mail transmission while port 587 is for mail submission (as is port 465, unofficially). The standards track RFC (4409) that defines mail submission, and especially the BCP on mail submission, describe how mail transit and submission are different. Indeed, the STARTTLS RFC calls out the submission port as being different from (and benefitting especially because) STARTTLS is operating on the submission port.

How about "supports SMTP"?

👍 yep, that works.

h-m-f-t · 2017-10-08T04:25:06Z

trustymail/trustymail.py

    "SPF Record", "Valid SPF", "SPF Results",
    "DMARC Record", "Valid DMARC", "DMARC Results",
    "DMARC Record on Base Domain", "Valid DMARC Record on Base Domain",
    "DMARC Results on Base Domain", "DMARC Policy",
    "Syntax Errors"
 ]

+# The ports that will be checked to see if an SMTP server is listening
+SMTP_PORTS = [25, 465, 587]


I think we need data from a full run on port 465. I'm of a mind not to include in our ports definition unless deployment is really strong in the federal space.

I will leave this as it is for now, and we can look at the results to determine if 465 should be removed or not. I will also create a flag to override the default ports with a comma-separated list of ports.

h-m-f-t · 2017-10-08T04:32:58Z

trustymail/domain.py

+
+    def has_starttls(self):
+        """
+        Returns True if STARTTLS information is present and otherwise


I think Has STARTTLS should be a Boolean indicating True when [at least one port? all ports?] use STARTTLS. The raw findings should probably be called something different than Supports STARTTLS; the nomenclature other fields in trustymail follow for this type of output is "x Results", like STARTTLS Results .

That was exactly my confusion. Should "Has STARTTLS" be true if the IP listens on 25 and 587 but only supports STARTTLS on 25?

I'm happy to split "Has STARTTLS" into a Boolean field and a results field, but I need a little guidance on what to do when one port supports STARTTLS but another (active) port does not.

options * Created a command-line flag for specifying the FQDN to submit to SMTP servers

* Added caching of SMTP results, which can be turned off via a command-line option. * Added more specific CSV output fields. * Edited Domain class to generate SMTP-related CSV output fields from an internal dictionary. The separate text fields with the server and port information were becoming too unwieldy.

jsf9k · 2017-10-10T18:21:33Z

@h-m-f-t, I think the changes I just committed speak to your comments. Let me know if you have any questions or issues, and let me know if you want to remove port 465 from the defaults.

Just realized that I failed to update the README. I'm doing that now and will commit soon.

feature/check_for_starttls and resolving conflicts.

h-m-f-t

Thanks @jsf9k, and sorry for my slowness in reviewing. I'm running a full current-federal test with this code and may have more feedback then, though it'd mostly be UX and not functionality.

h-m-f-t · 2017-10-23T21:48:13Z

trustymail/domain.py

@@ -90,6 +116,10 @@ def generate_results(self):

                        "MX Record": self.has_mail(),
                        "Mail Servers": self.format_list(self.mail_servers),
+                        "Mail Server Ports Tested": self.format_list(list(map(lambda x:str(x), self.ports_tested))),
+                        "Mail Server Is Listening": self.format_list(list(map(lambda x:x + ";" + str(self.starttls_results[x]["is_listening"]), self.starttls_results.keys()))),


If we're really interested in SMTP support, what advantage does knowing Mail Server Is Listening bring? Does this value need to be exposed in the output?

It doesn't have to be exposed. I just thought it may be useful to know whether something is actually listening in some edge cases. I'm fine removing it if you prefer that. Just confirm and I'll make the change.

@h-m-f-t, did you decide that this change is something you want? If so I can make the change.

h-m-f-t · 2017-10-23T21:49:08Z

trustymail/domain.py

@@ -90,6 +116,10 @@ def generate_results(self):

                        "MX Record": self.has_mail(),
                        "Mail Servers": self.format_list(self.mail_servers),
+                        "Mail Server Ports Tested": self.format_list(list(map(lambda x:str(x), self.ports_tested))),
+                        "Mail Server Is Listening": self.format_list(list(map(lambda x:x + ";" + str(self.starttls_results[x]["is_listening"]), self.starttls_results.keys()))),
+                        "Mail Server Supports SMTP": self.format_list(list(map(lambda x:x + ";" + str(self.starttls_results[x]["supports_smtp"]), self.starttls_results.keys()))),


Thoughts on only includes those hostnames/ports that return as True?

Do you mean something like this?

"Mail Server Supports SMTP": "blah.foo.com:467,mail.blah.foo.com:467"

I'm fine with that. Just confirm and I'll make the change.

@h-m-f-t, did you decide that this change is something you want? If so I can make the change.

h-m-f-t · 2017-10-23T21:49:17Z

trustymail/domain.py

+                        "Mail Server Ports Tested": self.format_list(list(map(lambda x:str(x), self.ports_tested))),
+                        "Mail Server Is Listening": self.format_list(list(map(lambda x:x + ";" + str(self.starttls_results[x]["is_listening"]), self.starttls_results.keys()))),
+                        "Mail Server Supports SMTP": self.format_list(list(map(lambda x:x + ";" + str(self.starttls_results[x]["supports_smtp"]), self.starttls_results.keys()))),
+                        "Mail Server Supports STARTTLS": self.format_list(list(map(lambda x:x + ";" + str(self.starttls_results[x]["starttls"]), self.starttls_results.keys()))),


Same comment as above.

IanLee1521

I'm relatively new to this code base, but left just a few comments.

Personally, I don't make much use of lambda functions in much of my coding, but seems like they are getting some heavier use here. I might argue that it is simpler / easier to read without them, but that's just me.

IanLee1521 · 2017-10-23T22:18:21Z

trustymail/cli.py

+        smtp_localhost = None
+
+    if args["--smtp-ports"] is not None:
+        smtp_ports = list(map(lambda x:int(x), args["--smtp-ports"].split(',')))


Forgive my new-ness to lambda's... Is this taking the arguments to the --smtp-ports options and returning a list of those numbers as ints? If so, perhaps a list comprehension is clearer?

smtp_ports = [int(port) for port in args['--smtp-ports'].split(',')]

Yes, that's all it's doing. I will change it to a list comprehension.

IanLee1521 · 2017-10-23T22:23:26Z

trustymail/trustymail.py

    if scan_types["spf"] and domain.is_live:
        spf_scan(domain)

    if scan_types["dmarc"] and domain.is_live:
        dmarc_scan(domain)

    # If the user didn't specify any scans then run a full scan.
-    if not (scan_types["mx"] or scan_types["spf"] or scan_types["dmarc"]):
+    if not (scan_types["mx"] or scan_types["starttls"] or scan_types["spf"] or scan_types["dmarc"]):


Not really related to the change, but should there also be a condition here like above for ... and domain.is_live ?

Good question. @h-m-f-t?

@IanLee1521, yeah, I think we do want that to add that! If the domain doesn't exist (isn't live), there's no reason to run any record- or service-specific scans on it.

Server Ports Tested" output field.

specify any scans (so all scans are run).

IanLee1521

LGTM, nothing major, some minor comments though.

IanLee1521 · 2017-10-27T15:00:46Z

trustymail/trustymail.py

+    for mail_server in domain.mail_servers:
+        for port in smtp_ports:
+            if port not in domain.ports_tested:
+                domain.ports_tested.append(port)


I wonder if it would be clearer to have domain.ports_tested be a set rather than a list and then you could just do: domain.ports_tested.add(port) and you don't need to do the conditional check of if it's in there.

IanLee1521 · 2017-10-27T15:29:09Z

trustymail/cli.py

@@ -26,6 +36,9 @@

 base_domains = {}

+# The default ports to be checked to see if an SMTP server is listening.
+_DEFAULT_SMTP_PORTS = [25, 465, 587]


See my other comment, but maybe this should be a set rather than a list?

_DEFAULT_SMTP_PORTS = {25, 465, 587}

IanLee1521 · 2017-10-27T15:29:38Z

trustymail/cli.py

+        smtp_localhost = None
+
+    if args["--smtp-ports"] is not None:
+        smtp_ports = [int(port) for port in args['--smtp-ports'].split(',')]


Same:

smtp_ports = {int(port) for port in args['--smtp-ports'].split(',')}

to check for uniqueness before adding new elements.

jsf9k · 2017-10-27T16:43:50Z

Great suggestions as usual, @IanLee1521!

h-m-f-t · 2017-10-27T21:43:57Z

@jsf9k @IanLee1521 excellent additions! I really appreciate your efforts.

I did a full run on current-federal with all the defaults, and I think the thing that's most difficult from a usability standpoint is the representation of the "Mail Server *" fields. It's hard to get an at-a-glance view of whether a Mail Server is Listening or Supports STARTTLS or Supports SMTP. I totally get this nuance is inherent in us checking multiple ports, but wonder if maybe we should "arbitrarily define" what those fields mean, making them a clear Boolean, and defining them away in the README as "Supports..." means that the mail server is responsive on a given port", with the actual Results in a separate field. This not only makes the tool more user-friendly, it would make our report-generation code need to parse less.

konklone · 2017-10-27T21:49:00Z

Since any run of pshtt will have the same ports tested for each row, and the hostnames won't differ, how about instead of a Mail Server Ports Tested field and a Mail Server Is Listening field, have one field with a value of whatever ports the hostname is listening on? So the value could just be 25,465 or 465.

jsf9k · 2017-10-27T21:50:05Z

@h-m-f-t, I liked your previous suggestion of only including those entries that end up being true. So Mail Server Supports SMTP would look like blah.blah.com:25, blah.blah.com:457, for example. I feel like reducing that to just a single Boolean value throws away a lot of information that in many cases would be useful to the user.

That said, I'll agree to whatever the group decides.

jsf9k · 2017-10-27T21:54:23Z

@konklone, a single domain can have multiple MX records. gmail.com, for instance, returns five of them. Each MX record is a potential mail server hostname. I think we have to include the hostname + port combinations since there can be more than one mail server hostname per row.

boolean flag indicating whether they are listening, support SMTP, and/or support STARTTLS. Now we omit the boolean flag and only list the hostname + port combinations for which the flag would have been true.

jsf9k · 2017-11-01T19:13:19Z

I went ahead and implemented @h-m-f-t's original suggestion. I also fixed a minor bug where the in-memory cache was being built (but not used) even if the user did not want to use the cache.

true if and only if: 1. At least one mail server supports SMTP 2. All mail servers that support SMTP also support STARTTLS

jsf9k · 2017-11-03T17:11:42Z

After discussion with @h-m-f-t, I added an extra boolean field to indicate the domain's overall compliance with STARTTLS.

h-m-f-t

@jsf9k Last request!, than let's merge this and work on including this data in our formal reports.

h-m-f-t · 2017-11-08T08:24:44Z

README.md

 * `Mail Servers` - The list of hosts found in the MX record.
+* `Mail Server Ports Tested` - A list of the ports tested for SMTP and
+  STARTTLS support.
+* `Mail Server Is Listening` - If the mail servers are actually


What does Mail Server Is Listening provide that Mail Server Supports SMTP doesn't?

In order to try to speak SMTP to a host on a given port I have to first connect via TCP. If that succeeds then I know something is listening at that host on that port, whether it's smtp, telnet, ssh, or something else. Since I get that information for free I was including it, but I could just as easily not record it.

I can imagine cases where the information is useful, but it may be more confusing to leave it in. In any event, the same information is duplicated (and expanded upon) in the CYHY report.

We can change our mind, but I agree dropping it makes sense for now.

h-m-f-t · 2017-11-08T08:28:18Z

README.md

+  STARTTLS support.
+* `Mail Server Is Listening` - If the mail servers are actually
+  listening on the tested ports.
+* `Mail Server Supports SMTP` - If the mail servers support SMTP.


I propose that the field Mail Server Supports SMTP become Boolean (true if at least one domain on the MX record "speaks" SMTP), with the current Mail Server Supports SMTP becoming Mail Server Supports SMTP Results.

Maybe it makes sense to rename both fields from Mail Server Supports... to Domain Supports..., since it's semantically odd for a *Mail Server* Supports SMTP to be false (for a "mail server" to not offer SMTP).

@jsf9k last thing: let's put the ...Supports Boolean before the ...Supports Results field, which is helpful from a human-using-the-tool-output use case.

h-m-f-t · 2017-11-08T08:33:50Z

README.md

+* `Mail Server Is Listening` - If the mail servers are actually
+  listening on the tested ports.
+* `Mail Server Supports SMTP` - If the mail servers support SMTP.
+* `Mail Server Supports STARTTLS` - If the mail servers support


Same comments as above. Mail Server Supports STARTTLS becomes Boolean (true if only all SMTP-offering endpoints enable STARTTLS, and Mail Server Supports STARTTLS Results` listing the results.

Also, same comment that maybe Domain Supports... is better language here, too.

* Renamed "Mail Server Supports SMTP" field to "Domain Supports SMTP Results". * Renamed "Mail Server Supports STARTTLS" field to "Domain Supports STARTTLS Results". * Created a new Boolean field called "Domain Supports SMTP" that is true if and only if at least one mail server/port combination supports SMTP. * Updated the documentation.

file when viewed in plain text

jsf9k · 2017-11-08T14:53:56Z

@h-m-f-t, if you're happy with the changes I made go ahead and merge.

... Results" columns

h-m-f-t

💯 let's do this

jsf9k · 2017-11-08T21:27:05Z

Yahoo! 🤘

IanLee1521 · 2017-11-09T15:00:01Z

trustymail/domain.py

@@ -83,30 +109,42 @@ def get_dmarc_policy(self):


    def generate_results(self):
+        mail_servers_that_are_listening = [x for x in self.starttls_results.keys() if self.starttls_results[x]["is_listening"]]


FYI -- Apparently mail_servers_that_are_listening isn't actually being used.

Good catch, @IanLee1521! We removed that field from the output yesterday. I'll create a new pull request for this.

Found it while flake8-ifying the code. Will be opening that PR in a bit.

Done in #20

Improvement bandit config

…es_from_skeleton_generic Pull in upstream changes from skeleton-generic

h-m-f-t requested changes Oct 8, 2017

View reviewed changes

jsf9k added 3 commits October 10, 2017 11:19

* Sepatated the DNS and SMTP timeouts into separate command-line

fae9641

options * Created a command-line flag for specifying the FQDN to submit to SMTP servers

* Renamed the "Sends Mail" field to "Supports SMTP".

72addca

jsf9k added 2 commits October 10, 2017 14:26

Edited README.md to agree with today's changes.

be9b7e2

Merge remote-tracking branch 'origin/master' into

fb65050

feature/check_for_starttls and resolving conflicts.

h-m-f-t requested changes Oct 23, 2017

View reviewed changes

IanLee1521 reviewed Oct 23, 2017

View reviewed changes

jsf9k added 3 commits October 23, 2017 19:03

Converted some lambda statements into equivalent list comprehensions.

31e4e82

Fixed a bug where ports could appear multiple times in teh "Mail

90d9ea8

Server Ports Tested" output field.

Added an is_live check for the default case where the user does not

0ffa888

specify any scans (so all scans are run).

IanLee1521 reviewed Oct 27, 2017

View reviewed changes

Changed a few variables from lists to sets. This means I don't have

277e4b2

to check for uniqueness before adding new elements.

jsf9k added 2 commits November 1, 2017 15:08

Only build up the cache object if the user wants to use it.

2c7e8d0

We used to list all possible hostname + port combinations with a

0818a80

boolean flag indicating whether they are listening, support SMTP, and/or support STARTTLS. Now we omit the boolean flag and only list the hostname + port combinations for which the flag would have been true.

Added a boolean field called Domain Supports STARTTLS. This field is

24a3792

true if and only if: 1. At least one mail server supports SMTP 2. All mail servers that support SMTP also support STARTTLS

h-m-f-t requested changes Nov 8, 2017

View reviewed changes

jsf9k added 2 commits November 8, 2017 09:36

No real changes, just improvements to the fromatting of the README

2141218

file when viewed in plain text

Switched the order of the "Domain Supports..." and "Domain Supports

dec079c

... Results" columns

jsf9k self-assigned this Nov 8, 2017

h-m-f-t approved these changes Nov 8, 2017

View reviewed changes

h-m-f-t merged commit aef9d51 into cisagov:master Nov 8, 2017

h-m-f-t mentioned this pull request Nov 8, 2017

Check for STARTTLS #5

Closed

jsf9k deleted the feature/check_for_starttls branch November 8, 2017 21:26

IanLee1521 reviewed Nov 9, 2017

View reviewed changes

mcdonnnj pushed a commit that referenced this pull request Jan 23, 2023

Merge pull request #8 from cisagov/improvement-bandit-config

91b5ccd

Improvement bandit config

mcdonnnj pushed a commit that referenced this pull request Jan 23, 2023

Merge pull request #8 from cisagov/improvement/pull_in_upstream_chang…

bba7bba

…es_from_skeleton_generic Pull in upstream changes from skeleton-generic

		@@ -83,30 +109,42 @@ def get_dmarc_policy(self):


		def generate_results(self):
		mail_servers_that_are_listening = [x for x in self.starttls_results.keys() if self.starttls_results[x]["is_listening"]]

Added support for STARTTLS. #8

Added support for STARTTLS. #8

Conversation

jsf9k commented Oct 3, 2017 • edited Loading

jsf9k commented Oct 3, 2017

h-m-f-t left a comment

Choose a reason for hiding this comment

h-m-f-t Oct 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h-m-f-t Oct 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsf9k commented Oct 10, 2017 • edited Loading

h-m-f-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsf9k Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsf9k Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IanLee1521 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IanLee1521 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsf9k commented Oct 27, 2017

h-m-f-t commented Oct 27, 2017

konklone commented Oct 27, 2017

jsf9k commented Oct 27, 2017 • edited Loading

jsf9k commented Oct 27, 2017

jsf9k commented Nov 1, 2017

jsf9k commented Nov 3, 2017

h-m-f-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsf9k Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

h-m-f-t Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsf9k commented Nov 8, 2017

h-m-f-t left a comment

Choose a reason for hiding this comment

jsf9k commented Nov 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsf9k commented Oct 3, 2017 •

edited

Loading

h-m-f-t Oct 8, 2017 •

edited

Loading

h-m-f-t Oct 8, 2017 •

edited

Loading

jsf9k commented Oct 10, 2017 •

edited

Loading

jsf9k Oct 23, 2017 •

edited

Loading

jsf9k Oct 23, 2017 •

edited

Loading

jsf9k commented Oct 27, 2017 •

edited

Loading

jsf9k Nov 8, 2017 •

edited

Loading

h-m-f-t Nov 8, 2017 •

edited

Loading