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

Make smtp_tls_policy_maps easily configurable #1902

Merged
merged 9 commits into from
Aug 22, 2021

Conversation

nextgens
Copy link
Contributor

@nextgens nextgens commented Aug 1, 2021

What type of PR?

Feature

What does this PR do?

  • Make smtp_tls_policy_maps easily configurable. This is useful to force TLS verification of specific destinations (or relays).
    We should probably discuss what's on the list by default. I have found a top100 list online, ran it through a script to check all the records and found 90 destinations we could use.
  • disable TLS session tickets (this reduces the PFS window from 1day to 1h)
  • enable system CAs by default (to allow for OUTBOUND_TLS_LEVEL above encrypt without additional overrides)

Related issue(s)

Prerequistes

Before we can consider review and merge, please make sure the following list is done and checked.
If an entry in not applicable, you can check it or remove it from the list.

  • In case of feature or enhancement: documentation updated accordingly
  • Unless it's docs or a minor change: add changelog entry file.

@nextgens nextgens added priority/p2 Minor bug / Could have type/security Related to security type/feature Introduces a new feature labels Aug 1, 2021
@mergify
Copy link
Contributor

mergify bot commented Aug 1, 2021

Thanks for submitting this pull request.
Bors-ng will now build test images. When it succeeds, we will continue to review and test your PR.

bors try

Note: if this build fails, read this.

bors bot added a commit that referenced this pull request Aug 1, 2021
@bors
Copy link
Contributor

bors bot commented Aug 1, 2021

try

Build succeeded:

@nextgens
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Aug 13, 2021
@bors
Copy link
Contributor

bors bot commented Aug 13, 2021

try

Build failed:

encrypt means "ensure we have some confidentiality" whereas secure means
"ensure we have confidentiality while talking to the right peer"
(protects against passive or/and active MITM attacks)
I have found a list of the top100 email destinations online and ran them
through a script to ensure that all of their MX servers had valid
configuration... this is the result
@nextgens
Copy link
Contributor Author

bors retry

bors bot added a commit that referenced this pull request Aug 14, 2021
@bors
Copy link
Contributor

bors bot commented Aug 14, 2021

try

Build succeeded:

This was referenced Aug 16, 2021
@Diman0 Diman0 assigned Diman0 and unassigned Diman0 Aug 18, 2021
@Diman0 Diman0 self-requested a review August 18, 2021 21:43
@Diman0
Copy link
Member

Diman0 commented Aug 19, 2021

During the last project meeting, you told us you could make a script available you used for testing?
Can you share it?

When testing I receive errors. I set the OUTBOUND_TLS_LEVEL=secure.
After that I send emails to https://havedane.net/

I can see the following in the logs. Do you see the same when you test the PR?

smtp_1      | Aug 19 17:10:45 test postfix/smtpd[369]: disconnect from mailu_webmail_1.mailu_default[192.168.203.8] ehlo=2 xclient=0/1 mail=1 rcpt=3 data=1 quit=1 commands=8/9
smtp_1      | Aug 19 17:10:46 test postfix/smtp[372]: error: unsupported dictionary type: hash
smtp_1      | Aug 19 17:10:46 test postfix/smtp[374]: error: unsupported dictionary type: hash
smtp_1      | Aug 19 17:10:46 test postfix/smtp[375]: error: unsupported dictionary type: hash
smtp_1      | Aug 19 17:10:46 test postfix/smtp[374]: warning: hash:/etc/postfix/tls_policy.map is unavailable. unsupported dictionary type: hash
smtp_1      | Aug 19 17:10:46 test postfix/smtp[374]: warning: hash:/etc/postfix/tls_policy.map lookup error for "dont.havedane.net"
smtp_1      | Aug 19 17:10:46 test postfix/smtp[374]: warning: smtp_tls_policy_maps, next-hop destination "dont.havedane.net": policy table lookup error
smtp_1      | Aug 19 17:10:46 test postfix/smtp[374]: warning: TLS policy lookup for dont.havedane.net/dont.havedane.net: client TLS configuration problem
smtp_1      | Aug 19 17:10:46 test postfix/smtp[374]: warning: TLS policy lookup for dont.havedane.net/dont.havedane.net: client TLS configuration problem
smtp_1      | Aug 19 17:10:46 test postfix/smtp[372]: warning: hash:/etc/postfix/tls_policy.map is unavailable. unsupported dictionary type: hash
smtp_1      | Aug 19 17:10:46 test postfix/smtp[372]: warning: hash:/etc/postfix/tls_policy.map lookup error for "do.havedane.net"
smtp_1      | Aug 19 17:10:46 test postfix/smtp[372]: warning: smtp_tls_policy_maps, next-hop destination "do.havedane.net": policy table lookup error
smtp_1      | Aug 19 17:10:46 test postfix/smtp[372]: warning: TLS policy lookup for do.havedane.net/do.havedane.net: client TLS configuration problem
smtp_1      | Aug 19 17:10:46 test postfix/smtp[372]: warning: TLS policy lookup for do.havedane.net/do.havedane.net: client TLS configuration problem

Maybe the postmap command did not run?

Diman0
Diman0 previously requested changes Aug 19, 2021
Copy link
Member

@Diman0 Diman0 left a comment

Choose a reason for hiding this comment

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

See in-line comments. I will check that my comments are correct, but it looks like you create a lmdb db instead of hash db because you did not specify the type in the postmap command.

core/postfix/start.py Show resolved Hide resolved
core/postfix/start.py Show resolved Hide resolved
@Diman0
Copy link
Member

Diman0 commented Aug 19, 2021

wait. alpine removed support for hash. See this PR #1918
So in this PR you should change from hash: to lmdb in the main.cf file.

@mergify mergify bot dismissed Diman0’s stale review August 19, 2021 17:47

Pull request has been modified.

@Diman0
Copy link
Member

Diman0 commented Aug 19, 2021

It should now be fine.

bors try

bors bot added a commit that referenced this pull request Aug 19, 2021
@bors
Copy link
Contributor

bors bot commented Aug 19, 2021

try

Build succeeded:

@nextgens
Copy link
Contributor Author

Yes sorry... this PR predates #1918 and I didn't realize that it required the same changes...

@nextgens
Copy link
Contributor Author

nextgens commented Aug 20, 2021

During the last project meeting, you told us you could make a script available you used for testing?
Can you share it?

Sure; here it is; ensure you run it from a host with a valid rDNS record.

#!/usr/bin/env python3
#
# check whether all the domains in the list have MX servers
#that can do STARTTLS with valid certificates
#
# I've used the top100 list from https://www.gmass.co/domains
#
# nextgens ~ 2021

from smtplib import SMTP
import ssl
import dns.resolver
import sys

sslctx = ssl.create_default_context()
sslctx.check_hostname = True
sslctx.verify_mode = ssl.CERT_REQUIRED

with open("list.txt","r") as f:
    with open("ok.txt","w") as ok:
            with open("err.txt","w") as err:
                for domain in f:
                    domain = domain.strip()
                    try:
                        for mx in dns.resolver.query(domain, 'MX'):
                            mx_host = mx.to_text().split(' ')[1][:-1]

                            with SMTP(mx_host) as smtp:
                                #smtp.set_debuglevel(2)
                                smtp.ehlo()
                                smtp.starttls(context=sslctx)
                                smtp.ehlo()
                        ok.write(f'{domain}\tsecure\n')
                    except Exception as e:
                            err.write(f'{domain}{e}\n')

When testing I receive errors. I set the OUTBOUND_TLS_LEVEL=secure.
After that I send emails to https://havedane.net/

I can see the following in the logs. Do you see the same when you test the PR?

You've now fixed the issue (was alpine removing support for BDB)... I wouldn't anticipate anything to work when you are sending mails to @havedane.net though: this PR is about web/PKIX support (hosts that have a certificate signed by a recognized CA), not DANE.

It does make configuring DANE for select destinations easier (by providing an easy to override tls_policy.map) but won't do it for you.

@Diman0
Copy link
Member

Diman0 commented Aug 20, 2021

Thank you for providing the script and the clarification on DANE. I will have to do more reading on this subject to get a real understanding of it. I will do some testing when I have time (possibly today) and will then approve the PR.

@nextgens
Copy link
Contributor Author

To put it in simple words: postfix as configured by mailu without the PR will do opportunistic encryption: it will use TLS to provide protection against a passive attacker.

With this PR, it will also provide protection against an active attacker (by verifying/validating the TLS certificate) for select domains.

We should probably discuss what's worth having on the list: it could be much bigger; the only thing we need to trust is that they won't fail at maintaining valid TLS certificates going forward.

@nextgens
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Aug 20, 2021
@bors
Copy link
Contributor

bors bot commented Aug 20, 2021

try

Build succeeded:

Copy link
Member

@Diman0 Diman0 left a comment

Choose a reason for hiding this comment

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

Lgtm

@mergify
Copy link
Contributor

mergify bot commented Aug 22, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented Aug 22, 2021

Build succeeded:

@bors bors bot merged commit 7efce99 into Mailu:master Aug 22, 2021
tonobo added a commit to tonobo/Mailu that referenced this pull request Sep 1, 2021
The alpine postfix package seems to have removed support for btree and hash map type. Mailu#1918 
The tls_policy.map stuff has been introduced in Mailu#1902 and it has been merged without fixing this before (Mailu#1902)
bors bot added a commit that referenced this pull request Sep 2, 2021
1965: postfix/tls_policy: Use lmdb map instead of hash r=mergify[bot] a=tonobo

## What type of PR?

bug-fix

## What does this PR do?

### Related issue(s)

#1918

#1902



Co-authored-by: Tim Foerster <timhormersdorf@googlemail.com>
@nextgens nextgens deleted the tls_policy_map branch October 31, 2021 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority/p2 Minor bug / Could have type/feature Introduces a new feature type/security Related to security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make smtp_tls_policy_maps easily configurable
2 participants