Improved authenticator system #218

pepoluan · 2020-12-14T08:39:18Z

What do these changes do?

In previous implementation of auth_callback, the authenticator callback function is only able to see three information: Mechanism of authentication, login/username, and password. The function cannot determine additional information such as the peer IP address, state of TLS, etc.

The proposed changes in this PR adds, as an alternative to auth_callback, a new parameter named authenticator.

The function passed to authenticator will not only see the three information as seen by auth_callback, it will also see server, session, and envelope (similar to how handler hooks are called -- although in AUTH case, envelope is likely empty and useless).

In addition, the return value of authenticator function is more informative instead of the overloaded, non-intuitive _TriStateType.

In addition, this PR also now supports AUTH LOGIN <b64encoded-username> because Python's smtplib.SMTP class has a buggy implementation of AUTH LOGIN (see bpo-27820).

Are there changes in behavior for the user?

Should be None. authenticator does not replace auth_callback; current users relying on auth_callback can continue doing so, while future users can use the much more powerful authenticator system.

Related issue number

Closes #219

Checklist

To prevent breakage, auth_callback is kept. But if `authenticator` is set, then auth_callback will be ignored. The internals of smtp_AUTH is also overhauled with the new system.

pepoluan · 2020-12-14T08:39:45Z

I'm keeping this as Draft until I have the time to properly test the changes on my test systems.

codecov · 2020-12-14T08:40:46Z

Codecov Report

Merging #218 (9975f43) into master (46540ff) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #218   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines         1100      1143   +43     
  Branches       197       202    +5     
=========================================
+ Hits          1100      1143   +43

Impacted Files	Coverage Δ
aiosmtpd/smtp.py	`100.00% <100.00%> (ø)`
aiosmtpd/lmtp.py	`100.00% <0.00%> (ø)`
aiosmtpd/main.py	`100.00% <0.00%> (ø)`
aiosmtpd/handlers.py	`100.00% <0.00%> (ø)`
aiosmtpd/controller.py	`100.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78ef869...2918c25. Read the comment docs.

pepoluan · 2020-12-21T07:18:11Z

I'm keeping this as Draft until I have the time to properly test the changes on my test systems.

Sorry, had been a very intense sprint in my job for the past 2.5 weeks. I can now pick up on things.

Testing progress:

Windows 10 (via PyCharm tox runner): (ALL)
Windows 10 (via PSCore 7.0.3): (ALL)
Windows 10 (via Cygwin): qa,py36-{nocov,cov}
Ubuntu 18.04 on WSL 1.0: (ALL) + pypy3-{nocov,cov,diffcov}
Ubuntu 18.04 on VBox: (ALL) + pypy3-{nocov,cov,diffcov}
Ubuntu 20.04 on VBox: (ALL) + pypy3-{nocov,cov,diffcov}
FreeBSD 12.1 on VBox: (ALL) + pypy3-nocov

All local testing passes, so I'm undrafting this.

... because the same intermittent error as the one explained in the comments started to rear its head again.

# Conflicts: # aiosmtpd/docs/NEWS.rst # aiosmtpd/docs/smtp.rst # aiosmtpd/smtp.py # aiosmtpd/tests/test_smtp.py

I originally use the term "stable", which was NOT used when I first created this file. Then Ned Deily put in the definition for "stable" -- which, for the record, I am actually disagreeing with -- so I had to coin a new, different term. I chose "frozen" to reflect the fact that the binaries won't ever be officially updated again.

pepoluan · 2020-12-22T06:46:10Z

.github/workflows/unit-testing-and-coverage.yml

+            # In addition, pypy3 implementation in Windows is ... complicated. We should stay
+            # away from all those complications; users choosing to use pypy3 on Windows to run
+            # aiosmtpd should be considered advanced, know what they're doing, and ready for
+            # all the consequences


A looong explanation on why we use 3.7 on Windows ... and a self-made term "frozen" because Ned thinks "stable" == "still receiving bugfixes".

(I personally think "stable" == "no possible (official) behavior change", but whatevs.)

pepoluan · 2020-12-22T06:46:54Z

aiosmtpd/docs/smtp.rst

+.. class:: SMTP(handler, *, data_size_limit=33554432, enable_SMTPUTF8=False, decode_data=False, \
+   hostname=None, ident=None, tls_context=None, require_starttls=False, timeout=300, \
+   auth_required=False, auth_require_tls=True, auth_exclude_mechanism=None, \
+   auth_callback=None, authenticator=None, \


Split overly-long signature line.

aiosmtpd/docs/smtp.rst

pepoluan · 2020-12-22T06:48:32Z

aiosmtpd/docs/smtp.rst

@@ -302,7 +332,4 @@ advertised, and the ``STARTTLS`` command will not be accepted.
 ``False``.

 .. _StreamReaderProtocol: https://docs.python.org/3/library/asyncio-stream.html#streamreaderprotocol
-.. _`RFC 3207`: http://www.faqs.org/rfcs/rfc3207.html
-.. _`RFC 2821`: https://www.ietf.org/rfc/rfc2821.txt


The :rfc: directive takes care of formatting and linking.

pepoluan · 2020-12-22T06:48:49Z

aiosmtpd/docs/smtp.rst

 .. _`asyncio transport`: https://docs.python.org/3/library/asyncio-protocol.html#asyncio-transport
-.. _ssl: https://docs.python.org/3/library/ssl.html


The :mod: directive takes care of formatting and linking.

pepoluan · 2020-12-22T06:50:47Z

aiosmtpd/smtp.py

        elif not arg:
-            await self.push('501 Not enough value')
-        else:


Code is pushed to the right and honestly looks really ugly. Especially as it has gained complexity due to having to cater to the new authenticator system while maintaining backward compatibility with the old auth_callback system.

So I changed all the naked awaits up there with return await

pepoluan · 2020-12-22T06:55:31Z

aiosmtpd/smtp.py

+    async def auth_LOGIN(self, _, args: List[str]) -> AuthResult:
+        if len(args) > 1:
+            try:
+                login = b64decode(args[1].encode(), validate=True)


This piece of code handles the -- actually RFC non-compliant -- AUTH LOGIN <b64_username> procedure.

Well, actually there is no RFC for AUTH LOGIN ... only a draft RFC that never gets adopted/promoted into full RFC status. So in this case, we can be more lenient with implementation of AUTH LOGIN.

(Also, Python>=3.5 smtplib.SMTP has a seriously buggy implementation of SASL LOGIN; see bpo-27820)

pepoluan · 2020-12-22T07:02:30Z

aiosmtpd/smtp.py

+            log.debug(f"auth_{mechanism} returned {auth_result}")
+
+            # New system using `authenticator` and AuthResult
+            if isinstance(auth_result, AuthResult):


This test handles backward- and forward-compatibility for auth_* methods.

pepoluan · 2020-12-22T07:06:05Z

aiosmtpd/tests/test_smtp.py

+
+    # Mark this as expectedFailure because smtplib.SMTP implementation in Python>=3.5
+    # is buggy. See bpo-27820
+    @unittest.expectedFailure


Until bpo-27820 is fixed, keep this expectedFailure line. When the bug is fixed, we'll ... figure something out.

pepoluan

I've left comments on possibly interesting points.

Because it seems that some SMTP client implementations expect to see "Username:" and "Password:" for the AUTH LOGIN challenges, while in actuality the nearest-to-authoritative documentation for AUTH LOGIN [1] _explicitly_ specifies that the contents of the challenges MUST be ignored by SMTP clients. So, these two knobs are added to cater for such noncompliant clients. [1] https://www.ietf.org/archive/id/draft-murchison-sasl-login-00.txt

# Conflicts: # aiosmtpd/docs/NEWS.rst # aiosmtpd/smtp.py # aiosmtpd/tests/test_smtp.py

For some reasons, PyCharm's code inspection insists that "self.parser" is not an instance of "FakeParser" and thus does not have the "message" attribute. Renaming "self.parser" to "self.fakeparser" totally fixes this issue.

As mentioned in Issue aio-libs#219

To prevent confusion with the new "authenticator" system

"enable_SMTPUTF8" is already in self.server_kwargs

And while we're at it, also make it into a list and do direct comparison, and using pytest's assert instead of assertEqual

After all, we only want to ensure it's successful, we no longer have to concern ourselves with login data. Also partially converted to pytest's assert system.

I have no idea why I tested that login_data must be None; we're testing for authentication success, and that would have had resulted in a not-None value. Anyways, the test is fixed and detailed into several tests that ensure authentication is successful and some fields are properly set.

AuthResult is now attrs-based and _requires_ keyword-only initializers. Also, the branch where auth is successful, it should have recorded the login's username & password.

This helps with troubleshooting. The piecemeal assertEquals failed when verifying the numeric code, while important information is "trapped" inside the message which did not get compared. With direct assert comparing the tuples, we see _both_ the wrong code _and_ the helpful message.

For characters <= \x7F, encode() and encode("ascii") will be identical anyways. But if the challenge str contains ... 'exotic' characters (for example, emojis), then encode("ascii") will fail while encode() will always succeeds. Whether client will be able to understand the challenge or not, that depends on the client, not our concern.

Also add another reference to another place in RFC4954 that explicitly requires trailing space after 334

pepoluan · 2021-01-19T16:46:27Z

aiosmtpd/docs/handlers.rst

@@ -183,7 +183,7 @@ those methods of the handler instance will override the built-in methods.

  :boldital:`server` is the instance of the ``SMTP`` class invoking the AUTH hook.
  This allows the AUTH hook implementation to invoke facilities such as the
-  ``push()`` and ``_auth_interact()`` methods.
+  ``push()`` and ``challenge_auth()`` methods.


Renamed the function so it doesn't start with "_"

Had to be creative here because "auth_*" prefix has its own meaning...

pepoluan · 2021-01-19T16:47:51Z

aiosmtpd/smtp.py

@@ -1,5 +1,6 @@
 import re
 import ssl
+import attr


Using attrs to enforce some behaviors.

pepoluan · 2021-01-19T16:50:10Z

aiosmtpd/testing/helpers.py

+    testcase.assertEqual(
+        (235, b"2.7.0 Authentication successful"),
+        response
+    )


Compare tuple against tuple directly so helpful error messages won't get hidden.

Also for assert_auth_invalid and assert_auth_required below.

pepoluan · 2021-01-19T16:50:52Z

aiosmtpd/tests/test_smtp.py

@@ -48,7 +57,7 @@ def tearDownModule():
    ModuleResources.close()


-def authenticator(mechanism, login, password):
+def auth_callback(mechanism, login, password):


Rename to prevent confusion with the brand new authenticator system.

pepoluan · 2021-01-19T16:53:29Z

aiosmtpd/tests/test_smtp.py

-            for actual, expected in zip(lines, expecteds):
-                self.assertEqual(actual, expected)
+            ]
+            assert lines == expecteds


Again, do direct compare list against list. The line-by-line comparison sometimes resulted in confusing failures. By comparing whole list against whole list, I can get a better grasp of what's going on.

Also, transition to pytest's assert-based testing.

pepoluan · 2021-01-19T16:54:49Z

aiosmtpd/tests/test_smtp.py

+            assert auth_peeker.login == b""
+            assert auth_peeker.password == b""
+            response = client.mail("alice@example.com")
+            assert response == (250, b"OK")


Replace the pointless "let's check that login_data doesn't change" test with a test that actually tests that 'protected' commands are accepted.

Also, migrated some tests to pytest's assert methodology.

pepoluan · 2021-01-19T16:56:13Z

aiosmtpd/tests/test_smtp.py

@@ -1154,9 +1344,8 @@ def test_help_authenticated(self):
                b64encode(b'\0goodlogin\0goodpasswd').decode()
            )
            assert_auth_success(self, code, response)
-            code, response = client.docmd('HELP')
-            self.assertEqual(code, 250)
-            self.assertEqual(response, SUPPORTED_COMMANDS_NOTLS)


Again, change from piece-by-piece comparison to direct tuple-vs-tuple comparison, in order to not hide possibly helpful error messages.

Also, migrate to pytest's assert syntax.

# Conflicts: # aiosmtpd/smtp.py # aiosmtpd/testing/helpers.py # aiosmtpd/tests/test_smtp.py

# Conflicts: # .github/workflows/unit-testing-and-coverage.yml # aiosmtpd/docs/NEWS.rst # aiosmtpd/tests/test_smtp.py

pepoluan · 2021-01-28T10:04:09Z

Huh, what's up with MacOS runner in GA? It's now intermittently failing. First time it failed with Pyhon 3.8, afterwards it failed with Python 3.6.

Luckily, with GA I can just ask it to "re-run all jobs" and 3rd time was the charm.

But this makes me wonder if I should drop the MacOS runner in the GA yml ...

pepoluan · 2021-01-28T16:21:12Z

As usual, I'm marinating this for 2x24 before squamerging.

# Conflicts: # aiosmtpd/docs/smtp.rst

pepoluan added 4 commits December 14, 2020 15:26

Implement authenticator system

b247b2f

To prevent breakage, auth_callback is kept. But if `authenticator` is set, then auth_callback will be ignored. The internals of smtp_AUTH is also overhauled with the new system.

Add test cases

0afce67

Update documentation

be0ffd9

Update NEWS

c6defe9

pepoluan added the enhancement label Dec 14, 2020

pepoluan added 4 commits December 21, 2020 14:26

Handle symlink creation error on Windows

33445d7

Silence some PyCharm warnings

22418ee

Increase loop.stop delay to 1.5

9a2c046

... because the same intermittent error as the one explained in the comments started to rear its head again.

Merge branch 'master' into authenticator_see_session

413f46f

# Conflicts: # aiosmtpd/docs/NEWS.rst # aiosmtpd/docs/smtp.rst # aiosmtpd/smtp.py # aiosmtpd/tests/test_smtp.py

pepoluan marked this pull request as ready for review December 22, 2020 06:42

pepoluan commented Dec 22, 2020

View reviewed changes

aiosmtpd/docs/smtp.rst Show resolved Hide resolved

pepoluan commented Dec 22, 2020

View reviewed changes

pepoluan requested review from waynew and warsaw December 22, 2020 07:11

pepoluan self-assigned this Dec 22, 2020

pepoluan added 3 commits December 22, 2020 14:28

Merge branch 'master' into authenticator_see_session

0ec5fd6

# Conflicts: # aiosmtpd/docs/NEWS.rst # aiosmtpd/smtp.py # aiosmtpd/tests/test_smtp.py

Silence code inspection warning

5684d0a

For some reasons, PyCharm's code inspection insists that "self.parser" is not an instance of "FakeParser" and thus does not have the "message" attribute. Renaming "self.parser" to "self.fakeparser" totally fixes this issue.

pepoluan added 13 commits January 16, 2021 23:33

Implement "AUTH LOGIN <username>" support

a85468e

As mentioned in Issue aio-libs#219

Rename "authenticator" func to "auth_callback"

e13650e

To prevent confusion with the new "authenticator" system

Tidy up imports

46f04ae

Fix: Prevent kwarg collision

98437d2

"enable_SMTPUTF8" is already in self.server_kwargs

Fix too-long line

f4ccdbd

And while we're at it, also make it into a list and do direct comparison, and using pytest's assert instead of assertEqual

Fix wrong thing being tested

e06145b

After all, we only want to ensure it's successful, we no longer have to concern ourselves with login data. Also partially converted to pytest's assert system.

Fix authenticator

45bb9d3

AuthResult is now attrs-based and _requires_ keyword-only initializers. Also, the branch where auth is successful, it should have recorded the login's username & password.

Remove leftover misunderstanding

b81c560

Add logging (DEBUG) of challenge sent

7e4d75d

Also add another reference to another place in RFC4954 that explicitly requires trailing space after 334

Merge branch 'master' into authenticator_see_session

c35b6bd

pepoluan commented Jan 19, 2021

View reviewed changes

aiosmtpd/smtp.py

@@ -1,5 +1,6 @@

import re

import ssl

import attr

Copy link

Collaborator Author

pepoluan Jan 19, 2021

Choose a reason for hiding this comment

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

Using attrs to enforce some behaviors.

pepoluan commented Jan 19, 2021

View reviewed changes

pepoluan added 2 commits January 24, 2021 18:55

Merge branch 'master' into authenticator_see_session

9975f43

# Conflicts: # aiosmtpd/smtp.py # aiosmtpd/testing/helpers.py # aiosmtpd/tests/test_smtp.py

Merge branch 'master' into authenticator_see_session

28c2459

# Conflicts: # .github/workflows/unit-testing-and-coverage.yml # aiosmtpd/docs/NEWS.rst # aiosmtpd/tests/test_smtp.py

pepoluan added the ready-to-merge PRs that are ready to merge label Jan 28, 2021

pepoluan added 2 commits January 29, 2021 21:28

Merge branch 'master' into authenticator_see_session

c122842

# Conflicts: # aiosmtpd/docs/smtp.rst

Merge branch 'master' into authenticator_see_session

2918c25

pepoluan merged commit 359d9c7 into aio-libs:master Jan 31, 2021

pepoluan deleted the authenticator_see_session branch February 19, 2021 19:16

amotzkau mentioned this pull request Apr 24, 2024

Old authenticators won't work #452

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved authenticator system #218

Improved authenticator system #218

pepoluan commented Dec 14, 2020 •

edited

Loading

pepoluan commented Dec 14, 2020

codecov bot commented Dec 14, 2020 •

edited

Loading

pepoluan commented Dec 21, 2020 •

edited

Loading

pepoluan Dec 22, 2020 •

edited

Loading

pepoluan Dec 22, 2020

pepoluan Dec 22, 2020

pepoluan Dec 22, 2020

pepoluan Dec 22, 2020

pepoluan Dec 22, 2020 •

edited

Loading

pepoluan Dec 22, 2020

pepoluan Dec 22, 2020

pepoluan left a comment •

edited

Loading

pepoluan Jan 19, 2021

pepoluan Jan 19, 2021

pepoluan Jan 19, 2021

pepoluan Jan 19, 2021

pepoluan Jan 19, 2021

pepoluan Jan 19, 2021

pepoluan Jan 19, 2021

pepoluan commented Jan 28, 2021

pepoluan commented Jan 28, 2021

		.. _`asyncio transport`: https://docs.python.org/3/library/asyncio-protocol.html#asyncio-transport
		.. _ssl: https://docs.python.org/3/library/ssl.html

Improved authenticator system #218

Improved authenticator system #218

Conversation

pepoluan commented Dec 14, 2020 • edited Loading

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

pepoluan commented Dec 14, 2020

codecov bot commented Dec 14, 2020 • edited Loading

Codecov Report

pepoluan commented Dec 21, 2020 • edited Loading

pepoluan Dec 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepoluan Dec 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepoluan left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepoluan commented Jan 28, 2021

pepoluan commented Jan 28, 2021

pepoluan commented Dec 14, 2020 •

edited

Loading

codecov bot commented Dec 14, 2020 •

edited

Loading

pepoluan commented Dec 21, 2020 •

edited

Loading

pepoluan Dec 22, 2020 •

edited

Loading

pepoluan Dec 22, 2020 •

edited

Loading

pepoluan left a comment •

edited

Loading