From f0f3cdad9c7bb49e1cb5c72b2a5f49aaf22971e4 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Tue, 28 Jan 2025 16:29:52 -0800 Subject: [PATCH] fixup fromstring return types (#10162) in https://github.com/certbot/certbot/pull/9124 we had the problem of certbot-nginx's `Addr.fromstring` method possibly returning None which is not possible in the `Addr` method in the certbot base class or in certbot-apache. we fixed this by telling mypy the common `Addr.fromstring` method returns an `Optional[Addr]` (despite it actually always returning an `Addr`) and then unnecessarily complicating certbot-apache's code a bit. the need for extra complexity with this approach is going even further in https://github.com/certbot/certbot/pull/10151 where we have to use `cast` to assure mypy that the type isn't actually `Optional`. i personally don't like all this --- .../certbot_apache/_internal/configurator.py | 15 +++++---------- .../certbot_nginx/_internal/http_01.py | 4 ++-- certbot-nginx/certbot_nginx/_internal/obj.py | 19 +++++++++++++++---- .../certbot_nginx/_internal/parser.py | 13 ++++++++----- .../certbot_nginx/_internal/tests/obj_test.py | 13 ++++++++----- certbot/certbot/plugins/common.py | 2 +- 6 files changed, 39 insertions(+), 27 deletions(-) diff --git a/certbot-apache/certbot_apache/_internal/configurator.py b/certbot-apache/certbot_apache/_internal/configurator.py index cf3dd46b5..d3eb43f56 100644 --- a/certbot-apache/certbot_apache/_internal/configurator.py +++ b/certbot-apache/certbot_apache/_internal/configurator.py @@ -994,9 +994,7 @@ class ApacheConfigurator(common.Configurator): for arg in args: arg_value = self.parser.get_arg(arg) if arg_value is not None: - addr = obj.Addr.fromstring(arg_value) - if addr is not None: - addrs.add(addr) + addrs.add(obj.Addr.fromstring(arg_value)) is_ssl = False if self.parser.find_dir("SSLEngine", "on", start=path, exclude=False): @@ -1126,9 +1124,7 @@ class ApacheConfigurator(common.Configurator): """ addrs = set() for param in node.parameters: - addr = obj.Addr.fromstring(param) - if addr: - addrs.add(addr) + addrs.add(obj.Addr.fromstring(param)) is_ssl = False # Exclusion to match the behavior in get_virtual_hosts_v2 @@ -1646,10 +1642,9 @@ class ApacheConfigurator(common.Configurator): for addr in ssl_addr_p: old_addr = obj.Addr.fromstring( str(self.parser.get_arg(addr))) - if old_addr: - ssl_addr = old_addr.get_addr_obj("443") - self.parser.aug.set(addr, str(ssl_addr)) - ssl_addrs.add(ssl_addr) + ssl_addr = old_addr.get_addr_obj("443") + self.parser.aug.set(addr, str(ssl_addr)) + ssl_addrs.add(ssl_addr) return ssl_addrs diff --git a/certbot-nginx/certbot_nginx/_internal/http_01.py b/certbot-nginx/certbot_nginx/_internal/http_01.py index 9ca02dc57..c7a36b767 100644 --- a/certbot-nginx/certbot_nginx/_internal/http_01.py +++ b/certbot-nginx/certbot_nginx/_internal/http_01.py @@ -146,7 +146,7 @@ class NginxHttp01(common.ChallengePerformer): :returns: list of :class:`certbot_nginx._internal.obj.Addr` to apply :rtype: list """ - addresses: List[Optional[Addr]] = [] + addresses: List[Addr] = [] default_addr = "%s" % self.configurator.config.http01_port ipv6_addr = "[::]:{0}".format( self.configurator.config.http01_port) @@ -169,7 +169,7 @@ class NginxHttp01(common.ChallengePerformer): logger.debug("Using default address %s for authentication.", default_addr) - return [address for address in addresses if address] + return addresses def _get_validation_path(self, achall: KeyAuthorizationAnnotatedChallenge) -> str: return os.sep + os.path.join(challenges.HTTP01.URI_ROOT_PATH, achall.chall.encode("token")) diff --git a/certbot-nginx/certbot_nginx/_internal/obj.py b/certbot-nginx/certbot_nginx/_internal/obj.py index 1e0dbba1a..67249ce4a 100644 --- a/certbot-nginx/certbot_nginx/_internal/obj.py +++ b/certbot-nginx/certbot_nginx/_internal/obj.py @@ -12,6 +12,10 @@ from certbot.plugins import common ADD_HEADER_DIRECTIVE = 'add_header' +class SocketAddrError(Exception): + """Raised when a UNIX-domain socket address is encountered.""" + + class Addr(common.Addr): r"""Represents an Nginx address, i.e. what comes after the 'listen' directive. @@ -51,8 +55,15 @@ class Addr(common.Addr): self.unspecified_address = host in self.UNSPECIFIED_IPV4_ADDRESSES @classmethod - def fromstring(cls, str_addr: str) -> Optional["Addr"]: - """Initialize Addr from string.""" + def fromstring(cls, str_addr: str) -> "Addr": + """Initialize Addr from string. + + :param str str_addr: nginx address string + :returns: parsed nginx address + :rtype: Addr + :raises SocketAddrError: if a UNIX-domain socket address is given + + """ parts = str_addr.split(' ') ssl = False default = False @@ -64,9 +75,9 @@ class Addr(common.Addr): # The first part must be the address addr = parts.pop(0) - # Ignore UNIX-domain sockets + # Raise for UNIX-domain sockets if addr.startswith('unix:'): - return None + raise SocketAddrError(f'encountered UNIX-domain socket address {str_addr}') # IPv6 check ipv6_match = re.match(r'\[.*\]', addr) diff --git a/certbot-nginx/certbot_nginx/_internal/parser.py b/certbot-nginx/certbot_nginx/_internal/parser.py index 867e22272..a43e6c4b8 100644 --- a/certbot-nginx/certbot_nginx/_internal/parser.py +++ b/certbot-nginx/certbot_nginx/_internal/parser.py @@ -794,11 +794,14 @@ def _parse_server_raw(server: UnspacedList) -> Dict[str, Any]: if not directive: continue if directive[0] == 'listen': - addr = obj.Addr.fromstring(" ".join(directive[1:])) - if addr: - addrs.add(addr) - if addr.ssl: - ssl = True + try: + addr = obj.Addr.fromstring(" ".join(directive[1:])) + except obj.SocketAddrError: + # Ignore UNIX-domain socket addresses + continue + addrs.add(addr) + if addr.ssl: + ssl = True elif directive[0] == 'server_name': names.update(x.strip('"\'') for x in directive[1:]) elif _is_ssl_on_directive(directive): diff --git a/certbot-nginx/certbot_nginx/_internal/tests/obj_test.py b/certbot-nginx/certbot_nginx/_internal/tests/obj_test.py index 43ac31d27..9dd56aaa4 100644 --- a/certbot-nginx/certbot_nginx/_internal/tests/obj_test.py +++ b/certbot-nginx/certbot_nginx/_internal/tests/obj_test.py @@ -16,8 +16,7 @@ class AddrTest(unittest.TestCase): self.addr4 = Addr.fromstring("*:80 default_server ssl") self.addr5 = Addr.fromstring("myhost") self.addr6 = Addr.fromstring("80 default_server spdy") - self.addr7 = Addr.fromstring("unix:/var/run/nginx.sock") - self.addr8 = Addr.fromstring("*:80 default ssl") + self.addr7 = Addr.fromstring("*:80 default ssl") def test_fromstring(self): assert self.addr1.get_addr() == "192.168.1.1" @@ -50,9 +49,13 @@ class AddrTest(unittest.TestCase): assert self.addr6.ssl is False assert self.addr6.default is True - assert self.addr8.default is True + assert self.addr7.default is True - assert self.addr7 is None + def test_fromstring_socket(self): + from certbot_nginx._internal.obj import Addr, SocketAddrError + socket_string = r"unix:/var/run/nginx.sock" + with pytest.raises(SocketAddrError, match=socket_string): + Addr.fromstring(socket_string) def test_str(self): assert str(self.addr1) == "192.168.1.1" @@ -61,7 +64,7 @@ class AddrTest(unittest.TestCase): assert str(self.addr4) == "*:80 default_server ssl" assert str(self.addr5) == "myhost" assert str(self.addr6) == "80 default_server" - assert str(self.addr8) == "*:80 default_server ssl" + assert str(self.addr7) == "*:80 default_server ssl" def test_to_string(self): assert self.addr1.to_string() == "192.168.1.1" diff --git a/certbot/certbot/plugins/common.py b/certbot/certbot/plugins/common.py index b38f4fc21..72919917b 100644 --- a/certbot/certbot/plugins/common.py +++ b/certbot/certbot/plugins/common.py @@ -261,7 +261,7 @@ class Addr: self.ipv6 = ipv6 @classmethod - def fromstring(cls: Type[GenericAddr], str_addr: str) -> Optional[GenericAddr]: + def fromstring(cls: Type[GenericAddr], str_addr: str) -> GenericAddr: """Initialize Addr from string.""" if str_addr.startswith('['): # ipv6 addresses starts with [