Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 3 additions & 1 deletion Doc/library/smtplib.rst
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,9 @@ Protocol) and :rfc:`1869` (SMTP Service Extensions).

If specified, *local_hostname* is used as the FQDN of the local host in the HELO/EHLO
command. Otherwise, the local hostname is found using
:func:`socket.getfqdn`. If the :meth:`connect` call returns anything other
:func:`socket.getfqdn`. If it is not fully qualified, the connected socket's
local address is used as an address literal instead. If the :meth:`connect`
call returns anything other
than a success code, an :exc:`SMTPConnectError` is raised. The optional
*timeout* parameter specifies a timeout in seconds for blocking operations
like the connection attempt (if not specified, the global default timeout
Expand Down
19 changes: 14 additions & 5 deletions Lib/smtplib.py
Original file line number Diff line number Diff line change
Expand Up @@ -257,11 +257,7 @@ def __init__(self, host='', port=0, local_hostname=None,
self.source_address = source_address
self._auth_challenge_count = 0

if host:
(code, msg) = self.connect(host, port)
if code != 220:
self.close()
raise SMTPConnectError(code, msg)
self._find_local_hostname = False
if local_hostname is not None:
self.local_hostname = local_hostname
else:
Expand All @@ -273,13 +269,20 @@ def __init__(self, host='', port=0, local_hostname=None,
self.local_hostname = fqdn
else:
# We can't find an fqdn hostname, so use a domain literal
self._find_local_hostname = True
addr = '127.0.0.1'
try:
addr = socket.gethostbyname(socket.gethostname())
except socket.gaierror:
pass
self.local_hostname = '[%s]' % addr

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As far as I can see, the second part of this inner else is now code that doesn't have any effect, since local_hostname then gets reset unconditionally in connect. I suggest the following:

        self.local_hostname = local_hostname
        if local_hostname is None:
            # RFC 2821 says we should use the fqdn in the EHLO/HELO verb, and
            # if that can't be calculated, that we should use a domain literal
            # instead (essentially an encoded IP address like [A.B.C.D]).
            fqdn = socket.getfqdn()
            if '.' in fqdn:
                self.local_hostname = fqdn
            else:
                # We can't find an fqdn hostname, so we'll compute the
                # correct domain literal in connect.
                self._find_local_hostname = True

And then of course changing _use_socket_hostname to _find_local_hostname in connect. What do you think? The tests, including the new ones, still pass.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Changed the flag to _find_local_hostname and now defer hostname fallback address selection to connect, following your suggested flow.

if host:
(code, msg) = self.connect(host, port)
if code != 220:
self.close()
raise SMTPConnectError(code, msg)

def __enter__(self):
return self

Expand Down Expand Up @@ -348,6 +351,12 @@ def connect(self, host='localhost', port=0, source_address=None):
self.sock = self._get_socket(host, port, self.timeout)
self.file = None
(code, msg) = self.getreply()
if code == 220 and self._find_local_hostname:
addr = self.sock.getsockname()[0]
if self.sock.family == socket.AF_INET6:
# RFC 5321 section 4.1.3 requires the IPv6 tag and excludes scope IDs.
addr = f"IPv6:{addr.split('%', 1)[0]}"
self.local_hostname = '[%s]' % addr

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it would be clearer to use an f-string here.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Changed the IPv6 literal construction to an f-string.

if self.debuglevel > 0:
self._print_debug('connect:', repr(msg))
return (code, msg)
Expand Down
64 changes: 64 additions & 0 deletions Lib/test/test_smtplib.py
Original file line number Diff line number Diff line change
Expand Up @@ -696,6 +696,70 @@ def testSendMessageMultipleResentRaises(self):
smtp.send_message(m)
smtp.close()

class LocalHostnameTests(unittest.TestCase):

def test_connected_address_literal(self):
cases = (
(socket.AF_INET, ('192.0.2.1', 1234), '[192.0.2.1]'),
(socket.AF_INET6, ('2001:db8::1', 1234, 0, 0),
'[IPv6:2001:db8::1]'),
(socket.AF_INET6, ('fe80::1%eth0', 1234, 0, 1), '[IPv6:fe80::1]'),
)
for family, address, expected in cases:
for connect_in_init in (True, False):
with self.subTest(family=family, address=address,
connect_in_init=connect_in_init):
sock = Mock(family=family)
sock.getsockname.return_value = address
with mock.patch('smtplib.socket.getfqdn', return_value='host'), \
mock.patch('smtplib.socket.gethostbyname',
return_value='127.0.0.1'), \
mock.patch.object(smtplib.SMTP, '_get_socket',
return_value=sock), \
mock.patch.object(smtplib.SMTP, 'getreply',
return_value=(220, b'OK')):
smtp = smtplib.SMTP('server' if connect_in_init else '')
self.addCleanup(smtp.close)
if not connect_in_init:
smtp.connect('server')
self.assertEqual(smtp.local_hostname, expected)
smtp.ehlo()
sock.sendall.assert_called_with(
('ehlo ' + expected + '\r\n').encode('ascii'))

def test_address_literal_updated_on_reconnect(self):
sock = Mock(family=socket.AF_INET6)
sock.getsockname.side_effect = [
('2001:db8::1', 1234, 0, 0), ('2001:db8::2', 1234, 0, 0)]
with mock.patch('smtplib.socket.getfqdn', return_value='host'), \
mock.patch('smtplib.socket.gethostbyname', return_value='127.0.0.1'), \
mock.patch.object(smtplib.SMTP, '_get_socket', return_value=sock), \
mock.patch.object(smtplib.SMTP, 'getreply', return_value=(220, b'OK')):
smtp = smtplib.SMTP('server')
self.addCleanup(smtp.close)
self.assertEqual(smtp.local_hostname, '[IPv6:2001:db8::1]')
smtp.close()
smtp.connect('other-server')
self.assertEqual(smtp.local_hostname, '[IPv6:2001:db8::2]')

def test_hostname_preserved(self):
for local_hostname, fqdn, expected in (
('custom', 'host', 'custom'),
(None, 'host.example', 'host.example'),
):
with self.subTest(local_hostname=local_hostname, fqdn=fqdn):
sock = Mock(family=socket.AF_INET6)
with mock.patch('smtplib.socket.getfqdn', return_value=fqdn), \
mock.patch.object(smtplib.SMTP, '_get_socket',
return_value=sock), \
mock.patch.object(smtplib.SMTP, 'getreply',
return_value=(220, b'OK')):
smtp = smtplib.SMTP('server', local_hostname=local_hostname)
self.addCleanup(smtp.close)
self.assertEqual(smtp.local_hostname, expected)
sock.getsockname.assert_not_called()


class NonConnectingTests(unittest.TestCase):

def testNotConnected(self):
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
When no fully qualified hostname is available, :mod:`smtplib` now uses the
connected socket's local address for the HELO/EHLO address literal. Previously,
IPv6 was not handled correctly.
Loading