Report policy exceptions back to the caller

Fixes: QubesOS/qubes-issues#10746

Author: ben-grande

qrexec/client.py

@@ -21,6 +21,17 @@
 from typing import Optional
 
 from .utils import prepare_subprocess_kwds
+# The PolicyAdmin*Exception is used by issubclass(..., PolicyAdminException).
+# pylint: disable=unused-import
+from .policy.admin import (
+    PolicyAdminException,
+    PolicyAdminTokenException,
+    PolicyAdminFileNotFoundException,
+    PolicyAdminProtocolException,
+    PolicyAdminSyntaxException,
+    PolicyAdminInvalidFileNameException,
+    PolicyAdminInvalidFilePathException,
+)
 
 QREXEC_CLIENT_DOM0 = "/usr/bin/qrexec-client"
 QREXEC_CLIENT_VM = "/usr/bin/qrexec-client-vm"
@@ -45,11 +56,22 @@ def call(dest: str, rpcname: str, arg: Optional[str] = None, *, payload=None):
     :param str or None arg: argument of the call
     :param str or bytes or file or None payload: an payload to the qrexec call
     :rtype: str
-    :raises subprocess.CalledProcessError: on failure
+    :raises subprocess.CalledProcessError: on unexpected failure, instance of \
+            PolicyAdminException otherwise
     """
     command = make_command(dest=dest, rpcname=rpcname, arg=arg)
     kwds = prepare_subprocess_kwds(payload, for_popen=False)
-    return subprocess.check_output(command, **kwds).decode()
+    try:
+        return subprocess.check_output(command, **kwds).decode()
+    except subprocess.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
 
 
 async def call_async(dest, rpcname, arg=None, *, payload=None):

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,29 +121,29 @@ 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)
             )
 
         func = self._find_method(service_name)
         if not func:
-            raise PolicyAdminException(
+            raise PolicyAdminProtocolException(
                 "unrecognized method: {}".format(service_name)
             )
 
         args = []
 
         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,15 +308,15 @@ 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."
             )
         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(

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]]"
@@ -131,10 +132,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)
 
 

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
         )

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,25 +72,22 @@ def manage_policy(self) -> None:
         # 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:
             original_content, token = client.policy_get(
                 policy=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 {tmpfile.name!r}: "
                 f"{exc}",

qrexec/utils.py

@@ -195,4 +195,5 @@ def prepare_subprocess_kwds(
         # XXX this breaks on file-like objects that don't have .fileno
         kwds["stdin"] = payload
         kwds["input"] = None
+    kwds["stderr"] = subprocess.PIPE
     return kwds