Fix bind unspecified address controller will raise TimeoutError

[wevsty]

What do these changes do?

Add a function is_unspecified_address() determine if the address is the unspecified address.
Automatic replacement of unspecified address with local addresses.

Are there changes in behavior for the user?

This will be more compatible with the user-specified behavior.

Related issue number

issue #272

Checklist

• I think the code is well written
• Unit tests for the changes exist
• tox testenvs have been executed in the following environments:
  • Linux (Ubuntu 18.04, Ubuntu 20.04, Arch): {py36,py37,py38,py39}-{nocov,cov,diffcov}, qa, docs
  • Windows (7, 10): {py36,py37,py38,py39}-{nocov,cov,diffcov}
  • WSL 1.0 (Ubuntu 18.04): {py36,py37,py38,py39}-{nocov,cov,diffcov}, pypy3-{nocov,cov}, qa, docs
  • FreeBSD (12.2, 12.1, 11.4): {py36,pypy3}-{nocov,cov,diffcov}, qa
  • Cygwin: py36-{nocov,cov,diffcov}, qa, docs
• Documentation reflects the changes
• Add a news fragment into the NEWS.rst file

[waynew]

I'm not 100% sure if this is the correct fix - 0.0.0.0 should bind on all interfaces right? I'm on my phone atm so I might not grok correctrly...

[lgtm-com]

This pull request introduces 1 alert when merging 28f3763 into 215b854 - view on LGTM.com

new alerts:

• 1 for Syntax error

[wevsty]

I'm not 100% sure if this is the correct fix - 0.0.0.0 should bind on all interfaces right? I'm on my phone atm so I might not grok correctrly...

Listening 0.0.0.0 usually means listening to all IPV4 interfaces.
However, 0.0.0.0 is not directly accessible, so there are errors that need special handling when detecting it.
On IPV6 :: has the same problem

[codecov]

Codecov Report

Merging #273 (5a0bde7) into master (e4bcd3f) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #273   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            7         7           
  Lines         1696      1713   +17     
  Branches       310       313    +3     
=========================================
+ Hits          1696      1713   +17     

Impacted Files	Coverage Δ
aiosmtpd/controller.py	100.00% <100.00%> (ø)
aiosmtpd/main.py	100.00% <100.00%> (ø)
aiosmtpd/proxy_protocol.py	100.00% <100.00%> (ø)
aiosmtpd/smtp.py	100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[wevsty]

I did my best to fix the problems with flake8 checks.
On my machine I passed all the tests.

But test_byclient still doesn't pass the test in some environments,This should be due to a change in Python's behavior.
see #219

My fix is special for python 3.8 and later

    def test_byclient(
        self, caplog, auth_peeker_controller, client, mechanism, init_resp
    ):
        self._ehlo(client)
        PW = "goodpasswd"
        client.user = "goodlogin"
        client.password = PW
        auth_meth = getattr(client, "auth_" + mechanism)
        if (mechanism, init_resp) == ("login", False):
            with pytest.raises(SMTPAuthenticationError):
                client.auth(mechanism, auth_meth, initial_response_ok=init_resp)
                # bpo-27820 has been fixed in python version 3.8 or later
                if sys.version_info >= (3, 8):
                    raise SMTPAuthenticationError(454, 'Expected failed')
            client.docmd("*")
            pytest.xfail(reason="smtplib.SMTP.auth_login is buggy (bpo-27820)")
        client.auth(mechanism, auth_meth, initial_response_ok=init_resp)
        peeker = auth_peeker_controller.handler
        assert isinstance(peeker, PeekerHandler)
        assert peeker.login == b"goodlogin"
        assert peeker.password == PW.encode("ascii")
        assert_nopassleak(PW, caplog.record_tuples)

I think this place needs to make some changes.
@pepoluan

[pepoluan]

I'm reservedly happy with the changes. Shall we LGTM?

[wevsty]

@pepoluan

Shall we LGTM?

All checks does not seem to find any problems. If there are other questions please contact me.

[waynew]

@pepoluan Any reason we shouldn't merge this?

[pepoluan]

@pepoluan

Shall we LGTM?

All checks does not seem to find any problems. If there are other questions please contact me.

After one year (sorry) conflicts arise ...

Can we fix this and revisit?

[wevsty]

@pepoluan

It's been a bit of a busy couple of weeks, so I've procrastinated a bit.
But I've resolved the conflicts submitted the new version and it should work fine now.

[pepoluan]

Okay so I've drunk enough coffee for today, I can think again 😁

This converts "unspecified" to localhost, rather than current IP adress, I think we need to mention that in the documentation as well. Other software when given "unspecified" will bind to all existing addresses including localhost. Though that is not a 'standard behavior' I think being explicit will be good.

[wevsty]

Okay so I've drunk enough coffee for today, I can think again 😁

This converts "unspecified" to localhost, rather than current IP adress, I think we need to mention that in the documentation as well. Other software when given "unspecified" will bind to all existing addresses including localhost. Though that is not a 'standard behavior' I think being explicit will be good.

I don't think it needs to be deliberately stated in the documentation.
Bind unspecified address is a common behavior, and on many platforms unspecified address is used instead of the IP address of all interfaces on the local machine.
And the code is not replacing bind 0.0.0.0 with 127.0.0.1.
It's just that aiosmtpd tries to connect to the bind address after the bind is done to make sure the bind is successful, but obviously most systems don't accept 0.0.0.0 as a connectable address, this PR just uses 127.0.0.1 when bind 0.0.0.0 to for connection testing, and does not change the external behavior.

[waynew]

I don't think it needs to be deliberately stated in the documentation.
Bind unspecified address is a common behavior, and on many platforms unspecified address is used instead of the IP address of all interfaces on the local machine.
And the code is not replacing bind 0.0.0.0 with 127.0.0.1.

It may be common behavior, but I only learned about this just recently. I'm sure I'm not the only one

[pepoluan]

And the code is not replacing bind 0.0.0.0 with 127.0.0.1.
It's just that aiosmtpd tries to connect to the bind address after the bind is done to make sure the bind is successful, but obviously most systems don't accept 0.0.0.0 as a connectable address, this PR just uses 127.0.0.1 when bind 0.0.0.0 to for connection testing, and does not change the external behavior.

Ahh thanks for the explanation!

Was a bit confused when I just look at the snippets.