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

[Maximiliano Sandoval]

Friedrich Weber <f.weber at proxmox.com> writes:

> 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.
>
> 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.
>
> What do you think?
>
> 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>

I am not sure whether the following trailers are redundant given the
Co-authored-by trailer, nevertheless:

Reviewed-by: Maximiliano Sandoval <m.sandoval at proxmox.com>
Tested-by: Maximiliano Sandoval <m.sandoval at proxmox.com>