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

sendmail: Use actual TLS connection if enabled #2896

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Conversation

MichaIng
Copy link
Member

Currently, enabling TLS actually enables STARTTLS while the initial connection is still done as plain SMTP. This is counter-intuitive, since TLS, the way providers use it in their docs, usually means a TLS-encrypted connection right from the start. STARTTLS is usually named as such, and handled on another port, or enforced automatically on plain connections.

This commit changes the behaviour to use an initial TLS-encrypted SMTP connection if this setting is enabled. While most providers allow and redirect all connection types on all ports, for some this may be a breaking change, i.e. it might be required to change the port e.g. from 587 to 465.

Currently, enabling TLS actually enables STARTTLS while the initial connection is still done as plain SMTP. This is counter-intuitive, since TLS, the way providers use it in their docs, usually means a TLS-encrypted connection right from the start. STARTTLS is usually named as such, and handled on another port, or enforced automatically on plain connections.

This commit changes the behaviour to use an initial TLS-encrypted SMTP connection if this setting is enabled. While most providers allow and redirect all connection types on all ports, for some this may be a breaking change, i.e. it might be required to change the port e.g. from 587 to 465.

Signed-off-by: MichaIng <micha@dietpi.com>
@zagrim
Copy link
Collaborator

zagrim commented Dec 25, 2023

I tested GMail app password in current dev (0.43.1b1) and with these changes, using the "Test Email" button in ME. I hope it doesn't function different from the actual email sending (I didn't check from code if it's the same code that is run in both cases. It should, I guess.).

Some notes from testing with dev branch:
I don't have email notifications configured normally so I wanted to check out how it works in dev branch.
The only working combination of port + "Use TLS" is 587 with "Use TLS" enabled, anything else results in either timeout or "SMTP AUTH extension not supported by server" even though Google's own instructions still mention ports 25 and 465 ("for SSL")... 🙄

Testing with this PR:
Now, with the changes from this PR, port 587 + "Use TLS" enabled I get "ssl.SSLError: [SSL: WRONG_VERSION_NUMBER] wrong version number (_ssl.c:1006)"

   ERROR: notification email test failed: [SSL: WRONG_VERSION_NUMBER] wrong version number (_ssl.c:1006)
Traceback (most recent call last):
  File "/mnt/project/github/me-py-venv/lib/python3.11/site-packages/motioneye/handlers/config.py", line 681, in test
    sendmail.send_mail(
  File "/mnt/project/github/me-py-venv/lib/python3.11/site-packages/motioneye/sendmail.py", line 45, in send_mail
    conn = smtplib.SMTP_SSL(server, port, timeout=settings.SMTP_TIMEOUT)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/smtplib.py", line 1050, in __init__
    SMTP.__init__(self, host, port, local_hostname, timeout,
  File "/usr/lib/python3.11/smtplib.py", line 255, in __init__
    (code, msg) = self.connect(host, port)
                  ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/smtplib.py", line 341, in connect
    self.sock = self._get_socket(host, port, self.timeout)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/smtplib.py", line 1057, in _get_socket
    new_socket = self.context.wrap_socket(new_socket,
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/ssl.py", line 517, in wrap_socket
    return self.sslsocket_class._create(
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/ssl.py", line 1108, in _create
    self.do_handshake()
  File "/usr/lib/python3.11/ssl.py", line 1383, in do_handshake
    self._sslobj.do_handshake()
ssl.SSLError: [SSL: WRONG_VERSION_NUMBER] wrong version number (_ssl.c:1006)

I wonder if https://stackoverflow.com/a/69975850 and the link provided to Python SSL lib docs there helps in figuring this out 🤔

With port 587 + "Use TLS" disabled I get "SMTP AUTH extension not supported by server" which I think is fine as this was not changed.

Additional finding:
I also noticed that even when I successfully get an test email in my inbox, the UI shows "Notification email failed:" (in black text, though, not in red as with the actual errors). That's an easy fix, there's just incorrect text in

showPopupMessage(i18n.gettext("Sciiga retpoŝto fiaskis:"), 'info');
for the success case.

I also noticed that due to the lack of IPv6 support in my LAN and also Internet connection I had to enter the IP instead of the Google SMTP host (log complained about network being not reachable). This however occurs locally also with e.g. ping, so I guess it's just something that cannot (and shouldn't ?) be dealt in ME. So that's a non-issue here.

@MichaIng
Copy link
Member Author

MichaIng commented Dec 25, 2023

The preferred options for Gmail, which also match their docs is, after this PR, port 465 with TLS.

It makes sense that port 465 with TLS did not work before since motionEye tries a plain connection (and STARTTLS) without this PR. Similarly it makes sense that it worked with port 587 and TLS before, since this is the port usually used for STARTTLS. Port 25 is usually for plain connections without STARTTLS, and I guess Gmail does not support this at all, but probably allows STARTTLS as well on this port. But then this should have worked before with TLS enabled in motionEye.

So basically if now TLS works on port 465, all is as intended. Not sure whether we should support STARTTLS at all.

@zagrim
Copy link
Collaborator

zagrim commented Dec 26, 2023

Ah, yes, port 465 + "Use TLS" enabled works ok with these changes 👍 (and 465 with TLS disabled times out like before). So like you suspected in the PR description, port change would indeed be needed at least for GMail users.

I'm wondering about the errors and instructions though, as at least I don't understand at all how the secure SMTP works with these two ports, so the regular user likely is no better. One easy option of course is to continue with the idea that "try 465 and 587 and see what works for you".

@MichaIng
Copy link
Member Author

MichaIng commented Dec 26, 2023

So far so good. As said, port 465 is for TLS while port 587 is for "connect without TLS and then switch to TLS for data transfer via STARTTLS protocol", which we now do not support anymore. So with most providers 465 will be correct, or port 25 for entirely plain connections and plain data transfer.

In case of the issue this PR is linked to, it did not work because the user followed Gmail instructions with port 465 + TLS which did not work because motionEye tried a plain connection and STARTTLS instead. Now TLS really means TLS.

What we could do for a more graceful migration: Have a drop down with "plain/TLS/STARTTLS", and convert the old boolean config into STARTTLS, so it keeps working for those where it worked before.

And we could/should add info to the port tooltip about the most commonly correct ports with Gmail as example.

@zagrim
Copy link
Collaborator

zagrim commented Dec 26, 2023

What we could do for a more graceful migration: Have a drop down with "plain/TLS/STARTTLS", and convert the old boolean config into plain/STARTTLS, so it keeps working for those where it worked before.

That would be pretty nice UX-wise, I think, and then there'd be also support for STARTTLS should some server require that. Some extra work, sure, but might save us from at least a couple of issues reporting upgrade broke email notifications...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Google will turn off plain smtp login - possible impact on motioneye-generated emails?
2 participants