Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make LHLO behave like EHLO #207

Merged
merged 10 commits into from
Jan 5, 2021
Merged

Make LHLO behave like EHLO #207

merged 10 commits into from
Jan 5, 2021

Conversation

emersion
Copy link
Contributor

@emersion emersion commented Nov 16, 2020

The current logic makes LHLO behave like the legacy HELO. However the
RFC says:

The LHLO command has identical semantics to the EHLO command of ESMTP.

Clients connecting to the LMTP server expect extensions such as 8BITMIME
to be advertised. However aiosmtpd won't send any extension string.

Fix this by making LHLO an alias for EHLO instead of HELO.

Related issue number

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • tox testenvs have been executed in the following environments:
    • Linux (Arch): {py38}-{nocov,cov,diffcov}, qa, docs
  • Documentation reflects the changes
  • Add a news fragment into the NEWS.rst file

The current logic makes LHLO behave like the legacy HELO. However the
RFC says:

> The LHLO command has identical semantics to the EHLO command of ESMTP.

Clients connecting to the LMTP server expect extensions such as 8BITMIME
to be advertised. However aiosmtpd won't send any extension string.

Fix this by making LHLO an alias for EHLO instead of HELO.

[1]: https://tools.ietf.org/html/rfc2033#section-4.1
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@codecov
Copy link

codecov bot commented Nov 16, 2020

Codecov Report

Merging #207 (b88735a) into master (17541ae) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #207   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines         1048      1049    +1     
  Branches       184       184           
=========================================
+ Hits          1048      1049    +1     
Impacted Files Coverage Δ
aiosmtpd/lmtp.py 100.00% <100.00%> (ø)
aiosmtpd/smtp.py 100.00% <100.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 17541ae...b88735a. Read the comment docs.

@ddevault
Copy link

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.

@pepoluan pepoluan mentioned this pull request Nov 17, 2020
@asvetlov
Copy link
Member

@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
Copy link

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
Copy link
Member

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
Copy link

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
Copy link
Contributor

warsaw commented Nov 18, 2020

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.

I don't personally require CLAs in my small open source projects. I think they just add unnecessary friction for very little value.

But every project has to make their own decisions about the level of risk that a CLA mitigates.

@asvetlov
Copy link
Member

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
Copy link

Thanks @asvetlov and @warsaw!

@warsaw
Copy link
Contributor

warsaw commented Nov 18, 2020

Yay! Glad to help.

@asvetlov
Copy link
Member

Thanks for not the silent disagree but opening a productive discussion!

@pepoluan pepoluan added this to the 1.2.3 milestone Dec 29, 2020
@pepoluan
Copy link
Collaborator

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 pepoluan self-assigned this Dec 30, 2020
# Conflicts:
#	aiosmtpd/docs/NEWS.rst
#	aiosmtpd/lmtp.py
Fully Closes #124

Also added 2 tests to ensure this.
* List issues closed by this PR
* Add info about NOOP-before-STARTTLS
@pepoluan pepoluan marked this pull request as draft December 30, 2020 09:57
@pepoluan
Copy link
Collaborator

Temporarily converting this to Draft. Will UnDraft after I test this on my testing systems.

* Now as a Sequence of globs
* Turned into an easily-maintainable CONSTANT at start of file
* Add future INTERP-based file names (idea from pytest-based branch)
* Better variable naming
* Better command grouping
* Additional comments
* Exclude certain parts from black-ification
Two reasons:

1. It is too similar to .coverage file used to record coverage data.

   There's a clear and present danger of accidentally cleaning up the
   wrong file(s)

2. .coveragerc does not get syntax highlighting in PyCharm. *.cfg files
   do, though.

Side benefit: Simpler MANIFEST.in :)
@pepoluan
Copy link
Collaborator

pepoluan commented Dec 31, 2020

Test Progress on my Test Systems:

  • Windows 10 (via PyCharm tox runner): (ALL)
  • Windows 10 (via PSCore 7.1.0): (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

@@ -1,3 +1,3 @@
graft aiosmtpd
include .coveragerc LICENSE *.cfg *.ini *.py *.rst *.yml
include LICENSE *.cfg *.ini *.py *.rst *.yml
Copy link
Collaborator

Choose a reason for hiding this comment

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

.coveragerc has been renamed to coverage.cfg

Reason is explained in a59fd41

@@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

.coveragerc has been renamed to coverage.cfg

Reason is explained in a59fd41

@pepoluan
Copy link
Collaborator

All testing done.

Will now UnDraft.

@pepoluan pepoluan marked this pull request as ready for review December 31, 2020 13:28
Copy link
Collaborator

@pepoluan pepoluan left a comment

Choose a reason for hiding this comment

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

I personally have no objections to the code / changes. But it will be cheating if I squamerge based on just my own approval 😅

@pepoluan
Copy link
Collaborator

pepoluan commented Jan 3, 2021

If no objections within 2x24 I'll squamerge this 😄

Edit: Just saw the merge conflicts. When did that creep in?? Lemme fix those and repush.

Edit 2: Merged and repushed. If no objections to the patches within 2x24, I'll squamerge.

# Conflicts:
#	aiosmtpd/docs/NEWS.rst
#	aiosmtpd/smtp.py
@pepoluan pepoluan merged commit 20bd337 into aio-libs:master Jan 5, 2021
@emersion emersion deleted the lhlo branch August 19, 2021 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants