From 415bac12e4e59f559d98fbb634c6762a088b0adf Mon Sep 17 00:00:00 2001 From: Jiri Hnidek Date: Tue, 15 Aug 2023 10:45:22 +0200 Subject: [PATCH 1/8] RHEL-13375: 1.28 Parse URL properly * Backport to 1.28 branch * Original commit: e83e637fd531ee9c9ba7b65218a4bb96cf323119 * Old tracking number for Bugzilla: 2225403 * Allow parsing of URLs containing IPv6 address like http://user:pass@[2001:db8::dead:beef:1]:3128/prefix * Use properties of urllib.parse to get username, password, hostname, port and prefix * Fixed unit tests for the case "https://www.redhat.com:/en", because it is IMHO correct URL * Extended unit tests related to parse_url(). --- src/rhsm/utils.py | 75 ++++++++++++------------------------- test/rhsm/unit/test_util.py | 20 +++++++--- test/test_utils.py | 16 +++++--- 3 files changed, 47 insertions(+), 64 deletions(-) diff --git a/src/rhsm/utils.py b/src/rhsm/utils.py index 038a164261..f9a5736d08 100644 --- a/src/rhsm/utils.py +++ b/src/rhsm/utils.py @@ -116,7 +116,6 @@ def parse_url(local_server_entry, :param default_port: default_port :return: a tuple of (username, password, hostname, port, path) """ - # Adding http:// onto the front of the hostname if local_server_entry == "": raise ServerUrlParseErrorEmpty(local_server_entry) @@ -142,65 +141,37 @@ def parse_url(local_server_entry, if not good_url: good_url = "https://%s" % local_server_entry - #FIXME: need a try except here? docs - # don't seem to indicate any expected exceptions + # FIXME: need a try except here? docs + # don't seem to indicate any expected exceptions result = six.moves.urllib.parse.urlparse(good_url) - username = default_username - password = default_password - #netloc = result[1].split(":") - - # to support username and password, let's split on @ - # since the format will be username:password@hostname:port - foo = result[1].split("@") - - # handle username/password portion, then deal with host:port - # just in case someone passed in @hostname without - # a username, we default to the default_username - if len(foo) > 1: - creds = foo[0].split(":") - netloc = foo[1].split(":") - - if len(creds) > 1: - password = creds[1] - if creds[0] is not None and len(creds[0]) > 0: - username = creds[0] - else: - netloc = foo[0].split(":") - - # in some cases, if we try the attr accessors, we'll - # get a ValueError deep down in urlparse, particular if - # port ends up being None - # - # So maybe check result.port/path/hostname for None, and - # throw an exception in those cases. - # adding the schem seems to avoid this though - port = default_port - if len(netloc) > 1: - if netloc[1] != "": - port = str(netloc[1]) - else: - raise ServerUrlParseErrorPort(local_server_entry) - # path can be None? - prefix = default_prefix - if result[2] is not None: - if result[2] != '': - prefix = result[2] + username = result.username + if username is None or username == "": + username = default_username - hostname = default_hostname - if netloc[0] is not None: - if netloc[0] != "": - hostname = netloc[0] + password = result.password + if password is None: + password = default_password + + hostname = result.hostname + if hostname is None: + hostname = default_hostname try: - if port: - int(port) - except TypeError: - raise ServerUrlParseErrorPort(local_server_entry) + port = result.port except ValueError: raise ServerUrlParseErrorPort(local_server_entry) - return (username, password, hostname, port, prefix) + if port is None: + port = default_port + else: + port = str(port) + + prefix = result.path + if prefix is None or prefix == "": + prefix = default_prefix + + return username, password, hostname, port, prefix def get_env_proxy_info(): diff --git a/test/rhsm/unit/test_util.py b/test/rhsm/unit/test_util.py index bfe243c331..15d227e853 100644 --- a/test/rhsm/unit/test_util.py +++ b/test/rhsm/unit/test_util.py @@ -137,16 +137,24 @@ def test_bad_http_scheme(self): local_url) def test_colon_but_no_port(self): + # This is correct URL local_url = "https://myhost.example.com:/myapp" - self.assertRaises(ServerUrlParseErrorPort, - parse_url, - local_url) + username, password, hostname, port, prefix = parse_url(local_url) + self.assertIsNone(username) + self.assertIsNone(password) + self.assertEqual(hostname, "myhost.example.com") + self.assertIsNone(port) + self.assertEqual(prefix, "/myapp") def test_colon_but_no_port_no_scheme(self): + # This is correct URL local_url = "myhost.example.com:/myapp" - self.assertRaises(ServerUrlParseErrorPort, - parse_url, - local_url) + username, password, hostname, port, prefix = parse_url(local_url) + self.assertIsNone(username) + self.assertIsNone(password) + self.assertEqual(hostname, "myhost.example.com") + self.assertIsNone(port) + self.assertEqual(prefix, "/myapp") def test_colon_slash_slash_but_nothing_else(self): local_url = "http://" diff --git a/test/test_utils.py b/test/test_utils.py index 9d0bacdef0..3f83e560e6 100644 --- a/test/test_utils.py +++ b/test/test_utils.py @@ -174,16 +174,20 @@ def test_bad_http_scheme(self): local_url) def test_colon_but_no_port(self): + # This is also correct! local_url = "https://myhost.example.com:/myapp" - self.assertRaises(ServerUrlParseErrorPort, - parse_server_info, - local_url) + hostname, port, prefix = parse_server_info(local_url) + self.assertEqual(hostname, "myhost.example.com") + self.assertEqual(port, DEFAULT_PORT) + self.assertEqual(prefix, "/myapp") def test_colon_but_no_port_no_scheme(self): + # This is also correct! local_url = "myhost.example.com:/myapp" - self.assertRaises(ServerUrlParseErrorPort, - parse_server_info, - local_url) + hostname, port, prefix = parse_server_info(local_url) + self.assertEqual(hostname, "myhost.example.com") + self.assertEqual(port, DEFAULT_PORT) + self.assertEqual(prefix, "/myapp") def test_colon_slash_slash_but_nothing_else(self): local_url = "http://" From 75f9adbb9131d6ce989a35b569dbc57cac8273ca Mon Sep 17 00:00:00 2001 From: Jiri Hnidek Date: Wed, 27 Sep 2023 17:08:55 +0200 Subject: [PATCH 2/8] Use parse_url() from utils.py for parsing URL, when --proxy is used. --- src/subscription_manager/managercli.py | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/src/subscription_manager/managercli.py b/src/subscription_manager/managercli.py index d14b85cf6c..95546bd439 100644 --- a/src/subscription_manager/managercli.py +++ b/src/subscription_manager/managercli.py @@ -40,7 +40,7 @@ import rhsm.config import rhsm.connection as connection from rhsm.connection import ProxyException, UnauthorizedException, ConnectionException, RemoteServerException, ConnectionOSErrorException -from rhsm.utils import cmd_name, remove_scheme, ServerUrlParseError +from rhsm.utils import cmd_name, ServerUrlParseError from subscription_manager import identity from subscription_manager.branding import get_branding @@ -462,17 +462,13 @@ def main(self, args=None): baseurl_server_prefix) config_changed = True - # support foo.example.com:3128 format if hasattr(self.options, "proxy_url") and self.options.proxy_url: - parts = remove_scheme(self.options.proxy_url).split(':') - self.proxy_hostname = parts[0] - # no ':' - if len(parts) > 1: - self.proxy_port = int(parts[1]) - else: - # if no port specified, use the one from the config, or fallback to the default - self.proxy_port = conf['server'].get_int('proxy_port') or rhsm.config.DEFAULT_PROXY_PORT - config_changed = True + default_proxy_port = conf["server"].get_int("proxy_port") or rhsm.config.DEFAULT_PROXY_PORT + proxy_user, proxy_pass, proxy_hostname, proxy_port, proxy_prefix = rhsm.utils.parse_url( + self.options.proxy_url, default_port=default_proxy_port + ) + self.proxy_hostname = proxy_hostname + self.proxy_port = int(proxy_port) if hasattr(self.options, "proxy_user") and self.options.proxy_user: self.proxy_user = self.options.proxy_user From f0396682b64742521a44f84aa78cb54abe6e534f Mon Sep 17 00:00:00 2001 From: Jiri Hnidek Date: Mon, 13 Nov 2023 14:57:35 +0100 Subject: [PATCH 3/8] Use username and password from --proxy=URL * When URL in --proxy=URL contains username and password (e.g. --proxy=https://user:pass@proxy.server.com:3128), then user and pass are used as proxy_user and proxy_pass * The --proxyuser and --proxypassword have higher priority. Thus, it does not break backward compatibility. * Added one unit test for this case --- src/subscription_manager/managercli.py | 2 ++ test/rhsm/unit/test_util.py | 18 ++++++++++++++++++ test/test_managercli.py | 18 ++++++++++++++++++ 3 files changed, 38 insertions(+) diff --git a/src/subscription_manager/managercli.py b/src/subscription_manager/managercli.py index 95546bd439..087786c9d3 100644 --- a/src/subscription_manager/managercli.py +++ b/src/subscription_manager/managercli.py @@ -467,6 +467,8 @@ def main(self, args=None): proxy_user, proxy_pass, proxy_hostname, proxy_port, proxy_prefix = rhsm.utils.parse_url( self.options.proxy_url, default_port=default_proxy_port ) + self.proxy_user = proxy_user + self.proxy_password = proxy_pass self.proxy_hostname = proxy_hostname self.proxy_port = int(proxy_port) diff --git a/test/rhsm/unit/test_util.py b/test/rhsm/unit/test_util.py index 15d227e853..22750435ae 100644 --- a/test/rhsm/unit/test_util.py +++ b/test/rhsm/unit/test_util.py @@ -281,6 +281,24 @@ def test_bad(self): class TestParseUrl(unittest.TestCase): + def test_ipv4_url(self): + local_url = "http://user:pass@192.168.0.10:3128/prefix" + (username, password, hostname, port, prefix) = parse_url(local_url) + self.assertEqual("user", username) + self.assertEqual("pass", password) + self.assertEqual("192.168.0.10", hostname) + self.assertEqual("3128", port) + self.assertEqual("/prefix", prefix) + + def test_ipv6_url(self): + local_url = "http://user:pass@[2001:db8::dead:beef:1]:3128/prefix" + (username, password, hostname, port, prefix) = parse_url(local_url) + self.assertEqual("user", username) + self.assertEqual("pass", password) + self.assertEqual("2001:db8::dead:beef:1", hostname) + self.assertEqual("3128", port) + self.assertEqual("/prefix", prefix) + def test_username_password(self): local_url = "http://user:pass@hostname:1111/prefix" (username, password, hostname, port, prefix) = parse_url(local_url) diff --git a/test/test_managercli.py b/test/test_managercli.py index 35ab637d38..40c28fb817 100644 --- a/test/test_managercli.py +++ b/test/test_managercli.py @@ -2484,6 +2484,24 @@ def test_list_by_default_with_options_from_super_class(self): self.cc._validate_options() self.assertTrue(self.cc.options.list) + def test_proxy_user_and_pass_from_url_overridden_by_cli_options(self): + """ + Test that --proxyuser and --proxypassword have higher priority than --proxy + """ + self.cc.main( + [ + "--proxy", + "https://foo:bar@www.example.com", + "--proxyuser", + "other-user", + "--proxypassword", + "other-password", + ] + ) + self.cc._validate_options() + self.assertEqual(self.cc.proxy_user, "other-user") + self.assertEqual(self.cc.proxy_password, "other-password") + def test_add_with_multiple_colons(self): self.cc.main(["--repo", "x", "--add", "url:http://example.com"]) self.cc._validate_options() From 154d98546fea1b9780a71336cd714c7e5553fedd Mon Sep 17 00:00:00 2001 From: Jiri Hnidek Date: Mon, 13 Nov 2023 15:02:35 +0100 Subject: [PATCH 4/8] Improved printing of addresses and URLs * Print IPv6 addresses correctly * Improved printing of proxy URL in debug mode --- src/rhsm/connection.py | 32 +++++++++++++++++++++++++------ test/rhsm/unit/test_connection.py | 8 ++++---- 2 files changed, 30 insertions(+), 10 deletions(-) diff --git a/src/rhsm/connection.py b/src/rhsm/connection.py index 6814c45261..e7fbe8a4ff 100644 --- a/src/rhsm/connection.py +++ b/src/rhsm/connection.py @@ -305,8 +305,12 @@ def __init__( connection_description = "" if proxy_description: connection_description += proxy_description - connection_description += "host=%s port=%s handler=%s %s" % (self.host, self.ssl_port, - self.handler, auth_description) + connection_description += "host=%s port=%s handler=%s %s" % ( + normalized_host(self.host), + safe_int(self.ssl_port), + self.handler, + auth_description, + ) log.debug("Connection built: %s", connection_description) @@ -679,6 +683,7 @@ def _print_debug_info_about_request(self, request_type, handler, final_headers, if 'SUBMAN_DEBUG_PRINT_REQUEST' in os.environ: yellow_col = '\033[93m' + magenta_col = "\033[95m" blue_col = '\033[94m' green_col = '\033[92m' red_col = '\033[91m' @@ -687,9 +692,24 @@ def _print_debug_info_about_request(self, request_type, handler, final_headers, msg = blue_col + "Making insecure request:" + end_col else: msg = blue_col + "Making request:" + end_col - msg += red_col + " %s:%s %s %s" % (self.host, self.ssl_port, request_type, handler) + end_col + msg += ( + red_col + + " https://" + + f"{normalized_host(self.host)}:{safe_int(self.ssl_port)}{handler} {request_type}" + + end_col + ) if self.proxy_hostname and self.proxy_port: - msg += blue_col + " using proxy " + red_col + f"{self.proxy_hostname}:{self.proxy_port}" + end_col + # Note: using only https:// is not a mistake. We use only https for proxy connection. + msg += blue_col + " Using proxy: " + magenta_col + "https://" + # Print username and eventually password + if self.proxy_user: + if self.proxy_user and self.proxy_password: + msg += f"{self.proxy_user}:{self.proxy_password}@" + elif self.proxy_user and not self.proxy_password: + msg += f"{self.proxy_user}@" + # Print hostname and port + msg += f"{normalized_host(self.proxy_hostname)}:{safe_int(self.proxy_port)}" + msg += end_col if 'SUBMAN_DEBUG_PRINT_REQUEST_HEADER' in os.environ: msg += blue_col + " %s" % final_headers + end_col if 'SUBMAN_DEBUG_PRINT_REQUEST_BODY' in os.environ and body is not None: @@ -810,7 +830,7 @@ def _request(self, request_type, method, info=None, headers=None, cert_key_pairs break # this client cert worked, no need to try more elif self.cert_dir: log.debug("Unable to get valid response: %s from CDN: %s" % - (result, self.host)) + (result, normalized_host(self.host))) except ssl.SSLError: if self.cert_file and not self.cert_dir: @@ -849,7 +869,7 @@ def _request(self, request_type, method, info=None, headers=None, cert_key_pairs if self.cert_dir: raise NoValidEntitlement( "Cannot access CDN content on: %s using any of entitlement cert-key pair: %s" % - (self.host, cert_key_pairs) + (normalized_host(self.host), cert_key_pairs) ) self._print_debug_info_about_response(result) diff --git a/test/rhsm/unit/test_connection.py b/test/rhsm/unit/test_connection.py index 01bf20866a..a304525ca5 100644 --- a/test/rhsm/unit/test_connection.py +++ b/test/rhsm/unit/test_connection.py @@ -165,7 +165,7 @@ def mock_config_without_proxy_settings(section, name): def test_https_proxy_info_allcaps(self): with patch.dict('os.environ', {'HTTPS_PROXY': 'http://u:p@host:4444'}): with patch.object(connection.config, 'get', self.mock_config_without_proxy_settings): - uep = UEPConnection(username="dummy", password="dummy", + uep = UEPConnection(host="dummy", username="dummy", password="dummy", handler="/Test/", insecure=True) self.assertEqual("u", uep.proxy_user) self.assertEqual("p", uep.proxy_password) @@ -177,7 +177,7 @@ def test_order(self): with patch.dict('os.environ', {'HTTPS_PROXY': 'http://u:p@host:4444', 'http_proxy': 'http://notme:orme@host:2222'}): with patch.object(connection.config, 'get', self.mock_config_without_proxy_settings): - uep = UEPConnection(username="dummy", password="dummy", + uep = UEPConnection(host="dummy", username="dummy", password="dummy", handler="/Test/", insecure=True) self.assertEqual("u", uep.proxy_user) self.assertEqual("p", uep.proxy_password) @@ -187,7 +187,7 @@ def test_order(self): def test_no_port(self): with patch.dict('os.environ', {'HTTPS_PROXY': 'http://u:p@host'}): with patch.object(connection.config, 'get', self.mock_config_without_proxy_settings): - uep = UEPConnection(username="dummy", password="dummy", + uep = UEPConnection(host="dummy", username="dummy", password="dummy", handler="/Test/", insecure=True) self.assertEqual("u", uep.proxy_user) self.assertEqual("p", uep.proxy_password) @@ -197,7 +197,7 @@ def test_no_port(self): def test_no_user_or_password(self): with patch.dict('os.environ', {'HTTPS_PROXY': 'http://host:1111'}): with patch.object(connection.config, 'get', self.mock_config_without_proxy_settings): - uep = UEPConnection(username="dummy", password="dummy", + uep = UEPConnection(host="dummy", username="dummy", password="dummy", handler="/Test/", insecure=True) self.assertEqual(None, uep.proxy_user) self.assertEqual(None, uep.proxy_password) From 960a4bec2aca6f647bcde04afdc6a1b711025db3 Mon Sep 17 00:00:00 2001 From: Pino Toscano Date: Tue, 10 Oct 2023 15:56:16 +0200 Subject: [PATCH 5/8] connection: normalize hostname in ConnectionOSErrorException Use the existing helper to provide a properly formatted hostname for user displaying (e.g. for exception); this will fix the printing of IPv6 addresses. Turn the existing "host" member as property, so the current users keep working as expected. --- src/rhsm/connection.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/rhsm/connection.py b/src/rhsm/connection.py index e7fbe8a4ff..b0788cf1f7 100644 --- a/src/rhsm/connection.py +++ b/src/rhsm/connection.py @@ -164,11 +164,15 @@ class ConnectionOSErrorException(ConnectionException): """ def __init__(self, host: str, port: int, handler: str, exc: OSError): - self.host = host + self._host = host self.port = port self.handler = handler self.exc = exc + @property + def host(self) -> str: + return normalized_host(self._host) + class BaseConnection(object): def __init__( From b58ffa54ed1fb3c6d5ff81631a9efed02cd07fcd Mon Sep 17 00:00:00 2001 From: Pino Toscano Date: Wed, 11 Oct 2023 15:17:10 +0200 Subject: [PATCH 6/8] cli: normalize hostname in error message Normalize the hostname printed for connection errors when pinging the server, in case anything was passed via --serverurl, --insecure, or --baseurl. --- src/subscription_manager/managercli.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/subscription_manager/managercli.py b/src/subscription_manager/managercli.py index 087786c9d3..f3830f1a02 100644 --- a/src/subscription_manager/managercli.py +++ b/src/subscription_manager/managercli.py @@ -524,7 +524,7 @@ def main(self, args=None): # this tries to actually connect to the server and ping it if not is_valid_server_info(self.no_auth_cp): system_exit(os.EX_UNAVAILABLE, _("Unable to reach the server at %s:%s%s") % ( - self.no_auth_cp.host, + connection.normalized_host(self.no_auth_cp.host), self.no_auth_cp.ssl_port, self.no_auth_cp.handler )) From c38d6bde770689f08af71c20e669b2cafbe7d5fc Mon Sep 17 00:00:00 2001 From: mhorky Date: Mon, 16 Oct 2023 15:43:05 +0200 Subject: [PATCH 7/8] CCT-10: Ensure IPv6-based URLs are properly formatted * Card ID: CCT-10 Recent changes (e83e637) fixed parsing information from IPv6 URLs. This patch fixes writing these addresses back as strings, mainly into configuration files. Previously, passing in IPv6 URL in e.g. `--baseurl` during registration resulted in broken address in the config file: $ subscription-manager register --baseurl https://[::1]:8443/prefix $ cat /etc/rhsm/rhsm.conf | grep baseurl baseurl=https://::1:8443/prefix After this patch, the square brackets are written when port is specified: $ cat /etc/rhsm/rhsm.conf | grep baseurl baseurl=https://[::1]:8443/prefix Due to the state of the code, it is likely that this problem also exists in other parts. If that is true, it is most likely in less sensitive parts, such as during logging. Looking back, using `ipaddress` stdlib would have been the right way to do this, but it is too late to do that. --- src/subscription_manager/utils.py | 5 +++++ test/test_utils.py | 16 ++++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/src/subscription_manager/utils.py b/src/subscription_manager/utils.py index 8244a8c419..56fc4a968a 100644 --- a/src/subscription_manager/utils.py +++ b/src/subscription_manager/utils.py @@ -97,6 +97,11 @@ def format_baseurl(hostname, port, prefix): if prefix == DEFAULT_CDN_PREFIX: prefix = prefix[:-1] + # Handle raw IPv6 addresses. + # RFC 1035 only allows characters `a-zA-Z0-9.`, we can do this. + if ":" in hostname: + hostname = f"[{hostname}]" + # just so we match how we format this by # default if port == DEFAULT_CDN_PORT: diff --git a/test/test_utils.py b/test/test_utils.py index 3f83e560e6..df244ebf56 100644 --- a/test/test_utils.py +++ b/test/test_utils.py @@ -297,6 +297,22 @@ def test_format_not_fqdn_with_port(self): self.assertEqual(prefix, DEFAULT_CDN_PREFIX) self.assertEqual("https://foo-bar:8088", format_baseurl(hostname, port, prefix)) + def test_ipv4(self): + result = format_baseurl(hostname="127.0.0.1", port="8080", prefix="/foo") + self.assertEqual("https://127.0.0.1:8080/foo", result) + + def test_ipv6(self): + result = format_baseurl(hostname="::1", port="8080", prefix="/foo") + self.assertEqual("https://[::1]:8080/foo", result) + + def test_ipv6_default_port(self): + result = format_baseurl(hostname="::1", port="443", prefix="/foo") + self.assertEqual("https://[::1]/foo", result) + + def test_ipv6_no_prefix(self): + result = format_baseurl(hostname="::1", port="8080", prefix="/") + self.assertEqual("https://[::1]:8080", result) + class TestUrlBaseJoinEmptyBase(fixture.SubManFixture): From a79a051658fc7b38172302469e9cea67b83bb9bd Mon Sep 17 00:00:00 2001 From: Jiri Hnidek Date: Wed, 25 Oct 2023 11:33:03 +0200 Subject: [PATCH 8/8] CCT-71: Try to ping server, when --proxy is used * When --proxy is used, then try to ping server before new configuration is used. --- src/subscription_manager/managercli.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/subscription_manager/managercli.py b/src/subscription_manager/managercli.py index f3830f1a02..22734ccb85 100644 --- a/src/subscription_manager/managercli.py +++ b/src/subscription_manager/managercli.py @@ -471,6 +471,7 @@ def main(self, args=None): self.proxy_password = proxy_pass self.proxy_hostname = proxy_hostname self.proxy_port = int(proxy_port) + config_changed = True if hasattr(self.options, "proxy_user") and self.options.proxy_user: self.proxy_user = self.options.proxy_user