From 37a7f3924d9e0727e58379fe817cc850af287e54 Mon Sep 17 00:00:00 2001 From: Oliver Smith Date: Thu, 25 Jan 2024 20:16:25 +0100 Subject: [PATCH] Fix preserving proxy variables (MR 2237) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 8 ++++++- pmb/chroot/user.py | 6 ++++- pmb/helpers/run.py | 5 ++++ pmb/helpers/run_core.py | 31 +++++++++++++++++------- test/test_proxy.py | 52 +++++++++++++++++++++++++++++++++++++++++ test/test_run_core.py | 6 ----- 6 files changed, 91 insertions(+), 17 deletions(-) create mode 100644 test/test_proxy.py diff --git a/pmb/chroot/root.py b/pmb/chroot/root.py index 4555638d..ce3c1165 100644 --- a/pmb/chroot/root.py +++ b/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"] diff --git a/pmb/chroot/user.py b/pmb/chroot/user.py index 9bad1e9b..05947e01 100644 --- a/pmb/chroot/user.py +++ b/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"): diff --git a/pmb/helpers/run.py b/pmb/helpers/run.py index 7ffe00c1..78f7517f 100644 --- a/pmb/helpers/run.py +++ b/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) diff --git a/pmb/helpers/run_core.py b/pmb/helpers/run_core.py index 10c52afb..5aa17632 100644 --- a/pmb/helpers/run_core.py +++ b/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() diff --git a/test/test_proxy.py b/test/test_proxy.py new file mode 100644 index 00000000..b48685b0 --- /dev/null +++ b/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" diff --git a/test/test_run_core.py b/test/test_run_core.py index d120fc0f..13428b55 100644 --- a/test/test_run_core.py +++ b/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):