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

Fix bind unspecified address controller will raise TimeoutError #273

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wevsty
Copy link
Contributor

@wevsty wevsty commented Jun 6, 2021

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

waynew commented Jun 6, 2021

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

lgtm-com bot commented Jun 6, 2021

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

new alerts:

  • 1 for Syntax error

@wevsty wevsty force-pushed the patch_bind branch 2 times, most recently from e1aea9d to 4744fc0 Compare June 6, 2021 19:19
@wevsty
Copy link
Contributor Author

wevsty commented Jun 6, 2021

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

@wevsty wevsty force-pushed the patch_bind branch 4 times, most recently from 4df94f7 to 4181f15 Compare June 6, 2021 20:13
@wevsty wevsty force-pushed the patch_bind branch 2 times, most recently from 53c8c8c to cbef7cc Compare June 16, 2021 17:05
@codecov
Copy link

codecov bot commented Jun 16, 2021

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

wevsty commented Jun 16, 2021

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

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'm reservedly happy with the changes. Shall we LGTM?

@wevsty
Copy link
Contributor Author

wevsty commented Oct 19, 2021

@pepoluan

Shall we LGTM?

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

@waynew
Copy link
Collaborator

waynew commented Nov 2, 2021

@pepoluan Any reason we shouldn't merge this?

@pepoluan
Copy link
Collaborator

@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 wevsty force-pushed the patch_bind branch 3 times, most recently from 4a0b8ce to 852d219 Compare January 8, 2023 11:57
The controller throws a TimeoutError when binding a unspecified address (e.g. 0.0.0.0)
fix flake8 warning
@wevsty
Copy link
Contributor Author

wevsty commented Jan 8, 2023

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

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.

@pepoluan pepoluan added this to the 1.4.5 milestone Jan 13, 2023
@wevsty
Copy link
Contributor Author

wevsty commented Jan 14, 2023

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

waynew commented Jan 14, 2023

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants