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.

Sign up for GitHub

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

Fix SMTPS / implicit TLS #292

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Fix SMTPS / implicit TLS #292

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
122f0ee
Moved smtps context determination up so that its state can be passed …
strongholdmedia Nov 21, 2021
c2cacac
Changes to account for SMTPS / implicit TLS usage
strongholdmedia Nov 21, 2021
4770b5c
Merge branch 'aio-libs:master' into master
strongholdmedia Dec 19, 2022
fc0bae0
Fixed AUTH with SMTPS
strongholdmedia Dec 19, 2022
4aa0cea
Added test for #281
strongholdmedia Dec 19, 2022
7f31ffb
Removed stale import statement
strongholdmedia Dec 20, 2022
d36f75e
Fixed assertion of content for changed error message in SMTP test
strongholdmedia Dec 20, 2022
3013c0c
Enable SMTPS explicitly on the controller
strongholdmedia Dec 20, 2022
5625213
Removed nonsense and added another assertion
strongholdmedia Dec 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions aiosmtpd/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,13 +241,21 @@ def main(args: Optional[Sequence[str]] = None) -> None:
else:
tls_context = None

if args.smtpscert and args.smtpskey:
smtps_context = ssl.create_default_context(ssl.Purpose.CLIENT_AUTH)
smtps_context.check_hostname = False
smtps_context.load_cert_chain(str(args.smtpscert), str(args.smtpskey))
else:
smtps_context = None

factory = partial(
SMTP,
args.handler,
data_size_limit=args.size,
enable_SMTPUTF8=args.smtputf8,
tls_context=tls_context,
require_starttls=args.requiretls,
is_using_smtps=smtps_context is not None,
)

logging.basicConfig(level=logging.ERROR)
Expand All @@ -261,13 +269,6 @@ def main(args: Optional[Sequence[str]] = None) -> None:
if args.debug > 2:
loop.set_debug(enabled=True)

if args.smtpscert and args.smtpskey:
smtps_context = ssl.create_default_context(ssl.Purpose.CLIENT_AUTH)
smtps_context.check_hostname = False
smtps_context.load_cert_chain(str(args.smtpscert), str(args.smtpskey))
else:
smtps_context = None

log.debug("Attempting to start server on %s:%s", args.host, args.port)
server = server_loop = None
try:
Expand Down
15 changes: 11 additions & 4 deletions aiosmtpd/smtp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,7 @@ def __init__(
ident: Optional[str] = None,
tls_context: Optional[ssl.SSLContext] = None,
require_starttls: bool = False,
is_using_smtps: bool = False,
Comment on lines 323 to +325
Copy link
Collaborator

Choose a reason for hiding this comment

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

After doing some tracing, a thought occurred to me:

Implicit TLS (SMTPS) is triggered if tls_context is set, right?

Can we just set is_using_smtps attribute from that? So self._is_using_smtps = (tls_context is not None)?

I might've overlooked something, so I welcome a discussion here.

Copy link
Author

Choose a reason for hiding this comment

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

Well, yes and no.
For once, tls_context is for STARTTLS; SMTPS uses ssl_context. (This was already such before, I did not change it.)

Otherwise we could set that, yep, but this option communicates clear intent.

As you've noted in #281, there is no possible means for detecting whether or not we are using implicit TLS (SMTPS) within the SMTP class.

This goes to the extent that ssl_context is actually not used by the SMTP class at all.
It is still kept by me, as if later authentication methods will require e.g. client re-validation or something, it could happen.

But unlike is_using_smtps, ssl_context may or may not be an explicit intent.
ssl_context being None could equally mean "should not use SMTPS", as well as "hey, we/they/... failed to handle a key / cert error / expiry / ... somewhere".

But as per the above, there is no means to detect whichever from inside the SMTP class; as such, if the described scenario happens, the client/user would just note that "hey, we don't have AUTH", or even "got SMTP error, pls help, it worked yesterday".

Currently, if is_using_smtps is set with no ssl_context, it should present an error.
I understand your point and will change it if that'd be the agreement, but I feel that would be a technically less correct approach.

timeout: float = 300,
auth_required: bool = False,
auth_require_tls: bool = True,
Expand Down Expand Up @@ -368,11 +369,13 @@ def __init__(
self.envelope: Optional[Envelope] = None
self.transport = None
self._handler_coroutine = None
if not auth_require_tls and auth_required:
if not auth_require_tls and not is_using_smtps and auth_required:
warn("Requiring AUTH while not requiring TLS "
"can lead to security vulnerabilities!")
log.warning("auth_required == True but auth_require_tls == False")
log.warning("auth_required == True but auth_require_tls == False "
"and not using SMTPS")
self._auth_require_tls = auth_require_tls
self._is_using_smtps = is_using_smtps

if proxy_protocol_timeout is not None:
if proxy_protocol_timeout <= 0:
Expand Down Expand Up @@ -835,7 +838,9 @@ async def smtp_EHLO(self, hostname: str):
self.command_size_limits['MAIL'] += 10
if self.tls_context and not self._tls_protocol:
response.append('250-STARTTLS')
if not self._auth_require_tls or self._tls_protocol:
if (not self._auth_require_tls
or self._tls_protocol
or self._is_using_smtps):
response.append(
"250-AUTH " + " ".join(sorted(self._auth_methods.keys()))
)
Expand Down Expand Up @@ -923,7 +928,9 @@ async def smtp_AUTH(self, arg: str) -> None:
return
elif not self.session.extended_smtp:
return await self.push("500 Error: command 'AUTH' not recognized")
elif self._auth_require_tls and not self._tls_protocol:
elif (self._auth_require_tls
and not self._tls_protocol
and not self._is_using_smtps):
return await self.push("538 5.7.11 Encryption required for requested "
"authentication mechanism")
elif self.session.authenticated:
Expand Down
3 changes: 2 additions & 1 deletion aiosmtpd/tests/test_smtp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1960,7 +1960,8 @@ def test_warn_authreqnotls(self, caplog):
assert caplog.record_tuples[0] == (
"mail.log",
logging.WARNING,
"auth_required == True but auth_require_tls == False",
"auth_required == True but auth_require_tls == False and "
"not using SMTPS",
)

def test_log_authmechanisms(self, caplog):
Expand Down
14 changes: 10 additions & 4 deletions aiosmtpd/tests/test_smtps.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@

from aiosmtpd.controller import Controller
from aiosmtpd.testing.helpers import ReceivingHandler
from aiosmtpd.testing.statuscodes import SMTP_STATUS_CODES as S

from .conftest import Global

Expand All @@ -21,7 +20,12 @@ def ssl_controller(
get_controller, ssl_context_server
) -> Generator[Controller, None, None]:
handler = ReceivingHandler()
controller = get_controller(handler, ssl_context=ssl_context_server)
# ssl_context is only used by create_server, and not accessed at all
controller = get_controller(
handler,
ssl_context=ssl_context_server,
is_using_smtps=True
)
controller.start()
Global.set_addr_from(controller)
#
Expand All @@ -41,8 +45,10 @@ class TestSMTPS:
def test_smtps(self, ssl_controller, smtps_client):
sender = "sender@example.com"
recipients = ["rcpt1@example.com"]
resp = smtps_client.helo("example.com")
assert resp == S.S250_FQDN
code, _ = smtps_client.ehlo("example.com")
assert code == 250
assert smtps_client.has_extn("AUTH")
assert not smtps_client.has_extn("STARTTLS")
results = smtps_client.send_message(MIMEText("hi"), sender, recipients)
assert results == {}
handler: ReceivingHandler = ssl_controller.handler
Expand Down