Repository: aio-libs/aiosmtpd

Issue:
LMTP server fails to list any extended services
The LHLO service awaits smtp_HELO instead of smtp_EHLO, preventing the greeting from displaying the service menu.

This is a violation of the LMTP spec, RFC 2033.

Patch attached.  This affects https://gitlab.com/mailman/mailman/issues/365

[lmtp_lhlo.zip](https://github.com/aio-libs/aiosmtpd/files/1132381/lmtp_lhlo.zip)



Discussion:
CLAassistant: [![CLA assistant check](https://cla-assistant.io/pull/badge/not_signed)](https://cla-assistant.io/aio-libs/aiosmtpd?pullRequest=207) <br/>Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our [Contributor License Agreement](https://cla-assistant.io/aio-libs/aiosmtpd?pullRequest=207) before we can accept your contribution.<br/><sub>You have signed the CLA already but the status is still pending? Let us [recheck](https://cla-assistant.io/check/aio-libs/aiosmtpd?pullRequest=207) it.</sub>
codecov[bot]: # [Codecov](https://codecov.io/gh/aio-libs/aiosmtpd/pull/207?src=pr&el=h1) Report
> Merging [#207](https://codecov.io/gh/aio-libs/aiosmtpd/pull/207?src=pr&el=desc) (3b25ae9) into [master](https://codecov.io/gh/aio-libs/aiosmtpd/commit/2d3d6b4a6f70bdc83f613dc3665a612392b38244?el=desc) (2d3d6b4) will **not change** coverage.
> The diff coverage is `100.00%`.

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

```diff
@@            Coverage Diff            @@
##            master      #207   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines          952       934   -18     
  Branches       168       168           
=========================================
- Hits           952       934   -18     
```


| [Impacted Files](https://codecov.io/gh/aio-libs/aiosmtpd/pull/207?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [aiosmtpd/lmtp.py](https://codecov.io/gh/aio-libs/aiosmtpd/pull/207/diff?src=pr&el=tree#diff-YWlvc210cGQvbG10cC5weQ==) | `100.00% <100.00%> (ø)` | |
| [aiosmtpd/smtp.py](https://codecov.io/gh/aio-libs/aiosmtpd/pull/207/diff?src=pr&el=tree#diff-YWlvc210cGQvc210cC5weQ==) | `100.00% <0.00%> (ø)` | |
| [aiosmtpd/handlers.py](https://codecov.io/gh/aio-libs/aiosmtpd/pull/207/diff?src=pr&el=tree#diff-YWlvc210cGQvaGFuZGxlcnMucHk=) | `100.00% <0.00%> (ø)` | |
| [aiosmtpd/controller.py](https://codecov.io/gh/aio-libs/aiosmtpd/pull/207/diff?src=pr&el=tree#diff-YWlvc210cGQvY29udHJvbGxlci5weQ==) | `100.00% <0.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/aio-libs/aiosmtpd/pull/207?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/207?src=pr&el=footer). Last update [13d5296...3b25ae9](https://codecov.io/gh/aio-libs/aiosmtpd/pull/207?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

ddevault: Hey, I'm the CEO of SourceHut, and @emersion wrote this patch under our direction.

We are opposed in principle to signing contributor license agreements, regardless of the apparently banality of the CLA in question. The changes @emersion made are available under the same terms as the aiosmtpd project itself: Apache 2.0. This gives you sufficient legal standing to apply the changes without additional fanfare. We use aiosmtpd to process tens of thousands of emails per year, and we're prepared to fork it if you insist.

Alternatively, you could consider adopting something like git and Linux's signed-off-by header, which indicates agreement with the Developer Certificate of Origin.
asvetlov: @ddevault I have a question to you. Did you found the CLA requirement for https://github.com/python/cpython also uncomfortable and not acceptable or agree to make an exception for that particular project?
ddevault: I don't think I have signed Python's CLA, and I probably would not do so. However, my level of investment in Python dwarfs that in aiosmtpd by scores, and it's much harder to fork it if I needed to. I have refused to sign Go's CLA, by the way.
asvetlov: That's sad to hear that CLA frustrates you.
I'm not a lawyer, my thought is the following:
If big projects like Python and Django think that they need to ask for CLA -- maybe aiohttp needs it too.
If aiohttp needs a CLA -- maybe the CLA requirement for all projects from aio-libs is a good idea.
ddevault: And if the largest project in the world, Linux, doesn't need one - maybe they have it right? The DCO is a good compromise that will get you everything you need to be confident that you can merge the patches.

And what of the hundreds of projects which don't have a CLA? Have you ever heard of any of them regretting it, for reasons other than the leadership's personal greed? It's precisely the horror stories of the fates of projects which have required a CLA, like MongoDB, RedisLabs, and many more, which should discredit the idea of CLAs. You should not sign one, and you *definitely* should not ask for one. Frankly, it's rude to.
warsaw: I hate to wade into this discussion, but I guess I will for whatever it's worth (probably not much :D).

I've obviously signed the Python CLA, and for Python I think it's both a good idea and very low friction for contributors.  Python has paid legal opinions that require this, and I have zero problem signing it.  Where I've needed my employer to give me permission, none have ever balked.

I've also contributed to and led GNU projects.  On Mailman we required copyright assignment to the FSF for all contributions above a certain size.  Some people didn't like that, but for the most part, it was never a problem.  Also pretty much a legal requirement for contributions.

Both of these are big projects, and both the PSF and FSF have made it quite clear that contributions under the CLA are guaranteed to be released open source / free software.  So there's no question of some nefarious organization profiting off labor of volunteers.  The CLAs are there to protect the legal interests of the organization owning the IP, and since both the PSF and FSF have strongly open reputations, everyone is cool with it.

I have also worked on non-GNU, GPL'd projects owned by other corporations, and there I think the CLAs have been a hinderance.  The biggest problem there was that the CLAs essentially said, "we'll release your contribution as FLOSS, but we reserve the right to include your contribution in closed commercial products".  It's no surprise to me that that stance isn't very well received.

That all said, what is the purpose of a CLA for aio-libs?  Are you afraid of being sued for copyright infringement?  E.g. someone who doesn't sign the CLA contributes something significant, but either they don't own the rights to that, or they are using that as a weapon to shut the project down.  I personally think that's very unlikely in practice, and also not very practical given that for the size of a project like aio-libs, almost all contributions are derivative works anyway, so it's unlikely that such a copyright claim would stand in court.  And it's also pretty unlikely that CLA or no, bad copyright actors could still make your life uncomfortable.
asvetlov: Thank you @warsaw for the comprehensive comment.
You've convinced me that the CLA is not *mandatory* and aio-libs can just drop it.

I apologize for the inconvenience, I never had a target to make harm but was trying to satisfy all really required things.

Will drop the CLA in the moment.
ddevault: Thanks @asvetlov and @warsaw!
warsaw: Yay!  Glad to help.
asvetlov: Thanks for not the silent disagree but opening a productive discussion!
pepoluan: Now that we're over the thing-that-shall-not-be-named, let's return to the PR proper 😉 

I'll pick this up.
pepoluan: **Temporarily** converting this to Draft. Will UnDraft after I test this on my testing systems.
pepoluan: Test Progress on my Test Systems:

- [x] Windows 10 (via PyCharm tox runner): `(ALL)`
- [x] Windows 10 (via PSCore 7.1.0): `(ALL)`
- [ ] Windows 10 (via Cygwin):

**This comment is undergoing editing**
pepoluan: [All testing done](https://github.com/aio-libs/aiosmtpd/pull/207#issuecomment-752939487).

Will now UnDraft.
pepoluan: If no objections within 2x24 I'll squamerge this 😄 

Patch:
diff --git a/MANIFEST.in b/MANIFEST.in
index 41d11f0..5a8020d 100644
--- a/MANIFEST.in
+++ b/MANIFEST.in
@@ -1,3 +1,3 @@
 graft aiosmtpd
-include .coveragerc LICENSE *.cfg *.ini *.py *.rst *.yml
+include LICENSE *.cfg *.ini *.py *.rst *.yml
 global-exclude *.py[oc]
diff --git a/aiosmtpd/docs/NEWS.rst b/aiosmtpd/docs/NEWS.rst
index 05628e0..4b0cab1 100644
--- a/aiosmtpd/docs/NEWS.rst
+++ b/aiosmtpd/docs/NEWS.rst
@@ -2,6 +2,7 @@
  NEWS for aiosmtpd
 ===================
 
+
 1.2.3 (aiosmtpd-next)
 =====================
 
@@ -15,12 +16,14 @@ Fixed/Improved
 * Implement & enforce line-length-limit, thus becoming Compliant with RFC 5321 § 4.5.3.1.6
 * Delay all SMTP Status Code replies during ``DATA`` phase until the phase termination (Closes #9)
 * Now catches ``Controller.factory()`` failure during ``Controller.start()`` (Closes #212)
-* :class:`SMTP` no longer edits user-supplied SSL Context (closes #191)
-* Implement waiting for SSL setup/handshake within STARTTLS handler to be able to catch and handle
+* :class:`SMTP` no longer edits user-supplied SSL Context (Closes #191)
+* 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
+* LMTP servers now correctly advertise extensions in reply to ``LHLO`` (Closes #123, #124)
+* ``NOOP`` now accepted before ``STARTTLS`` even if ``require_starttls=True`` (Closes #124)
 
 
 1.2.2 (2020-11-08)
diff --git a/aiosmtpd/lmtp.py b/aiosmtpd/lmtp.py
index 8d763fd..5a7ba40 100644
--- a/aiosmtpd/lmtp.py
+++ b/aiosmtpd/lmtp.py
@@ -9,7 +9,7 @@ class LMTP(SMTP):
     @syntax('LHLO hostname')
     async def smtp_LHLO(self, arg):
         """The LMTP greeting, used instead of HELO/EHLO."""
-        await super().smtp_HELO(arg)
+        await super().smtp_EHLO(arg)
 
     async def smtp_HELO(self, arg):
         """HELO is not a valid LMTP command."""
diff --git a/aiosmtpd/smtp.py b/aiosmtpd/smtp.py
index 1564d9b..5447640 100644
--- a/aiosmtpd/smtp.py
+++ b/aiosmtpd/smtp.py
@@ -59,7 +59,7 @@ __all__ = [
     "AuthMechanismType",
     "MISSING",
 ]  # Will be added to by @public
-__version__ = '1.2.3a4'
+__version__ = '1.2.3a5'
 __ident__ = 'Python SMTP {}'.format(__version__)
 log = logging.getLogger('mail.log')
 
@@ -71,6 +71,9 @@ MISSING = _Missing()
 NEWLINE = '\n'
 VALID_AUTHMECH = re.compile(r"[A-Z0-9_-]+\Z")
 
+# https://tools.ietf.org/html/rfc3207.html#page-3
+ALLOWED_BEFORE_STARTTLS = {"NOOP", "EHLO", "STARTTLS", "QUIT"}
+
 # endregion
 
 
@@ -447,7 +450,7 @@ class SMTP(asyncio.StreamReaderProtocol):
                     continue
                 if (self.require_starttls
                         and not self._tls_protocol
-                        and command not in ['EHLO', 'STARTTLS', 'QUIT']):
+                        and command not in ALLOWED_BEFORE_STARTTLS):
                     # RFC3207 part 4
                     await self.push('530 Must issue a STARTTLS command first')
                     continue
diff --git a/.coveragerc b/coverage.cfg
similarity index 100%
rename from .coveragerc
rename to coverage.cfg
diff --git a/housekeep.py b/housekeep.py
index f1e6acf..cf35a3f 100644
--- a/housekeep.py
+++ b/housekeep.py
@@ -31,6 +31,8 @@ except ImportError:
 TOX_ENV_NAME = os.environ.get("TOX_ENV_NAME", None)
 
 WORKDIRS = (
+    ".mypy_cache",
+    ".pytype",
     ".pytest-cache",
     ".pytest_cache",
     ".tox",
@@ -41,6 +43,14 @@ WORKDIRS = (
     "prof",
 )
 
+WORKFILES = (
+    ".coverage",
+    "coverage.xml",
+    "diffcov.html",
+    "coverage-*.xml",
+    "diffcov-*.html",
+)
+
 
 # region #### Helper funcs ############################################################
 
@@ -79,11 +89,13 @@ def dump_env():
 def move_prof():
     """Move profiling files to per-testenv dirs"""
     profpath = Path("prof")
+    # fmt: off
     prof_files = [
-        f
-        for p in ("*.prof", "*.svg")
-        for f in profpath.glob(p)
+        filepath
+        for fileglob in ("*.prof", "*.svg")
+        for filepath in profpath.glob(fileglob)
     ]
+    # fmt: on
     if not prof_files:
         return
     targpath = profpath / TOX_ENV_NAME
@@ -119,17 +131,10 @@ def rm_work():
         deldir(Path(dd))
         print(" ", end="", flush=True)
     print(f"\n{Style.BRIGHT}Removing work files ...", end="")
-    for fn in (".coverage", "coverage.xml", "diffcov.html"):
-        print(".", end="", flush=True)
-        fp = Path(fn)
-        if fp.exists():
-            fp.unlink()
-    for fp in Path(".").glob("coverage-*.xml"):
-        print(".", end="", flush=True)
-        fp.unlink()
-    for fp in Path(".").glob("diffcov-*.html"):
-        print(".", end="", flush=True)
-        fp.unlink()
+    for fnglob in WORKFILES:
+        for fp in Path(".").glob(fnglob):
+            print(".", end="", flush=True)
+            fp.exists() and fp.unlink()
     print()
 
 
@@ -197,10 +202,11 @@ def get_opts(argv):
     parser.add_argument(
         "--force", "-F", action="store_true", help="Force action even if in CI"
     )
+
+    # From: https://stackoverflow.com/a/49999185/149900
     parser.add_argument(
         "cmd", metavar="COMMAND", choices=sorted(dispers.keys()), help="(See below)"
     )
-
     cgrp = parser.add_argument_group(title="COMMAND is one of")
     for name, doc in sorted(dispers.items()):
         cgrp.add_argument(name, help=doc, action="no_action")
diff --git a/tox.ini b/tox.ini
index 4bee309..bb8fe53 100644
--- a/tox.ini
+++ b/tox.ini
@@ -30,7 +30,8 @@ deps =
     coverage
     diff_cover
 setenv =
-    cov: COVERAGE_PROCESS_START=.coveragerc
+    cov: COVERAGE_RCFILE=coverage.cfg
+    cov: COVERAGE_PROCESS_START=coverage.cfg
     cov: COVERAGE_OPTIONS="-p"
     cov: COVERAGE_FILE={toxinidir}/.coverage
     py36: INTERP=py36


Test patch:
diff --git a/aiosmtpd/tests/test_lmtp.py b/aiosmtpd/tests/test_lmtp.py
index 66fe8f3..f7cfac8 100644
--- a/aiosmtpd/tests/test_lmtp.py
+++ b/aiosmtpd/tests/test_lmtp.py
@@ -40,7 +40,15 @@ class TestLMTP(unittest.TestCase):
         with SMTP(*self.address) as client:
             code, response = client.docmd('LHLO', 'example.com')
             self.assertEqual(code, 250)
-            self.assertEqual(response, bytes(socket.getfqdn(), 'utf-8'))
+            lines = response.splitlines()
+            expecteds = (
+                bytes(socket.getfqdn(), 'utf-8'),
+                b'SIZE 33554432',
+                b'8BITMIME',
+                b'HELP',
+            )
+            for actual, expected in zip(lines, expecteds):
+                self.assertEqual(actual, expected)
 
     def test_helo(self):
         # HELO and EHLO are not valid LMTP commands.
diff --git a/aiosmtpd/tests/test_starttls.py b/aiosmtpd/tests/test_starttls.py
index d658b9a..e38e887 100644
--- a/aiosmtpd/tests/test_starttls.py
+++ b/aiosmtpd/tests/test_starttls.py
@@ -292,6 +292,18 @@ class TestRequireTLS(unittest.TestCase):
             code, response = client.docmd('DATA')
             self.assertEqual(code, 530)
 
+    def test_noop_okay(self):
+        with SMTP(*self.address) as client:
+            client.ehlo('example.com')
+            code, response = client.docmd('NOOP')
+            self.assertEqual(code, 250)
+
+    def test_quit_okay(self):
+        with SMTP(*self.address) as client:
+            client.ehlo('example.com')
+            code, response = client.docmd('QUIT')
+            self.assertEqual(code, 221)
+
 
 class TestRequireTLSAUTH(unittest.TestCase):
     def setUp(self):