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

feat: Add MTA-STS support for outbound mail #3592

Merged
merged 23 commits into from
Jan 13, 2024

Conversation

jsonn
Copy link
Contributor

@jsonn jsonn commented Oct 21, 2023

Description

Add optional MTA-STS support for outgoing mails. Domains can signal STARTTLS
support out-of-band using a combination of DNS and HTTPS to prevent
man-in-the-middle attacks that filter out the feature.

Fixes #3590

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (README.md or the documentation under docs/)
  • If necessary I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Copy link
Member

@polarathene polarathene left a comment

Choose a reason for hiding this comment

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

While I'd like to see a complimentary test (eg: as described here), this is not an easy ask due to needing a DNS service setup if we want to avoid external network requests.

We could probably still implement the postmap query against gmail? Trusted delivery verification on the logs won't be viable though.


The PR is fine as-is, but should additionally update the ENV docs page, and reference the DANE issue for awareness in case any users have configured that.


function _setup_mta_sts() {
_log 'trace' 'Adding MTA-STS lookup to the Postfix TLS policy map'
postconf 'smtp_tls_policy_maps = socketmap:inet:127.0.0.1:8461:postfix'
Copy link
Member

Choose a reason for hiding this comment

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

Your feature request issue noted that a separate container was "messy" to support?


What is the messy part requiring this in DMS?

I'm assuming it's container to container connection as unlike the port publishing from the endorsed example, you'd not have the port mapped to an interface reachable outside of the container? (IIRC, I think I've run into that networking problem in the past)

If so then the messy part is just providing your own config that adjusts the daemon config for host?

Copy link
Member

Choose a reason for hiding this comment

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

Additionally, we could use a unix socket here instead with the daemons config if we were to accept it. The only benefit of the TCP socket is using the defaults (EDIT: it seems there may be some ownership issues with a unix socket 🤷‍♂️ ).

For reference the socketmap table config is using :postfix in the name parameter which I assume is expected by the daemon service.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's messy as it requires inter-dependencies between containers. For something that I would consider an important security property, this problematic. I don't have a strong preference for communication locally via TCP oder UDS.

I'm still looking into the best option for testing. It seems to be a pity that there currently is no easy test domain in the wild and I'm trying to reach out to folks in the hope of something being willing to host one.

Copy link
Member

@polarathene polarathene Oct 21, 2023

Choose a reason for hiding this comment

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

It's not too difficult to do this all locally, we don't need an external test domain.

It's just something I've been putting off until the test suite is refactored more to switch to docker compose as I can more easily manage flexible configurations.


If you'd like to implement it, I'll allow that.

  • Use a Caddy container to host the policy file. We can use our existing local testing certs, Caddy needs to be provided these along with a little config.
  • Use a CoreDNS container with a BIND file zone to handle the DNS records.
  • Ensure that DMS has the CA root cert in the trust store for verifying trust against the Caddy container.
  • I'm not sure if submitting mail to be sent to a domain the DMS container manages itself will work for the logs, it might optimize that delivery if it recognizes that it loops back into itself. That may require another DMS instance, but I don't think we're setup for that supporting that in tests yet 🤔(maybe)

I have familiarity with all of the above, so could probably contribute the Caddy + CoreDNS setup via a separate PR, then you could rebase to leverage that. I'll let you know if I get that handled, if you know of a better way I'm open to ideas :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main reason I would like to see a test domain exist is to give the user a way to test the correct behavior on their own. E.g. for DKIM there are validation services and the report functions. Without this testing, #3593 might have gone undetected for a while. I was just trying to resist the setup so far :)

Copy link
Member

Choose a reason for hiding this comment

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

I have a working local offline setup with caddy + coredns to resolve the lookup via mta-sts-query from the package. Uses the test CA + cert I linked for TLS.

A user could setup the same locally with two DMS instances to test setup is correct, then after that is successful use real public DNS + certs 🤷‍♂️

I'll put a test together tomorrow to handle the sending and logs verification.


Our test suite is slowly getting better, once it adopts the local DNS setup more broadly it can have better test coverage to catch issues like you've linked, assuming contributors have time to write the tests 😅

The main reason I would like to see a test domain exist is to give the user a way to test the correct behavior on their own.
E.g. for DKIM there are validation services and the report functions.

I can't assist much with public validation concern, but I think being able to have an offline setup is a useful tool for getting confidence in getting everything working for new users :)

mailserver.env Outdated Show resolved Hide resolved
@polarathene polarathene added kind/new feature A new feature is requested in this issue or implemeted with this PR service/postfix area/security area/features area/networking labels Oct 21, 2023
@polarathene polarathene added this to the v13.0.0 milestone Oct 21, 2023
@georglauterbach
Copy link
Member

georglauterbach commented Oct 23, 2023

Just FYI: This PR and #3593 are the last blockers for v13.0.0.

UPDATE: This and #3599 are the last blockers (#3599 is a pseudo-folloup on #3593)

@georglauterbach georglauterbach self-requested a review October 23, 2023 19:23
Copy link
Member

@polarathene polarathene left a comment

Choose a reason for hiding this comment

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

I'll apply these.

docs/content/config/advanced/mail-mta-sts.md Outdated Show resolved Hide resolved
docs/content/config/advanced/mail-mta-sts.md Outdated Show resolved Hide resolved
docs/content/config/advanced/mail-mta-sts.md Outdated Show resolved Hide resolved
docs/content/config/advanced/mail-mta-sts.md Outdated Show resolved Hide resolved
docs/content/config/advanced/mail-mta-sts.md Outdated Show resolved Hide resolved
docs/content/config/environment.md Outdated Show resolved Hide resolved
mailserver.env Outdated Show resolved Hide resolved
target/scripts/startup/setup.d/mta_sts.sh Outdated Show resolved Hide resolved
target/scripts/startup/setup.d/mta_sts.sh Outdated Show resolved Hide resolved
@polarathene polarathene changed the title feat: add support for MTA-STS for outgoing mails feat: Add MTA-STS support for outbound mail Oct 24, 2023
Copy link
Member

@polarathene polarathene left a comment

Choose a reason for hiding this comment

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

The docs contribution is quite nice, thanks for tackling that! I revised it a little bit and removed the line length limit (a11y concern).

These remaining changes are for you to handle:

  • Update frontmatter title + add setting to hide ToC
  • Rename docs file and relocate it to Best Practices section.
  • Update references to the new location.
  • Add an entry to the docs config mkdocs.yml for the page to be accessible.
  • Rename startup script to use kebab-case.

docs/content/config/advanced/mail-mta-sts.md Outdated Show resolved Hide resolved
docs/content/config/environment.md Outdated Show resolved Hide resolved
mailserver.env Show resolved Hide resolved
target/scripts/startup/setup.d/mta_sts.sh Outdated Show resolved Hide resolved
docs/mkdocs.yml Outdated Show resolved Hide resolved
polarathene
polarathene previously approved these changes Oct 24, 2023
Copy link
Member

@polarathene polarathene left a comment

Choose a reason for hiding this comment

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

LGTM 👍

This can be merged or wait until a test is ready.

@reneploetz
Copy link
Contributor

reneploetz commented Oct 24, 2023

If I read the config correctly, the resolver will be backed by an sqlite database in /var/lib/mta-sts/cache.db. Does it make sense to move that file to our general mail state directory?

Also, the debian bullseye version is 1.0.0-4. Upstream has 1.4.0, though apart from documentation there is not much change that seems relevant to us. This issue seems to be the only relevant one fixed apart from the ability to run as a specific user (and not root): Snawoot/postfix-mta-sts-resolver#90

@jsonn
Copy link
Contributor Author

jsonn commented Oct 24, 2023

I've pinged the Debian maintainer.

Copy link
Member

@georglauterbach georglauterbach left a comment

Choose a reason for hiding this comment

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

Approving here as well as changes look good to me 👍🏼

@polarathene
Copy link
Member

Does it make sense to move that file to our general mail state directory?

We could, not too important as it's just cache?

If @jsonn thinks it'd be good to persist, this is what you need to do for that support:

Add the conditional ENV here:

# Only consolidate state for services that are enabled
# Notably avoids copying over 200MB for the ClamAV database
[[ ${ENABLE_AMAVIS} -eq 1 ]] && SERVICEDIRS+=('lib/amavis')

next is restoring ownership since the UID/GID assigned aren't necessarily stable across upgrades, you'd need to know what the original ownership is:

_log 'trace' "Fixing ${STATEDIR}/* permissions"
[[ ${ENABLE_AMAVIS} -eq 1 ]] && chown -R amavis:amavis "${STATEDIR}/lib-amavis"

That should be it :)

@jsonn
Copy link
Contributor Author

jsonn commented Oct 25, 2023

I don't think the cache is too important for now, but I will look at it as follow up.

Related question: would documentation and configuration for DANE be useful as well? The world can't agree on a single scheme after all.

@polarathene
Copy link
Member

polarathene commented Oct 25, 2023

Related question: would documentation and configuration for DANE be useful as well? The world can't agree on a single scheme after all.

I rarely see interest in it expressed by users here, and last I recall there were issues with adoption or something with DANE?

You're welcome to contribute docs for that if you like, maybe it'll help our users adopt it, especially if they're not aware or feel uneasy with third-party advice? (as in resources not endorsed by our own docs) 🤷‍♂️

@georglauterbach
Copy link
Member

What's the status here?

@georglauterbach georglauterbach removed the meta/feature freeze On hold due to upcoming release process label Nov 26, 2023
@github-actions github-actions bot added the meta/stale This issue / PR has become stale and will be closed if there is no further activity label Dec 17, 2023
@williamdes williamdes removed the meta/stale This issue / PR has become stale and will be closed if there is no further activity label Dec 17, 2023
@georglauterbach georglauterbach added the meta/feature freeze On hold due to upcoming release process label Dec 23, 2023
@georglauterbach georglauterbach modified the milestones: v13.1.0, v14.0.0 Dec 29, 2023
@georglauterbach georglauterbach removed the meta/feature freeze On hold due to upcoming release process label Dec 29, 2023
@docker-mailserver docker-mailserver deleted a comment from github-actions bot Jan 3, 2024
@polarathene
Copy link
Member

I've troubleshooted the test failure. A separate PR will resolve that by changing the test. Then this PR can be merged 👍

Copy link
Member

@polarathene polarathene left a comment

Choose a reason for hiding this comment

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

I'll apply this.

target/supervisor/conf.d/supervisor-app.conf Outdated Show resolved Hide resolved
The python3 shebang will run it, which will now meet the expectations of the process testing via pgrep. fail2ban has the same approach.
@polarathene
Copy link
Member

This just needs a changelog entry added under the features section.

I'll add within 12 hours time if this isn't handled when I return 👍


While I'd like the improved testing (dependent upon compose.yaml + DNS), I'm presently short on time to handle that and rather see this feature merged.

Thanks for contributing this new feature! 😁

@jsonn
Copy link
Contributor Author

jsonn commented Jan 12, 2024

Feel to adjust. The indentation of the block looks off?

While this feature in DMS supports ensuring STARTTLS is used when mail is sent to another mail server, you may setup similar for mail servers sending mail to DMS.

This requires configuring your DNS and hosting the MTA-STS policy file via a webserver. A good introduction can be found on [dmarcian.com](https://dmarcian.com/mta-sts/).

Copy link
Contributor

@williamdes williamdes Jan 12, 2024

Choose a reason for hiding this comment

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

just in case it is interesting to document, here is my very simple nginx no files config

server {
    listen 80 default_server;
    listen [::]:80 default_server;
    server_name _;
    server_tokens off;
    autoindex off;

    location /robots.txt {
        return 200 "User-agent: *\nDisallow: /";
    }

    location / {
        return 204 "";
    }

    # 86400 = 1d
    location /.well-known/mta-sts.txt {
        return 200 "version: STSv1\nmode: testing\nmx: mx1.mails.wdes.eu\nmx: mx2.mails.wdes.eu\nmx: mx3.mails.wdes.eu\nmx: mx4.mails.wdes.eu\nmax_age: 86400\n";
    }
}

With DNS config:

  • _mta-sts.<domain>. TXT: v=STSv1; id=<integer-mta-sts-revision>

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also recommend tools like

And nice CLI to perform tests yourself: https://github.com/PennockTech/smtpdane

Copy link
Member

Choose a reason for hiding this comment

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

just in case it is interesting to document

A docs contribution for handling this portion would be good in the meantime yes.

Web server for policy file

I find Caddy is nicer than Nginx for such configuration, here's the Caddyfile config:

# MTA-STS needs to serve the policy for supported mail domains:
mta-sts.example.test {
  tls /dms/certs/cert.ecdsa.pem /dms/certs/key.ecdsa.pem
  root * /dms/files
  file_server
}

The mta-tsts.txt file to serve, this can be inlined into the Caddyfile with it's HereDoc support to respond with instead.

version: STSv1
mode: testing
mx: mail.example.test
max_age: 604800
services:
  # Serves the MTA-STS policy:
  web:
    image: caddy:2.7
    container_name: mail-web
    volumes:
      - ./Caddyfile:/etc/caddy/Caddyfile:ro
      - ./mta-sts.txt:/dms/files/.well-known/mta-sts.txt:ro
      # Caddy data volume for persistence:
      - ./data/:/data
      # Sharing the same cert with DMS:
      - ./tls/wildcard:/dms/certs:ro

DNS

services:
  # Optional, DNS service used for local testing
  dns:
    image: coredns/coredns:1.11.1
    container_name: mail-dns
    volumes:
      - ./Corefile:/Corefile:ro
      - ./zones/:/zones/:ro
    working_dir: /

CoreDNS Corefile:

example.test {
    file /zones/example.test.zone {
        reload 15s
    }
    log
}

. {
  forward . 1.1.1.1
  log
}

zones/example.test.zone:

$ORIGIN example.test.
$TTL 60

@       SOA    example.test. admin.example.test. 2023102034 4h 1h 14d 10m
@       NS     ns1.example.test.

; Mail
; SPF `mx`: Authorize the DMS mail-server at MX record
@          MX  10 mail.example.test.
@          TXT    "v=spf1 mx -all"

; MTA-STS
_mta-sts   TXT    "v=STSv1; id=2023102030"
_smtp._tls TXT    "v=TLSRPTv1; rua=mailto:tls-reports@example.test"

; A records
@       A      172.16.42.11
mail    A      172.16.42.12
ns1     A      172.16.42.10
mta-sts A      172.16.42.11

Copy link
Contributor

Documentation preview for this PR is ready! 🎉

Built with commit: 4ee42bd

@polarathene polarathene merged commit e3331b0 into docker-mailserver:master Jan 13, 2024
9 checks passed
@georglauterbach georglauterbach modified the milestones: v14.0.0, v13.3.0 Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/features area/networking area/security kind/new feature A new feature is requested in this issue or implemeted with this PR service/postfix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support MTA-STS out of the box
5 participants