From 9162f94dee0fb3b67de4ad6a4a27ec8021d048ff Mon Sep 17 00:00:00 2001 From: Ben Grande Date: Mon, 2 Mar 2026 19:14:50 +0100 Subject: [PATCH 01/19] Reduce number of statements --- qrexec/tools/qubes_policy_editor.py | 48 ++++++++++++----------------- 1 file changed, 20 insertions(+), 28 deletions(-) diff --git a/qrexec/tools/qubes_policy_editor.py b/qrexec/tools/qubes_policy_editor.py index ffe938dd..78429e94 100644 --- a/qrexec/tools/qubes_policy_editor.py +++ b/qrexec/tools/qubes_policy_editor.py @@ -63,32 +63,27 @@ def manage_policy(name, is_include=False): # not found, ignore, else abort as the request was refused. file_exists = False if is_include: - try: - original_content, token = client.policy_include_get(name) - file_exists = True - except subprocess.CalledProcessError as e: - wanted_path = str(INCLUDEPATH) + "/" + name + "\n" - not_found = "Not found: " + wanted_path - if e.output.decode() != not_found: - print("Failed to get file: " + name) - sys.exit(1) + policy_get = client.policy_include_get + policy_replace = client.policy_include_replace + wanted_path = str(INCLUDEPATH) + "/" + name + "\n" + suffix = "_include_" else: - try: - original_content, token = client.policy_get(name) - file_exists = True - except subprocess.CalledProcessError as e: - wanted_path = str(POLICYPATH) + "/" + name + ".policy\n" - not_found = "Not found: " + wanted_path - if e.output.decode() != not_found: - print("Failed to get file: " + name) - sys.exit(1) + policy_get = client.policy_get + policy_replace = client.policy_replace + wanted_path = str(POLICYPATH) + "/" + name + ".policy\n" + suffix = "_" - if is_include: - # pylint: disable=consider-using-with - tmpfile = tempfile.NamedTemporaryFile(suffix="_include_" + name) - else: - # pylint: disable=consider-using-with - tmpfile = tempfile.NamedTemporaryFile(suffix="_" + name) + try: + original_content, token = policy_get(name) + file_exists = True + except subprocess.CalledProcessError as e: + not_found = "Not found: " + wanted_path + if e.output.decode() != not_found: + print("Failed to get file: " + name) + sys.exit(1) + + # pylint: disable=consider-using-with + tmpfile = tempfile.NamedTemporaryFile(suffix=suffix + name) if file_exists: with open(tmpfile.name, "w", encoding="utf-8") as current_file: @@ -104,10 +99,7 @@ def manage_policy(name, is_include=False): current_file.close() try: - if is_include: - client.policy_include_replace(name, content, token) - else: - client.policy_replace(name, content, token) + policy_replace(name, content, token) except subprocess.CalledProcessError as e: print("Failed to replace file: " + name) sys.exit(1) From 28c173b016f362b513636cce1f49528c2f45e668 Mon Sep 17 00:00:00 2001 From: Ben Grande Date: Mon, 2 Mar 2026 19:15:18 +0100 Subject: [PATCH 02/19] Pass end of options separator to shell commands --- qrexec/tools/qubes_policy_editor.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/qrexec/tools/qubes_policy_editor.py b/qrexec/tools/qubes_policy_editor.py index 78429e94..9b63c4c1 100644 --- a/qrexec/tools/qubes_policy_editor.py +++ b/qrexec/tools/qubes_policy_editor.py @@ -132,13 +132,13 @@ def lint_policy(path, is_include=False): :param path: path or "-" :param is_include: Boolean """ - edit_cmd = "${VISUAL:-${EDITOR:-vi}} " + path + edit_cmd = "${VISUAL:-${EDITOR:-vi}} -- " + path subprocess.run(edit_cmd, shell=True, check=True) + lint_cmd = "qubes-policy-lint " if is_include: - lint_cmd = "qubes-policy-lint --include-service " + path - else: - lint_cmd = "qubes-policy-lint " + path + lint_cmd += "--include-service " + lint_cmd += "-- " + path try: subprocess.run(lint_cmd, shell=True, check=True) From 8c87f3743db91f7db53de928dbfeccedb0622767 Mon Sep 17 00:00:00 2001 From: Ben Grande Date: Mon, 2 Mar 2026 19:20:24 +0100 Subject: [PATCH 03/19] Use policy extension to allow file type detection --- qrexec/tools/qubes_policy_editor.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/qrexec/tools/qubes_policy_editor.py b/qrexec/tools/qubes_policy_editor.py index 9b63c4c1..a3281323 100644 --- a/qrexec/tools/qubes_policy_editor.py +++ b/qrexec/tools/qubes_policy_editor.py @@ -66,12 +66,12 @@ def manage_policy(name, is_include=False): policy_get = client.policy_include_get policy_replace = client.policy_include_replace wanted_path = str(INCLUDEPATH) + "/" + name + "\n" - suffix = "_include_" + suffix = "_include_" + name else: policy_get = client.policy_get policy_replace = client.policy_replace wanted_path = str(POLICYPATH) + "/" + name + ".policy\n" - suffix = "_" + suffix = "_" + name + ".policy" try: original_content, token = policy_get(name) @@ -83,7 +83,7 @@ def manage_policy(name, is_include=False): sys.exit(1) # pylint: disable=consider-using-with - tmpfile = tempfile.NamedTemporaryFile(suffix=suffix + name) + tmpfile = tempfile.NamedTemporaryFile(suffix=suffix) if file_exists: with open(tmpfile.name, "w", encoding="utf-8") as current_file: From b3ac26836018a5373c5b0a2b894aa5a16690d937 Mon Sep 17 00:00:00 2001 From: Ben Grande Date: Mon, 2 Mar 2026 19:21:42 +0100 Subject: [PATCH 04/19] Remove non-root blocker Root is not necessary, since we are using the PolicyClient to get and replace the policy. Not using root also helps editors load extensions that would not be loaded if running as root. --- qrexec/tools/qubes_policy.py | 7 ------- qrexec/tools/qubes_policy_editor.py | 6 ------ 2 files changed, 13 deletions(-) diff --git a/qrexec/tools/qubes_policy.py b/qrexec/tools/qubes_policy.py index 440ee006..06347b1a 100755 --- a/qrexec/tools/qubes_policy.py +++ b/qrexec/tools/qubes_policy.py @@ -23,12 +23,10 @@ import argparse import sys -import os import subprocess from ..policy.admin_client import PolicyClient from .. import RPCNAME_ALLOWED_CHARSET -from ..client import IN_DOM0 parser = argparse.ArgumentParser( usage="qubes-policy {[-l]|-g|-r|-d} [include/][RPCNAME[+ARGUMENT]]" @@ -115,11 +113,6 @@ def run_method(method, name, client, is_include): def main(args=None): args = parser.parse_args(args) - - if IN_DOM0 and os.getuid() != 0: - print("You need to run as root in dom0") - sys.exit(1) - client = PolicyClient() name = args.name diff --git a/qrexec/tools/qubes_policy_editor.py b/qrexec/tools/qubes_policy_editor.py index a3281323..b87fe027 100644 --- a/qrexec/tools/qubes_policy_editor.py +++ b/qrexec/tools/qubes_policy_editor.py @@ -25,13 +25,11 @@ from __future__ import print_function import argparse -import os import subprocess import sys import tempfile from ..policy.admin_client import PolicyClient from .. import RPCNAME_ALLOWED_CHARSET, POLICYPATH, INCLUDEPATH -from ..client import IN_DOM0 def validate_name(name): @@ -177,10 +175,6 @@ def main(): ) args = parser.parse_args() - if IN_DOM0 and os.getuid() != 0: - print("You need to run as root in dom0") - sys.exit(1) - name = args.file include_prefix = "include/" is_include = False From a02c8b806adafdbaa24d3fb9aba02fa50e7195de Mon Sep 17 00:00:00 2001 From: Ben Grande Date: Wed, 4 Mar 2026 19:12:57 +0100 Subject: [PATCH 05/19] Print policy editor messages to stderr --- qrexec/tools/qubes_policy_editor.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/qrexec/tools/qubes_policy_editor.py b/qrexec/tools/qubes_policy_editor.py index b87fe027..01ef663a 100644 --- a/qrexec/tools/qubes_policy_editor.py +++ b/qrexec/tools/qubes_policy_editor.py @@ -42,7 +42,7 @@ def validate_name(name): if invalid_chars: print( "invalid character(s) in the file name: {!r}".format( - "".join(sorted(invalid_chars)) + "".join(sorted(invalid_chars)), file=sys.stderr ) ) sys.exit(1) @@ -77,7 +77,7 @@ def manage_policy(name, is_include=False): except subprocess.CalledProcessError as e: not_found = "Not found: " + wanted_path if e.output.decode() != not_found: - print("Failed to get file: " + name) + print("Failed to get file: " + name, file=sys.stderr) sys.exit(1) # pylint: disable=consider-using-with @@ -99,7 +99,7 @@ def manage_policy(name, is_include=False): try: policy_replace(name, content, token) except subprocess.CalledProcessError as e: - print("Failed to replace file: " + name) + print("Failed to replace file: " + name, file=sys.stderr) sys.exit(1) tmpfile.close() @@ -112,7 +112,7 @@ def get_reply(path, is_include=False): :param path: path or "-" :param is_include: Boolean """ - print("What now? ", end="") + print("What now? ", end="", file=sys.stderr) reply = str(input()) if reply == "e": lint_policy(path, is_include=is_include) @@ -145,13 +145,17 @@ def lint_policy(path, is_include=False): if return_code == 0: return if return_code == 127: - print("The linting program 'qubes-policy-lint' is not installed.") + print( + "The linting program 'qubes-policy-lint' is not installed.", + file=sys.stderr, + ) sys.exit(1) else: print( "Linting failed, do you want to:\n" " (e)dit again\n" - " (q)uit without saving changes?" + " (q)uit without saving changes?", + file=sys.stderr, ) get_reply(path, is_include=is_include) From f9dc03e2c7c9de7967d3121ca3dcd8c7f0f0a0b7 Mon Sep 17 00:00:00 2001 From: Ben Grande Date: Mon, 2 Mar 2026 19:15:35 +0100 Subject: [PATCH 06/19] Print policy editor exceptions --- qrexec/tools/qubes_policy_editor.py | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/qrexec/tools/qubes_policy_editor.py b/qrexec/tools/qubes_policy_editor.py index 01ef663a..c52317c8 100644 --- a/qrexec/tools/qubes_policy_editor.py +++ b/qrexec/tools/qubes_policy_editor.py @@ -74,10 +74,10 @@ def manage_policy(name, is_include=False): try: original_content, token = policy_get(name) file_exists = True - except subprocess.CalledProcessError as e: + except subprocess.CalledProcessError as exc: not_found = "Not found: " + wanted_path - if e.output.decode() != not_found: - print("Failed to get file: " + name, file=sys.stderr) + if exc.output.decode() != not_found: + print(f"Failed to get policy {name!r}: {exc}", file=sys.stderr) sys.exit(1) # pylint: disable=consider-using-with @@ -98,8 +98,12 @@ def manage_policy(name, is_include=False): try: policy_replace(name, content, token) - except subprocess.CalledProcessError as e: - print("Failed to replace file: " + name, file=sys.stderr) + except subprocess.CalledProcessError as exc: + print( + f"Failed to replace policy {name!r} with file {tmpfile.name!r}: " + f"{exc}", + file=sys.stderr, + ) sys.exit(1) tmpfile.close() @@ -131,7 +135,11 @@ def lint_policy(path, is_include=False): :param is_include: Boolean """ edit_cmd = "${VISUAL:-${EDITOR:-vi}} -- " + path - subprocess.run(edit_cmd, shell=True, check=True) + try: + subprocess.run(edit_cmd, shell=True, check=True) + except subprocess.CalledProcessError as exc: + print(f"Failed to open editor: {exc}", file=sys.stderr) + sys.exit(1) lint_cmd = "qubes-policy-lint " if is_include: @@ -146,7 +154,7 @@ def lint_policy(path, is_include=False): return if return_code == 127: print( - "The linting program 'qubes-policy-lint' is not installed.", + "The linting program 'qubes-policy-lint' is not installed", file=sys.stderr, ) sys.exit(1) From 4ea6c7943c41322cf8b1a49ce056e40347cd7520 Mon Sep 17 00:00:00 2001 From: Ben Grande Date: Thu, 5 Mar 2026 18:42:47 +0100 Subject: [PATCH 07/19] Correct ordering of environment variables checked --- qrexec/tools/qubes_policy_editor.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qrexec/tools/qubes_policy_editor.py b/qrexec/tools/qubes_policy_editor.py index c52317c8..c8bd766a 100644 --- a/qrexec/tools/qubes_policy_editor.py +++ b/qrexec/tools/qubes_policy_editor.py @@ -180,7 +180,7 @@ def main(): default=default_file, help="set file to be edited. The '.policy' suffix " "must not be included. Will search for an " - "editor by looking at $EDITOR, $VISUAL if " + "editor by looking at $VISUAL, $EDITOR, and if " "previous entry is unset or 'vi' if previous " "entry is also unset. Defaults to the user file.", nargs="?", From 73b0843337ee761cff6d3f8920a133c9431f8b90 Mon Sep 17 00:00:00 2001 From: Ben Grande Date: Thu, 5 Mar 2026 19:18:10 +0100 Subject: [PATCH 08/19] Make a class for qubes-policy-editor Helps avoid passing variables that could be defined on the class level. --- qrexec/tools/qubes_policy_editor.py | 217 ++++++++++++++-------------- 1 file changed, 111 insertions(+), 106 deletions(-) diff --git a/qrexec/tools/qubes_policy_editor.py b/qrexec/tools/qubes_policy_editor.py index c8bd766a..56c21681 100644 --- a/qrexec/tools/qubes_policy_editor.py +++ b/qrexec/tools/qubes_policy_editor.py @@ -53,119 +53,123 @@ def validate_name(name): return name -def manage_policy(name, is_include=False): - client = PolicyClient() - - # Don't use policy(.include).List to support restricted AdminVMs. Instead, - # try to policy(.include).Get the file and if it fails because the file is - # not found, ignore, else abort as the request was refused. - file_exists = False - if is_include: - policy_get = client.policy_include_get - policy_replace = client.policy_include_replace - wanted_path = str(INCLUDEPATH) + "/" + name + "\n" - suffix = "_include_" + name - else: - policy_get = client.policy_get - policy_replace = client.policy_replace - wanted_path = str(POLICYPATH) + "/" + name + ".policy\n" - suffix = "_" + name + ".policy" - - try: - original_content, token = policy_get(name) - file_exists = True - except subprocess.CalledProcessError as exc: - not_found = "Not found: " + wanted_path - if exc.output.decode() != not_found: - print(f"Failed to get policy {name!r}: {exc}", file=sys.stderr) - sys.exit(1) +class PolicyManager: + + def __init__(self, policy: str, is_include: bool = False) -> None: + self.policy = policy + self.is_include = is_include + self.tmpfile_name: str | None = None + + def manage_policy(self) -> None: + client = PolicyClient() + + # Don't use policy(.include).List to support restricted AdminVMs. + # Instead, try to policy(.include).Get the file and if it fails because + # the file is not found, ignore, else abort as the request was refused. + file_exists = False + if self.is_include: + policy_get = client.policy_include_get + policy_replace = client.policy_include_replace + wanted_path = str(INCLUDEPATH) + "/" + self.policy + "\n" + suffix = "_include_" + self.policy + else: + policy_get = client.policy_get + policy_replace = client.policy_replace + wanted_path = str(POLICYPATH) + "/" + self.policy + ".policy\n" + suffix = "_" + self.policy + ".policy" + + try: + original_content, token = policy_get(self.policy) + file_exists = True + except subprocess.CalledProcessError as exc: + not_found = "Not found: " + wanted_path + if exc.output.decode() != not_found: + print( + f"Failed to get policy {self.policy!r}: {exc}", + file=sys.stderr, + ) + sys.exit(1) + + # pylint: disable=consider-using-with + tmpfile = tempfile.NamedTemporaryFile(suffix=suffix) + + if file_exists: + with open(tmpfile.name, "w", encoding="utf-8") as current_file: + current_file.write(original_content) + current_file.close() + else: + token = "new" - # pylint: disable=consider-using-with - tmpfile = tempfile.NamedTemporaryFile(suffix=suffix) + self.tmpfile_name = tmpfile.name + self.lint_policy() - if file_exists: - with open(tmpfile.name, "w", encoding="utf-8") as current_file: - current_file.write(original_content) + with open(tmpfile.name, "r", encoding="utf-8") as current_file: + content = current_file.read() current_file.close() - else: - token = "new" - - lint_policy(tmpfile.name, is_include=is_include) - with open(tmpfile.name, "r", encoding="utf-8") as current_file: - content = current_file.read() - current_file.close() - - try: - policy_replace(name, content, token) - except subprocess.CalledProcessError as exc: - print( - f"Failed to replace policy {name!r} with file {tmpfile.name!r}: " - f"{exc}", - file=sys.stderr, - ) - sys.exit(1) - - tmpfile.close() - - -def get_reply(path, is_include=False): - """ - Get reply from user. - - :param path: path or "-" - :param is_include: Boolean - """ - print("What now? ", end="", file=sys.stderr) - reply = str(input()) - if reply == "e": - lint_policy(path, is_include=is_include) - return - if reply == "q": - sys.exit(0) - else: - get_reply(path, is_include=is_include) - - -def lint_policy(path, is_include=False): - """ - Open file and lint after closing it. If lint fails, wait for user reply. - - :param path: path or "-" - :param is_include: Boolean - """ - edit_cmd = "${VISUAL:-${EDITOR:-vi}} -- " + path - try: - subprocess.run(edit_cmd, shell=True, check=True) - except subprocess.CalledProcessError as exc: - print(f"Failed to open editor: {exc}", file=sys.stderr) - sys.exit(1) - - lint_cmd = "qubes-policy-lint " - if is_include: - lint_cmd += "--include-service " - lint_cmd += "-- " + path - - try: - subprocess.run(lint_cmd, shell=True, check=True) - except subprocess.CalledProcessError as exc: - return_code = exc.returncode - if return_code == 0: - return - if return_code == 127: + try: + policy_replace(self.policy, content, token) + except subprocess.CalledProcessError as exc: print( - "The linting program 'qubes-policy-lint' is not installed", + f"Failed to replace policy {self.policy!r} with file " + f"{tmpfile.name!r}: {exc}", file=sys.stderr, ) sys.exit(1) + + tmpfile.close() + + def get_reply(self) -> None: + """ + Get reply from user. + """ + print("What now? ", end="", file=sys.stderr) + reply = str(input()) + if reply == "e": + self.lint_policy() + return + if reply == "q": + sys.exit(0) else: - print( - "Linting failed, do you want to:\n" - " (e)dit again\n" - " (q)uit without saving changes?", - file=sys.stderr, - ) - get_reply(path, is_include=is_include) + self.get_reply() + + def lint_policy(self) -> None: + """ + Open file and lint after closing it. If lint fails, wait for user reply. + """ + assert isinstance(self.tmpfile_name, str) + edit_cmd = "${VISUAL:-${EDITOR:-vi}} -- " + self.tmpfile_name + try: + subprocess.run(edit_cmd, shell=True, check=True) + except subprocess.CalledProcessError as exc: + print(f"Failed to open editor: {exc}", file=sys.stderr) + sys.exit(1) + + lint_cmd = "qubes-policy-lint " + if self.is_include: + lint_cmd += "--include-service " + lint_cmd += "-- " + self.tmpfile_name + + try: + subprocess.run(lint_cmd, shell=True, check=True) + except subprocess.CalledProcessError as exc: + return_code = exc.returncode + if return_code == 0: + return + if return_code == 127: + print( + "The linting program 'qubes-policy-lint' is not installed", + file=sys.stderr, + ) + sys.exit(1) + else: + print( + "Linting failed, do you want to:\n" + " (e)dit again\n" + " (q)uit without saving changes?", + file=sys.stderr, + ) + self.get_reply() def main(): @@ -194,8 +198,9 @@ def main(): name = name[len(include_prefix) :] is_include = True - name = validate_name(name) - manage_policy(name, is_include) + policy = validate_name(name) + policy_manager = PolicyManager(policy=policy, is_include=is_include) + policy_manager.manage_policy() if __name__ == "__main__": From fbe7ddea9a30786aec5fabbf7c37706e553134ad Mon Sep 17 00:00:00 2001 From: Ben Grande Date: Thu, 5 Mar 2026 20:28:58 +0100 Subject: [PATCH 09/19] Return policy admin exceptions to client stderr For: https://github.com/QubesOS/qubes-issues/issues/10746 --- qrexec/tools/qubes_policy_admin.py | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/qrexec/tools/qubes_policy_admin.py b/qrexec/tools/qubes_policy_admin.py index b759588d..54a394fb 100644 --- a/qrexec/tools/qubes_policy_admin.py +++ b/qrexec/tools/qubes_policy_admin.py @@ -27,14 +27,19 @@ import sys import logging -from ..policy.admin import PolicyAdmin, PolicyAdminException +from ..policy.admin import ( + PolicyAdmin, + PolicyAdminException, + PolicyAdminTokenException, +) from .. import POLICYPATH def main(): + log_file = "/var/log/qubes/policy-admin.log" logging.basicConfig( level=logging.INFO, - filename="/var/log/qubes/policy-admin.log", + filename=log_file, format="%(asctime)s %(message)s", ) @@ -51,19 +56,21 @@ def main(): admin = PolicyAdmin(POLICYPATH) try: response = admin.handle_request(service_name, argument, payload) - except PolicyAdminException as exc: + except (PolicyAdminException, PolicyAdminTokenException) as exc: logging.warning( "%s+%s (%s): error: %s", service_name, argument, source, exc ) - print(exc) + pretty_exc = "{} {}".format(exc.__class__.__name__, exc) + sys.stderr.buffer.write(pretty_exc.encode()) sys.exit(1) except Exception: # pylint: disable=broad-except logging.exception( "%s+%s (%s): exception", service_name, argument, source ) - print( - "Internal error. See /var/log/qubes/policy-admin.log in dom0 for details." + error_msg = "Internal error. See {!r} in dom0 for details.".format( + log_file ) + sys.stderr.buffer.write(error_msg.encode()) sys.exit(2) else: logging.info("%s+%s (%s)", service_name, argument, source) From f6d3665987228fc56225ffb4ebd808e95f0c32af Mon Sep 17 00:00:00 2001 From: Ben Grande Date: Thu, 5 Mar 2026 20:47:29 +0100 Subject: [PATCH 10/19] Deduplicate subprocess keyword generator --- qrexec/client.py | 27 +++++---------------------- qrexec/utils.py | 12 +++++++++++- 2 files changed, 16 insertions(+), 23 deletions(-) diff --git a/qrexec/client.py b/qrexec/client.py index b836c338..602bf9cd 100644 --- a/qrexec/client.py +++ b/qrexec/client.py @@ -50,9 +50,8 @@ def call(dest: str, rpcname: str, arg: Optional[str] = None, *, input=None): # pylint: disable=redefined-builtin command = make_command(dest, rpcname, arg) - return subprocess.check_output( - command, **prepare_subprocess_kwds(input) - ).decode() + kwds = prepare_subprocess_kwds(input, for_popen=False, for_check=True) + return subprocess.check_output(command, **kwds).decode() async def call_async(dest, rpcname, arg=None, *, input=None): @@ -72,25 +71,9 @@ async def call_async(dest, rpcname, arg=None, *, input=None): # pylint: disable=redefined-builtin command = make_command(dest, rpcname, arg) - - if input is None: - stdin = subprocess.DEVNULL - to_communicate = None - elif isinstance(input, bytes): - stdin = subprocess.PIPE - to_communicate = input - elif isinstance(input, str): - stdin = subprocess.PIPE - to_communicate = input.encode() - else: - # Assume this is a file - stdin = input - to_communicate = None - - process = await asyncio.create_subprocess_exec( - *command, stdin=stdin, stdout=subprocess.PIPE - ) - + kwds = prepare_subprocess_kwds(input) + to_communicate = kwds.pop("input") + process = await asyncio.create_subprocess_exec(*command, **kwds) stdout, _stderr = await process.communicate(to_communicate) if process.returncode != 0: raise subprocess.CalledProcessError(process.returncode, command) diff --git a/qrexec/utils.py b/qrexec/utils.py index 81fd51d3..bb38b56e 100644 --- a/qrexec/utils.py +++ b/qrexec/utils.py @@ -175,16 +175,26 @@ def get_system_info() -> FullSystemInfo: return system_info_decoded -def prepare_subprocess_kwds(input: object) -> Dict[str, object]: +def prepare_subprocess_kwds( + input: object, for_popen: bool = True, for_check: bool = False +) -> Dict[str, object]: """Prepare kwds for :py:func:`subprocess.run` for given input""" # pylint: disable=redefined-builtin kwds: Dict[str, object] = {} if input is None: kwds["stdin"] = subprocess.DEVNULL + kwds["input"] = None elif isinstance(input, bytes): + if for_popen: + kwds["stdin"] = subprocess.PIPE kwds["input"] = input elif isinstance(input, str): + if for_popen: + kwds["stdin"] = subprocess.PIPE kwds["input"] = input.encode() else: # XXX this breaks on file-like objects that don't have .fileno kwds["stdin"] = input + kwds["input"] = None + if not for_check: + kwds["stdout"] = subprocess.PIPE return kwds From c84ab95a655d4b07aa0ba4a7eaf2191424273d89 Mon Sep 17 00:00:00 2001 From: Ben Grande Date: Fri, 6 Mar 2026 10:25:20 +0100 Subject: [PATCH 11/19] Unify include methods with normal policy methods Instead of making client deal with using different include methods, let the API handle that with is_include parameter. --- qrexec/client.py | 4 +- qrexec/policy/admin_client.py | 83 +++++++++++++++++++++++------ qrexec/tools/qubes_policy.py | 33 ++++-------- qrexec/tools/qubes_policy_editor.py | 15 +++--- 4 files changed, 88 insertions(+), 47 deletions(-) diff --git a/qrexec/client.py b/qrexec/client.py index 602bf9cd..345bcbb3 100644 --- a/qrexec/client.py +++ b/qrexec/client.py @@ -49,7 +49,7 @@ def call(dest: str, rpcname: str, arg: Optional[str] = None, *, input=None): """ # pylint: disable=redefined-builtin - command = make_command(dest, rpcname, arg) + command = make_command(dest=dest, rpcname=rpcname, arg=arg) kwds = prepare_subprocess_kwds(input, for_popen=False, for_check=True) return subprocess.check_output(command, **kwds).decode() @@ -70,7 +70,7 @@ async def call_async(dest, rpcname, arg=None, *, input=None): """ # pylint: disable=redefined-builtin - command = make_command(dest, rpcname, arg) + command = make_command(dest=dest, rpcname=rpcname, arg=arg) kwds = prepare_subprocess_kwds(input) to_communicate = kwds.pop("input") process = await asyncio.create_subprocess_exec(*command, **kwds) diff --git a/qrexec/policy/admin_client.py b/qrexec/policy/admin_client.py index f9692607..0086ad28 100644 --- a/qrexec/policy/admin_client.py +++ b/qrexec/policy/admin_client.py @@ -34,41 +34,90 @@ """ from typing import List, Tuple +from warnings import warn from ..client import call class PolicyClient: - def policy_list(self) -> List[str]: - return self.call("policy.List").rstrip("\n").split("\n") + def policy_list(self, is_include: bool = False) -> List[str]: + rpc = "List" + return self.call(rpc, is_include=is_include).rstrip("\n").split("\n") def policy_include_list(self) -> List[str]: - return self.call("policy.include.List").rstrip("\n").split("\n") + warn( + "Method 'policy_include_list()' is deprecated, use 'policy_list()' " + "instead", + DeprecationWarning, + ) + return self.policy_list(is_include=True) - def policy_get(self, name: str) -> Tuple[str, str]: - token, content = self.call("policy.Get", name).split("\n", 1) + def policy_get( + self, name: str, is_include: bool = False + ) -> Tuple[str, str]: + rpc = "Get" + token, content = self.call( + service_name=rpc, arg=name, is_include=is_include + ).split("\n", 1) return content, token def policy_include_get(self, name: str) -> Tuple[str, str]: - token, content = self.call("policy.include.Get", name).split("\n", 1) - return content, token + warn( + "Method 'policy_include_get()' is deprecated, use 'policy_get()' " + "instead", + DeprecationWarning, + ) + return self.policy_get(name=name, is_include=True) - def policy_replace(self, name: str, content: str, token="any"): - self.call("policy.Replace", name, token + "\n" + content) + def policy_replace( + self, + name: str, + content: str, + token: str = "any", + is_include: bool = False, + ): + rpc = "Replace" + self.call( + service_name=rpc, + arg=name, + payload=token + "\n" + content, + is_include=is_include, + ) def policy_include_replace(self, name: str, content: str, token="any"): - self.call("policy.include.Replace", name, token + "\n" + content) + warn( + "Method 'policy_include_replace()' is deprecated, use " + "'policy_replace()' instead", + DeprecationWarning, + ) + return self.policy_replace( + name=name, content=content, token=token, is_include=True + ) - def policy_remove(self, name: str, token="any"): - self.call("policy.Remove", name, token) + def policy_remove( + self, name: str, token: str = "any", is_include: bool = False + ): + rpc = "Remove" + self.call( + service_name=rpc, arg=name, payload=token, is_include=is_include + ) def policy_include_remove(self, name: str, token="any"): - self.call("policy.include.Remove", name, token) + warn( + "Method 'policy_include_remove()' is deprecated, use " + "'policy_remove()' instead", + DeprecationWarning, + ) + return self.policy_remove(name=name, token=token, is_include=True) - def policy_get_files(self, name: str): - result = self.call("policy.GetFiles", name) + def policy_get_files(self, name: str) -> list: + rpc = "GetFiles" + result = self.call(service_name=rpc, arg=name) return [] if result == "" else result.rstrip("\n").split("\n") @staticmethod - def call(service_name, arg=None, payload=""): - return call("dom0", service_name, arg, input=payload) + def call(service_name, arg=None, payload="", is_include: bool = False): + if is_include: + service_name = "include." + service_name + service_name = "policy." + service_name + return call(dest="dom0", rpcname=service_name, arg=arg, input=payload) diff --git a/qrexec/tools/qubes_policy.py b/qrexec/tools/qubes_policy.py index 06347b1a..7aa38ed2 100755 --- a/qrexec/tools/qubes_policy.py +++ b/qrexec/tools/qubes_policy.py @@ -79,34 +79,18 @@ parser.set_defaults(method="list", name="") -def run_method(method, name, client, is_include): +def run_method(method, name, is_include, client): if method == "list": - if is_include: - result = client.policy_include_list() - else: - result = client.policy_list() + result = client.policy_list(is_include=is_include) print("\n".join(result)) - elif method == "get": - if is_include: - content, _token = client.policy_include_get(name) - else: - content, _token = client.policy_get(name) + content, _token = client.policy_get(name=name, is_include=is_include) print(content.rstrip()) - elif method == "replace": content = sys.stdin.read() - if is_include: - client.policy_include_replace(name, content) - else: - client.policy_replace(name, content) - + client.policy_replace(name=name, content=content, is_include=is_include) elif method == "remove": - if is_include: - client.policy_include_remove(name) - else: - client.policy_remove(name) - + client.policy_remove(name=name, is_include=is_include) else: assert False, method @@ -136,7 +120,12 @@ def main(args=None): parser.error("you need to provide a file name") try: - run_method(args.method, name, client, is_include) + run_method( + method=args.method, + name=name, + is_include=is_include, + client=client, + ) except subprocess.CalledProcessError as e: print("Command failed") output = e.output.decode().rstrip() diff --git a/qrexec/tools/qubes_policy_editor.py b/qrexec/tools/qubes_policy_editor.py index 56c21681..dbdd9b4f 100644 --- a/qrexec/tools/qubes_policy_editor.py +++ b/qrexec/tools/qubes_policy_editor.py @@ -68,18 +68,16 @@ def manage_policy(self) -> None: # the file is not found, ignore, else abort as the request was refused. file_exists = False if self.is_include: - policy_get = client.policy_include_get - policy_replace = client.policy_include_replace wanted_path = str(INCLUDEPATH) + "/" + self.policy + "\n" suffix = "_include_" + self.policy else: - policy_get = client.policy_get - policy_replace = client.policy_replace wanted_path = str(POLICYPATH) + "/" + self.policy + ".policy\n" suffix = "_" + self.policy + ".policy" try: - original_content, token = policy_get(self.policy) + original_content, token = client.policy_get( + name=self.policy, is_include=self.is_include + ) file_exists = True except subprocess.CalledProcessError as exc: not_found = "Not found: " + wanted_path @@ -108,7 +106,12 @@ def manage_policy(self) -> None: current_file.close() try: - policy_replace(self.policy, content, token) + client.policy_replace( + name=self.policy, + content=content, + token=token, + is_include=self.is_include, + ) except subprocess.CalledProcessError as exc: print( f"Failed to replace policy {self.policy!r} with file " From 9626955a3873b362af652d9973429e1663482c30 Mon Sep 17 00:00:00 2001 From: Ben Grande Date: Fri, 6 Mar 2026 14:57:59 +0100 Subject: [PATCH 12/19] Report policy exceptions back to the caller Fixes: https://github.com/QubesOS/qubes-issues/issues/10746 --- qrexec/policy/admin.py | 60 ++++++++++++++++++++++------- qrexec/policy/admin_client.py | 27 ++++++++++++- qrexec/tests/policy_admin.py | 40 +++++++++++++------ qrexec/tools/qubes_policy.py | 8 +++- qrexec/tools/qubes_policy_admin.py | 3 +- qrexec/tools/qubes_policy_editor.py | 23 +++++------ qrexec/utils.py | 1 + 7 files changed, 120 insertions(+), 42 deletions(-) diff --git a/qrexec/policy/admin.py b/qrexec/policy/admin.py index e2318b38..05b3fcde 100644 --- a/qrexec/policy/admin.py +++ b/qrexec/policy/admin.py @@ -38,12 +38,44 @@ class PolicyAdminException(Exception): """ -class PolicyAdminTokenException(Exception): +class PolicyAdminTokenException(PolicyAdminException): """ A token check exception, indicating that a file is in unexpected state. """ +class PolicyAdminFileNotFoundException(PolicyAdminException): + """ + Policy cannot be found. + """ + + +class PolicyAdminProtocolException(PolicyAdminException): + """ + Client sent an invalid request that it should have known to not send in the + first place. + """ + + +class PolicyAdminSyntaxException(PolicyAdminProtocolException): + """ + Client sent an invalid policy. That it should have known to not send in the + first place. Clients should validate the policy before sending them. + """ + + +class PolicyAdminInvalidFileNameException(PolicyAdminProtocolException): + """ + Client sent a policy with invalid name. + """ + + +class PolicyAdminInvalidFilePathException(PolicyAdminProtocolException): + """ + Client sent a policy using a path out of bounds. + """ + + def method(service_name, *, no_arg=False, no_payload=False): def decorator(func): func.api_service_name = service_name @@ -89,7 +121,7 @@ def handle_request( """ if not all(char in RPCNAME_ALLOWED_CHARSET for char in arg): - raise PolicyAdminException( + raise PolicyAdminProtocolException( 'Invalid argument: "{}"\n' "Valid characters are letters, numbers, dot, plus, hyphen and " "underline".format(arg) @@ -97,7 +129,7 @@ def handle_request( func = self._find_method(service_name) if not func: - raise PolicyAdminException( + raise PolicyAdminProtocolException( "unrecognized method: {}".format(service_name) ) @@ -105,13 +137,13 @@ def handle_request( if func.api_no_arg: if arg != "": - raise PolicyAdminException("Unexpected argument") + raise PolicyAdminProtocolException("Unexpected argument") else: args.append(arg) if func.api_no_payload: if payload != b"": - raise PolicyAdminException("Unexpected payload") + raise PolicyAdminProtocolException("Unexpected payload") else: args.append(payload) @@ -178,7 +210,7 @@ def policy_include_get(self, arg): def _common_get(self, path: Path) -> bytes: if not path.is_file(): - raise PolicyAdminException("Not found: {}".format(path)) + raise PolicyAdminFileNotFoundException("Not found: {}".format(path)) data = path.read_bytes() token = compute_token(data) @@ -198,7 +230,7 @@ def policy_include_replace(self, arg, payload): def _common_replace(self, path: Path, payload: bytes) -> bytes: if b"\n" not in payload: - raise PolicyAdminException( + raise PolicyAdminProtocolException( "Payload needs to include first line with token" ) token, data = payload.split(b"\n", 1) @@ -234,7 +266,7 @@ def _common_remove(self, path: Path, payload: str) -> None: self._check_token(payload, path) if not path.is_file(): - raise PolicyAdminException("Not found: {}".format(path)) + raise PolicyAdminFileNotFoundException("Not found: {}".format(path)) self._validate(path, None) @@ -245,10 +277,10 @@ def _common_remove(self, path: Path, payload: str) -> None: @method("policy.GetFiles", no_payload=True) def policy_get_files(self, arg): if not isinstance(arg, str) or not arg: - raise PolicyAdminException("Service cannot be empty.") + raise PolicyAdminProtocolException("Service cannot be empty.") invalid_chars = get_invalid_characters(arg, disallowed="+") if invalid_chars: - raise PolicyAdminException( + raise PolicyAdminProtocolException( "Service {!r} contains invalid characters: {!r}".format( arg, invalid_chars ) @@ -276,7 +308,7 @@ def policy_get_files(self, arg): def _get_path(self, arg: str, dir_path: str, suffix: str) -> Path: if not re.compile(r"^[\w-]+$").match(arg): - raise PolicyAdminException( + raise PolicyAdminInvalidFileNameException( f"Invalid policy file name: {arg}\n" "Names must contain only alphanumeric characters, " "underscore and hyphen." @@ -284,7 +316,7 @@ def _get_path(self, arg: str, dir_path: str, suffix: str) -> Path: path = dir_path / (arg + suffix) path = path.resolve() if path.parent != dir_path: - raise PolicyAdminException( + raise PolicyAdminInvalidFilePathException( "Expecting a path inside {}".format(dir_path) ) @@ -296,7 +328,7 @@ def _validate(self, path: Path, content: Optional[str]): policy_path=self.policy_path, overrides={path: content} ) except PolicySyntaxError as exc: - raise PolicyAdminException( + raise PolicyAdminSyntaxException( "Policy change validation failed: {}".format(exc) ) from exc @@ -312,7 +344,7 @@ def _check_token(self, token: bytes, path: Path): return if not token.startswith(b"sha256:"): - raise PolicyAdminException("Unrecognized token") + raise PolicyAdminProtocolException("Unrecognized token") if not path.exists(): raise PolicyAdminTokenException( diff --git a/qrexec/policy/admin_client.py b/qrexec/policy/admin_client.py index 0086ad28..85815b58 100644 --- a/qrexec/policy/admin_client.py +++ b/qrexec/policy/admin_client.py @@ -35,9 +35,22 @@ from typing import List, Tuple from warnings import warn +from subprocess import CalledProcessError from ..client import call +# The PolicyAdmin*Exception is used by issubclass(..., PolicyAdminException). +# pylint: disable=unused-import +from .admin import ( + PolicyAdminException, + PolicyAdminTokenException, + PolicyAdminFileNotFoundException, + PolicyAdminProtocolException, + PolicyAdminSyntaxException, + PolicyAdminInvalidFileNameException, + PolicyAdminInvalidFilePathException, +) + class PolicyClient: def policy_list(self, is_include: bool = False) -> List[str]: @@ -120,4 +133,16 @@ def call(service_name, arg=None, payload="", is_include: bool = False): if is_include: service_name = "include." + service_name service_name = "policy." + service_name - return call(dest="dom0", rpcname=service_name, arg=arg, input=payload) + try: + return call( + dest="dom0", rpcname=service_name, arg=arg, input=payload + ) + except CalledProcessError as exc: + stderr_exc = exc.stderr.decode() + stderr_exc_type = stderr_exc.split(" ")[0] + if stderr_exc_type in globals(): + exception = globals()[stderr_exc_type] + if issubclass(exception, PolicyAdminException): + stderr_exc_msg = stderr_exc[len(exception.__name__ + " ") :] + raise exception(stderr_exc_msg) from exc + raise diff --git a/qrexec/tests/policy_admin.py b/qrexec/tests/policy_admin.py index fed7d8cc..3be1287f 100644 --- a/qrexec/tests/policy_admin.py +++ b/qrexec/tests/policy_admin.py @@ -24,7 +24,10 @@ from ..policy.admin import ( PolicyAdmin, - PolicyAdminException, + PolicyAdminFileNotFoundException, + PolicyAdminInvalidFileNameException, + PolicyAdminProtocolException, + PolicyAdminSyntaxException, PolicyAdminTokenException, compute_token, ) @@ -73,19 +76,25 @@ def test_api_get(policy_dir, api): assert data.startswith("sha256:") assert data.endswith("\ninclude text") - with pytest.raises(PolicyAdminException, match="Not found"): + with pytest.raises(PolicyAdminFileNotFoundException, match="Not found"): api.handle_request("policy.Get", "nonexistent", b"") - with pytest.raises(PolicyAdminException, match="Invalid policy file"): + with pytest.raises( + PolicyAdminInvalidFileNameException, match="Invalid policy file" + ): api.handle_request("policy.Get", ".hidden_evil_policy", b"") - with pytest.raises(PolicyAdminException, match="Invalid policy file"): + with pytest.raises( + PolicyAdminInvalidFileNameException, match="Invalid policy file" + ): api.handle_request("policy.include.Get", "..", b"") - with pytest.raises(PolicyAdminException, match="Invalid policy file"): + with pytest.raises( + PolicyAdminInvalidFileNameException, match="Invalid policy file" + ): api.handle_request("policy.include.Get", "", b"") - with pytest.raises(PolicyAdminException, match="Invalid argument"): + with pytest.raises(PolicyAdminProtocolException, match="Invalid argument"): api.handle_request("policy.include.Get", "space in argument", b"") @@ -124,11 +133,13 @@ def test_api_replace_check_token(policy_dir, api): def test_api_replace_validate(api): - with pytest.raises(PolicyAdminException, match="wrong number of fields"): + with pytest.raises( + PolicyAdminSyntaxException, match="wrong number of fields" + ): api.handle_request("policy.Replace", "file1", b"any\nxxx") # Trying to include a nonexistent file - with pytest.raises(PolicyAdminException, match="not a file"): + with pytest.raises(PolicyAdminSyntaxException, match="not a file"): api.handle_request( "policy.Replace", "file1", b"any\n!include include/inc" ) @@ -138,7 +149,9 @@ def test_api_replace_validate(api): "policy.include.Replace", "inc", b"any\nrpc.Name * * * deny" ) api.handle_request("policy.Replace", "file1", b"any\n!include include/inc") - with pytest.raises(PolicyAdminException, match="invalid number of params"): + with pytest.raises( + PolicyAdminSyntaxException, match="invalid number of params" + ): api.handle_request( "policy.Replace", "file1", b"any\n!include-service include/inc" ) @@ -172,7 +185,8 @@ def test_api_remove_validate(policy_dir, api): (policy_dir / "include/inc").touch() with pytest.raises( - PolicyAdminException, match="including a file that will be removed" + PolicyAdminSyntaxException, + match="including a file that will be removed", ): api.handle_request("policy.include.Remove", "inc", b"any") @@ -212,10 +226,12 @@ def test_api_get_files(policy_dir, api): "policy.GetFiles", "third.service", b"" ) == f"{other_dir / 'third.service'}\nfile2\n".encode("utf-8") - with pytest.raises(PolicyAdminException, match="Service cannot be empty"): + with pytest.raises( + PolicyAdminProtocolException, match="Service cannot be empty" + ): api.handle_request("policy.GetFiles", "", b"") with pytest.raises( - PolicyAdminException, match="contains invalid characters" + PolicyAdminProtocolException, match="contains invalid characters" ): api.handle_request("policy.GetFiles", "service+param", b"") diff --git a/qrexec/tools/qubes_policy.py b/qrexec/tools/qubes_policy.py index 7aa38ed2..10789f18 100755 --- a/qrexec/tools/qubes_policy.py +++ b/qrexec/tools/qubes_policy.py @@ -27,6 +27,7 @@ from ..policy.admin_client import PolicyClient from .. import RPCNAME_ALLOWED_CHARSET +from ..policy.admin import PolicyAdminException parser = argparse.ArgumentParser( usage="qubes-policy {[-l]|-g|-r|-d} [include/][RPCNAME[+ARGUMENT]]" @@ -127,10 +128,13 @@ def main(args=None): client=client, ) except subprocess.CalledProcessError as e: - print("Command failed") + print("Command failed", file=sys.stderr) output = e.output.decode().rstrip() if output: - print(output) + print(output, file=sys.stderr) + sys.exit(1) + except PolicyAdminException as e: + print(e, file=sys.stderr) sys.exit(1) diff --git a/qrexec/tools/qubes_policy_admin.py b/qrexec/tools/qubes_policy_admin.py index 54a394fb..9e111d51 100644 --- a/qrexec/tools/qubes_policy_admin.py +++ b/qrexec/tools/qubes_policy_admin.py @@ -30,7 +30,6 @@ from ..policy.admin import ( PolicyAdmin, PolicyAdminException, - PolicyAdminTokenException, ) from .. import POLICYPATH @@ -56,7 +55,7 @@ def main(): admin = PolicyAdmin(POLICYPATH) try: response = admin.handle_request(service_name, argument, payload) - except (PolicyAdminException, PolicyAdminTokenException) as exc: + except PolicyAdminException as exc: logging.warning( "%s+%s (%s): error: %s", service_name, argument, source, exc ) diff --git a/qrexec/tools/qubes_policy_editor.py b/qrexec/tools/qubes_policy_editor.py index dbdd9b4f..cee6ea4b 100644 --- a/qrexec/tools/qubes_policy_editor.py +++ b/qrexec/tools/qubes_policy_editor.py @@ -29,7 +29,11 @@ import sys import tempfile from ..policy.admin_client import PolicyClient -from .. import RPCNAME_ALLOWED_CHARSET, POLICYPATH, INCLUDEPATH +from ..policy.admin import ( + PolicyAdminException, + PolicyAdminFileNotFoundException, +) +from .. import RPCNAME_ALLOWED_CHARSET def validate_name(name): @@ -68,10 +72,8 @@ def manage_policy(self) -> None: # the file is not found, ignore, else abort as the request was refused. file_exists = False if self.is_include: - wanted_path = str(INCLUDEPATH) + "/" + self.policy + "\n" suffix = "_include_" + self.policy else: - wanted_path = str(POLICYPATH) + "/" + self.policy + ".policy\n" suffix = "_" + self.policy + ".policy" try: @@ -79,14 +81,13 @@ def manage_policy(self) -> None: name=self.policy, is_include=self.is_include ) file_exists = True + except PolicyAdminFileNotFoundException: + pass except subprocess.CalledProcessError as exc: - not_found = "Not found: " + wanted_path - if exc.output.decode() != not_found: - print( - f"Failed to get policy {self.policy!r}: {exc}", - file=sys.stderr, - ) - sys.exit(1) + print( + f"Failed to get policy {self.policy!r}: {exc}", file=sys.stderr + ) + sys.exit(1) # pylint: disable=consider-using-with tmpfile = tempfile.NamedTemporaryFile(suffix=suffix) @@ -112,7 +113,7 @@ def manage_policy(self) -> None: token=token, is_include=self.is_include, ) - except subprocess.CalledProcessError as exc: + except PolicyAdminException as exc: print( f"Failed to replace policy {self.policy!r} with file " f"{tmpfile.name!r}: {exc}", diff --git a/qrexec/utils.py b/qrexec/utils.py index bb38b56e..af1176f6 100644 --- a/qrexec/utils.py +++ b/qrexec/utils.py @@ -197,4 +197,5 @@ def prepare_subprocess_kwds( kwds["input"] = None if not for_check: kwds["stdout"] = subprocess.PIPE + kwds["stderr"] = subprocess.PIPE return kwds From 26a35f8a149e16f54dca2cf75d7d757233c6b58f Mon Sep 17 00:00:00 2001 From: Ben Grande Date: Fri, 10 Apr 2026 04:52:16 +0200 Subject: [PATCH 13/19] Test policy client Largely duplicate of the server side methods, but creating it anyway as it serves two purposes: - If the client can receive appropriate exceptions - If the client methods works --- qrexec/policy/admin.py | 4 +- qrexec/tests/policy_admin_client.py | 292 ++++++++++++++++++++++++++++ qrexec/tools/qubes_policy_admin.py | 7 +- rpm_spec/qubes-qrexec.spec.in | 1 + 4 files changed, 300 insertions(+), 4 deletions(-) create mode 100644 qrexec/tests/policy_admin_client.py diff --git a/qrexec/policy/admin.py b/qrexec/policy/admin.py index 05b3fcde..329cf495 100644 --- a/qrexec/policy/admin.py +++ b/qrexec/policy/admin.py @@ -108,8 +108,8 @@ class PolicyAdmin: # pylint: disable=no-self-use def __init__(self, policy_path): - self.policy_path = policy_path - self.include_path = policy_path / "include" + self.policy_path = Path(policy_path) + self.include_path = self.policy_path / "include" def handle_request( self, service_name: str, arg: str, payload: bytes diff --git a/qrexec/tests/policy_admin_client.py b/qrexec/tests/policy_admin_client.py new file mode 100644 index 00000000..53ccd23d --- /dev/null +++ b/qrexec/tests/policy_admin_client.py @@ -0,0 +1,292 @@ +# +# The Qubes OS Project, https://www.qubes-os.org/ +# +# Copyright (C) 2020 Paweł Marczewski +# +# This library is free software; you can redistribute it and/or +# modify it under the terms of the GNU Lesser General Public +# License as published by the Free Software Foundation; either +# version 2.1 of the License, or (at your option) any later version. +# +# This library is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +# Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public +# License along with this library; if not, see . +# + +from pathlib import Path +import os +import shutil +import subprocess +import tempfile +import unittest + +import pytest + +from ..policy.admin_client import PolicyClient +from ..policy.admin import ( + PolicyAdminFileNotFoundException, + PolicyAdminInvalidFileNameException, + PolicyAdminProtocolException, + PolicyAdminSyntaxException, + PolicyAdminTokenException, + compute_token, +) + + +class Client(unittest.TestCase): + + def setUp(self): + super().setUp() + self.client = PolicyClient() + self.make_command_patch = unittest.mock.patch( + "qrexec.client.make_command", + side_effect=self.make_command_side_effect, + ) + self.make_command_patch.start() + self.make_command_fail_patch = unittest.mock.patch( + "qrexec.client.make_command", + side_effect=self.make_command_fail_side_effect, + ) + with tempfile.TemporaryDirectory(delete=False) as dir_name: + self.policy_dir = Path(dir_name) + (Path(self.policy_dir) / "include").mkdir() + with tempfile.NamedTemporaryFile(delete=False) as file_name: + self.log_file = file_name.name + + def tearDown(self): + super().tearDown() + self.make_command_patch.stop() + self.make_command_fail_patch.stop() + shutil.rmtree(self.policy_dir) + os.remove(self.log_file) + + def make_command_side_effect(self, dest, rpcname, arg): + # pylint: disable=unused-argument + if arg: + rpcname += "+" + arg + return [ + "env", + "QREXEC_POLICY_DIR=" + str(self.policy_dir), + "QREXEC_POLICY_ADMIN_LOG=" + self.log_file, + "QREXEC_SERVICE_FULL_NAME=" + rpcname, + "QREXEC_REMOTE_DOMAIN=dom0", + "python3", + "-m", + "qrexec.tools.qubes_policy_admin", + ] + + def make_command_fail_side_effect(self, dest, rpcname, arg): + # pylint: disable=unused-argument + if arg: + rpcname += "+" + arg + return [ + "env", + "QREXEC_POLICY_DIR=" + "FAIL" + "QREXEC_POLICY_ADMIN_LOG=" + self.log_file, + "QREXEC_SERVICE_FULL_NAME=" + rpcname, + "QREXEC_REMOTE_DOMAIN=dom0", + "python3", + "-m", + "qrexec.tools.qubes_policy_admin", + ] + + def test_api_list(self): + (self.policy_dir / "file1.policy").touch() + (self.policy_dir / "file2.policy").touch() + (self.policy_dir / "file3").touch() + assert self.client.policy_list() == ["file1", "file2"] + assert self.client.policy_list(is_include=True) == [""] + (self.policy_dir / "include/admin-ro").touch() + assert self.client.policy_list(is_include=True) == ["admin-ro"] + assert ( + self.client.policy_list(is_include=True) + == self.client.policy_include_list() + ) + + def test_api_get(self): + with pytest.raises( + PolicyAdminProtocolException, match="Invalid argument" + ): + self.client.policy_get(name="space in here") + with pytest.raises( + PolicyAdminInvalidFileNameException, match="Invalid policy file" + ): + self.client.policy_get(name=".hidden_evil_policy") + with pytest.raises(PolicyAdminFileNotFoundException, match="Not found"): + self.client.policy_get(name="hey") + + (self.policy_dir / "file1.policy").write_text("policy text") + text, token = self.client.policy_get(name="file1") + assert text == "policy text" + assert token.startswith("sha256:") + + (self.policy_dir / "include/inc").write_text("include text") + text, token = self.client.policy_get(name="inc", is_include=True) + assert text == "include text" + assert token.startswith("sha256:") + old_text, _ = self.client.policy_include_get(name="inc") + assert text == old_text + + def test_api_replace(self): + self.client.policy_replace(name="file1", content="", token="any") + assert (self.policy_dir / "file1.policy").read_text() == "" + + with pytest.raises( + PolicyAdminSyntaxException, match="wrong number of fields" + ): + self.client.policy_replace( + name="file1", content="policy text", token="any" + ) + with pytest.raises( + PolicyAdminSyntaxException, match="contains invalid characters" + ): + self.client.policy_replace( + name="file1", + content="service ** source dest allow", + token="any", + ) + assert (self.policy_dir / "file1.policy").read_text() == "" + + self.client.policy_replace( + name="file1", content="rpc.Name * * * deny", token="any" + ) + assert ( + self.policy_dir / "file1.policy" + ).read_text() == "rpc.Name * * * deny" + + with pytest.raises( + PolicyAdminProtocolException, match="Unrecognized token" + ): + self.client.policy_replace( + name="file1", content="rpc.Name * * * deny", token="what" + ) + + with pytest.raises( + PolicyAdminTokenException, match="File exists but token is 'new'" + ): + self.client.policy_replace( + name="file1", content="rpc.Name * * * deny", token="new" + ) + + with pytest.raises(PolicyAdminTokenException, match="Token mismatch"): + self.client.policy_replace( + name="file1", content="", token="sha256:aaaa" + ) + + with pytest.raises( + PolicyAdminTokenException, match="File doesn't exist" + ): + self.client.policy_replace( + name="file2", content="", token="sha256:aaaa" + ) + + self.client.policy_replace( + name="inc", + content="rpc.Name * * * deny", + token="any", + is_include=True, + ) + assert ( + self.policy_dir / "include/inc" + ).read_text() == "rpc.Name * * * deny" + + self.client.policy_include_replace( + name="inc", content="rpc.Name * * * allow", token="any" + ) + assert ( + self.policy_dir / "include/inc" + ).read_text() == "rpc.Name * * * allow" + + with pytest.raises(PolicyAdminSyntaxException, match="not a file"): + self.client.policy_replace( + name="file1", + content="!include include/nonexistent", + token="any", + ) + + self.client.policy_replace( + name="file1", content="!include include/inc", token="any" + ) + with pytest.raises( + PolicyAdminSyntaxException, match="invalid number of params" + ): + self.client.policy_replace( + name="file1", + content="!include-service include/inc", + token="any", + ) + + def test_api_remove(self): + with pytest.raises(PolicyAdminFileNotFoundException, match="Not found"): + self.client.policy_remove(name="file1") + + (self.policy_dir / "file1.policy").touch() + (self.policy_dir / "include/inc").touch() + + self.client.policy_remove(name="file1", token="any") + assert not (self.policy_dir / "file1").exists() + + self.client.policy_remove(name="inc", token="any", is_include=True) + assert not (self.policy_dir / "include/inc").exists() + + (self.policy_dir / "include/inc").touch() + self.client.policy_include_remove(name="inc", token="any") + assert not (self.policy_dir / "include/inc").exists() + + def test_api_remove_check_token(self): + file_path = self.policy_dir / "file1.policy" + + file_path.touch() + self.client.policy_remove(name="file1", token=compute_token(b"")) + assert not file_path.exists() + + file_path.touch() + with pytest.raises(PolicyAdminTokenException, match="Token mismatch"): + self.client.policy_remove(name="file1", token="sha256:aaaa") + + def test_api_remove_validate(self): + (self.policy_dir / "file1.policy").write_text("!include include/inc") + (self.policy_dir / "include/inc").touch() + + with pytest.raises( + PolicyAdminSyntaxException, + match="including a file that will be removed", + ): + self.client.policy_remove(name="inc", token="any", is_include=True) + + def test_api_get_files(self): + assert not self.client.policy_get_files("nonexistent") + + (self.policy_dir / "file1.policy").write_text( + "test.service * @anyvm dom0 deny\n" + "test.service * @anyvm @anyvm allow" + ) + (self.policy_dir / "file2.policy").write_text( + "other.service * dom0 dom0 allow\n" + "third.service * @anyvm @anyvm deny" + ) + + assert self.client.policy_get_files(name="test.service") == ["file1"] + assert self.client.policy_get_files(name="other.service") == ["file2"] + + with pytest.raises( + PolicyAdminProtocolException, match="Service cannot be empty" + ): + self.client.policy_get_files(name="") + + with pytest.raises( + PolicyAdminProtocolException, match="contains invalid characters" + ): + self.client.policy_get_files(name="service+param") + + def test_api_failure(self): + self.make_command_patch.stop() + self.make_command_fail_patch.start() + with pytest.raises( + subprocess.CalledProcessError, match="returned non-zero exit" + ): + self.client.policy_list() diff --git a/qrexec/tools/qubes_policy_admin.py b/qrexec/tools/qubes_policy_admin.py index 9e111d51..d9c9522c 100644 --- a/qrexec/tools/qubes_policy_admin.py +++ b/qrexec/tools/qubes_policy_admin.py @@ -35,7 +35,10 @@ def main(): - log_file = "/var/log/qubes/policy-admin.log" + policy_path = os.environ.get("QREXEC_POLICY_DIR", POLICYPATH) + log_file = os.environ.get( + "QREXEC_POLICY_ADMIN_LOG", "/var/log/qubes/policy-admin.log" + ) logging.basicConfig( level=logging.INFO, filename=log_file, @@ -52,7 +55,7 @@ def main(): payload = sys.stdin.buffer.read() - admin = PolicyAdmin(POLICYPATH) + admin = PolicyAdmin(policy_path) try: response = admin.handle_request(service_name, argument, payload) except PolicyAdminException as exc: diff --git a/rpm_spec/qubes-qrexec.spec.in b/rpm_spec/qubes-qrexec.spec.in index 9352b7f0..d04799ac 100644 --- a/rpm_spec/qubes-qrexec.spec.in +++ b/rpm_spec/qubes-qrexec.spec.in @@ -204,6 +204,7 @@ rm -f %{name}-%{version} %{python3_sitelib}/qrexec/tests/policy_graph.py %{python3_sitelib}/qrexec/tests/server.py %{python3_sitelib}/qrexec/tests/policy_admin.py +%{python3_sitelib}/qrexec/tests/policy_admin_client.py %dir %{python3_sitelib}/qrexec/tests/socket %dir %{python3_sitelib}/qrexec/tests/socket/__pycache__ From d276ecdd1cc33c05d81b50a98a1b3d35b8127e4d Mon Sep 17 00:00:00 2001 From: Ben Grande Date: Fri, 6 Mar 2026 15:00:56 +0100 Subject: [PATCH 14/19] Don't delete local policy on failures Fixes: https://github.com/qubesos/qubes-issues/issues/10745 --- qrexec/tools/qubes_policy_editor.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/qrexec/tools/qubes_policy_editor.py b/qrexec/tools/qubes_policy_editor.py index cee6ea4b..804181a9 100644 --- a/qrexec/tools/qubes_policy_editor.py +++ b/qrexec/tools/qubes_policy_editor.py @@ -25,9 +25,11 @@ from __future__ import print_function import argparse +import os import subprocess import sys import tempfile + from ..policy.admin_client import PolicyClient from ..policy.admin import ( PolicyAdminException, @@ -90,7 +92,7 @@ def manage_policy(self) -> None: sys.exit(1) # pylint: disable=consider-using-with - tmpfile = tempfile.NamedTemporaryFile(suffix=suffix) + tmpfile = tempfile.NamedTemporaryFile(suffix=suffix, delete=False) if file_exists: with open(tmpfile.name, "w", encoding="utf-8") as current_file: @@ -122,6 +124,7 @@ def manage_policy(self) -> None: sys.exit(1) tmpfile.close() + os.remove(self.tmpfile_name) def get_reply(self) -> None: """ From d9e13c873d7228b77856e71e2e5eac2b5509021e Mon Sep 17 00:00:00 2001 From: Ben Grande Date: Fri, 10 Apr 2026 15:01:10 +0200 Subject: [PATCH 15/19] Fix pylintrc max line length to 80 chars The repository is already reformatted with black using that same line length. --- .pylintrc | 2 +- qrexec/policy/parser.py | 38 ++++++++++++++++++----------- qrexec/tools/qrexec_policy_graph.py | 10 ++++++-- 3 files changed, 33 insertions(+), 17 deletions(-) diff --git a/.pylintrc b/.pylintrc index ce5735a9..65098279 100644 --- a/.pylintrc +++ b/.pylintrc @@ -258,7 +258,7 @@ spelling-store-unknown-words=no [FORMAT] # Maximum number of characters on a single line. -max-line-length=100 +max-line-length=80 # Regexp for a line that is allowed to be longer than the limit. ignore-long-lines=^\s*(# )??$ diff --git a/qrexec/policy/parser.py b/qrexec/policy/parser.py index 522b9a57..780f9bd6 100644 --- a/qrexec/policy/parser.py +++ b/qrexec/policy/parser.py @@ -411,7 +411,9 @@ def __new__( ) -> "Redirect": if value is None: return None # type: ignore - return super().__new__(cls, value, filepath=filepath, lineno=lineno) # type: ignore + return super().__new__( + cls, value, filepath=filepath, lineno=lineno # type: ignore + ) # this method (with overloads in subclasses) was verify_target_value @@ -819,7 +821,8 @@ async def execute(self) -> str: if target_info.get("type") == "RemoteVM" and self.user is not None: logging.warning( - "Ignoring user directive in policy. This is not supported in the case of RemoveVM." + "Ignoring user directive in policy. This is not supported in " + "the case of RemoveVM." ) user_line = "user=DEFAULT" else: @@ -838,13 +841,15 @@ async def execute(self) -> str: relayvm_info = request.system_info["domains"][relayvm_name] transport_rpc = target_info["transport_rpc"] + service = f"{transport_rpc}+{request.target}+{request.service}" + service += f"{request.argument}" lines.extend( [ f"target={relayvm_name}", f"target_uuid=uuid:{relayvm_info['uuid']}", f"autostart={self.autostart}", f"requested_target={request.target}", - f"service={transport_rpc}+{request.target}+{request.service}{request.argument}", + f"service={service}", ] ) if request.requested_source: @@ -1024,7 +1029,8 @@ def __init__( ) if requested_source_info.get("type") != "RemoteVM": raise RequestError( - f"{self.requested_source}: requested source is only authorized for RemoteVM" + f"{self.requested_source}: requested source is only " + "authorized for RemoteVM" ) if requested_source_info.get("relayvm", None) != self.source: raise RequestError( @@ -1798,7 +1804,8 @@ def handle_rule(self, rule, *, filepath, lineno): @abc.abstractmethod def handle_compat40(self, *, filepath, lineno): - """Handle ``!compat-4.0`` line when encountered in :meth:`policy_load_file`. + """Handle ``!compat-4.0`` line when encountered in + :meth:`policy_load_file`. This method is to be provided by subclass. """ @@ -1877,11 +1884,11 @@ def collect_targets_for_ask(self, request): ): try: # The policy agent cannot handle UUIDs (and rightly so, - # those are meaningless to humans). Convert them to names. - # VM names can be reused, but in practice, this is unlikely - # to happen except by user request or DispVM name reuse. - # The latter will not happen for a week and so is unlikely - # to confuse humans. + # those are meaningless to humans). Convert them to + # names. VM names can be reused, but in practice, this + # is unlikely to happen except by user request or DispVM + # name reuse. The latter will not happen for a week and + # so is unlikely to confuse humans. expansion.add(uuid_to_name(info, potential_target)) except KeyError: continue @@ -2059,7 +2066,8 @@ def __init__( iterable_policy_paths = policy_path else: raise TypeError( - "unexpected type of policy path in AbstractFileSystemLoader.__init__!" + "unexpected type of policy path in " + "AbstractFileSystemLoader.__init__!" ) try: self.load_policy_dirs(iterable_policy_paths) @@ -2258,8 +2266,8 @@ def save_included_path(self, included_path, *, filepath, lineno): raise PolicySyntaxError( filepath, lineno, - "invalid path {}, only paths inside the directories {policypath} and " - "{policypath}/include are considered".format( + "invalid path {}, only paths inside the directories " + "{policypath} and {policypath}/include are considered".format( included_path, policypath=POLICYPATH ), ) @@ -2276,7 +2284,9 @@ def handle_include( filepath, ) self.save_included_path(included_path, filepath=filepath, lineno=lineno) - super().handle_include(included_path, filepath=filepath, lineno=lineno) # type: ignore + super().handle_include( # type: ignore + included_path, filepath=filepath, lineno=lineno + ) def handle_include_service( self, diff --git a/qrexec/tools/qrexec_policy_graph.py b/qrexec/tools/qrexec_policy_graph.py index d100fa75..e3e60ce3 100644 --- a/qrexec/tools/qrexec_policy_graph.py +++ b/qrexec/tools/qrexec_policy_graph.py @@ -100,9 +100,15 @@ def handle_single_action(args, action): ) if isinstance(action, parser.AskResolution): if args.include_ask: - return f' "{action.request.source}" -> "{target}" [label="{service}" color=orange];\n' + return ( + f' "{action.request.source}" -> "{target}" ' + f'[label="{service}" color=orange];\n' + ) elif isinstance(action, parser.AllowResolution): - return f' "{action.request.source}" -> "{target}" [label="{service}" color=red];\n' + return ( + f' "{action.request.source}" -> "{target}" ' + f'[label="{service}" color=red];\n' + ) return "" From 3a42e74ca12259295d6a905e3a7af34445854fb5 Mon Sep 17 00:00:00 2001 From: Ben Grande Date: Wed, 29 Apr 2026 11:47:24 +0200 Subject: [PATCH 16/19] Lint from editor with module instead of subprocess --- qrexec/tools/qubes_policy_editor.py | 35 ++++++++++------------------- 1 file changed, 12 insertions(+), 23 deletions(-) diff --git a/qrexec/tools/qubes_policy_editor.py b/qrexec/tools/qubes_policy_editor.py index 804181a9..ba83cace 100644 --- a/qrexec/tools/qubes_policy_editor.py +++ b/qrexec/tools/qubes_policy_editor.py @@ -36,6 +36,7 @@ PolicyAdminFileNotFoundException, ) from .. import RPCNAME_ALLOWED_CHARSET +from .qubes_policy_lint import main as lint def validate_name(name): @@ -152,31 +153,19 @@ def lint_policy(self) -> None: print(f"Failed to open editor: {exc}", file=sys.stderr) sys.exit(1) - lint_cmd = "qubes-policy-lint " + lint_args = ["--", self.tmpfile_name] if self.is_include: - lint_cmd += "--include-service " - lint_cmd += "-- " + self.tmpfile_name - + lint_args.insert(0, "--include-service") try: - subprocess.run(lint_cmd, shell=True, check=True) - except subprocess.CalledProcessError as exc: - return_code = exc.returncode - if return_code == 0: - return - if return_code == 127: - print( - "The linting program 'qubes-policy-lint' is not installed", - file=sys.stderr, - ) - sys.exit(1) - else: - print( - "Linting failed, do you want to:\n" - " (e)dit again\n" - " (q)uit without saving changes?", - file=sys.stderr, - ) - self.get_reply() + lint(lint_args) + except SystemExit: + print( + "Linting failed, do you want to:\n" + " (e)dit again\n" + " (q)uit without saving changes?", + file=sys.stderr, + ) + self.get_reply() def main(): From 4ac20d322a31aa35d2c277637496ac33f5743c69 Mon Sep 17 00:00:00 2001 From: Ben Grande Date: Fri, 24 Apr 2026 11:58:01 +0200 Subject: [PATCH 17/19] Add API tests for coverage --- qrexec/tests/policy_admin.py | 29 ++++ qrexec/tests/policy_admin_client.py | 2 - qrexec/tests/server.py | 26 +++- qrexec/tests/tools/.gitignore | 1 + qrexec/tests/tools/__init__.py | 0 qrexec/tests/tools/qubes_policy.py | 172 +++++++++++++++++++++++ qrexec/tests/tools/qubes_policy_admin.py | 112 +++++++++++++++ qrexec/tools/qubes_policy.py | 4 - qrexec/tools/qubes_policy_editor.py | 2 +- rpm_spec/qubes-qrexec.spec.in | 7 + 10 files changed, 347 insertions(+), 8 deletions(-) create mode 100644 qrexec/tests/tools/.gitignore create mode 100644 qrexec/tests/tools/__init__.py create mode 100644 qrexec/tests/tools/qubes_policy.py create mode 100644 qrexec/tests/tools/qubes_policy_admin.py diff --git a/qrexec/tests/policy_admin.py b/qrexec/tests/policy_admin.py index 3be1287f..30ba077f 100644 --- a/qrexec/tests/policy_admin.py +++ b/qrexec/tests/policy_admin.py @@ -49,6 +49,21 @@ def api(policy_dir): return PolicyAdmin(policy_dir) +def test_api_basic(api): + with pytest.raises( + PolicyAdminProtocolException, match="unrecognized method" + ): + api.handle_request("policy.NonExistent", "", b"") + with pytest.raises( + PolicyAdminProtocolException, match="Unexpected argument" + ): + api.handle_request("policy.List", "arg", b"") + with pytest.raises( + PolicyAdminProtocolException, match="Unexpected payload" + ): + api.handle_request("policy.List", "", b"payload") + + def test_api_list(policy_dir, api): (policy_dir / "file1.policy").touch() (policy_dir / "file2.policy").touch() @@ -122,6 +137,17 @@ def test_api_replace_check_token(policy_dir, api): api.handle_request("policy.Replace", "file1", compute_token(sample) + b"\n") assert (policy_dir / "file1.policy").read_bytes() == b"" + with pytest.raises( + PolicyAdminProtocolException, + match="Payload needs to include first line with token", + ): + api.handle_request("policy.Replace", "file1", b"new") + + with pytest.raises( + PolicyAdminProtocolException, match="Unrecognized token" + ): + api.handle_request("policy.Replace", "file1", b"oops\n") + with pytest.raises(PolicyAdminTokenException, match="File exists"): api.handle_request("policy.Replace", "file1", b"new\n") @@ -190,6 +216,9 @@ def test_api_remove_validate(policy_dir, api): ): api.handle_request("policy.include.Remove", "inc", b"any") + with pytest.raises(PolicyAdminFileNotFoundException, match="Not found"): + api.handle_request("policy.Remove", "file100000", b"any") + def test_api_get_files(policy_dir, api): (policy_dir / "file1.policy").write_text( diff --git a/qrexec/tests/policy_admin_client.py b/qrexec/tests/policy_admin_client.py index 53ccd23d..0763b216 100644 --- a/qrexec/tests/policy_admin_client.py +++ b/qrexec/tests/policy_admin_client.py @@ -81,8 +81,6 @@ def make_command_side_effect(self, dest, rpcname, arg): def make_command_fail_side_effect(self, dest, rpcname, arg): # pylint: disable=unused-argument - if arg: - rpcname += "+" + arg return [ "env", "QREXEC_POLICY_DIR=" + "FAIL" diff --git a/qrexec/tests/server.py b/qrexec/tests/server.py index e4c7abb2..14df67ce 100644 --- a/qrexec/tests/server.py +++ b/qrexec/tests/server.py @@ -22,12 +22,17 @@ import os import asyncio import socket +import unittest from unittest import mock import pytest import pytest_asyncio -from ..server import SocketService, call_socket_service_local +from ..server import ( + SocketService, + call_socket_service_local, + call_socket_service_remote, +) # Disable warnings that conflict with Pytest's use of fixtures. # pylint: disable=redefined-outer-name, unused-argument @@ -99,3 +104,22 @@ async def test_call_socket_service_local(temp_dir, server): "service": "Service", "source_domain": "source", } + + +@pytest.mark.asyncio +async def test_call_socket_service_remote(temp_dir, server): + def make_command_side_effect(dest, rpcname, arg): + # pylint: disable=unused-argument + return ["printf", "%s", rpcname] + + make_command_patch = unittest.mock.patch( + "qrexec.client.make_command", + side_effect=make_command_side_effect, + ) + make_command_patch.start() + + for i in range(2): + response = await call_socket_service_remote( + "remote", "Service", {"request": i} + ) + assert response == "Service" diff --git a/qrexec/tests/tools/.gitignore b/qrexec/tests/tools/.gitignore new file mode 100644 index 00000000..0d20b648 --- /dev/null +++ b/qrexec/tests/tools/.gitignore @@ -0,0 +1 @@ +*.pyc diff --git a/qrexec/tests/tools/__init__.py b/qrexec/tests/tools/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/qrexec/tests/tools/qubes_policy.py b/qrexec/tests/tools/qubes_policy.py new file mode 100644 index 00000000..19df4ab7 --- /dev/null +++ b/qrexec/tests/tools/qubes_policy.py @@ -0,0 +1,172 @@ +# +# The Qubes OS Project, https://www.qubes-os.org/ +# +# Copyright (C) 2026 Ben Grande +# +# This library is free software; you can redistribute it and/or +# modify it under the terms of the GNU Lesser General Public +# License as published by the Free Software Foundation; either +# version 2.1 of the License, or (at your option) any later version. +# +# This library is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +# Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public +# License along with this library; if not, see . +# + + +import argparse +import io +import os +import re +import shutil +import sys +import unittest + +import pytest + +from qrexec.tools.qubes_policy import main, run_method + + +class TestPolicyTool: + + def _call_api( + self, + args: list = [], + expected_stdout: str = "", + expected_stderr: str = "", + stdin: str = "", + exc: Exception = None, # type: ignore[assignment] + ): + # pylint: disable=too-many-positional-arguments + + with unittest.mock.patch.object(sys, "stdin") as mock_stdin: # type: ignore[attr-defined] + mock_stdin.read.return_value = stdin + if exc: + with pytest.raises(exc): # type: ignore[call-overload] + main(args) + else: + main(args) + + captured = self.capsys.readouterr() + assert re.search(expected_stdout, captured.out, flags=re.MULTILINE) + assert re.search(expected_stderr, captured.err, flags=re.MULTILINE) + + @pytest.fixture + def setup_dirs(self, capsys, tmp_path): + # pylint: disable=attribute-defined-outside-init + policy_dir = tmp_path / "policy.d" + policy_include_dir = policy_dir / "include" + policy_include_dir.mkdir(parents=True) + log_file = tmp_path / "policy-admin.log" + log_file.touch() + # Pytest is finicky about having an __init__ method. + self.policy_dir = policy_dir + self.policy_include_dir = policy_include_dir + self.log_file = log_file + self.capsys = capsys + + self.make_command_patch = unittest.mock.patch( + "qrexec.client.make_command", + side_effect=self.make_command_side_effect, + ) + self.make_command_patch.start() + + def make_command_side_effect(self, dest, rpcname, arg): + # pylint: disable=unused-argument + if arg: + rpcname += "+" + arg + return [ + "env", + "QREXEC_POLICY_DIR=" + str(self.policy_dir), + "QREXEC_POLICY_ADMIN_LOG=" + str(self.log_file), + "QREXEC_SERVICE_FULL_NAME=" + rpcname, + "QREXEC_REMOTE_DOMAIN=dom0", + "python3", + "-m", + "qrexec.tools.qubes_policy_admin", + ] + + def test_tool_cli_specific_exception(self, setup_dirs): + # pylint: disable=unused-argument + self._call_api( + args=["--get", "unexistent"], + expected_stderr="^Not found: .*", + exc=SystemExit, + ) + + self._call_api( + args=["--list", "file"], + expected_stderr=".*--list doesn't work with a file name.*", + exc=SystemExit, + ) + + self._call_api( + args=["--get", "fi*le"], + expected_stderr=".*invalid character.*", + exc=SystemExit, + ) + + def test_tool_cli_unhandled_exception(self, setup_dirs): + # pylint: disable=unused-argument + shutil.rmtree(self.policy_dir) + self._call_api( + stdin=b"", + expected_stdout="", + expected_stderr="^Command failed", + exc=SystemExit, + ) + + with pytest.raises(AssertionError): + run_method( + method="inexistent", name="file1", is_include=False, client=None + ) + + def test_tool_cli_result(self, setup_dirs): + # pylint: disable=unused-argument + self._call_api( + args=["--list"], + expected_stdout="\n", + ) + self._call_api( + args=[], + expected_stdout="\n", + ) + + (self.policy_dir / "file1.policy").touch() + (self.policy_dir / "file2.policy").touch() + (self.policy_dir / "file3").touch() + + self._call_api( + args=["--list"], + expected_stdout="file1\nfile2\n", + ) + + self._call_api( + args=["--get", "file1"], + ) + + rule = "srv +arg src dst deny" + escaped_rule = r"srv \+arg src dst deny\n" + self._call_api( + args=["--replace", "file1"], + stdin=rule, + ) + + self._call_api( + args=["--get", "file1"], + expected_stdout=escaped_rule, + ) + + self._call_api( + args=["--remove", "file1"], + ) + + (self.policy_include_dir / "inc").touch() + self._call_api( + args=["--get", "include/inc"], + expected_stdout="\n", + ) diff --git a/qrexec/tests/tools/qubes_policy_admin.py b/qrexec/tests/tools/qubes_policy_admin.py new file mode 100644 index 00000000..b704b472 --- /dev/null +++ b/qrexec/tests/tools/qubes_policy_admin.py @@ -0,0 +1,112 @@ +# +# The Qubes OS Project, https://www.qubes-os.org/ +# +# Copyright (C) 2026 Ben Grande +# +# This library is free software; you can redistribute it and/or +# modify it under the terms of the GNU Lesser General Public +# License as published by the Free Software Foundation; either +# version 2.1 of the License, or (at your option) any later version. +# +# This library is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +# Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public +# License along with this library; if not, see . +# + +import io +import os +import re +import shutil +import unittest + +import pytest + +from qrexec.tools.qubes_policy_admin import main + + +class TestPolicyAdminTool: + + def _call_api( + self, + stdin: bytes = b"", + expected_stdout: str = "", + expected_stderr: str = "", + exc: Exception = None, # type: ignore[assignment] + ): + service = "policy.List" + stdin_patch = io.TextIOBase() + payload = stdin + stdin_patch.buffer = io.BytesIO(payload) # type: ignore[attr-defined] + os.environ.update( + { + "QREXEC_SERVICE_FULL_NAME": service, + "QREXEC_REMOTE_DOMAIN": "dummy", + "QREXEC_POLICY_DIR": str(self.policy_dir), + "QREXEC_POLICY_ADMIN_LOG": str(self.log_file), + } + ) + with unittest.mock.patch("sys.stdin", stdin_patch): + if exc: + with pytest.raises(exc): # type: ignore[call-overload] + main() + else: + main() + + captured = self.capsys.readouterr() + assert re.match(expected_stdout, captured.out) + assert re.match(expected_stderr, captured.err) + + @pytest.fixture + def setup_dirs(self, capsys, tmp_path): + # pylint: disable=attribute-defined-outside-init + policy_dir = tmp_path / "policy.d" + policy_include_dir = policy_dir / "include" + policy_include_dir.mkdir(parents=True) + log_file = tmp_path / "policy-admin.log" + log_file.touch() + # Pytest is finicky about having an __init__ method. + self.policy_dir = policy_dir + self.policy_include_dir = policy_include_dir + self.log_file = log_file + self.capsys = capsys + + def test_tool_admin_specific_exception(self, setup_dirs): + # pylint: disable=unused-argument + self._call_api( + stdin=b"test\n", + expected_stdout="", + expected_stderr="PolicyAdminProtocolException Unexpected payload", + exc=SystemExit, + ) + + def test_tool_admin_unhandled_exception(self, setup_dirs): + # pylint: disable=unused-argument + shutil.rmtree(self.policy_dir) + self._call_api( + stdin=b"", + expected_stdout="", + expected_stderr="^Internal error.*", + exc=SystemExit, + ) + + def test_tool_admin_result(self, setup_dirs): + # pylint: disable=unused-argument + self._call_api( + stdin=b"", + expected_stdout="", + expected_stderr="", + ) + + (self.policy_dir / "file1.policy").touch() + (self.policy_dir / "file2.policy").touch() + (self.policy_dir / "file3").touch() + + self._call_api( + stdin=b"", + expected_stdout="^file1\nfile2\n$", + expected_stderr="", + ) diff --git a/qrexec/tools/qubes_policy.py b/qrexec/tools/qubes_policy.py index 10789f18..d0ec7e87 100755 --- a/qrexec/tools/qubes_policy.py +++ b/qrexec/tools/qubes_policy.py @@ -136,7 +136,3 @@ def main(args=None): except PolicyAdminException as e: print(e, file=sys.stderr) sys.exit(1) - - -if __name__ == "__main__": - sys.exit(main()) diff --git a/qrexec/tools/qubes_policy_editor.py b/qrexec/tools/qubes_policy_editor.py index ba83cace..4baf61c4 100644 --- a/qrexec/tools/qubes_policy_editor.py +++ b/qrexec/tools/qubes_policy_editor.py @@ -168,7 +168,7 @@ def lint_policy(self) -> None: self.get_reply() -def main(): +def main(args=None): """ Main. """ diff --git a/rpm_spec/qubes-qrexec.spec.in b/rpm_spec/qubes-qrexec.spec.in index d04799ac..a0012527 100644 --- a/rpm_spec/qubes-qrexec.spec.in +++ b/rpm_spec/qubes-qrexec.spec.in @@ -206,6 +206,13 @@ rm -f %{name}-%{version} %{python3_sitelib}/qrexec/tests/policy_admin.py %{python3_sitelib}/qrexec/tests/policy_admin_client.py +%dir %{python3_sitelib}/qrexec/tests/tools +%dir %{python3_sitelib}/qrexec/tests/tools/__pycache__ +%{python3_sitelib}/qrexec/tests/tools/__pycache__/* +%{python3_sitelib}/qrexec/tests/tools/__init__.py +%{python3_sitelib}/qrexec/tests/tools/qubes_policy.py +%{python3_sitelib}/qrexec/tests/tools/qubes_policy_admin.py + %dir %{python3_sitelib}/qrexec/tests/socket %dir %{python3_sitelib}/qrexec/tests/socket/__pycache__ %{python3_sitelib}/qrexec/tests/socket/__pycache__/* From db7d08463c95a3853f6a578a59d56612871df28f Mon Sep 17 00:00:00 2001 From: Ben Grande Date: Wed, 29 Apr 2026 13:17:12 +0200 Subject: [PATCH 18/19] Use absolute import on tests Easier to move test files around. --- qrexec/tests/cli.py | 6 +++--- qrexec/tests/policy_admin.py | 2 +- qrexec/tests/policy_admin_client.py | 4 ++-- qrexec/tests/policy_cache.py | 2 +- qrexec/tests/policy_graph.py | 2 +- qrexec/tests/policy_parser.py | 6 ++---- qrexec/tests/qrexec_legacy_convert.py | 2 +- qrexec/tests/qrexec_policy_daemon.py | 2 +- qrexec/tests/server.py | 2 +- qrexec/tests/socket/agent.py | 4 ++-- qrexec/tests/socket/daemon.py | 6 +++--- 11 files changed, 18 insertions(+), 20 deletions(-) diff --git a/qrexec/tests/cli.py b/qrexec/tests/cli.py index eba267d6..34eebaa0 100644 --- a/qrexec/tests/cli.py +++ b/qrexec/tests/cli.py @@ -24,9 +24,9 @@ import pytest -from ..exc import AccessDenied -from .. import QREXEC_CLIENT -from ..tools import qrexec_policy_exec +from qrexec.exc import AccessDenied +from qrexec import QREXEC_CLIENT +from qrexec.tools import qrexec_policy_exec # Disable warnings that conflict with Pytest's use of fixtures. # pylint: disable=redefined-outer-name diff --git a/qrexec/tests/policy_admin.py b/qrexec/tests/policy_admin.py index 30ba077f..ec45dff3 100644 --- a/qrexec/tests/policy_admin.py +++ b/qrexec/tests/policy_admin.py @@ -22,7 +22,7 @@ import pytest -from ..policy.admin import ( +from qrexec.policy.admin import ( PolicyAdmin, PolicyAdminFileNotFoundException, PolicyAdminInvalidFileNameException, diff --git a/qrexec/tests/policy_admin_client.py b/qrexec/tests/policy_admin_client.py index 0763b216..dd2dd7a9 100644 --- a/qrexec/tests/policy_admin_client.py +++ b/qrexec/tests/policy_admin_client.py @@ -26,8 +26,8 @@ import pytest -from ..policy.admin_client import PolicyClient -from ..policy.admin import ( +from qrexec.policy.admin_client import PolicyClient +from qrexec.policy.admin import ( PolicyAdminFileNotFoundException, PolicyAdminInvalidFileNameException, PolicyAdminProtocolException, diff --git a/qrexec/tests/policy_cache.py b/qrexec/tests/policy_cache.py index e7baae01..37a0a125 100644 --- a/qrexec/tests/policy_cache.py +++ b/qrexec/tests/policy_cache.py @@ -25,7 +25,7 @@ import unittest.mock import pathlib -from ..policy import utils +from qrexec.policy import utils class TestPolicyCache: diff --git a/qrexec/tests/policy_graph.py b/qrexec/tests/policy_graph.py index b23c5985..dff3722e 100644 --- a/qrexec/tests/policy_graph.py +++ b/qrexec/tests/policy_graph.py @@ -23,7 +23,7 @@ from unittest import mock from types import MappingProxyType as Proxy -from ..tools.qrexec_policy_graph import main +from qrexec.tools.qrexec_policy_graph import main @pytest.fixture(autouse=True) diff --git a/qrexec/tests/policy_parser.py b/qrexec/tests/policy_parser.py index a63a78e3..bfa7fc3f 100644 --- a/qrexec/tests/policy_parser.py +++ b/qrexec/tests/policy_parser.py @@ -30,12 +30,10 @@ import pytest from types import MappingProxyType +from qrexec import QREXEC_CLIENT, QUBESD_INTERNAL_SOCK, exc, utils +from qrexec.policy import parser, parser_compat from qrexec.utils import FullSystemInfo -from .. import QREXEC_CLIENT, QUBESD_INTERNAL_SOCK -from .. import exc, utils -from ..policy import parser, parser_compat - _SYSTEM_INFO = { "domains": { diff --git a/qrexec/tests/qrexec_legacy_convert.py b/qrexec/tests/qrexec_legacy_convert.py index a474654b..d9dcf95d 100644 --- a/qrexec/tests/qrexec_legacy_convert.py +++ b/qrexec/tests/qrexec_legacy_convert.py @@ -23,7 +23,7 @@ import pytest from pathlib import Path from types import MappingProxyType as Proxy -from ..tools import qrexec_legacy_convert +from qrexec.tools import qrexec_legacy_convert @pytest.fixture(autouse=True) diff --git a/qrexec/tests/qrexec_policy_daemon.py b/qrexec/tests/qrexec_policy_daemon.py index 34f5e28a..f5983145 100644 --- a/qrexec/tests/qrexec_policy_daemon.py +++ b/qrexec/tests/qrexec_policy_daemon.py @@ -29,7 +29,7 @@ import unittest import unittest.mock -from ..tools import qrexec_policy_daemon +from qrexec.tools import qrexec_policy_daemon server_types = [b"Simple", b"GUI"] import logging diff --git a/qrexec/tests/server.py b/qrexec/tests/server.py index 14df67ce..ae4250a7 100644 --- a/qrexec/tests/server.py +++ b/qrexec/tests/server.py @@ -28,7 +28,7 @@ import pytest import pytest_asyncio -from ..server import ( +from qrexec.server import ( SocketService, call_socket_service_local, call_socket_service_remote, diff --git a/qrexec/tests/socket/agent.py b/qrexec/tests/socket/agent.py index 62363f67..87a6af1e 100644 --- a/qrexec/tests/socket/agent.py +++ b/qrexec/tests/socket/agent.py @@ -31,8 +31,8 @@ import psutil -from . import qrexec -from . import util +from qrexec.tests.socket import qrexec +from qrexec.tests.socket import util ROOT_PATH = os.path.abspath( os.path.join(os.path.dirname(__file__), "..", "..", "..") diff --git a/qrexec/tests/socket/daemon.py b/qrexec/tests/socket/daemon.py index 9d667ed8..b03d84d3 100644 --- a/qrexec/tests/socket/daemon.py +++ b/qrexec/tests/socket/daemon.py @@ -32,10 +32,10 @@ import psutil -from . import qrexec -from . import util +from qrexec.tests.socket import qrexec +from qrexec.tests.socket import util -from .qrexec import QREXEC_PROTOCOL_V2, QREXEC_PROTOCOL_V3 +from qrexec.tests.socket.qrexec import QREXEC_PROTOCOL_V2, QREXEC_PROTOCOL_V3 ROOT_PATH = os.path.abspath( os.path.join(os.path.dirname(__file__), "..", "..", "..") From 842a543cb71ee872423b0ffd4e47e5d7704b0ed0 Mon Sep 17 00:00:00 2001 From: Ben Grande Date: Wed, 29 Apr 2026 13:22:11 +0200 Subject: [PATCH 19/19] Organize test directory structure Easier to identify which test file relates to which file when they are under the same directory structure. --- qrexec/tests/policy/.gitignore | 1 + qrexec/tests/policy/__init__.py | 0 .../{policy_admin.py => policy/admin.py} | 0 .../admin_client.py} | 0 .../{policy_parser.py => policy/parser.py} | 0 .../{policy_cache.py => policy/utils.py} | 0 .../{ => tools}/qrexec_legacy_convert.py | 0 .../tests/{ => tools}/qrexec_policy_daemon.py | 0 .../{cli.py => tools/qrexec_policy_exec.py} | 0 .../qrexec_policy_graph.py} | 0 rpm_spec/qubes-qrexec.spec.in | 23 +++++++++++-------- 11 files changed, 15 insertions(+), 9 deletions(-) create mode 100644 qrexec/tests/policy/.gitignore create mode 100644 qrexec/tests/policy/__init__.py rename qrexec/tests/{policy_admin.py => policy/admin.py} (100%) rename qrexec/tests/{policy_admin_client.py => policy/admin_client.py} (100%) rename qrexec/tests/{policy_parser.py => policy/parser.py} (100%) rename qrexec/tests/{policy_cache.py => policy/utils.py} (100%) rename qrexec/tests/{ => tools}/qrexec_legacy_convert.py (100%) rename qrexec/tests/{ => tools}/qrexec_policy_daemon.py (100%) rename qrexec/tests/{cli.py => tools/qrexec_policy_exec.py} (100%) rename qrexec/tests/{policy_graph.py => tools/qrexec_policy_graph.py} (100%) diff --git a/qrexec/tests/policy/.gitignore b/qrexec/tests/policy/.gitignore new file mode 100644 index 00000000..0d20b648 --- /dev/null +++ b/qrexec/tests/policy/.gitignore @@ -0,0 +1 @@ +*.pyc diff --git a/qrexec/tests/policy/__init__.py b/qrexec/tests/policy/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/qrexec/tests/policy_admin.py b/qrexec/tests/policy/admin.py similarity index 100% rename from qrexec/tests/policy_admin.py rename to qrexec/tests/policy/admin.py diff --git a/qrexec/tests/policy_admin_client.py b/qrexec/tests/policy/admin_client.py similarity index 100% rename from qrexec/tests/policy_admin_client.py rename to qrexec/tests/policy/admin_client.py diff --git a/qrexec/tests/policy_parser.py b/qrexec/tests/policy/parser.py similarity index 100% rename from qrexec/tests/policy_parser.py rename to qrexec/tests/policy/parser.py diff --git a/qrexec/tests/policy_cache.py b/qrexec/tests/policy/utils.py similarity index 100% rename from qrexec/tests/policy_cache.py rename to qrexec/tests/policy/utils.py diff --git a/qrexec/tests/qrexec_legacy_convert.py b/qrexec/tests/tools/qrexec_legacy_convert.py similarity index 100% rename from qrexec/tests/qrexec_legacy_convert.py rename to qrexec/tests/tools/qrexec_legacy_convert.py diff --git a/qrexec/tests/qrexec_policy_daemon.py b/qrexec/tests/tools/qrexec_policy_daemon.py similarity index 100% rename from qrexec/tests/qrexec_policy_daemon.py rename to qrexec/tests/tools/qrexec_policy_daemon.py diff --git a/qrexec/tests/cli.py b/qrexec/tests/tools/qrexec_policy_exec.py similarity index 100% rename from qrexec/tests/cli.py rename to qrexec/tests/tools/qrexec_policy_exec.py diff --git a/qrexec/tests/policy_graph.py b/qrexec/tests/tools/qrexec_policy_graph.py similarity index 100% rename from qrexec/tests/policy_graph.py rename to qrexec/tests/tools/qrexec_policy_graph.py diff --git a/rpm_spec/qubes-qrexec.spec.in b/rpm_spec/qubes-qrexec.spec.in index a0012527..f9a4be72 100644 --- a/rpm_spec/qubes-qrexec.spec.in +++ b/rpm_spec/qubes-qrexec.spec.in @@ -192,24 +192,29 @@ rm -f %{name}-%{version} %dir %{python3_sitelib}/qrexec/tests %dir %{python3_sitelib}/qrexec/tests/__pycache__ -%{python3_sitelib}/qrexec/tests/__pycache__/* %{python3_sitelib}/qrexec/tests/__init__.py -%{python3_sitelib}/qrexec/tests/cli.py +%{python3_sitelib}/qrexec/tests/__pycache__/* %{python3_sitelib}/qrexec/tests/gtkhelpers.py %{python3_sitelib}/qrexec/tests/rpcconfirmation.py -%{python3_sitelib}/qrexec/tests/policy_parser.py -%{python3_sitelib}/qrexec/tests/qrexec_policy_daemon.py -%{python3_sitelib}/qrexec/tests/qrexec_legacy_convert.py -%{python3_sitelib}/qrexec/tests/policy_cache.py -%{python3_sitelib}/qrexec/tests/policy_graph.py %{python3_sitelib}/qrexec/tests/server.py -%{python3_sitelib}/qrexec/tests/policy_admin.py -%{python3_sitelib}/qrexec/tests/policy_admin_client.py + +%dir %{python3_sitelib}/qrexec/tests/policy +%dir %{python3_sitelib}/qrexec/tests/policy/__pycache__ +%{python3_sitelib}/qrexec/tests/policy/__pycache__/* +%{python3_sitelib}/qrexec/tests/policy/__init__.py +%{python3_sitelib}/qrexec/tests/policy/admin.py +%{python3_sitelib}/qrexec/tests/policy/admin_client.py +%{python3_sitelib}/qrexec/tests/policy/parser.py +%{python3_sitelib}/qrexec/tests/policy/utils.py %dir %{python3_sitelib}/qrexec/tests/tools %dir %{python3_sitelib}/qrexec/tests/tools/__pycache__ %{python3_sitelib}/qrexec/tests/tools/__pycache__/* %{python3_sitelib}/qrexec/tests/tools/__init__.py +%{python3_sitelib}/qrexec/tests/tools/qrexec_legacy_convert.py +%{python3_sitelib}/qrexec/tests/tools/qrexec_policy_daemon.py +%{python3_sitelib}/qrexec/tests/tools/qrexec_policy_exec.py +%{python3_sitelib}/qrexec/tests/tools/qrexec_policy_graph.py %{python3_sitelib}/qrexec/tests/tools/qubes_policy.py %{python3_sitelib}/qrexec/tests/tools/qubes_policy_admin.py