2225403: Parse URL properly

[jirihnidek]

• BZ: https://bugzilla.redhat.com/show_bug.cgi?id=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 --proxy CLI option to use parse_url() function
• Print IPv6 addresses correctly
• Fixed unit tests for the case "https://www.redhat.com:/en", because it is correct URL

[jirihnidek]

I want to test it more.

[github-actions]

Coverage (computed on Fedora latest) •

File	Stmts	Miss	Cover	Missing
rhsm
connection.py	1007	456	54%	48–49, 53, 55–56, 81, 101–102, 109, 150, 280, 311, 377–382, 386–395, 456, 458, 560, 563, 570–576, 581, 637, 672–676, 678, 691, 718, 721–722, 724–725, 727, 738–742, 746, 750, 752–753, 780, 783, 787–788, 793, 796–797, 812, 816, 818–819, 846–852, 854, 856–863, 865, 868, 873, 876, 878, 880–884, 886–887, 889–894, 896–897, 899–900, 907–916, 918, 921–927, 929–930, 932–933, 944–949, 961–964, 969, 1033, 1035–1040, 1042, 1047–1051, 1057–1060, 1062–1067, 1071–1076, 1083, 1120, 1122, 1127, 1138, 1147–1150, 1154, 1156–1158, 1162–1163, 1165–1172, 1174, 1176, 1179–1186, 1189–1190, 1195, 1197, 1247, 1264–1267, 1291, 1313, 1343, 1348, 1351, 1354–1355, 1360, 1363, 1368, 1371, 1414–1418, 1425–1426, 1428, 1436–1437, 1439, 1456, 1469–1471, 1474, 1487, 1494, 1498, 1526–1528, 1533–1534, 1536–1537, 1539–1540, 1542–1556, 1558–1560, 1562–1573, 1575, 1592–1594, 1596–1598, 1600–1602, 1607, 1612–1614, 1619, 1646, 1678–1706, 1711–1712, 1714–1716, 1719–1720, 1723–1724, 1727–1728, 1747–1748, 1757–1758, 1768–1769, 1776–1777, 1783–1786, 1792–1795, 1801–1802, 1808–1809, 1829–1830, 1839–1843, 1851–1852, 1878–1881, 1906–1907, 1916–1917, 1925–1926, 1947, 1949–1951, 1953, 1955, 1958, 1960–1973, 1975–1976, 1985–1987, 1999–2000, 2009–2010, 2012, 2014–2016, 2023–2025, 2034–2036, 2044–2045, 2056, 2058–2059, 2061, 2063–2066, 2068–2070, 2073, 2075, 2082–2083, 2090–2091, 2101–2102, 2112–2115, 2125–2131, 2138–2141
utils.py	289	50	82%	217, 311, 331, 419, 421, 425, 428–430, 433–435, 438, 441–443, 519, 525–526, 531, 540–544, 546–547, 553–555, 562–566, 569–571, 573–574, 576–577, 580–582, 586–590
subscription_manager/cli_command
cli.py	209	31	85%	65, 69, 124, 128–129, 144, 201, 205, 207, 250, 289, 300–302, 316–318, 340, 360, 386, 395–396, 400–401, 417, 419–420, 422–423, 428, 436
TOTAL	18154	4561	74%

Tests	Skipped	Failures	Errors	Time
2636	9 💤	0 ❌	0 🔥	1m 3s ⏱️

[jirihnidek]

How to test this PR? First you will have to create some virtual network with IPv6 support. This example of configuration file can help:

<network>
  <name>network</name>
  <uuid>94a19657-4fe9-4fec-8bf2-af6808800d66</uuid>
  <bridge name='virbr1' stp='on' delay='0'/>
  <mac address='52:54:00:e7:82:6d'/>
  <domain name='network'/>
  <ip address='192.168.100.1' netmask='255.255.255.0'>
    <dhcp>
      <range start='192.168.100.128' end='192.168.100.254'/>
    </dhcp>
  </ip>
  <ip family='ipv6' address='fd00:3210:4567::1' prefix='64'>
    <dhcp>
      <range start='fd00:3210:4567::100' end='fd00:3210:4567::1ff'/>
    </dhcp>
  </ip>
</network>

Then configure some virtual machine to work as proxy server. You can use squid for that. Do not forget to add following line:

acl SSL_ports port 8443         # candlepin

to file /etc/squid/squid.conf. And you will have to configure this VM to use this new "IPv6" network.

Then you can test subscription-manager like this:

subscription-manager version --proxy="https://[fd00:3210:4567::1c7]:3128"

https://download.libvirt.org/virshcmdref/html/sect-net-create.html
https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/9/html/configuring_and_managing_virtualization/configuring-virtual-machine-network-connections_configuring-and-managing-virtualization

[jirihnidek]

Note: when both server.hostname and server.proxy_hostname are defines using IPv6 addresses, then it seems that connection is not possible. I tried to solve this issue, but it seems that there are some issues in 3rd parties modules.

[m-horky]

LGTM

[m-horky]

It is true that when both hostname and proxy are defined as IPv6 addresses, nothing happens (in Wireshark I can see that the connection is not being established at all?). However, when the proxy is specified from the CLI as ip:port pair, it does work. I wonder if we are doing something strange and treating those two options slightly differently.

[ptoscano]

Thanks for the changes, it generally LGTM; I left a couple of notes.

Also, please squash the last/4th commit (Extend unit tests related to parse_url().) together with the 1st one (Parse URL properly): since the first one changes/fixes the behaviour of parse_url, then IMHO all the unit test changes for it should be part of the same commit (and the bits changed by the 4th commit are changed in the 1st already).

[ptoscano]

this enables username & password for a proxy server to be specified via --proxy, whereas they were ignored before this change; considering there are the --proxyuser & --proxypassword option, and those will still override those attributes; for example:

subscription-manager --proxy proxy-user:proxy-password@proxy-host:port --proxyuser new-user

this will actually try to use the proxy-host with username new-user and password proxy-password, and proxy-user is silently discarded, which I do not think it is good for the user (they specify an option, or part of it, which gets overridden by another option)

I'd remove this, and keep --proxy to a simple hostname+port definition.

[jirihnidek]

I think that current behavior is correct for two reasons.

1. It does not break old behavior, when user and password were ignored in URL.
2. When you use --proxyuser then you explicitly say that you want to use specified user. This desire is IMHO expressed more explicitly than using --proxy URL.

If your proposal was implemented, then somebody could argue that --proxyuser is overridden by --proxy and it is breaking old behavior.

[ptoscano]

If your proposal was implemented

I guess I explained myself badly: my proposal here is to not pick username & password from --proxy, retaining the current behaviour.

[jirihnidek]

Why not to pick username & password from --proxy, when this CLI option is used and it contains username & password? I think that we should parse the URL properly according specification.

[ptoscano]

I understand the reasons about proper parsing, and normally I'd agree with that. However, the challenge here is that, even if small, this is a behaviour change: suddenly parts of --proxy are used whereas they were not previously.

Since the rest of the changes are good, what do you think about keep the existing behaviour of the --proxy parsing in this PR, and discuss the usage of credentials from --proxy as a separate change? This way we can discuss that with QE to check for the impact.

[jirihnidek]

If this behavior change will cause any issue to anyone (e.g. when the --proxy is used in some script), then there would be simple workaround for it (use --proxy with the correct and desired URL).

This PR is already split into 3 logical commits. I don't see reason, why this should be split more. Of course, we can discuss this with QE team and provide them some scratch build.

[ptoscano]

I'm asking to split the behaviour change for --proxy in its separate PR because, as we both clearly see, we do not agree on having this change in, while the actual fix (parsing IPv6 addresses correctly) is good and can be merge (and it would had been merged already long ago, if not for the behaviour change).

So I'll reiterate my request: can you please move the --proxy behaviour change away from this PR to a new one, so we can merge this? Thanks in advance!

[jirihnidek]

The change for --proxy just cannot be split away. If it was split away, then it would not fix the bug mentioned in the first comment. I believe that we want to fix bugs.

I will retrace what is content of this PR. There were two ways how to parse URLs. Both were wrong. First commit fixes parsing of URLs from rhsm.conf. Second commit just uses fixed parse_url() (fixed in first commit) for --proxy CLI option. Last commit fixes only minor things related to debug print of traffic.

I would like to clarify my previous comments. I want to have this change in. I do not see any reason, why this change should not be in. I still believe that we want to fix bugs.

[ptoscano]

The change for --proxy just cannot be split away.

Sure it can: it's about removing these two added lines:

            self.proxy_user = proxy_user
            self.proxy_password = proxy_pass

This is the behaviour change I'm pointing out, and I'd like to see discussed and handled separately.

Second commit just uses fixed parse_url() (fixed in first commit) for --proxy CLI option.

Yes, exactly, and I don't see where it says "change the behaviour of --proxy" as part of that, since the parsing issue can be fixed without the behaviour change.

The bug fix was "parsing IPv6 addresses properly", and this is a behaviour change that was never discussed earlier and it is not even required to fix the parsing of IPv6 addresses.

I want the change of this bug fix in already as well. Without this behaviour change, I would have approved and merged this few weeks ago already.

[jirihnidek]

Let me clarify one thing. Is your concern following? Current version of subscription-manager ignores proxy_username and proxy_password in URL of following command line option --proxy="https://proxy_username:[email protected]:3128" and there is almost zero probability that somebody uses such URL, because username and password are ignored. If somebody needs to use proxy_username and proxy_password, then they use --proxyuser and --proxypassword command line options, right?

If this PR was merged, then proxy_username and proxy_password of --proxy="https://proxy_username:[email protected]:3128" would not be ignored, but the username & password would not be ignored only in the case, when --proxyuser and --proxypassword were not provided, because --proxyuser and --proxypassword still has higher priority over proxy_username and proxy_password of --proxy="https://proxy_username:[email protected]:3128".

[jirihnidek]

It looks like that rawhide is broken in very strange ways.

[jirihnidek]

FYI: This PR has 4 commits since Wednesday.

[ptoscano]

Thanks, LGTM now.