marrow / mailer

A light-weight, modular, message representation and mail delivery framework for Python.
MIT License
279 stars 62 forks source link

SMTP regression and failure during TLS negotiation under Python 3.7. #83

Open mentat51 opened 4 years ago

mentat51 commented 4 years ago

Upstream tracking bug: https://bugs.python.org/issue36094


from marrow.mailer import Mailer, Message

mailer = Mailer(dict(
        transport = dict(
                use = 'smtp',
                host = 'smtp.office365.com',
                port = 587,
                tls='required',
                username='username@test.com',
                password='password')))
print('connection...')
mailer.start()
print('connected')

message = Message(author="from@test.com", to="to@test.com")
message.subject = "Testing Marrow Mailer"
message.plain = "This is a test."
print('send mail')
mailer.send(message)

print('disconnect')
mailer.stop()

This works great with python 2.7, but with python 3.7 (tested on 3.7.1 and 3.7.4), I have this error :

Delivery of message <157121638590.46272.10119950910494763987@server.local> failed.
Traceback (most recent call last):
  File "C:\usr\Python37\lib\site-packages\marrow\mailer\manager\util.py", line 50, in __enter__
    transport = pool.transports.get(False)
  File "C:\usr\Python37\lib\queue.py", line 167, in get
    raise Empty
_queue.Empty

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "minimal_test.py", line 32, in <module>
    mailer.send(message)
  File "C:\usr\Python37\lib\site-packages\marrow\mailer\__init__.py", line 149, in send
    result = self.manager.deliver(message)
  File "C:\usr\Python37\lib\site-packages\marrow\mailer\manager\immediate.py", line 41, in deliver
    with self.transport() as transport:
  File "C:\usr\Python37\lib\site-packages\marrow\mailer\manager\util.py", line 57, in __enter__
    transport.startup()
  File "C:\usr\Python37\lib\site-packages\marrow\mailer\transport\smtp.py", line 50, in startup
    self.connect_to_server()
  File "C:\usr\Python37\lib\site-packages\marrow\mailer\transport\smtp.py", line 86, in connect_to_server
    connection.starttls(self.keyfile, self.certfile)
  File "C:\usr\Python37\lib\smtplib.py", line 771, in starttls
    server_hostname=self._host)
  File "C:\usr\Python37\lib\ssl.py", line 412, in wrap_socket
    session=session
  File "C:\usr\Python37\lib\ssl.py", line 846, in _create
    owner=self, session=self._session,
ValueError: server_hostname cannot be an empty string or start with a leading dot.
iamraa commented 4 years ago

You have to monkey-patch this method marrow.mailer.transport.smtp.SMTPTransport.connect_to_server():

# ....
    if self.tls == "ssl":
        connection = SMTP_SSL(
            host=self.host,
            local_hostname=self.local_hostname,
            keyfile=self.keyfile,
            certfile=self.certfile,
            timeout=self.timeout,
        )
    else:
        connection = SMTP(
            host=self.host, local_hostname=self.local_hostname, timeout=self.timeout
        )
# ....
hyperknot commented 4 years ago

Is there any other way to fix this without monkey patching? Python 3.7 is 1.5 years old, what is the proper way to use this library with it? I hope not a monkey patch.

amcgregor commented 3 years ago

Upstream tracking bug: https://bugs.python.org/issue36094 (my back-referencing comment therein)

Please stop submitting pull requests passing the host name to the constructor. These solutions will be immediately rejected. See also: http://xyproblem.info

amcgregor commented 3 years ago

See also: python/cpython#11998 — pull request correcting the issue, pending changes from code review.

Minstel commented 3 years ago

This error still exists. No solution except monkey-patching?

amcgregor commented 3 years ago

@Minstel Please see the official bug report against CPython, linked above your comment.

amcgregor commented 2 years ago

if I'm understanding properly, this library is rejecting PRs that make it... work? I understand there may be a longstanding upstream issue, but is there any chance we can document "if you're using python 3.7 do exactly this monkey patch?"

feels pedantic?

While I do appreciate the fact this issue comment was subsequently deleted, e-mail notifications make that moot. A point was further ground home; I'm just not sure it's the one that may have been intended. (That was some laborious redaction for politeness, right there.)

To reiterate: the PRs were not accepted because they introduce regressions in expected behavior relating to the ability to enable diagnostics when establishing the initial connection. That's not blowing wind, that's a literal description of the exact problem. The PRs prevented any ability to do so—establishing the connection immediately upon construction of the connection management object. Initiating prior to any ability to invoke the method to set the debug level, which must happen post-initialization. This was documented in at least a few of them.

If there's a correction that can be applied downstream from the actual fault, which does not break existing expectations or behaviors or involve complete reimplementation of SMTP connection handling to preserve them, I'm all ears.

fdemian commented 1 year ago

Hi, I'm having this issue with python3.9.

Is this issue supposed to be exclusive to python3.8 and 3.7? Because I switched to 3.9 but it keeps happening. This is my code (note: there are some irrelevant methods missing).

import sys  # isort:skip
sys.modules["cgi.parse_qsl"] = None
from marrow.mailer import Mailer, Message
from os import path
from email.mime.text import MIMEText
from email.mime.multipart import MIMEMultipart

"""
... 
"""

def construct_email_message(mail_info, settings):
    activation_url = f"{mail_info['url']}/activation/{mail_info['code']}"
    plain_text = f"""Hello, {mail_info['user'].username}. This is your activation url
    {activation_url}
    """
    template_text = get_html_from_template("useractivation")
    html_message = modify_html_template(template_text, mail_info['user'], activation_url)

    # Construct message with plain and html text options.
    message = Message(author=settings["mailAddress"], to=mail_info['user'].email)
    message.subject = 'This is a subject!'
    message.plain = plain_text
    message.rich = html_message

    return message

async def send_email(mail_info):

    # Get mail settings.
    mail_settings = mail_info['options']
    url = mail_settings['url']
    port = mail_settings['port']
    server_url = mail_settings["smtpServer"]
    message = construct_email_message(mail_info, mail_settings)

    if mail_info['type'] == 'local':
        mailer = Mailer({
            'transport.use': 'maildir',
            'transport.directory': 'maildir'
        })
    else:
        user_name = mail_settings['auth']['username']
        user_pass = mail_settings['auth']['password']
        mailer = Mailer(
         dict(
          transport=dict(
            use='smtp',
            host=server_url,
            port='587',
            username=user_name,
            password=user_pass,
            tls='required'
          )
         )
        )

    mailer.start()
    mailer.send(message)
    mailer.stop()
    return

And I get: ValueError: server_hostname cannot be an empty string or start with a leading dot.

amcgregor commented 9 months ago

Been doing the rounds updating issues here; so glad to finally have a chance to do this during work hours. https://github.com/marrow/mailer/issues/66#issuecomment-1749135127 referencing this issue points out that this correction has been conditionally applied within the cegid branch:

marrow/mailer/transport/smtp.py

seventhridge commented 8 months ago

From what I can tell, in the event you want to initiate connection later and not during SMTP_SSL instantiation, you can explicitly pass host=None in Python 3.

Some detailed code inspection indicates that the error happens in the code as it is because the SMTP_SSL connection defaults host to empty string instead of None, and the socket library balks on the SMTP_SSL's self._host internal variable being an empty string. I do not know why SMTP_SSL's constructor defaults host to be an empty string when that clearly does not work with a later call to connect().

I believe according to the docs that providing the host during SMTP_SSL instantiation actually initiates a connection, which might prompt a few other code changes. In my fork, setting host=None works using Python 3.9. I haven't tested it on Python versions before 3.5 as in the cegid branch.

amcgregor commented 8 months ago

@seventhridge Notably, a diagnostic level / debug flag can't be set prior to connection initiation if a host is explicitly passed; this is the key point I've mentioned across all prior pull requests which "fixed" this by passing the host in. Thank you very much for a pull request I actually need to validate rather than immediately reject! 🙂

Also… so… uh… the Python standard library doesn't follow its own guidelines about is None comparisons? Gasp. Shock. Awe. 🤪 Tell me it ain't so!

mohllal commented 1 month ago

For those looking for a temporary solution to this problem until the underlying Python's issue is addressed.

I've created a custom SMTP transport class based on the standard SMTPTransport class that comes with the mailer package, with the sole adjustment being providing host and port arguments to the constructors of SMTP and SMTP_SSL classes in the connect_to_server method.

https://gist.github.com/mohllal/ae99252a79bda4af30f16630e49f72ea