bpo-27820: Fix AUTH LOGIN logic in smtplib.SMTP #24118

pepoluan · 2021-01-05T14:06:10Z

Previously, smtplib.SMTP.auth_login responded with username if challenge is not given, and always responded with password if challenge is given. This causes auth failure when initial_response_ok=False.

The standard for AUTH LOGIN specifies that clients should simply respond with base64-encoded username on first challenge, and with base64-encoded password on second challenge, disregarding the actual contents of the challenges themselves.

Suitable test cases have been added to test_smtplib.py + a fix for a long-standing bug in test_smtplib.py where setting initial_response_ok=False will always result in "Internal Confusion" error.

https://bugs.python.org/issue27820

pepoluan · 2021-01-10T11:00:56Z

Lib/test/test_smtplib.py

@@ -785,7 +785,7 @@ def found_terminator(self):
            except ResponseException as e:
                self.smtp_state = self.COMMAND
                self.push('%s %s' % (e.smtp_code, e.smtp_error))
-                return
+            return


The mis-indented return caused the test for smtpd state to "fall through" to the next test, resulting ultimately in "Internal Confusion" error.

pepoluan · 2021-01-10T11:03:38Z

Lib/smtplib.py

@@ -662,7 +666,7 @@ def auth_plain(self, challenge=None):
    def auth_login(self, challenge=None):
        """ Authobject to use with LOGIN authentication. Requires self.user and
        self.password to be set."""
-        if challenge is None:
+        if challenge is None or self._auth_challenge_count < 2:


The draft standard for AUTH LOGIN specifies that clients SHOULD ignore the actual contents of the challenge, and respond only based on how many challenges have been received.

github-actions · 2021-02-10T00:23:36Z

This PR is stale because it has been open for 30 days with no activity.

pepoluan · 2021-02-10T02:53:39Z

This PR is stale because it has been open for 30 days with no activity.

Well, it's a clean, non-conflicting PR (so far) ... so there's no need to keep updating it? 😅

pepoluan · 2021-02-22T18:40:30Z

Bump

Lib/smtplib.py

And in the process also fix a longstanding bug in the SimSMTPChannel.found_terminator() method that causes inability to test SMTP AUTH with initial_response_ok=False.

pepoluan

Response to some questions.

Lib/smtplib.py

Broken/buggy server might 'trap' us in AUTH. We raise an exception and let the user handle things.

pepoluan · 2021-03-10T04:23:49Z

Lib/smtplib.py

+            # If server keeps sending challenges, something is wrong.
+            if self._auth_challenge_count > _MAXCHALLENGE:
+                raise SMTPException(
+                    "Server AUTH mechanism infinite loop. Last response: "
+                    + repr((code, resp))
+                )


This guards against broken/buggy server that traps us in the AUTH state.

At this point, state is indeterminate, and we should let user handle things. That's why it simply raises. (The only "guaranteed safe" thing to do is close(), actually.)

Lib/smtplib.py

"AUTH trap" is when server keeps sending AUTH Challenge (334 ...)

pepoluan · 2021-03-10T19:09:37Z

Lib/test/test_smtplib.py

+    def _auth_buggy(self, arg=None):
+        # This AUTH mechanism will 'trap' client in a neverending 334
+        # base64 encoded 'BuGgYbUgGy'
+        self.push('334 QnVHZ1liVWdHeQ==')


Totally made-up AUTH mechanism whose purpose is to 'trap' the client in an infinite loop of 334 (AUTH Challenge)

pepoluan · 2021-03-10T19:11:06Z

Lib/test/test_smtplib.py

+        smtp = smtplib.SMTP(
+            HOST, self.port, local_hostname='localhost',
+            timeout=support.LOOPBACK_TIMEOUT
+        )
+        try:
+            smtp.user, smtp.password = sim_auth
+            smtp.ehlo("test_auth_buggy")
+            expect = r"^Server AUTH mechanism infinite loop.*"
+            with self.assertRaisesRegex(smtplib.SMTPException, expect) as cm:
+                smtp.auth("BUGGY", auth_buggy, initial_response_ok=False)
+        finally:
+            smtp.close()


Can't use smtplib.SMTP as context manager here ... its __exit__ causes a different exception to be raised in addition. So the test uses a decidedly "lower level" mechanism to close the connection.

VSCode did not indicate any problems, that's why I missed this during previous push.

orsenthil

LGTM. Thank you, @pepoluan .

miss-islington · 2021-03-12T23:25:52Z

Thanks @pepoluan for the PR, and @orsenthil for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8, 3.9.
🐍🍒⛏🤖

* Fix auth_login logic (bpo-27820) * Also fix a longstanding bug in the SimSMTPChannel.found_terminator() method that causes inability to test SMTP AUTH with initial_response_ok=False. (cherry picked from commit 7591d94) Co-authored-by: Pandu E POLUAN <[email protected]>

bedevere-bot · 2021-03-12T23:26:15Z

GH-24832 is a backport of this pull request to the 3.9 branch.

miss-islington · 2021-03-12T23:26:16Z

Sorry, @pepoluan and @orsenthil, I could not cleanly backport this to 3.8 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 7591d9455eb37525c832da3d65e1a7b3e6dbf613 3.8

bedevere-bot · 2021-03-12T23:38:21Z

GH-24833 is a backport of this pull request to the 3.8 branch.

* Fix auth_login logic (bpo-27820) * Also fix a longstanding bug in the SimSMTPChannel.found_terminator() method that causes inability to test SMTP AUTH with initial_response_ok=False. (cherry picked from commit 7591d94) Co-authored-by: Pandu E POLUAN <[email protected]>

) * bpo-27820: Fix AUTH LOGIN logic in smtplib.SMTP (GH-24118) * Fix auth_login logic (bpo-27820) * Also fix a longstanding bug in the SimSMTPChannel.found_terminator() method that causes inability to test SMTP AUTH with initial_response_ok=False. (cherry picked from commit 7591d94) * Set timeout to 15 directly. Co-authored-by: Pandu E POLUAN <[email protected]>

bpo-27820: Fix AUTH LOGIN logic in smtplib.SMTP (pythonGH-24118)

pepoluan requested a review from a team as a code owner January 5, 2021 14:06

the-knights-who-say-ni added the CLA signed label Jan 5, 2021

bedevere-bot added the awaiting review label Jan 5, 2021

pepoluan mentioned this pull request Jan 5, 2021

Implement support for "AUTH LOGIN <username>" aio-libs/aiosmtpd#219

Closed

pepoluan commented Jan 10, 2021

View reviewed changes

github-actions bot added the stale Stale PR or inactive for long period of time. label Feb 10, 2021

github-actions bot removed the stale Stale PR or inactive for long period of time. label Feb 11, 2021

orsenthil reviewed Mar 9, 2021

View reviewed changes

Lib/smtplib.py Show resolved Hide resolved

orsenthil self-assigned this Mar 9, 2021

pepoluan added 2 commits March 10, 2021 11:06

Fix auth_login logic (bpo-27820)

be73188

Add test for bpo-27820 fix

dca899d

And in the process also fix a longstanding bug in the SimSMTPChannel.found_terminator() method that causes inability to test SMTP AUTH with initial_response_ok=False.

pepoluan commented Mar 10, 2021

View reviewed changes

Lib/smtplib.py Show resolved Hide resolved

Implement challenge count limit

481ca55

Broken/buggy server might 'trap' us in AUTH. We raise an exception and let the user handle things.

pepoluan force-pushed the bpo-27820-fix-authlogin branch from 5c8f4d8 to 481ca55 Compare March 10, 2021 04:21

pepoluan commented Mar 10, 2021

View reviewed changes

Add NEWS entry for bpo-27820 fix

c28a745

orsenthil reviewed Mar 10, 2021

View reviewed changes

Lib/smtplib.py Show resolved Hide resolved

Add test case for AUTH trap

08b9d63

"AUTH trap" is when server keeps sending AUTH Challenge (334 ...)

pepoluan commented Mar 10, 2021

View reviewed changes

Fix whitespace

4304e2b

VSCode did not indicate any problems, that's why I missed this during previous push.

orsenthil approved these changes Mar 12, 2021

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Mar 12, 2021

orsenthil added needs backport to 3.8 needs backport to 3.9 only security fixes labels Mar 12, 2021

orsenthil merged commit 7591d94 into python:master Mar 12, 2021

bedevere-bot removed the awaiting merge label Mar 12, 2021

bedevere-bot removed the needs backport to 3.9 only security fixes label Mar 12, 2021

bedevere-bot removed the needs backport to 3.8 label Mar 12, 2021

sthagen added a commit to sthagen/python-cpython that referenced this pull request Mar 13, 2021

Merge pull request #424 from python/master

922c420

bpo-27820: Fix AUTH LOGIN logic in smtplib.SMTP (pythonGH-24118)

Uh oh!

bpo-27820: Fix AUTH LOGIN logic in smtplib.SMTP #24118

bpo-27820: Fix AUTH LOGIN logic in smtplib.SMTP #24118

Uh oh!

Conversation

pepoluan commented Jan 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pepoluan Jan 10, 2021

Choose a reason for hiding this comment

Uh oh!

pepoluan Jan 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 10, 2021

Uh oh!

pepoluan commented Feb 10, 2021

Uh oh!

pepoluan commented Feb 22, 2021

Uh oh!

Uh oh!

pepoluan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pepoluan Mar 10, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pepoluan Mar 10, 2021

Choose a reason for hiding this comment

Uh oh!

pepoluan Mar 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orsenthil left a comment

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Mar 12, 2021

Uh oh!

bedevere-bot commented Mar 12, 2021

Uh oh!

miss-islington commented Mar 12, 2021

Uh oh!

bedevere-bot commented Mar 12, 2021

Uh oh!

Uh oh!

pepoluan commented Jan 5, 2021 •

edited

Loading

pepoluan Jan 10, 2021 •

edited

Loading

pepoluan Mar 10, 2021 •

edited

Loading