Repository: aio-libs/aiosmtpd
Pull request #232 (2021-01-15 07:12:06)

BUGFIX: RFC4954 behavior for lone "="
<!-- Thank you for your contribution! -->

## What do these changes do?

When implementing AUTH support, I misunderstood the meaning of `=` (lone equal sign) in [RFC 4954](https://tools.ietf.org/html/rfc4954).

This has been fixed to strictly comply with RFC 4954 and also [RFC 4616](https://tools.ietf.org/html/rfc4616)

This mostly impacts `AUTH PLAIN` which requires a special null-separated struct.

## Are there changes in behavior for the user?

If user (of this library, not an SMTP client) wants to test AUTH PLAIN with null login, then sending `AUTH PLAIN =` or as a response to the challenge will no longer work. This shouldn't be a problem if test is done using Standard Lib's `smtplib.SMTP`, because `smtplib.SMTP.auth_plain` already properly creates the necessary null-separated struct.

## Related issue number

None

## Checklist

- [x] I think the code is well written
- [x] Unit tests for the changes exist
- [x] tox testenvs have been executed in the following environments:
   - [x] Windows 10 (via PyCharm tox runner): `(ALL)`
   - [x] Windows 10 (via PSCore 7.1.0): `(ALL)`
   - [x] Windows 10 (via Cygwin): `qa,py36-{nocov,cov}`
   - [x] Ubuntu 18.04 on WSL 1.0: `(ALL)` + `pypy3-{nocov,cov,diffcov}`
   - [x] Ubuntu 18.04 on VBox: `(ALL)` + `pypy3-{nocov,cov,diffcov}`
   - [x] Ubuntu 20.04 on VBox: `(ALL)` + `pypy3-{nocov,cov,diffcov}`
   - [x] FreeBSD 12.1 on VBox: `(ALL)` + `pypy3-nocov`
- [x] Documentation reflects the changes
- [x] Add a news fragment into the `NEWS.rst` file


codecov[bot]: # [Codecov](https://codecov.io/gh/aio-libs/aiosmtpd/pull/232?src=pr&el=h1) Report
> Merging [#232](https://codecov.io/gh/aio-libs/aiosmtpd/pull/232?src=pr&el=desc) (a802e4f) into [master](https://codecov.io/gh/aio-libs/aiosmtpd/commit/9343a296c01f2d8988623b79260709212c8785aa?el=desc) (9343a29) will **decrease** coverage by `0.27%`.
> The diff coverage is `91.89%`.

[![Impacted file tree graph](https://codecov.io/gh/aio-libs/aiosmtpd/pull/232/graphs/tree.svg?width=650&height=150&src=pr&token=k9dc7NVG69)](https://codecov.io/gh/aio-libs/aiosmtpd/pull/232?src=pr&el=tree)

```diff
@@             Coverage Diff             @@
##            master     #232      +/-   ##
===========================================
- Coverage   100.00%   99.72%   -0.28%     
===========================================
  Files            5        5              
  Lines         1087     1091       +4     
  Branches       195      193       -2     
===========================================
+ Hits          1087     1088       +1     
- Misses           0        2       +2     
- Partials         0        1       +1     
```


| [Impacted Files](https://codecov.io/gh/aio-libs/aiosmtpd/pull/232?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [aiosmtpd/smtp.py](https://codecov.io/gh/aio-libs/aiosmtpd/pull/232/diff?src=pr&el=tree#diff-YWlvc210cGQvc210cC5weQ==) | `99.60% <91.89%> (-0.40%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/aio-libs/aiosmtpd/pull/232?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/aio-libs/aiosmtpd/pull/232?src=pr&el=footer). Last update [9343a29...a802e4f](https://codecov.io/gh/aio-libs/aiosmtpd/pull/232?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/aiosmtpd/docs/NEWS.rst b/aiosmtpd/docs/NEWS.rst
index db1576a..260d382 100644
--- a/aiosmtpd/docs/NEWS.rst
+++ b/aiosmtpd/docs/NEWS.rst
@@ -3,6 +3,16 @@
 ===================
 
 
+1.2.4 (aiosmtpd-next)
+=====================
+
+Fixed/Improved
+--------------
+* Remove special handling for lone ``=`` during AUTH;
+  it is now treated as simple Base64-encoded ``b""``.
+  This is the correct, strict interpretation of :rfc:`4954` mentions about ``=``
+
+
 1.2.3 (2021-01-14)
 ==================
 
diff --git a/aiosmtpd/docs/handlers.rst b/aiosmtpd/docs/handlers.rst
index f661c93..7f45007 100644
--- a/aiosmtpd/docs/handlers.rst
+++ b/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.
 
   :boldital:`args` is a list of string split from the string after the ``AUTH`` command.
   ``args[0]`` is always equal to ``MECHANISM``.
diff --git a/aiosmtpd/docs/requirements.txt b/aiosmtpd/docs/requirements.txt
new file mode 100644
index 0000000..3323f6d
--- /dev/null
+++ b/aiosmtpd/docs/requirements.txt
@@ -0,0 +1,2 @@
+sphinx>=2.1
+atpublic
diff --git a/aiosmtpd/smtp.py b/aiosmtpd/smtp.py
index c245fd0..da9c7a9 100644
--- a/aiosmtpd/smtp.py
+++ b/aiosmtpd/smtp.py
@@ -9,12 +9,13 @@ import binascii
 import collections
 import asyncio.sslproto as sslproto
 
-from base64 import b64decode
+from base64 import b64decode, b64encode
 from email._header_value_parser import get_addr_spec, get_angle_addr
 from email.errors import HeaderParseError
 from public import public
 from typing import (
     Any,
+    AnyStr,
     Awaitable,
     Callable,
     Dict,
@@ -59,7 +60,7 @@ __all__ = [
     "AuthMechanismType",
     "MISSING",
 ]  # Will be added to by @public
-__version__ = '1.2.3'
+__version__ = '1.2.4a1'
 __ident__ = 'Python SMTP {}'.format(__version__)
 log = logging.getLogger('mail.log')
 
@@ -383,10 +384,14 @@ class SMTP(asyncio.StreamReaderProtocol):
         """Reset all state variables except the greeting."""
         self._set_post_data_state()
 
-    async def push(self, status):
-        response = bytes(
-            status + '\r\n', 'utf-8' if self.enable_SMTPUTF8 else 'ascii')
-        self._writer.write(response)
+    async def push(self, status: AnyStr):
+        if isinstance(status, str):
+            response = bytes(
+                status, 'utf-8' if self.enable_SMTPUTF8 else 'ascii')
+        else:
+            response = status
+        assert isinstance(response, bytes)
+        self._writer.write(response + b"\r\n")
         log.debug("%r << %r", self.session.peer, response)
         await self._writer.drain()
 
@@ -723,18 +728,36 @@ class SMTP(asyncio.StreamReaderProtocol):
             if status is not None:  # pragma: no branch
                 await self.push(status)
 
-    async def _auth_interact(self, server_message) -> _TriStateType:
-        blob: bytes
-        await self.push(server_message)
+    async def challenge_auth(
+            self,
+            challenge: AnyStr,
+            encode_to_b64: bool = True,
+    ) -> Union[_Missing, bytes]:
+        """
+        Send challenge during authentication. "334 " will be prefixed, so do NOT
+        put "334 " at start of server_message.
+
+        :param challenge: Challenge to send to client. If str, will be utf8-encoded.
+        :param encode_to_b64: If true, then perform Base64 encoding on challenge
+        :return: Response from client, or MISSING
+        """
+        challenge = (
+            challenge.encode() if isinstance(challenge, str) else challenge
+        )
+        assert isinstance(challenge, bytes)
+        # Trailing space is MANDATORY even if challenge is empty.
+        # See:
+        #   - https://tools.ietf.org/html/rfc4954#page-4 ¶ 5
+        #   - https://tools.ietf.org/html/rfc4954#page-13 "continue-req"
+        challenge = b"334 " + (b64encode(challenge) if encode_to_b64 else challenge)
+        log.debug("Send challenge to %r: %r", self.session.peer, challenge)
+        await self.push(challenge)
         line = await self._reader.readline()
-        blob = line.strip()
-        # '=' and '*' handling are in accordance with RFC4954
-        if blob == b"=":
-            log.debug("%r responded with '='", self.session.peer)
-            return None
+        blob: bytes = line.strip()
+        # '*' handling in accordance with RFC4954
         if blob == b"*":
-            log.warning("%r aborted with '*'", self.session.peer)
-            await self.push("501 Auth aborted")
+            log.warning("%r aborted AUTH with '*'", self.session.peer)
+            await self.push("501 5.7.0 Auth aborted")
             return MISSING
         try:
             decoded_blob = b64decode(blob, validate=True)
@@ -744,6 +767,22 @@ class SMTP(asyncio.StreamReaderProtocol):
             return MISSING
         return decoded_blob
 
+    _334_PREFIX = re.compile(r"^334 ")
+
+    async def _auth_interact(
+            self,
+            server_message: str
+    ) -> Union[_Missing, bytes]:  # pragma: nocover
+        warn(
+            "_auth_interact will be deprecated in version 2.0. "
+            "Please use challenge_auth() instead.",
+            DeprecationWarning
+        )
+        return await self.challenge_auth(
+            challenge=self._334_PREFIX.sub("", server_message),
+            encode_to_b64=False,
+        )
+
     # IMPORTANT NOTES FOR THE auth_* METHODS
     #
     # 1. For internal methods, due to how they are called, we must ignore
@@ -758,9 +797,6 @@ class SMTP(asyncio.StreamReaderProtocol):
     #      - 'identity' is not always username, depending on the auth mecha-
     #        nism. Might be a session key, a one-time user ID, or any kind of
     #        object, actually.
-    #      - If the client provides "=" for username during interaction, the
-    #        method MUST return b"" (empty bytes) NOT None, because None has been
-    #        used to indicate error/login failure.
     # 3. Auth credentials checking is performed in the auth_* methods because
     #    more advanced auth mechanism might not return login+password pair
     #    (see #2 above)
@@ -768,54 +804,45 @@ class SMTP(asyncio.StreamReaderProtocol):
     async def auth_PLAIN(self, _, args: List[str]):
         login_and_password: _TriStateType
         if len(args) == 1:
-            # Trailing space is MANDATORY
-            # See https://tools.ietf.org/html/rfc4954#page-4 ¶ 5
-            login_and_password = await self._auth_interact("334 ")
+            login_and_password = await self.challenge_auth("")
             if login_and_password is MISSING:
                 return
-        else:
-            blob = args[1]
-            if blob == "=":
-                login_and_password = None
-            else:
-                try:
-                    login_and_password = b64decode(blob, validate=True)
-                except Exception:
-                    await self.push("501 5.5.2 Can't decode base64")
-                    return
-        # Decode login data
-        if login_and_password is None:
-            login = password = None
         else:
             try:
-                _, login, password = login_and_password.split(b"\x00")
-            except ValueError:  # not enough args
-                await self.push("501 5.5.2 Can't split auth value")
+                login_and_password = b64decode(args[1].encode(), validate=True)
+            except Exception:
+                await self.push("501 5.5.2 Can't decode base64")
                 return
+        try:
+            # login data is "{authz_id}\x00{login_id}\x00{password}"
+            # authz_id can be null, and currently ignored
+            # See https://tools.ietf.org/html/rfc4616#page-3
+            _, login, password = login_and_password.split(b"\x00")
+        except ValueError:  # not enough args
+            await self.push("501 5.5.2 Can't split auth value")
+            return
         # Verify login data
+        assert login is not None
+        assert password is not None
         if self._auth_callback("PLAIN", login, password):
-            if login is None:
-                login = EMPTYBYTES
             return login
         else:
             return MISSING
 
     async def auth_LOGIN(self, _, args: List[str]):
         login: _TriStateType
-        # 'User Name\x00'
-        login = await self._auth_interact("334 VXNlciBOYW1lAA==")
+        login = await self.challenge_auth(b"User Name\x00")
         if login is MISSING:
             return
 
         password: _TriStateType
-        # 'Password\x00'
-        password = await self._auth_interact("334 UGFzc3dvcmQA")
+        password = await self.challenge_auth(b"Password\x00")
         if password is MISSING:
             return
 
+        assert login is not None
+        assert password is not None
         if self._auth_callback("LOGIN", login, password):
-            if login is None:  # pragma: no branch
-                login = EMPTYBYTES
             return login
         else:
             return MISSING
diff --git a/readthedocs.yml b/readthedocs.yml
new file mode 100644
index 0000000..634d7e3
--- /dev/null
+++ b/readthedocs.yml
@@ -0,0 +1,14 @@
+version: 2
+
+build:
+  image: latest
+
+sphinx:
+  builder: html
+  configuration: conf.py
+  fail_on_warning: true
+
+python:
+  version: 3.7
+  install:
+    - requirements: aiosmtpd/docs/requirements.txt


Test patch:
diff --git a/aiosmtpd/testing/helpers.py b/aiosmtpd/testing/helpers.py
index b35b4ae..a02568e 100644
--- a/aiosmtpd/testing/helpers.py
+++ b/aiosmtpd/testing/helpers.py
@@ -65,19 +65,25 @@ def start(plugin):
         warnings.filterwarnings('always', category=ResourceWarning)
 
 
-def assert_auth_success(testcase: TestCase, code, response):
-    testcase.assertEqual(code, 235)
-    testcase.assertEqual(response, b"2.7.0 Authentication successful")
+def assert_auth_success(testcase: TestCase, *response):
+    testcase.assertEqual(
+        (235, b"2.7.0 Authentication successful"),
+        response
+    )
 
 
-def assert_auth_invalid(testcase: TestCase, code, response):
-    testcase.assertEqual(code, 535)
-    testcase.assertEqual(response, b'5.7.8 Authentication credentials invalid')
+def assert_auth_invalid(testcase: TestCase, *response):
+    testcase.assertEqual(
+        (535, b"5.7.8 Authentication credentials invalid"),
+        response
+    )
 
 
-def assert_auth_required(testcase: TestCase, code, response):
-    testcase.assertEqual(code, 530)
-    testcase.assertEqual(response, b'5.7.0 Authentication required')
+def assert_auth_required(testcase: TestCase, *response):
+    testcase.assertEqual(
+        (530, b"5.7.0 Authentication required"),
+        response
+    )
 
 
 SUPPORTED_COMMANDS_TLS: bytes = (
diff --git a/aiosmtpd/tests/test_smtp.py b/aiosmtpd/tests/test_smtp.py
index e9319f3..c2eed49 100644
--- a/aiosmtpd/tests/test_smtp.py
+++ b/aiosmtpd/tests/test_smtp.py
@@ -921,7 +921,7 @@ class TestSMTP(unittest.TestCase):
             with patch("logging.Logger.warning"):
                 code, response = client.docmd('*')
             self.assertEqual(code, 501)
-            self.assertEqual(response, b'Auth aborted')
+            self.assertEqual(response, b"5.7.0 Auth aborted")
 
     def test_auth_two_steps_bad_base64_encoding(self):
         with SMTP(*self.address) as client:
@@ -941,11 +941,14 @@ class TestSMTP(unittest.TestCase):
             )
             assert_auth_success(self, code, response)
 
-    def test_auth_no_credentials(self):
+    def test_auth_plain_null(self):
         with SMTP(*self.address) as client:
             client.ehlo('example.com')
-            code, response = client.docmd('AUTH PLAIN =')
-            assert_auth_invalid(self, code, response)
+            response = client.docmd('AUTH PLAIN =')
+            self.assertEqual(
+                (501, b"5.5.2 Can't split auth value"),
+                response
+            )
 
     def test_auth_two_steps_no_credentials(self):
         with SMTP(*self.address) as client:
@@ -953,7 +956,9 @@ class TestSMTP(unittest.TestCase):
             code, response = client.docmd('AUTH PLAIN')
             self.assertEqual(code, 334)
             self.assertEqual(response, b'')
-            code, response = client.docmd('=')
+            # "AAA=" is Base64 encoded "\x00\x00", representing null username and null
+            # password. See https://tools.ietf.org/html/rfc4616#page-3
+            code, response = client.docmd("AAA=")
             assert_auth_invalid(self, code, response)
 
     def test_auth_login_multisteps_no_credentials(self):
@@ -1000,10 +1005,12 @@ class TestSMTPAuth(unittest.TestCase):
             code, response = client.docmd("AUTH PLAIN")
             self.assertEqual(code, 334)
             self.assertEqual(response, b"")
-            code, response = client.docmd('=')
+            # "AAA=" is Base64 encoded "\x00\x00", representing null username and
+            # null password. See https://tools.ietf.org/html/rfc4616#page-3
+            code, response = client.docmd("AAA=")
             assert_auth_success(self, code, response)
-            self.assertEqual(auth_peeker.login, None)
-            self.assertEqual(auth_peeker.password, None)
+            self.assertEqual(auth_peeker.login, b"")
+            self.assertEqual(auth_peeker.password, b"")
             code, response = client.mail("alice@example.com")
             sess: SMTPSess = self.handler.session
             self.assertEqual(sess.login_data, b"")
@@ -1019,8 +1026,8 @@ class TestSMTPAuth(unittest.TestCase):
             self.assertEqual(response, b"UGFzc3dvcmQA")
             code, response = client.docmd('=')
             assert_auth_success(self, code, response)
-            self.assertEqual(auth_peeker.login, None)
-            self.assertEqual(auth_peeker.password, None)
+            self.assertEqual(auth_peeker.login, b"")
+            self.assertEqual(auth_peeker.password, b"")
             code, response = client.mail("alice@example.com")
             sess: SMTPSess = self.handler.session
             self.assertEqual(sess.login_data, b"")
@@ -1035,7 +1042,7 @@ class TestSMTPAuth(unittest.TestCase):
             with patch("logging.Logger.warning"):
                 code, response = client.docmd('*')
             self.assertEqual(code, 501)
-            self.assertEqual(response, b"Auth aborted")
+            self.assertEqual(response, b"5.7.0 Auth aborted")
 
     def test_auth_login_abort_password(self):
         auth_peeker.return_val = False
@@ -1051,7 +1058,7 @@ class TestSMTPAuth(unittest.TestCase):
             with patch("logging.Logger.warning"):
                 code, response = client.docmd('*')
             self.assertEqual(code, 501)
-            self.assertEqual(response, b"Auth aborted")
+            self.assertEqual(response, b"5.7.0 Auth aborted")
 
     def test_auth_custom_mechanism(self):
         auth_peeker.return_val = False
@@ -1075,10 +1082,12 @@ class TestSMTPAuth(unittest.TestCase):
             code, mesg = client.docmd("AUTH PLAIN")
             self.assertEqual(code, 334)
             self.assertEqual(mesg, b"")
-            code, mesg = client.docmd('=')
+            # "AAA=" is Base64 encoded "\x00\x00", representing null username and
+            # null password. See https://tools.ietf.org/html/rfc4616#page-3
+            code, mesg = client.docmd("AAA=")
             assert_auth_success(self, code, mesg)
-            self.assertEqual(auth_peeker.login, None)
-            self.assertEqual(auth_peeker.password, None)
+            self.assertEqual(auth_peeker.login, b"")
+            self.assertEqual(auth_peeker.password, b"")
             code, mesg = client.mail("alice@example.com")
             sess: SMTPSess = self.handler.session
             self.assertEqual(sess.login_data, b"")
@@ -1096,7 +1105,9 @@ class TestSMTPAuth(unittest.TestCase):
                 code, mesg = client.docmd("AUTH PLAIN")
                 self.assertEqual(code, 334)
                 self.assertEqual(mesg, b"")
-                code, mesg = client.docmd('=')
+                # "AAA=" is Base64 encoded "\x00\x00", representing null username and
+                # null password. See https://tools.ietf.org/html/rfc4616#page-3
+                code, mesg = client.docmd("AAA=")
                 assert_auth_success(self, code, mesg)