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

config: disable SMTP authentication on port 25 #3006

Merged
merged 6 commits into from
Feb 23, 2023
Merged

config: disable SMTP authentication on port 25 #3006

merged 6 commits into from
Feb 23, 2023

Conversation

mazzz1y
Copy link
Contributor

@mazzz1y mazzz1y commented Jan 16, 2023

Description

Fixes #2895

  1. Remove smtpd_sasl_auth_enable parameter from postfix configuration file. It disabled by default: docs
  2. Tests: use openssl for smtp-auth and make sure that auth on 25 port is disabled

Type of change

Improvement (non-breaking change that does improve existing functionality)

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

@mazzz1y mazzz1y changed the title Disable authentication on 25 port Disable SMTP Authentication on 25 port Jan 16, 2023
@mazzz1y mazzz1y marked this pull request as draft January 16, 2023 09:43
@polarathene polarathene added this to the v12.0.0 milestone Jan 16, 2023
@polarathene
Copy link
Member

polarathene commented Jan 17, 2023

@georglauterbach did you want this PR to be tackled and reviewed before / after your upcoming one? It looks like there would minimal conflicts so far.

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.

Little bit of feedback with the current state of the PR

test/tests/serial/mail_with_imap.bats Outdated Show resolved Hide resolved
@casperklein
Copy link
Member

casperklein commented Jan 18, 2023

Remove smtpd_sasl_auth_enable parameter from postfix configuration file

I am not sure if there is demand, but we might consider to make this configurable (with default disabled)?

The old test could be leave untouched, but new tests should be added.

@polarathene
Copy link
Member

polarathene commented Jan 18, 2023

I am not sure if there is demand, but we might consider to make this configurable (with default disabled)?

As the linked issue notes, authentication on port 25 isn't something popular vendors like GMAIL offer, nor competing dockerized mail-server projects. It's disabled by default in Postfix config upstream as well.

Mail submission requiring authentication is intended for the submissions ports. Port 25 for inbound connections should only be responsible for receiving mail, not handling authenticated submissions.

It's true that some relay host services like SendGrid do offer authenticated submission on port 25, but they also typically have the standard 587 and 465 submission ports, and additional alternative ports (eg: 2525) that vary for users where outbound port 25 is blocked.

That said, I haven't looked at our relay support in a while and I know that needs some work. Tests don't presently cover relaying between DMS instances properly IIRC (at least not to the extent I wanted to see).

Some users might be relying on it, but this change in default config would be a breaking change for them regardless. We'd get a better idea for demand making it configurable via ENV after release. I'm not fussed though, as it's a single main.cf setting, we already support that with a postfix-main.cf file to override / append config changes.


The smtpd_sasl_auth_enable = yes config was added in May 2016 for v2 release (actual commit here) which added Dovecot with SASL.

There was also a discussion about this in Feb 2018 (user wanted to AUTH on port 25), but maintainer / contributor agree that it should not be supported.

@georglauterbach
Copy link
Member

@georglauterbach did you want this PR to be tackled and reviewed before / after your upcoming one? It looks like there would minimal conflicts so far.

I'm fine with merging this (when reviewed of course).


I don't see the need for an ENV as well. I think this should be a proper default, and for those that need it, there are enough ways of enabling it again.

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.

Just some minor things that need a bit of clarification. Otherwise looks good!

EDIT: Gave a review when this PR is still WIP - sorry!

target/postfix/main.cf Show resolved Hide resolved
test/tests/serial/mail_with_imap.bats Outdated Show resolved Hide resolved
@georglauterbach
Copy link
Member

NOTE: I have merged a PR that interferes here; we need to pay special attention when reviewing the resolving of the conflicts.

@polarathene
Copy link
Member

At this point due to the activity going on to refactor the test suite, it might be best to hold off resolving any conflicts. The changes are small enough to apply cleanly again after all the churn is out of the way 😅

@georglauterbach
Copy link
Member

At this point due to the activity going on to refactor the test suite, it might be best to hold off resolving any conflicts. The changes are small enough to apply cleanly again after all the churn is out of the way 😅

Yep, good idea 👍🏼

@georglauterbach
Copy link
Member

@mazzz1y I will pick this up and provide the proper integration when we're done finishing the test refactoring (~ 2 weeks).

@georglauterbach georglauterbach self-assigned this Feb 5, 2023
@georglauterbach
Copy link
Member

@mazzz1y sorry for the delay.. could you re-apply your changes to the current state of master? We will then try to merge this and then continue with test refactoring, in order to merge these changes and to have them in v12.

@polarathene
Copy link
Member

I'll handle it within the following week hopefully 😅

Of note, this will likely contribute to this concern? #1247

SMTP_ONLY=1 for sending mail, would need a way to enable a way to authenticate (since we usually defer that to Dovecot?).

@georglauterbach
Copy link
Member

I'll handle it within the following week hopefully sweat_smile

Of note, this will likely contribute to this concern? #1247

SMTP_ONLY=1 for sending mail, would need a way to enable a way to authenticate (since we usually defer that to Dovecot?).

You're right.. we could check if SMTP_ONLY=1 though. I've not yet thought about this in too much detail. Auth on port 465/587 is still enabled, right? I'm no SASL expert though.

@polarathene
Copy link
Member

Auth on port 465/587 is still enabled, right? I'm no SASL expert though.

Yeah, I think so. Just no Dovecot for the SASL with that ENV AFAIK? There's SASLAUTHD instead (used for LDAP, but offers other modes too I think). I would need to properly look over the referenced issue.

@georglauterbach
Copy link
Member

One option would be to just skip the tests like we did with other tests that fail randomly or tests that are broken.. @polarathene what's your opinion?

@polarathene
Copy link
Member

not ok 22 checking spoofing: uses senders filter in 173ms
# (from function `assert_success' in file test/test_helper/bats-assert/src/assert_success.bash, line 42,
#  in test file test/tests/serial/mail_with_ldap.bats, line 212)
#   `assert_success' failed
#
# -- command failed --
# status : 1
# output (3 lines):
#   Can't use SSL_get_servername
#   depth=0 CN = docker-mailserver.invalid
#   verify return:1
# --
#

Current failure for the LDAP spoof check is related to it failing to verify the cert I think? Probably due to port 465 instead of 587 with StartTLS? (just a guess)

I don't have time to look into the failure myself as my backlog is too much already.

I do recognize docker-mailserver.invalid, we set that as /etc/hostname within the Dockerfile build:

echo "echo 'docker-mailserver.invalid'" >/bin/hostname

I don't believe that is used anywhere else. Our testing self-signed certs should be for example.test / mail.example.test.

It doesn't help that the LDAP test hasn't been ported to our new testing conventions + methods, which may affect that, but I guess it could be something else going on related to LDAP specifically 😅

Port 25 will permit plain-text if a secure connection cannot be established, but 587 (and more obviously 465) should not.


What bothers me as well is: connect to 127.0.0.1[127.0.0.1]:10024: Connection refused. Why would Amavis refuse connections? It was not touched at all..

I doubt it's related to this PR. Tests relying on Amavis may fail transiently due to startup being ready with Postfix to send / receive mail before Amavis itself is ready (now that we've optimized the startup). Affected tests can delay sending with some sleep after the setup_file() / setup() and it should be fine. Otherwise just restart the test.


One option would be to just skip the tests like we did with other tests that fail randomly or tests that are broken.. @polarathene what's your opinion?

For LDAP, since we lack maintainers and no one is stepping up, we can't provide guarantees or official support. If there is no one able to take the time to resolve it, I'd be in favor of adding a skip to an LDAP test case with comment referencing the PR message that describes the reason / problem.

@mazzz1y
Copy link
Contributor Author

mazzz1y commented Feb 22, 2023

Current failure for the LDAP spoof check is related to it failing to verify the cert I think?

@polarathene no, please ignore these lines. It is just a warning and can be safely ignored. Please check my full explanation and try to debug this test manually.

It shows only first three lines of output:

root@mail:/# openssl s_client -quiet -connect 0.0.0.0:465 < /tmp/docker-mailserver-test/auth/ldap-smtp-auth-spoofed-sender-with-filter-exception.txt
Can't use SSL_get_servername
depth=0 CN = docker-mailserver.invalid
verify return:1
220 mail.my-domain.com ESMTP
250-mail.my-domain.com
250-PIPELINING
250-SIZE 10240000
250-ETRN
250-AUTH PLAIN LOGIN
250-AUTH=PLAIN LOGIN
250-ENHANCEDSTATUSCODES
250-8BITMIME
250-DSN
250 CHUNKING
334 VXNlcm5hbWU6
334 UGFzc3dvcmQ6
535 5.7.8 Error: authentication failed: authentication failure
250 2.1.0 Ok
554 5.7.1 <localhost[127.0.0.1]>: Client host rejected: Access denied
554 5.5.1 Error: no valid recipients
221 2.7.0 Error: I can break rules, too. Goodbye.

@georglauterbach
Copy link
Member

Current failure for the LDAP spoof check is related to it failing to verify the cert I think?

@polarathene no, please ignore these lines. It is just a warning and can be safely ignored. Please check my full explanation and try to debug this test manually

We will add a skip to this test. No current maintainer can reliable debug LDAP related code. I will adjust the PR.

The test seems to have been broken from the beginning.

Sadly, no LDAP maintainers can verify. Added a TODO item if ever a LDAP maintainer comes around.
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.

LGTM now 👍🏼

@georglauterbach georglauterbach marked this pull request as ready for review February 22, 2023 14:04
@georglauterbach georglauterbach requested review from polarathene and removed request for casperklein February 22, 2023 14:04
@georglauterbach georglauterbach changed the title Disable SMTP Authentication on 25 port config: disable SMTP authentication on port 25 Feb 22, 2023
@polarathene
Copy link
Member

Please check my full explanation and try to debug this test manually.

Ah gotcha, thanks for that. skip is fine for now. The test needs to be refactored and we can revisit that issue at that point 👍

@polarathene
Copy link
Member

LGTM, happy to approve but would like to clarify why some nc calls have been switched to openssl ones? I had a quick search over this PR and the linked issue it resolves, but I didn't see an explanation for openssl usage.

Could you please just comment here about when / why that is needed sometimes in our tests? After that I'll approve.

@mazzz1y
Copy link
Contributor Author

mazzz1y commented Feb 23, 2023

LGTM, happy to approve but would like to clarify why some nc calls have been switched to openssl ones? I had a quick search over this PR and the linked issue it resolves, but I didn't see an explanation for openssl usage.

Could you please just comment here about when / why that is needed sometimes in our tests? After that I'll approve.

@polarathene

Because 465/587 listeners rejects plain-text requests there. Where it doesn't work I just replaced it with openssl.
For example:

root@mail:/# nc 0.0.0.0 465 < /tmp/docker-mailserver-test/auth/ldap-smtp-auth-spoofed.txt

root@mail:/# nc 0.0.0.0 587 < /tmp/docker-mailserver-test/auth/ldap-smtp-auth-spoofed.txt
220 mail.my-domain.com ESMTP
250-mail.my-domain.com
250-PIPELINING
250-SIZE 10240000
250-ETRN
250-STARTTLS
250-ENHANCEDSTATUSCODES
250-8BITMIME
250-DSN
250 CHUNKING
530 5.7.0 Must issue a STARTTLS command first
502 5.5.2 Error: command not recognized
502 5.5.2 Error: command not recognized
530 5.7.0 Must issue a STARTTLS command first
530 5.7.0 Must issue a STARTTLS command first
530 5.7.0 Must issue a STARTTLS command first
221 2.7.0 Error: I can break rules, too. Goodbye.

### expected behavior ###
root@mail:/# openssl s_client -quiet -connect 0.0.0.0:465 < /tmp/docker-mailserver-test/auth/ldap-smtp-auth-spoofed.txt
Can't use SSL_get_servername
depth=0 CN = docker-mailserver.invalid
verify return:1
220 mail.my-domain.com ESMTP
250-mail.my-domain.com
250-PIPELINING
250-SIZE 10240000
250-ETRN
250-AUTH PLAIN LOGIN
250-AUTH=PLAIN LOGIN
250-ENHANCEDSTATUSCODES
250-8BITMIME
250-DSN
250 CHUNKING
334 VXNlcm5hbWU6
334 UGFzc3dvcmQ6
235 2.7.0 Authentication successful
250 2.1.0 Ok
553 5.7.1 <ldap@localhost.localdomain>: Sender address rejected: not owned by user some.user@localhost.localdomain
554 5.5.1 Error: no valid recipients
221 2.7.0 Error: I can break rules, too. Goodbye.

@georglauterbach
Copy link
Member

Would this generally apply? I.e. when using port 465/587 that we should use openssl? This is important for the helper _send_mail to make the corect decision for the port.

@mazzz1y
Copy link
Contributor Author

mazzz1y commented Feb 23, 2023

Would this generally apply? I.e. when using port 465/587 that we should use openssl? This is important for the helper _send_mail to make the corect decision for the port.

No, in other tests listener accepts only plain text messages, looks like it is related to test server configuration, but I didn't deep into it

## test/tests/parallel/set3/mta/smtp_delivery.bats
## @test "should successfully authenticate with good password (plain)"

root@mail:/# nc -w 5 0.0.0.0 465 < /tmp/docker-mailserver-test/auth/smtp-auth-plain.txt
220 mail.example.test ESMTP
250-mail.example.test
250-PIPELINING
250-SIZE 10240000
250-ETRN
250-AUTH PLAIN LOGIN
250-AUTH=PLAIN LOGIN
250-ENHANCEDSTATUSCODES
250-8BITMIME
250-DSN
250 CHUNKING
235 2.7.0 Authentication successful
221 2.0.0 Bye

root@mail:/# openssl s_client -quiet -connect 0.0.0.0:465 < /tmp/docker-mailserver-test/auth/smtp-auth-plain.txt
139985178903872:error:1408F10B:SSL routines:ssl3_get_record:wrong version number:../ssl/record/ssl3_record.c:331:

root@mail:/# openssl s_client -starttls smtp -quiet -connect 0.0.0.0:587 < /tmp/docker-mailserver-test/auth/smtp-auth-plain.txt
Didn't find STARTTLS in server response, trying anyway...
140094189851968:error:1408F10B:SSL routines:ssl3_get_record:wrong version number:../ssl/record/ssl3_record.c:331:

@georglauterbach
Copy link
Member

I see, thanks for the quick reply. I will tend to this in #3105.

@georglauterbach georglauterbach merged commit 199e3c7 into docker-mailserver:master Feb 23, 2023
georglauterbach pushed a commit that referenced this pull request Feb 24, 2023
config: disable SMTP authentication on port 25 (#3006)

* postfix: remove smtpd_sasl_auth_enable global setting

* tests: disable auth on 25 port

* tests: revert ldap-smtp-auth-spoofed-sender-with-filter-exception.txt

* Skip failing test

The test seems to have been broken from the beginning.

Sadly, no LDAP maintainers can verify. Added a TODO item if ever a LDAP maintainer comes around.

* Apply PR feedback

---------

Co-authored-by: Georg Lauterbach <44545919+georglauterbach@users.noreply.github.com>

added plausibility checks for milter insertion

corrected grep in tests

revert changes to milter insertion
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FR] Disable "AUTH LOGIN" for non-submission smtp port
4 participants