Repository: aio-libs/aiosmtpd (python)

Issue:
Current AUTH hook implementation cannot support mechanisms with dash "-" in the name
The current implementation simply searches for `auth_*` methods in the handler, chop off the `auth_` part, and present the chopped name as is to the list of supported authentications.

This means that AUTH mechanisms such as "CRAM-MD5" and "DIGEST-MD5" are not representable.

Fixing this is a bit tricky; the simple trick of replacing "_" with "-" will result in SASL mechanisms that _does_ contain underscore in their names (see [SASL mechanisms registry on IANA](https://www.iana.org/assignments/sasl-mechanisms/sasl-mechanisms.xhtml)) to be not representable.

My suggestion would be to add another decorator (similar to the existing `@syntax` decorator) that will add an attribute, say `__auth_mechanism_name__` ; if the attribute exists, use that. If the attribute does not exist, use the existing name-mangling strategy.

Pull request description:
Workaround to specify additional AUTH mechanisms with dash
## What do these changes do?

The current implementation for AUTH Hooks does not allow AUTH mechanisms with dash(es) in their names.

For example, it is not possible to create a handler method for `CRAM-MD5` or `DIGEST-MD5`, because the code simply takes the method name beginning with "auth_" and chop off that part.

This PR provides two methods of specifying dashes:

  * Using "double underscore", so `MECH__NAME__WITH__DASH` ==> `MECH-NAME-WITH-DASH`
  *  Using `@auth_mechanism(actual_name)` decorator

## Are there changes in behavior for the user?

None for existing users unless they had subclassed `SMTP` to implement this feature.

## Related issue number

Closes #224 

## 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 runner): `(ALL)`
  - [x] Windows 10 (via PSCore 7.0.3): `(ALL)`
  - [x] Windows 10 (via Cygwin): `qa,py36-{nocov,cov}`
  - [x] Ubuntu 18.04 on WSL 1.0: `(ALL)` + `pypy3-{nocov,cov}`
  - [x] Ubuntu 18.04 on VBox: `(ALL)` + `pypy3-{nocov,cov}`
  - [x] Ubuntu 20.04 on VBox: `(ALL)` + `pypy3-{nocov,cov}`
  - [x] FreeBSD 12.1 on VBox: `(ALL)` + `pypy3-nocov`
- [x] Documentation reflects the changes
- [x] Add a news fragment into the `NEWS.rst` file



Patch:
diff --git a/.gitignore b/.gitignore
index 34d0ea4..5a24133 100644
--- a/.gitignore
+++ b/.gitignore
@@ -30,3 +30,4 @@ _dynamic/
 coverage.xml
 diffcov.html
 .python-version
+.pytype/
diff --git a/aiosmtpd/docs/NEWS.rst b/aiosmtpd/docs/NEWS.rst
index 167e830..05628e0 100644
--- a/aiosmtpd/docs/NEWS.rst
+++ b/aiosmtpd/docs/NEWS.rst
@@ -19,6 +19,9 @@ Fixed/Improved
 * Implement waiting for SSL setup/handshake within STARTTLS handler to be able to catch and handle
   (log) errors and to avoid session hanging around until timeout in such cases
 * Add session peer information to some logging output where it was missing
+* Support AUTH mechanisms with dash(es) in their names (Closes #224)
+* Remove some double-logging of commands sent by clients
+
 
 1.2.2 (2020-11-08)
 ==================
diff --git a/aiosmtpd/docs/handlers.rst b/aiosmtpd/docs/handlers.rst
index d98af40..f661c93 100644
--- a/aiosmtpd/docs/handlers.rst
+++ b/aiosmtpd/docs/handlers.rst
@@ -156,6 +156,24 @@ Every AUTH hook is named ``auth_MECHANISM`` where ``MECHANISM`` is the all-upper
 mechanism that the hook will implement. AUTH hooks will be called with the SMTP
 server instance and a list of str following the ``AUTH`` command.
 
+.. important::
+
+   If ``MECHANISM`` has a dash within its name,
+   use **double-underscore** to represent the dash.
+   For example, to implement a ``MECH-WITH-DASHES`` mechanism,
+   name the AUTH hook as ``auth_MECH__WITH__DASHES``.
+
+   Single underscores will not be modified.
+   So a hook named ``auth_MECH_WITH_UNDERSCORE``
+   will implement the ``MECH_WITH_UNDERSCORE`` mechanism.
+
+   (If in the future a SASL mechanism with double underscores in its name gets defined,
+   this name-mangling mechanism will be revisited.
+   That is very unlikely to happen, though.)
+
+   Alternatively, you can also use the ``@auth_mechanism(actual_name)`` decorator,
+   which you can import from the :mod:`aiosmtpd.smtp` module.
+
 The SMTP class provides built-in AUTH hooks for the ``LOGIN`` and ``PLAIN``
 mechanisms, named ``auth_LOGIN`` and ``auth_PLAIN``, respectively.
 If the handler class implements ``auth_LOGIN`` and/or ``auth_PLAIN``, then
diff --git a/aiosmtpd/smtp.py b/aiosmtpd/smtp.py
index 2f5f8dd..1564d9b 100644
--- a/aiosmtpd/smtp.py
+++ b/aiosmtpd/smtp.py
@@ -1,7 +1,9 @@
+import re
 import ssl
 import enum
 import socket
 import asyncio
+import inspect
 import logging
 import binascii
 import collections
@@ -42,6 +44,7 @@ class _DataState(enum.Enum):
     TOO_MUCH = enum.auto()
 
 
+AuthCallbackType = Callable[[str, Optional[bytes], Optional[bytes]], bool]
 AuthMechanismType = Callable[["SMTP", List[str]], Awaitable[Any]]
 _TriStateType = Union[None, _Missing, bytes]
 
@@ -51,7 +54,12 @@ _TriStateType = Union[None, _Missing, bytes]
 
 # region #### Constant & Constant-likes ###############################################
 
-__version__ = '1.2.3a3'
+__all__ = [
+    "AuthCallbackType",
+    "AuthMechanismType",
+    "MISSING",
+]  # Will be added to by @public
+__version__ = '1.2.3a4'
 __ident__ = 'Python SMTP {}'.format(__version__)
 log = logging.getLogger('mail.log')
 
@@ -61,6 +69,7 @@ EMPTY_BARR = bytearray()
 EMPTYBYTES = b''
 MISSING = _Missing()
 NEWLINE = '\n'
+VALID_AUTHMECH = re.compile(r"[A-Z0-9_-]+\Z")
 
 # endregion
 
@@ -75,6 +84,10 @@ class Session:
         self.loop = loop
         self.login_data = None
 
+    @property
+    def authenticated(self):
+        return self.login_data is not None
+
 
 @public
 class Envelope:
@@ -95,7 +108,16 @@ def make_loop():
     return asyncio.get_event_loop()
 
 
-def syntax(text, extended=None, when=None):
+@public
+def syntax(text, extended=None, when: Optional[str] = None):
+    """
+    Provides helptext for (E)SMTP HELP. Applies for smtp_* methods only!
+
+    :param text: Help text for (E)SMTP HELP
+    :param extended: Additional text for ESMTP HELP (appended to text)
+    :param when: The name of the attribute of SMTP class to check; if the value
+        of the attribute is false-y then HELP will not be available for the command
+    """
     def decorator(f):
         f.__smtp_syntax__ = text
         f.__smtp_syntax_extended__ = extended
@@ -104,6 +126,24 @@ def syntax(text, extended=None, when=None):
     return decorator
 
 
+@public
+def auth_mechanism(actual_name: str):
+    """
+    Explicitly specifies the name of the AUTH mechanism implemented by
+    the function/method this decorates
+
+    :param actual_name: Name of AUTH mechanism. Must consists of [A-Z0-9_-] only.
+        Will be converted to uppercase
+    """
+    def decorator(f):
+        f.__auth_mechanism_name__ = actual_name
+        return f
+    actual_name = actual_name.upper()
+    if not VALID_AUTHMECH.match(actual_name):
+        raise ValueError(f"Invalid AUTH mechanism name: {actual_name}")
+    return decorator
+
+
 def login_always_fail(mechanism, login, password):
     return False
 
@@ -137,7 +177,7 @@ class SMTP(asyncio.StreamReaderProtocol):
             auth_required=False,
             auth_require_tls=True,
             auth_exclude_mechanism: Optional[Iterable[str]] = None,
-            auth_callback: Callable[[str, bytes, bytes], bool] = None,
+            auth_callback: AuthCallbackType = None,
             loop=None,
     ):
         self.__ident__ = ident or __ident__
@@ -183,21 +223,25 @@ class SMTP(asyncio.StreamReaderProtocol):
         self._auth_require_tls = auth_require_tls
         self._auth_callback = auth_callback or login_always_fail
         self._auth_required = auth_required
-        self.authenticated = False
         # Get hooks & methods to significantly speedup getattr's
         self._auth_methods: Dict[str, _AuthMechAttr] = {
-            m.replace("auth_", ""): _AuthMechAttr(getattr(h, m), h is self)
-            for h in (self, handler)
-            for m in dir(h)
-            if m.startswith("auth_")
+            getattr(
+                mfunc, "__auth_mechanism_name__",
+                mname.replace("auth_", "").replace("__", "-")
+            ): _AuthMechAttr(mfunc, obj is self)
+            for obj in (self, handler)
+            for mname, mfunc in inspect.getmembers(obj)
+            if mname.startswith("auth_")
         }
         for m in (auth_exclude_mechanism or []):
             self._auth_methods.pop(m, None)
-        msg = "Available AUTH mechanisms:"
-        for m, impl in sorted(
-                self._auth_methods.items()):  # type: str, _AuthMechAttr
-            msg += f" {m}{'(builtin)' if impl.is_builtin else ''}"
-        log.info(msg)
+        log.info(
+            "Available AUTH mechanisms: "
+            + " ".join(
+                m + "(builtin)" if impl.is_builtin else m
+                for m, impl in sorted(self._auth_methods.items())
+            )
+        )
         self._handle_hooks: Dict[str, Callable] = {
             m.replace("handle_", ""): getattr(handler, m)
             for m in dir(handler)
@@ -315,7 +359,7 @@ class SMTP(asyncio.StreamReaderProtocol):
         response = bytes(
             status + '\r\n', 'utf-8' if self.enable_SMTPUTF8 else 'ascii')
         self._writer.write(response)
-        log.debug(response)
+        log.debug("%r << %r", self.session.peer, response)
         await self._writer.drain()
 
     async def handle_exception(self, error):
@@ -350,10 +394,10 @@ class SMTP(asyncio.StreamReaderProtocol):
                     # send error response and read the next command line.
                     await self.push('500 Command line too long')
                     continue
-                log.debug('_handle_client readline: %s', line)
+                log.debug('_handle_client readline: %r', line)
                 # XXX this rstrip may not completely preserve old behavior.
                 line = line.rstrip(b'\r\n')
-                log.info('%r Data: %s', self.session.peer, line)
+                log.info('%r >> %r', self.session.peer, line)
                 if not line:
                     await self.push('500 Error: bad syntax')
                     continue
@@ -467,7 +511,7 @@ class SMTP(asyncio.StreamReaderProtocol):
         :param caller_method: The SMTP method needing a check (for logging)
         :return: True if AUTH is needed
         """
-        if self._auth_required and not self.authenticated:
+        if self._auth_required and not self.session.authenticated:
             log.info(f'{caller_method}: Authentication required')
             await self.push('530 5.7.0 Authentication required')
             return True
@@ -536,7 +580,6 @@ class SMTP(asyncio.StreamReaderProtocol):
 
     @syntax('STARTTLS', when='tls_context')
     async def smtp_STARTTLS(self, arg):
-        log.info('%r STARTTLS', self.session.peer)
         if arg:
             await self.push('501 Syntax: STARTTLS')
             return
@@ -580,7 +623,7 @@ class SMTP(asyncio.StreamReaderProtocol):
         elif self._auth_require_tls and not self._tls_protocol:
             await self.push("538 5.7.11 Encryption required for requested "
                             "authentication mechanism")
-        elif self.authenticated:
+        elif self.session.authenticated:
             await self.push('503 Already authenticated')
         elif not arg:
             await self.push('501 Not enough value')
@@ -615,7 +658,6 @@ class SMTP(asyncio.StreamReaderProtocol):
                     # is rejected / not valid
                     status = '535 5.7.8 Authentication credentials invalid'
                 else:
-                    self.authenticated = True
                     self.session.login_data = login_data
                     status = '235 2.7.0 Authentication successful'
             if status is not None:  # pragma: no branch
@@ -628,15 +670,16 @@ class SMTP(asyncio.StreamReaderProtocol):
         blob = line.strip()
         # '=' and '*' handling are in accordance with RFC4954
         if blob == b"=":
-            log.debug("User responded with '='")
+            log.debug("%r responded with '='", self.session.peer)
             return None
         if blob == b"*":
-            log.warning("User requested abort with '*'")
+            log.warning("%r aborted with '*'", self.session.peer)
             await self.push("501 Auth aborted")
             return MISSING
         try:
             decoded_blob = b64decode(blob, validate=True)
         except binascii.Error:
+            log.debug("%r can't decode base64: %s", self.session.peer, blob)
             await self.push("501 5.5.2 Can't decode base64")
             return MISSING
         return decoded_blob
@@ -656,37 +699,40 @@ class SMTP(asyncio.StreamReaderProtocol):
     #        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)
+    #        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)
 
     async def auth_PLAIN(self, _, args: List[str]):
-        loginpassword: _TriStateType
+        login_and_password: _TriStateType
         if len(args) == 1:
             # Trailing space is MANDATORY
-            # See https://tools.ietf.org/html/rfc4954#page-4
-            loginpassword = await self._auth_interact("334 ")
-            if loginpassword is MISSING:
+            # See https://tools.ietf.org/html/rfc4954#page-4 ¶ 5
+            login_and_password = await self._auth_interact("334 ")
+            if login_and_password is MISSING:
                 return
         else:
-            blob = args[1].encode()
-            if blob == b"=":
-                loginpassword = None
+            blob = args[1]
+            if blob == "=":
+                login_and_password = None
             else:
                 try:
-                    loginpassword = b64decode(blob, validate=True)
+                    login_and_password = b64decode(blob, validate=True)
                 except Exception:
                     await self.push("501 5.5.2 Can't decode base64")
                     return
-        if loginpassword is None:
+        # Decode login data
+        if login_and_password is None:
             login = password = None
         else:
             try:
-                _, login, password = loginpassword.split(b"\x00")
+                _, 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
         if self._auth_callback("PLAIN", login, password):
             if login is None:
                 login = EMPTYBYTES
@@ -801,7 +847,6 @@ class SMTP(asyncio.StreamReaderProtocol):
             return
         if await self.check_auth_needed("MAIL"):
             return
-        log.debug('===> MAIL %s', arg)
         syntaxerr = '501 Syntax: MAIL FROM: <address>'
         if self.session.extended_smtp:
             syntaxerr += ' [SP <mail-parameters>]'
@@ -869,7 +914,6 @@ class SMTP(asyncio.StreamReaderProtocol):
             return
         if await self.check_auth_needed("RCPT"):
             return
-        log.debug('===> RCPT %s', arg)
         if not self.envelope.mail_from:
             await self.push('503 Error: need MAIL command')
             return


Test patch:
diff --git a/aiosmtpd/tests/test_smtp.py b/aiosmtpd/tests/test_smtp.py
index 2730c0d..04f541a 100644
--- a/aiosmtpd/tests/test_smtp.py
+++ b/aiosmtpd/tests/test_smtp.py
@@ -9,7 +9,7 @@ import warnings
 from aiosmtpd.controller import Controller
 from aiosmtpd.handlers import Sink
 from aiosmtpd.smtp import (
-    MISSING, Session as SMTPSess, SMTP as Server, __ident__ as GREETING
+    MISSING, Session as SMTPSess, SMTP as Server, __ident__ as GREETING, auth_mechanism
 )
 from aiosmtpd.testing.helpers import (
     ReceivingHandler,
@@ -76,6 +76,16 @@ class PeekerHandler:
     ):
         return MISSING
 
+    async def auth_WITH_UNDERSCORE(self, server, args):
+        return "250 OK"
+
+    @auth_mechanism("with-dash")
+    async def auth_WITH_DASH(self, server, args):
+        return "250 OK"
+
+    async def auth_WITH__MULTI__DASH(self, server, args):
+        return "250 OK"
+
 
 class PeekerAuth:
     def __init__(self):
@@ -972,7 +982,7 @@ class TestSMTPAuth(unittest.TestCase):
                 bytes(socket.getfqdn(), 'utf-8'),
                 b'SIZE 33554432',
                 b'SMTPUTF8',
-                b'AUTH LOGIN NULL PLAIN',
+                b'AUTH LOGIN NULL PLAIN WITH-DASH WITH-MULTI-DASH WITH_UNDERSCORE',
                 b'HELP',
             )
             for actual, expected in zip(lines, expecteds):
@@ -1052,6 +1062,37 @@ class TestSMTPAuth(unittest.TestCase):
             self.assertEqual(response,
                              b"5.5.4 Unrecognized authentication type")
 
+    def test_rset_maintain_authenticated(self):
+        """RSET resets only Envelope not Session"""
+        with SMTP(*self.address) as client:
+            client.ehlo("example.com")
+            code, mesg = client.docmd("AUTH PLAIN")
+            self.assertEqual(code, 334)
+            self.assertEqual(mesg, b"")
+            code, mesg = client.docmd('=')
+            assert_auth_success(self, code, mesg)
+            self.assertEqual(auth_peeker.login, None)
+            self.assertEqual(auth_peeker.password, None)
+            code, mesg = client.mail("alice@example.com")
+            sess: SMTPSess = self.handler.session
+            self.assertEqual(sess.login_data, b"")
+            code, mesg = client.rset()
+            self.assertEqual(code, 250)
+            code, mesg = client.docmd("AUTH PLAIN")
+            self.assertEqual(503, code)
+            self.assertEqual(b'Already authenticated', mesg)
+
+    def test_auth_individually(self):
+        """AUTH state of different clients must be independent"""
+        with SMTP(*self.address) as client1, SMTP(*self.address) as client2:
+            for client in client1, client2:
+                client.ehlo("example.com")
+                code, mesg = client.docmd("AUTH PLAIN")
+                self.assertEqual(code, 334)
+                self.assertEqual(mesg, b"")
+                code, mesg = client.docmd('=')
+                assert_auth_success(self, code, mesg)
+
 
 class TestRequiredAuthentication(unittest.TestCase):
     def setUp(self):
@@ -1842,3 +1883,8 @@ class TestAuthArgs(unittest.TestCase):
         mock_info.assert_any_call(
             f"Available AUTH mechanisms: {' '.join(auth_mechs)}"
         )
+
+    def test_authmechname_decorator_badname(self):
+        self.assertRaises(ValueError, auth_mechanism, "has space")
+        self.assertRaises(ValueError, auth_mechanism, "has.dot")
+        self.assertRaises(ValueError, auth_mechanism, "has/slash")