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

Add MTA-STS verification #1556

Closed
wants to merge 16 commits into from
Closed

Add MTA-STS verification #1556

wants to merge 16 commits into from

Conversation

ctrl-i
Copy link
Contributor

@ctrl-i ctrl-i commented Apr 11, 2019

Copy link
Member

@JoshData JoshData left a comment

Choose a reason for hiding this comment

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

Very cool. Thank you.

management/web_update.py Outdated Show resolved Hide resolved
setup/web.sh Outdated Show resolved Hide resolved
setup/web.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@jvolkenant jvolkenant left a comment

Choose a reason for hiding this comment

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

Looks like pip gives the long winded install text, hide_output would be best added here.

setup/mail-postfix.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@jvolkenant jvolkenant left a comment

Choose a reason for hiding this comment

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

tools/editconf.py /etc/postfix/main.cf -s ... leaves postfix unable to restart since
smtp_tls_policy_maps socketmap:inet:127.0.0.1:8461:postfix is added instead of smtp_tls_policy_maps=socketmap:inet:127.0.0.1:8461:postfix -s should be omitted

setup/mail-postfix.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@jvolkenant jvolkenant left a comment

Choose a reason for hiding this comment

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

I get the correct domain name entry with \ removed

setup/mail-postfix.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@jvolkenant jvolkenant left a comment

Choose a reason for hiding this comment

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

mta-sts shell should be /usr/sbin/nologin instead

setup/mail-postfix.sh Outdated Show resolved Hide resolved
@@ -228,6 +228,10 @@ def has_rec(qname, rtype, prefix=None):
defaults = [
(None, "A", env["PUBLIC_IP"], "Required. May have a different value. Sets the IP address that %s resolves to for web hosting and other services besides mail. The A record must be present but its value does not affect mail delivery." % domain),
(None, "AAAA", env.get('PUBLIC_IPV6'), "Optional. Sets the IPv6 address that %s resolves to, e.g. for web hosting. (It is not necessary for receiving mail on this domain.)" % domain),
("mta-sts", "A", env["PUBLIC_IP"], "Required. For MTA-STS verification."),
Copy link
Member

Choose a reason for hiding this comment

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

The descriptions here are for the External DNS page of the control panel and are tips for people managing their own DNS. These should actually say Optional at the start since they are not required for mail deliverability, and they should not that the record should only be used if the HTTP part of the MTA-STS specification is implemented too.

Copy link
Contributor Author

@ctrl-i ctrl-i Apr 15, 2019

Choose a reason for hiding this comment

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

Let me know any further tweaks you would like to the wording here. See 5cb6fcc

I added an additional comment on the _smtp._tls TXT record as this is optional optional, it is not necessary for the MTA-STS to function correctly and does request/cause email reports to be sent to the postmaster@ email account by all mail servers that have connected to the box. These reports might be confusing to some and if you receive email from a large number of servers could result in a large number of additional emails (I have only received these from Google so far as I do not have many contacts).

See: https://www.hardenize.com/blog/smtp-tls-reporting-tls-rpt

# ### MTA-STS - SMTP Mail Transfer Agent Strict Transport Security - SETUP
# See: https://github.com/mail-in-a-box/mailinabox/pull/1556
# create the MTA-STS policy
hide_output mkdir -p /var/lib/mailinabox/
Copy link
Member

Choose a reason for hiding this comment

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

There's no need for hide_output here. mkdir -p never has output.

Copy link
Member

Choose a reason for hiding this comment

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

Let's also clarify in the comment that this part is what announces the local MTA-STS policy to senders and the file is served by nginx on mta-sts subdomains (actually all domains, but that's fine).

Copy link
Contributor Author

@ctrl-i ctrl-i Apr 15, 2019

Choose a reason for hiding this comment

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

There's no need for hide_output here. mkdir -p never has output.

Similarly, is the hide_output in the "hide_output /bin/systemctl start postfix-mta-sts.service" line necessary? It probably is in the "enable" line as this often creates chatter when it is created, I'm not sure what happens if the service would fail to start (starting is silent).

Let me know if you would like any corrections to the additional comment.

See 6f05788 and f7de8eb

setup/mail-postfix.sh Show resolved Hide resolved
EOF
chmod a+r /var/lib/mailinabox/mta-sts.txt

# install the postfix MTA-STS resolver
Copy link
Member

Choose a reason for hiding this comment

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

Let's clarify in the comment here that the MTA-STS resolver service is used by Postfix to ensure outgoing mail uses TLS when the recipient announces MTA-STS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed with 3c3bc5a

@JoshData
Copy link
Member

I left some more comments. This is fantastic work by the way.

# install the postfix MTA-STS resolver. the MTA-STS resolver service is
# used by Postfix to ensure outgoing mail uses TLS when the recipient
# announces MTA-STS.
hide_output /usr/bin/pip3 install postfix-mta-sts-resolver
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this be altered to include an upgrade parameter if postfix-mta-sts-resolver is already installed?

hide_output /usr/bin/pip3 install -U postfix-mta-sts-resolver

@ctrl-i ctrl-i closed this Apr 26, 2019
@JoshData
Copy link
Member

I'm not sure why the PR was closed. It is pretty cool. I have two more comments:

  • We should pin the installation of the Python package postfix-mta-sts-resolver to a particular version number for security and compatibility reasons.
  • I'm not sure if the postfix-mta-sts-resolver config file is necessary. I'm not sure if we want to have the strict_testing directives in it.

@jvolkenant
Copy link
Contributor

jvolkenant commented May 15, 2019

I agree we should have strict_testing disabled as it seems that it is the default.

I wonder about the "zones:" section. I don't understand why myzone is there (or any value really). I've seen a few variations of names in that space on different websites, so I'm not sure if it is supposed to be a domain name or not. I imagine since the settings are the same as the defaults we can omit that section.

Looks like an upcoming version will change the default config location namely Snawoot/postfix-mta-sts-resolver@6dc046c. Keep it in /etc/postfix, move it to /etc/, or even /home/user-data?

@anroots
Copy link

anroots commented Jun 19, 2019

Why was this closed? PR and implementation discussion seemed good to me; and this would be yet another valuable thing to pimp the security of MIAB. Would very much like to see this supported by default.

@ctrl-i
Copy link
Contributor Author

ctrl-i commented Jun 19, 2019

I closed the PR as the discussion had ground to a halt and the questions I asked received no replies for over a week. People are busy and sometimes do not have the time to follow through on their comments which I understand but I needed this implemented on my version of MIAB so I went ahead on my own.

MIAB would rather be stable than introduce new features. This isn't a criticism and it is a valid and understandable choice. I chose instead to fork the project and merge it with some of the many other excellent and working PR that have not been approved in the past (to name but a few quotas, tasks and code style improvements).

This PR works and I have been using it for several months on my own fork of MIAB without any issues so feel free to add it to your own fork too without any problems.

@just4t
Copy link
Contributor

just4t commented Jun 19, 2019

... I chose instead to fork the project and merge it with some of the many other excellent and working PR that have not been approved in the past ...

  • That's exactly the single way we have available now knowing how busy is the main MiaB owner/ developer (still closed to accept changes or new features because he's scary about the impact of additional post maintenace) :(

» If you're having special needs I'm sincerely recommending you to fork the official 'master' branch and build you needed custom version on top of it ;)

@jpoirierlavoie
Copy link

It's a genuine shame the PR was closed, this would have been an invaluable standard to achieve. I'm hoping this contribution will eventually find its way into future versions of MIAB.

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.

6 participants