Fix preserving proxy variables (MR 2237)

Fix "pmbootstrap chroot" and others not passing the proxy environment
variables correctly. Thanks to notfound405 for pointing this out!

Instead of only preserving proxy environment variables in
pmb.helpers.run_core, which should never be called directly, do it in
the calling functions:

* pmb.helpers.run.user
* pmb.helpers.run.root
* pmb.chroot.root
* pmb.chroot.user

This fixes that the environment variables were only really passed by
pmb.helpers.run.user, because the other functions would result in
something like:
  HTTP_PROXY=mytestproxy sudo env -i /usr/bin/sh -c '…'
This is needed to either elevate to root, or to elevate to root first
and then enter the chroot as root or user. Due to the "env -i", the
environment intentionally gets cleaned, but unintentionally also removes
the proxy environment variables that were explicitly set.

By adjusting the functions, they now run a variant of:
  sudo env -i /usr/bin/sh -c 'HTTP_PROXY=mytestproxy …'

The escaping is simplified in this example, run "pmbootstrap -v" to see
the not very readable, but proper escaping with shutil.quote().

Remove the previous test for preserving the environment variables in
pmb.helpers.run_core (as it should never be called directly), and test
instead the new behavior.

Fixes: issue 2299
Fixes: 13c4ac42 ("pmb.helpers.run_core: fix proxy env var logic")

pmb/chroot/root.py

@@ -27,13 +27,17 @@ def executables_absolute_path():
 
 def root(args, cmd, suffix="native", working_dir="/", output="log",
          output_return=False, check=None, env={}, auto_init=True,
-         disable_timeout=False):
+         disable_timeout=False, add_proxy_env_vars=True):
     """
     Run a command inside a chroot as root.
 
     :param env: dict of environment variables to be passed to the command, e.g.
                 {"JOBS": "5"}
     :param auto_init: automatically initialize the chroot
+    :param add_proxy_env_vars: if True, preserve HTTP_PROXY etc. vars from host
+                               environment. pmb.chroot.user sets this to False
+                               when calling pmb.chroot.root, because it already
+                               makes the variables part of the cmd argument.
 
     See pmb.helpers.run_core.core() for a detailed description of all other
     arguments and the return value.
@@ -64,6 +68,8 @@ def root(args, cmd, suffix="native", working_dir="/", output="log",
                "TERM": "xterm"}
     for key, value in env.items():
         env_all[key] = value
+    if add_proxy_env_vars:
+        pmb.helpers.run_core.add_proxy_env_vars(env_all)
 
     # Build the command in steps and run it, e.g.:
     # cmd: ["echo", "test"]

pmb/chroot/user.py

@@ -19,13 +19,17 @@ def user(args, cmd, suffix="native", working_dir="/", output="log",
     See pmb.helpers.run_core.core() for a detailed description of all other
     arguments and the return value.
     """
+    env = env.copy()
+    pmb.helpers.run_core.add_proxy_env_vars(env)
+
     if "HOME" not in env:
         env["HOME"] = "/home/pmos"
 
     flat_cmd = pmb.helpers.run_core.flat_cmd(cmd, env=env)
     cmd = ["busybox", "su", "pmos", "-c", flat_cmd]
     return pmb.chroot.root(args, cmd, suffix, working_dir, output,
-                           output_return, check, {}, auto_init)
+                           output_return, check, {}, auto_init,
+                           add_proxy_env_vars=False)
 
 
 def exists(args, username, suffix="native"):

pmb/helpers/run.py

@@ -23,6 +23,8 @@ def user(args, cmd, working_dir=None, output="log", output_return=False,
     msg += " ".join(cmd)
 
     # Add environment variables and run
+    env = env.copy()
+    pmb.helpers.run_core.add_proxy_env_vars(env)
     if env:
         cmd = ["sh", "-c", pmb.helpers.run_core.flat_cmd(cmd, env=env)]
     return pmb.helpers.run_core.core(args, msg, cmd, working_dir, output,
@@ -40,6 +42,9 @@ def root(args, cmd, working_dir=None, output="log", output_return=False,
     See pmb.helpers.run_core.core() for a detailed description of all other
     arguments and the return value.
     """
+    env = env.copy()
+    pmb.helpers.run_core.add_proxy_env_vars(env)
+
     if env:
         cmd = ["sh", "-c", pmb.helpers.run_core.flat_cmd(cmd, env=env)]
     cmd = pmb.config.sudo(cmd)

pmb/helpers/run_core.py

@@ -280,6 +280,28 @@ def sudo_timer_start():
     sudo_timer_iterate()
 
 
+def add_proxy_env_vars(env):
+    """
+    Add proxy environment variables present on the host to the environment of
+    the command we are running.
+    :param env: dict of environment variables, it will be extended with all of
+                the proxy env vars that are set on the host
+    """
+    proxy_env_vars = [
+        "FTP_PROXY",
+        "HTTPS_PROXY",
+        "HTTP_PROXY",
+        "HTTP_PROXY_AUTH"
+        "ftp_proxy",
+        "http_proxy",
+        "https_proxy",
+    ]
+
+    for var in proxy_env_vars:
+        if var in os.environ:
+            env[var] = os.environ[var]
+
+
 def core(args, log_message, cmd, working_dir=None, output="log",
          output_return=False, check=None, sudo=False, disable_timeout=False):
     """
@@ -340,15 +362,6 @@ def core(args, log_message, cmd, working_dir=None, output="log",
     """
     sanity_checks(output, output_return, check)
 
-    # Preserve proxy environment variables
-    env = {}
-    for var in ["FTP_PROXY", "ftp_proxy", "HTTP_PROXY", "http_proxy",
-                "HTTPS_PROXY", "https_proxy", "HTTP_PROXY_AUTH"]:
-        if var in os.environ:
-            env[var] = os.environ[var]
-    if env:
-        cmd = ["sh", "-c", flat_cmd(cmd, env=env)]
-
     if args.sudo_timer and sudo:
         sudo_timer_start()
 

test/test_proxy.py

@@ -0,0 +1,52 @@
+# Copyright 2024 Oliver Smith
+# SPDX-License-Identifier: GPL-3.0-or-later
+""" Test preserving HTTP_PROXY and other proxy env vars with all pmbootstrap
+    run functions. """
+import os
+import pytest
+import sys
+
+import pmb_test  # noqa
+import pmb.chroot.root
+import pmb.chroot.user
+import pmb.helpers.run
+import pmb.helpers.run_core
+
+
+@pytest.fixture
+def args(request):
+    import pmb.parse
+    sys.argv = ["pmbootstrap.py", "chroot"]
+    args = pmb.parse.arguments()
+    args.log = args.work + "/log_testsuite.txt"
+    pmb.helpers.logging.init(args)
+    request.addfinalizer(pmb.helpers.logging.logfd.close)
+    return args
+
+
+def test_proxy_user(args, monkeypatch):
+    func = pmb.helpers.run.user
+    monkeypatch.setattr(os, "environ", {"HTTP_PROXY": "testproxy"})
+    ret = func(args, ["sh", "-c", 'echo "$HTTP_PROXY"'], output_return=True)
+    assert ret == "testproxy\n"
+
+
+def test_proxy_root(args, monkeypatch):
+    func = pmb.helpers.run.root
+    monkeypatch.setattr(os, "environ", {"HTTP_PROXY": "testproxy"})
+    ret = func(args, ["sh", "-c", 'echo "$HTTP_PROXY"'], output_return=True)
+    assert ret == "testproxy\n"
+
+
+def test_proxy_chroot_user(args, monkeypatch):
+    func = pmb.chroot.user
+    monkeypatch.setattr(os, "environ", {"HTTP_PROXY": "testproxy"})
+    ret = func(args, ["sh", "-c", 'echo "$HTTP_PROXY"'], output_return=True)
+    assert ret == "testproxy\n"
+
+
+def test_proxy_chroot_root(args, monkeypatch):
+    func = pmb.chroot.root
+    monkeypatch.setattr(os, "environ", {"HTTP_PROXY": "testproxy"})
+    ret = func(args, ["sh", "-c", 'echo "$HTTP_PROXY"'], output_return=True)
+    assert ret == "testproxy\n"

test/test_run_core.py

@@ -1,7 +1,6 @@
 # Copyright 2023 Oliver Smith
 # SPDX-License-Identifier: GPL-3.0-or-later
 """ Test pmb.helpers.run_core """
-import os
 import pytest
 import re
 import subprocess
@@ -158,11 +157,6 @@ def test_core(args, monkeypatch):
         func(args, msg, ["sleep", "1"], output="log")
     assert re.search(r"^Command failed \(exit code -?\d*\): ", str(e.value))
 
-    # Preserve proxy environment variables
-    monkeypatch.setattr(os, "environ", {"FTP_PROXY": "testproxy"})
-    ret = func(args, msg, ["sh", "-c", 'echo "$FTP_PROXY"'], output_return=True)
-    assert ret == "testproxy\n"
-
 
 @pytest.mark.skip_ci
 def test_sudo_timer(args):