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/client.py b/qrexec/client.py index b836c338..345bcbb3 100644 --- a/qrexec/client.py +++ b/qrexec/client.py @@ -49,10 +49,9 @@ 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() + 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() async def call_async(dest, rpcname, arg=None, *, input=None): @@ -71,26 +70,10 @@ 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 - ) - + 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) stdout, _stderr = await process.communicate(to_communicate) if process.returncode != 0: raise subprocess.CalledProcessError(process.returncode, command) diff --git a/qrexec/policy/admin.py b/qrexec/policy/admin.py index e2318b38..329cf495 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 @@ -76,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 @@ -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 f9692607..85815b58 100644 --- a/qrexec/policy/admin_client.py +++ b/qrexec/policy/admin_client.py @@ -34,41 +34,115 @@ """ 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) -> 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 + 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/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/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 75% rename from qrexec/tests/policy_admin.py rename to qrexec/tests/policy/admin.py index fed7d8cc..ec45dff3 100644 --- a/qrexec/tests/policy_admin.py +++ b/qrexec/tests/policy/admin.py @@ -22,9 +22,12 @@ import pytest -from ..policy.admin import ( +from qrexec.policy.admin import ( PolicyAdmin, - PolicyAdminException, + PolicyAdminFileNotFoundException, + PolicyAdminInvalidFileNameException, + PolicyAdminProtocolException, + PolicyAdminSyntaxException, PolicyAdminTokenException, compute_token, ) @@ -46,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() @@ -73,19 +91,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"") @@ -113,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") @@ -124,11 +159,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 +175,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,10 +211,14 @@ 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") + 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( @@ -212,10 +255,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/tests/policy/admin_client.py b/qrexec/tests/policy/admin_client.py new file mode 100644 index 00000000..dd2dd7a9 --- /dev/null +++ b/qrexec/tests/policy/admin_client.py @@ -0,0 +1,290 @@ +# +# 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 qrexec.policy.admin_client import PolicyClient +from qrexec.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 + 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/tests/policy_parser.py b/qrexec/tests/policy/parser.py similarity index 99% rename from qrexec/tests/policy_parser.py rename to 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/policy_cache.py b/qrexec/tests/policy/utils.py similarity index 99% rename from qrexec/tests/policy_cache.py rename to qrexec/tests/policy/utils.py index e7baae01..37a0a125 100644 --- a/qrexec/tests/policy_cache.py +++ b/qrexec/tests/policy/utils.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/server.py b/qrexec/tests/server.py index e4c7abb2..ae4250a7 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 qrexec.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/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__), "..", "..", "..") 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/qrexec_legacy_convert.py b/qrexec/tests/tools/qrexec_legacy_convert.py similarity index 99% rename from qrexec/tests/qrexec_legacy_convert.py rename to qrexec/tests/tools/qrexec_legacy_convert.py index a474654b..d9dcf95d 100644 --- a/qrexec/tests/qrexec_legacy_convert.py +++ b/qrexec/tests/tools/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/tools/qrexec_policy_daemon.py similarity index 99% rename from qrexec/tests/qrexec_policy_daemon.py rename to qrexec/tests/tools/qrexec_policy_daemon.py index 34f5e28a..f5983145 100644 --- a/qrexec/tests/qrexec_policy_daemon.py +++ b/qrexec/tests/tools/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/cli.py b/qrexec/tests/tools/qrexec_policy_exec.py similarity index 99% rename from qrexec/tests/cli.py rename to qrexec/tests/tools/qrexec_policy_exec.py index eba267d6..34eebaa0 100644 --- a/qrexec/tests/cli.py +++ b/qrexec/tests/tools/qrexec_policy_exec.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_graph.py b/qrexec/tests/tools/qrexec_policy_graph.py similarity index 99% rename from qrexec/tests/policy_graph.py rename to qrexec/tests/tools/qrexec_policy_graph.py index b23c5985..dff3722e 100644 --- a/qrexec/tests/policy_graph.py +++ b/qrexec/tests/tools/qrexec_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/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/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 "" diff --git a/qrexec/tools/qubes_policy.py b/qrexec/tools/qubes_policy.py index 440ee006..d0ec7e87 100755 --- a/qrexec/tools/qubes_policy.py +++ b/qrexec/tools/qubes_policy.py @@ -23,12 +23,11 @@ import argparse import sys -import os import subprocess from ..policy.admin_client import PolicyClient from .. import RPCNAME_ALLOWED_CHARSET -from ..client import IN_DOM0 +from ..policy.admin import PolicyAdminException parser = argparse.ArgumentParser( usage="qubes-policy {[-l]|-g|-r|-d} [include/][RPCNAME[+ARGUMENT]]" @@ -81,45 +80,24 @@ 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 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 @@ -143,14 +121,18 @@ 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") + 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) - - -if __name__ == "__main__": - sys.exit(main()) diff --git a/qrexec/tools/qubes_policy_admin.py b/qrexec/tools/qubes_policy_admin.py index b759588d..d9c9522c 100644 --- a/qrexec/tools/qubes_policy_admin.py +++ b/qrexec/tools/qubes_policy_admin.py @@ -27,14 +27,21 @@ import sys import logging -from ..policy.admin import PolicyAdmin, PolicyAdminException +from ..policy.admin import ( + PolicyAdmin, + PolicyAdminException, +) from .. import POLICYPATH def main(): + 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="/var/log/qubes/policy-admin.log", + filename=log_file, format="%(asctime)s %(message)s", ) @@ -48,22 +55,24 @@ 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: 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) diff --git a/qrexec/tools/qubes_policy_editor.py b/qrexec/tools/qubes_policy_editor.py index ffe938dd..4baf61c4 100644 --- a/qrexec/tools/qubes_policy_editor.py +++ b/qrexec/tools/qubes_policy_editor.py @@ -29,9 +29,14 @@ import subprocess import sys import tempfile + from ..policy.admin_client import PolicyClient -from .. import RPCNAME_ALLOWED_CHARSET, POLICYPATH, INCLUDEPATH -from ..client import IN_DOM0 +from ..policy.admin import ( + PolicyAdminException, + PolicyAdminFileNotFoundException, +) +from .. import RPCNAME_ALLOWED_CHARSET +from .qubes_policy_lint import main as lint def validate_name(name): @@ -44,7 +49,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) @@ -55,118 +60,115 @@ def validate_name(name): return name -def manage_policy(name, is_include=False): - client = PolicyClient() +class PolicyManager: - # 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: - 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) - 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) - - 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) + def __init__(self, policy: str, is_include: bool = False) -> None: + self.policy = policy + self.is_include = is_include + self.tmpfile_name: str | None = None - 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" + def manage_policy(self) -> None: + client = PolicyClient() - lint_policy(tmpfile.name, is_include=is_include) + # 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: + suffix = "_include_" + self.policy + else: + suffix = "_" + self.policy + ".policy" - with open(tmpfile.name, "r", encoding="utf-8") as current_file: - content = current_file.read() - current_file.close() + try: + original_content, token = client.policy_get( + name=self.policy, is_include=self.is_include + ) + file_exists = True + except PolicyAdminFileNotFoundException: + pass + except subprocess.CalledProcessError as exc: + print( + f"Failed to get policy {self.policy!r}: {exc}", file=sys.stderr + ) + sys.exit(1) - try: - if is_include: - client.policy_include_replace(name, content, token) - else: - client.policy_replace(name, content, token) - except subprocess.CalledProcessError as e: - print("Failed to replace file: " + name) - sys.exit(1) + # pylint: disable=consider-using-with + tmpfile = tempfile.NamedTemporaryFile(suffix=suffix, delete=False) - tmpfile.close() + 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" + self.tmpfile_name = tmpfile.name + self.lint_policy() -def get_reply(path, is_include=False): - """ - Get reply from user. + with open(tmpfile.name, "r", encoding="utf-8") as current_file: + content = current_file.read() + current_file.close() - :param path: path or "-" - :param is_include: Boolean - """ - print("What now? ", end="") - 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. + try: + client.policy_replace( + name=self.policy, + content=content, + token=token, + is_include=self.is_include, + ) + except PolicyAdminException as exc: + print( + f"Failed to replace policy {self.policy!r} with file " + f"{tmpfile.name!r}: {exc}", + file=sys.stderr, + ) + sys.exit(1) - :param path: path or "-" - :param is_include: Boolean - """ - edit_cmd = "${VISUAL:-${EDITOR:-vi}} " + path - subprocess.run(edit_cmd, shell=True, check=True) - - if is_include: - lint_cmd = "qubes-policy-lint --include-service " + path - else: - lint_cmd = "qubes-policy-lint " + path - - try: - subprocess.run(lint_cmd, shell=True, check=True) - except subprocess.CalledProcessError as exc: - return_code = exc.returncode - if return_code == 0: + tmpfile.close() + os.remove(self.tmpfile_name) + + 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 return_code == 127: - print("The linting program 'qubes-policy-lint' is not installed.") - sys.exit(1) + if reply == "q": + sys.exit(0) else: + 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_args = ["--", self.tmpfile_name] + if self.is_include: + lint_args.insert(0, "--include-service") + try: + lint(lint_args) + except SystemExit: 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) + self.get_reply() -def main(): +def main(args=None): """ Main. """ @@ -178,17 +180,13 @@ 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="?", ) 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 @@ -196,8 +194,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__": diff --git a/qrexec/utils.py b/qrexec/utils.py index 81fd51d3..af1176f6 100644 --- a/qrexec/utils.py +++ b/qrexec/utils.py @@ -175,16 +175,27 @@ 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 + kwds["stderr"] = subprocess.PIPE return kwds diff --git a/rpm_spec/qubes-qrexec.spec.in b/rpm_spec/qubes-qrexec.spec.in index 9352b7f0..f9a4be72 100644 --- a/rpm_spec/qubes-qrexec.spec.in +++ b/rpm_spec/qubes-qrexec.spec.in @@ -192,18 +192,31 @@ 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 + +%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 %dir %{python3_sitelib}/qrexec/tests/socket %dir %{python3_sitelib}/qrexec/tests/socket/__pycache__