[pve-devel] applied-series: [PATCH manager/storage 0/2] fix #3716: allow downloading iso/vztmpl/ova via https in proxied environments

[Thomas Lamprecht]

Am 26.03.25 um 11:51 schrieb Friedrich Weber:
> A user in enterprise support reported (and users also reported elsewhere [1]
> [2]) that ISO downloads via https currently do not work in environments using a
> proxy configured via the datacenter option `http_proxy`, if the connection to
> the ISO repository needs to go via the proxy. Proxied ISO downloads via http
> work. OVA and VZ template downloads are also affected.
> 
> The reason is that
> 
> - when querying the metadata via LWP, the proxy is only set for the `http`
>   scheme, not `https`
> 
> - when spawning wget to download the ISO, only the `http_proxy` environment
>   variable is set, not `https_proxy`, so wget will not use a proxy for the https
>   connection
> 
> Hence, neither operation uses the proxy for https. If the node cannot reach the
> destination without the proxy, both operations time out.
> 
> Fix this by
> 
> - patch 1: setting the `http_proxy` from the datacenter config also for the
>   https scheme when querying the metadata via LWP
> 
> - patch 2: passing the `https_proxy` environment variable to the wget command,
>   setting it to the `http_proxy` from the datacenter config
> 
> Tested by running squid in a container, and setting up the firewall to drop
> outgoing traffic from the PVE to everything but the proxy. Running
> tcpconnect-bpfcc from bpfcc-tools helps for tracing the destination of the
> http/https connections.
> 
> Maximiliano and I discussed this and looked into earlier iterations on this.
> 
> When a similar series was initially sent in 2021 [3], Thomas raised the concern
> [4] that our proxy settings should be more fine-grained and allow the user to
> differentiate between resources that should be proxied and that should not be
> proxied. For example, ISO repositories may be external (and thus may only be
> reachable via proxying) or internal (and thus may not be reachable via the
> proxy). Same for ACME endpoints. One idea was to group http requests issued by
> our stack into categories (such as `base`, `acme`, `template`) and allow the
> proxy setting to only apply to certain categories. I agree that something like
> this sounds like a useful feature, and one user also requested [5] something
> along these lines.
> 
> However, Maximiliano and I would argue this concern is orthogonal to the issue
> fixed by this patch. If a user has configured `http_proxy`, having the ISO
> download work via http and fail via https is inconsistent and thus confusing
> (see also Dominik's post [6] from back then). It might even nudge users into
> using http instead (which can still give the same integrity guarantees if they
> retrieve the checksum via https and compare them, but this is easy to get
> wrong). We'd propose we use the `http_proxy` for both https and https for now,
> and can still look into the categorization feature #5420 later.

I agree with that it being orthogonal, so while it's not ideal it still
improves the status quo.


> 
> Other places in our stack also use the `http_proxy` datacenter option for https
> connections, e.g. the ones that use proxmox_http::HttpClient with ProxyConfig
> such as with the notification system's webhook endpoint.
> 
> One argument against this patch is that it breaks backwards compatibility:
> Existing setups with `http_proxy` and an *internal* ISO repository from which
> they download via https will break. If this is a concern, I'd suggest we wait
> for PVE 9 to apply this.
It is a concern, but not sure if delaying this to a major release is of any
help if there is then still no way to workaround that without disabling and
then re-setting the proxy setting before/after such an operation.


> What do you think?

I've seen published papers that were less elaborate and less convincing ;-)

> 
> FTR, there is some overlap with a patch series by Hannes Laimer [7] but that
> one only concerned the `query-url-metadata` and the `apl_download` endpoints
> (for downloading appliance templates), not the ISO download.
> 
> [1] https://bugzilla.proxmox.com/show_bug.cgi?id=3716
> [2] https://bugzilla.proxmox.com/show_bug.cgi?id=5420#c2
> [3] https://lore.proxmox.com/pve-devel/20211109141359.990235-1-o.bektas@proxmox.com/
> [4] https://lore.proxmox.com/pve-devel/42391428-bd80-2d55-5cb6-7c8ecd97a3a8@proxmox.com/
> [5] https://bugzilla.proxmox.com/show_bug.cgi?id=5420#c0
> [6] https://lore.proxmox.com/pve-devel/a03631a3-fe78-7f6f-137d-7ee6fdf8f9ed@proxmox.com/
> [7] https://git.proxmox.com/?p=proxmox.git;a=blob;f=proxmox-notify/src/endpoints/webhook.rs;h=34dbac5488;hb=7abd2da759d#l266
> [8] https://lore.proxmox.com/pve-devel/20240308123535.1500-1-h.laimer@proxmox.com/
> 
> Co-authored-by: Maximiliano Sandoval <m.sandoval at proxmox.com>
> 
> storage:
> 
> Friedrich Weber (1):
>   fix #3716: api: download from url: use proxy option for https
> 
>  src/PVE/API2/Storage/Status.pm | 1 +
>  1 file changed, 1 insertion(+)
> 
> 
> manager:
> 
> Friedrich Weber (1):
>   fix #3716: api: nodes: query metadata: use proxy option for https
> 
>  PVE/API2/Nodes.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 
> Summary over all repositories:
>   2 files changed, 2 insertions(+), 1 deletions(-)
> 


applied series, thanks for bringing this up again and adding strong arguments!