Skip to content

gh-47711: Use connected source address for SMTP address literals - #159166

Open
ishu7w wants to merge 1 commit into
python:mainfrom
ishu7w:fix-smtp-ipv6-ehlo
Open

ishu7w wants to merge 1 commit into
python:mainfrom
ishu7w:fix-smtp-ipv6-ehlo

Conversation

@ishu7w

@ishu7w ishu7w commented Oct 11, 2026 •

Copy link
Copy Markdown

@ishu7w
ishu7w requested a review from a team as a code owner October 11, 2026 06:35
@python-cla-bot

python-cla-bot Bot commented Oct 11, 2026 •

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.

CLA signed

@read-the-docs-community

read-the-docs-community Bot commented Oct 11, 2026 •

Copy link
Copy Markdown

Comment thread Doc/library/smtplib.rst Outdated
: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 IP address is used as an address literal instead. IPv6 addresses use
the ``[IPv6:address]`` form. If the :meth:`connect` call returns anything other

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'm not sure it is useful to call out IPv6 separately here, since what we are doing is (now) following the standard, but I don't have a strong objection to including it.

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.

Updated the parameter documentation to describe the socket local address literal without singling IPv6 out.

Comment thread Lib/test/test_smtplib.py Outdated
)
for family, address, expected in cases:
for connect_in_init in (True, False):
with self.subTest(family=family, connect_in_init=connect_in_init):

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 good to include address in the subtest description.

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.

Added the address to the subTest description.

Comment thread Lib/smtplib.py
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.

Comment thread Lib/smtplib.py Outdated
if code == 220 and self._use_socket_hostname:
addr = self.sock.getsockname()[0]
if self.sock.family == socket.AF_INET6:
# An SMTP address literal does not include a scope identifier.

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.

For clarity, let's add: but does require an IPv6 "tag" per RFC 5321 Section 4.1.3

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.

Updated the comment to cite RFC 5321 section 4.1.3 and explain the IPv6 tag and scope-ID handling.

Comment thread Lib/smtplib.py
if self.sock.family == socket.AF_INET6:
# An SMTP address literal does not include a scope identifier.
addr = '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.

@@ -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, including
properly formatted IPv6 literals.

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'd add: "Previously IPv6 was not handled correctly."

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.

Updated the news entry to note that IPv6 was previously not handled correctly.

@bedevere-app

bedevere-app Bot commented Oct 15, 2026

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@ishu7w
ishu7w force-pushed the fix-smtp-ipv6-ehlo branch from a58dc50 to 93d6c56 Compare October 17, 2026 05:56
@ishu7w

ishu7w commented Oct 17, 2026 •

Copy link
Copy Markdown
Author

I pushed the requested code and documentation updates in 93d6c56 and replied to each inline review comment. The focused test_smtplib suite passes locally. The Unresolved review check still fails because the PR retains the awaiting changes label; GitHub denies label removal for my account, so a maintainer with label permission needs to clear it or update review status. The latest MSan run also failed only in unrelated test_socket.NetworkConnectionAttributesTest.testSourceAddress with OSError: [Errno 9] Bad file descriptor; the failing line calls getsockname() on a socket that the test runner reports as closed. Could a maintainer rerun MSan after clearing the review label?

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants