Handle KeyboardInterrupt, improve configure error handling (#168)

Author: ikretz

README.md

@@ -44,7 +44,7 @@ To check whether the installation succeeded, run the following command and verif
 
 ```bash
 $ scfw --version
-2.3.0
+2.4.0
 ```
 
 ### Post-installation steps

requirements-dev.txt

@@ -2,4 +2,4 @@
 A supply-chain "firewall" for preventing the installation of vulnerable or malicious `pip` and `npm` packages.
 """
 
-__version__ = "2.3.0"
+__version__ = "2.4.0"

requirements.txt

@@ -3,12 +3,15 @@
 """
 
 from argparse import Namespace
+import logging
 
 import scfw.configure.dd_agent as dd_agent
 import scfw.configure.env as env
 import scfw.configure.interactive as interactive
 from scfw.configure.interactive import GREETING
 
+_log = logging.getLogger(__name__)
+
 
 def run_configure(args: Namespace) -> int:
     """
@@ -18,27 +21,27 @@ def run_configure(args: Namespace) -> int:
         args: A `Namespace` containing the parsed `configure` subcommand command line.
 
     Returns:
-        An integer status code indicating normal exit.
+        An integer status code indicating normal or error exit.
     """
+    dd_agent_status = 0
+    env_status = 0
+
     if args.remove:
-        # These options result in the firewall's configuration block being removed
-        env.update_config_files({
-            "alias_npm": False,
-            "alias_pip": False,
-            "alias_poetry": False,
-            "dd_agent_port": None,
-            "dd_api_key": None,
-            "dd_log_level": None,
-            "scfw_home": None,
-        })
-        dd_agent.remove_agent_logging()
+        try:
+            dd_agent.remove_agent_logging()
+        except Exception as e:
+            _log.warning(f"Failed to remove Datadog Agent configuration: {e}")
+            dd_agent_status = 1
+
+        env_status = env.remove_config()
+
         print(
             "All Supply-Chain Firewall-managed configuration has been removed from your environment."
             "\n\nPost-removal tasks:"
             "\n* Update your current shell environment by sourcing from your .bashrc/.zshrc file."
             "\n* If you had previously configured Datadog Agent log forwarding, restart the Agent."
         )
-        return 0
+        return dd_agent_status or env_status
 
     # The CLI parser guarantees that all of these arguments are present
     is_interactive = not any({
@@ -60,12 +63,18 @@ def run_configure(args: Namespace) -> int:
     if not answers:
         return 0
 
-    env.update_config_files(answers)
-
     if (port := answers.get("dd_agent_port")):
-        dd_agent.configure_agent_logging(port)
+        try:
+            dd_agent.configure_agent_logging(port)
+        except Exception as e:
+            _log.warning(f"Failed to configure Datadog Agent for Supply-Chain Firewall: {e}")
+            dd_agent_status = 1
+            # Don't set the Agent port environment variable if Agent configuration failed
+            answers["dd_agent_port"] = None
+
+    env_status = env.update_config_files(answers)
 
     if is_interactive:
         print(interactive.get_farewell(answers))
 
-    return 0
+    return dd_agent_status or env_status

scfw/__init__.py

@@ -7,6 +7,7 @@
 from pathlib import Path
 import shutil
 import subprocess
+from typing import Optional
 
 from scfw.constants import DD_SERVICE, DD_SOURCE
 
@@ -22,9 +23,9 @@ def configure_agent_logging(port: str):
 
     Raises:
         ValueError: An invalid port number was provided.
-        RuntimeError: An error occurred while querying the Agent's status.
+        RuntimeError: Failed to determine Datadog Agent configuration directory.
     """
-    if not (0 < int(port) < 2 ** 16):
+    if not (0 < int(port) < 65536):
         raise ValueError("Invalid port number provided for Datadog Agent logging")
 
     config_file = (
@@ -36,6 +37,9 @@ def configure_agent_logging(port: str):
     )
 
     scfw_config_dir = _dd_agent_scfw_config_dir()
+    if not scfw_config_dir:
+        raise RuntimeError("Failed to determine Datadog Agent configuration directory")
+
     scfw_config_file = scfw_config_dir / "conf.yaml"
 
     if not scfw_config_dir.is_dir():
@@ -49,63 +53,57 @@ def configure_agent_logging(port: str):
 def remove_agent_logging():
     """
     Remove Datadog Agent configuration for Supply-Chain Firewall, if it exists.
-
-    Raises:
-        RuntimeError: An error occurred while attempting to remove the configuration directory.
     """
-    try:
-        scfw_config_dir = _dd_agent_scfw_config_dir()
-    except FileNotFoundError:
-        _log.info("Datadog Agent binary is not available; no configuration to remove")
-        return
-
-    if not scfw_config_dir.is_dir():
+    scfw_config_dir = _dd_agent_scfw_config_dir()
+    if not (scfw_config_dir and scfw_config_dir.is_dir()):
         _log.info("No Datadog Agent configuration directory to remove")
         return
 
     try:
         shutil.rmtree(scfw_config_dir)
-        _log.info(f"Deleted directory {scfw_config_dir} with Datadog Agent configuration")
-    except Exception:
-        raise RuntimeError(
-            f"Failed to delete directory {scfw_config_dir} with Datadog Agent configuration for Supply-Chain Firewall"
+        _log.info(f"Removed directory {scfw_config_dir} with Datadog Agent configuration")
+    except Exception as e:
+        _log.warning(
+            f"Failed to remove Datadog Agent configuration directory {scfw_config_dir}: {e}"
         )
 
 
-def _dd_agent_scfw_config_dir() -> Path:
+def _dd_agent_scfw_config_dir() -> Optional[Path]:
     """
-    Get the filesystem path to the firewall's configuration directory for
-    Datadog Agent log forwarding.
+    Return the filesystem path to Supply-Chain Firewall's configuration directory
+    for Datadog Agent log forwarding.
 
     Returns:
-        A `Path` indicating the absolute filesystem path to this directory.
+        A `Path` containing the local filesystem path to Supply-Chain Firewall's
+        configuration directory for the Datadog Agent or `None` if the Agent binary
+        is inaccessible or the Agent's global configuration directory (always the
+        returned directory's parent) does not exist.
+
+        The returned path is what Supply-Chain Firewall's configuration directory
+        would be if it existed, but this function does not check that this directory
+        actually exists. It is the caller's responsibility to do so.
 
     Raises:
-        RuntimeError:
-            * Unable to query Datadog Agent status to read the location of its global
-              configuration directory
-            * Datadog Agent global configuration directory is not set or does not exist
-        ValueError: Failed to parse Datadog Agent status JSON report.
+        RuntimeError: Failed to query the Datadog Agent's status.
     """
+    agent_path = shutil.which("datadog-agent")
+    if not agent_path:
+        _log.info("No Datadog Agent binary is accessible in the current environment")
+        return None
+
+    agent_config_dir = None
     try:
         agent_status = subprocess.run(
-            ["datadog-agent", "status", "--json"], check=True, text=True, capture_output=True
+            [agent_path, "status", "--json"], check=True, text=True, capture_output=True
         )
-        config_confd_path = json.loads(agent_status.stdout).get("config", {}).get("confd_path")
-        agent_config_dir = Path(config_confd_path) if config_confd_path else None
+        if (config_confd_path := json.loads(agent_status.stdout).get("config", {}).get("confd_path")):
+            agent_config_dir = Path(config_confd_path).absolute()
 
-    except subprocess.CalledProcessError:
-        raise RuntimeError(
-            "Unable to query Datadog Agent status: please ensure the Agent is running. "
-            "Linux users may need sudo to run this command."
-        )
-
-    except json.JSONDecodeError:
-        raise ValueError("Failed to parse Datadog Agent status report as JSON")
+    except Exception as e:
+        raise RuntimeError(f"Failed to query Datadog Agent status: {e}")
 
     if not (agent_config_dir and agent_config_dir.is_dir()):
-        raise RuntimeError(
-            "Datadog Agent global configuration directory is not set or does not exist"
-        )
+        _log.info("No Datadog Agent global configuration directory found")
+        return None
 
     return agent_config_dir / "scfw.d"

scfw/configure/__init__.py

@@ -16,22 +16,52 @@
 _BLOCK_END = "# END SCFW MANAGED BLOCK"
 
 
-def update_config_files(answers: dict):
+def remove_config() -> int:
+    """
+    Remove Supply-Chain Firewall configuration from all supported files.
+
+    Returns:
+        An integer status code indicating normal or error exit.
+    """
+    # These options result in the firewall's configuration block being removed
+    return update_config_files({
+        "alias_npm": False,
+        "alias_pip": False,
+        "alias_poetry": False,
+        "dd_agent_port": None,
+        "dd_api_key": None,
+        "dd_log_level": None,
+        "scfw_home": None,
+    })
+
+
+def update_config_files(answers: dict) -> int:
     """
     Update the Supply-Chain Firewall configuration in all supported files.
 
     Args:
         answers: A `dict` of configuration options to format and write to each file.
+
+    Returns:
+        An integer status code indicating normal or error exit.
     """
+    error_count = 0
     scfw_config = _format_answers(answers)
 
     for config_file in [Path.home() / file for file in _CONFIG_FILES]:
         if not config_file.exists():
             _log.info(f"Skipped adding configuration to file {config_file}: file does not already exist")
             continue
 
-        _update_config_file(config_file, scfw_config)
-        _log.info(f"Successfully added configuration to file {config_file}")
+        try:
+            _update_config_file(config_file, scfw_config)
+            _log.info(f"Successfully updated configuration in file {config_file}")
+
+        except Exception as e:
+            _log.warning(f"Failed to update configuration in file {config_file}: {e}")
+            error_count += 1
+
+    return 1 if error_count else 0
 
 
 def _update_config_file(config_file: Path, scfw_config: str):

scfw/configure/dd_agent.py

@@ -28,7 +28,7 @@ def __init__(self, agent_port: int):
         Raises:
             ValueError: An invalid port number was given.
         """
-        if not 0 < agent_port < 2 ** 16:
+        if not 0 < agent_port < 65536:
             raise ValueError(f"Invalid port number {agent_port}")
         self._agent_port = agent_port
 

scfw/configure/env.py

@@ -47,6 +47,10 @@ def main() -> int:
         _log.error(e)
         return 1
 
+    except KeyboardInterrupt:
+        _log.info("Exiting after receiving keyboard interrupt")
+        return 1
+
 
 def _configure_logging(level: int):
     """