Skip to content

Commit

Permalink
Fix bind unspecified address controller will raise TimeoutError
Browse files Browse the repository at this point in the history
The controller throws a TimeoutError when binding a unspecified address (e.g. 0.0.0.0)
fix flake8 warning
  • Loading branch information
wevsty committed Jan 8, 2023
1 parent 057474e commit 852d219
Show file tree
Hide file tree
Showing 7 changed files with 82 additions and 12 deletions.
39 changes: 34 additions & 5 deletions aiosmtpd/controller.py
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,25 @@ def _server_to_client_ssl_ctx(server_ctx: ssl.SSLContext) -> ssl.SSLContext:
return client_ctx


@public
def is_unspecified_address(address: str) -> bool:
unspecified_address_list = ["", "0.0.0.0", "::"] # nosec
return address in unspecified_address_list


@public
def convert_unspecified_address_to_localhost(
address: str
) -> str:
localhost_address = str(get_localhost())
address_dict = {
"": localhost_address,
"0.0.0.0": "127.0.0.1", # nosec
"::": "::1", # nosec
}
return address_dict.get(address, localhost_address)


class _FakeServer(asyncio.StreamReaderProtocol):
"""
Returned by _factory_invoker() in lieu of an SMTP instance in case
Expand Down Expand Up @@ -414,8 +433,15 @@ def __init__(
loop,
**kwargs,
)
self._localhost = get_localhost()
self.hostname = self._localhost if hostname is None else hostname
if is_unspecified_address(hostname) is True:
self._localhost = convert_unspecified_address_to_localhost(hostname)
self.hostname = hostname
elif hostname is None:
self._localhost = get_localhost()
self.hostname = self._localhost
else:
self._localhost = get_localhost()
self.hostname = hostname
self.port = port

def _create_server(self) -> Coroutine:
Expand All @@ -438,10 +464,13 @@ def _trigger_server(self):
gets invoked.
"""
# At this point, if self.hostname is Falsy, it most likely is "" (bind to all
# addresses). In such case, it should be safe to connect to localhost)
hostname = self.hostname or self._localhost
# addresses). In such case, it should be safe to connect to localhost.
if is_unspecified_address(self.hostname) is True:
connect_hostname = self._localhost
else:
connect_hostname = self.hostname
with ExitStack() as stk:
s = stk.enter_context(create_connection((hostname, self.port), 1.0))
s = stk.enter_context(create_connection((connect_hostname, self.port), 1.0))
if self.ssl_context:
client_ctx = _server_to_client_ssl_ctx(self.ssl_context)
s = stk.enter_context(client_ctx.wrap_socket(s))
Expand Down
2 changes: 1 addition & 1 deletion aiosmtpd/docs/NEWS.rst
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ Fixed/Improved
--------------
* All Controllers now have more rationale design, as they are now composited from a Base + a Mixin
* A whole bunch of annotations

* Fix bind unspecified address controller will raise TimeoutError

1.4.4a0 (ad hoc)
================
Expand Down
2 changes: 1 addition & 1 deletion aiosmtpd/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@ def main(args: Optional[Sequence[str]] = None) -> None:
server_loop = loop.run_until_complete(server)
except RuntimeError: # pragma: nocover
raise
log.debug(f"server_loop = {server_loop}")
log.debug("server_loop = %s", server_loop)
log.info("Server is listening on %s:%s", args.host, args.port)

# Signal handlers are only supported on *nix, so just ignore the failure
Expand Down
8 changes: 5 additions & 3 deletions aiosmtpd/proxy_protocol.py
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,8 @@ class ProxyData:
"""
Represents data received during PROXY Protocol Handshake, in an already-parsed form
"""


# pytype: disable=annotation-type-mismatch
version: Optional[int] = attr.ib(kw_only=True, init=True)
"""PROXY Protocol version; None if not recognized/malformed"""
command: Optional[V2_CMD] = _anoinit(default=None)
Expand Down Expand Up @@ -295,7 +296,8 @@ class ProxyData:
If not an empty string, contains the error encountered when parsing
"""
_tlv: Optional[ProxyTLV] = _anoinit(default=None)

# pytype: enable=annotation-type-mismatch

@property
def valid(self) -> bool:
return not (self.error or self.version is None or self.protocol is None)
Expand All @@ -316,7 +318,7 @@ def with_error(self, error_msg: str, log_prefix: bool = True) -> "ProxyData":
:param log_prefix: If True, add "PROXY error:" prefix to log message
"""
if log_prefix:
log.warning(f"PROXY error: {error_msg}")
log.warning("PROXY error: %s", error_msg)
else:
log.warning(error_msg)
self.error = error_msg
Expand Down
4 changes: 2 additions & 2 deletions aiosmtpd/smtp.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@

# region #### Custom Data Types #######################################################

class _Missing:
class _Missing: # noqa: SIM119
def __repr__(self) -> str:
return "MISSING"

Expand Down Expand Up @@ -797,7 +797,7 @@ async def check_auth_needed(self, caller_method: str) -> bool:
:return: True if AUTH is needed
"""
if self._auth_required and not self.session.authenticated:
log.info(f'{caller_method}: Authentication required')
log.info('%s: Authentication required', caller_method)
await self.push('530 5.7.0 Authentication required')
return True
return False
Expand Down
36 changes: 36 additions & 0 deletions aiosmtpd/tests/test_server.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@
_FakeServer,
get_localhost,
_server_to_client_ssl_ctx,
is_unspecified_address,
convert_unspecified_address_to_localhost,
)
from aiosmtpd.handlers import Sink
from aiosmtpd.smtp import SMTP as Server
Expand Down Expand Up @@ -297,6 +299,20 @@ def test_hostname_none(self):
finally:
cont.stop()

def test_hostname_unspecified_ipv4(self):
cont = Controller(Sink(), hostname="0.0.0.0") # nosec
try:
cont.start()
finally:
cont.stop()

def test_hostname_unspecified_ipv6(self):
cont = Controller(Sink(), hostname="::") # nosec
try:
cont.start()
finally:
cont.stop()

def test_testconn_raises(self, mocker: MockFixture):
mocker.patch("socket.socket.recv", side_effect=RuntimeError("MockError"))
cont = Controller(Sink(), hostname="")
Expand Down Expand Up @@ -351,6 +367,26 @@ def test_getlocalhost_error(self, mocker):
assert exc.value.errno == errno.EFAULT
mock_makesock.assert_called_with(socket.AF_INET6, socket.SOCK_STREAM)

def test_is_unspecified_address(self):
assert is_unspecified_address("127.0.0.1") is False
assert is_unspecified_address("0.0.0.0") is True # nosec
assert is_unspecified_address("::") is True # nosec
assert is_unspecified_address("") is True

def test_convert_unspecified_to_localhost(self):
assert convert_unspecified_address_to_localhost(
""
) == get_localhost()
assert convert_unspecified_address_to_localhost(
"0.0.0.0"
) == "127.0.0.1" # nosec
assert convert_unspecified_address_to_localhost(
"::"
) == "::1" # nosec
assert convert_unspecified_address_to_localhost(
"0.0.0.1"
) == get_localhost()

def test_stop_default(self):
controller = Controller(Sink())
with pytest.raises(AssertionError, match="SMTP daemon not running"):
Expand Down
3 changes: 3 additions & 0 deletions aiosmtpd/tests/test_smtp.py
Original file line number Diff line number Diff line change
Expand Up @@ -1070,6 +1070,9 @@ def test_byclient(
# See https://github.com/python/cpython/pull/24118 for the fixes.:
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)
Expand Down

0 comments on commit 852d219

Please sign in to comment.