From 25bd8ff548d9550e7bed877cc0628ab2e2681d30 Mon Sep 17 00:00:00 2001 From: Dawei Huang Date: Mon, 16 Dec 2024 11:20:50 +0800 Subject: [PATCH] Implement run function for docker_services * DBUS Services required for GNOI Containerz StartContainer. * Add a placeholder function that prevent arbitrary request to run any container. * Update to only allow running known images. * Rewrite image validation so it get recognized by semgrep. * Rewrite command validation so it get recognize by semgrep. * maybe semgrep will recognize inline function * semgrep only allow hardcoded image name. We need to bypass it. * add documentation. * documentation need to be before nosemgrep * Add allowed_image_name and use it to verify the run function. * address copilot comment. * address comment and reformat. * increase test coverage and address comment. * Seperate allowed images and allowed containers. * Address comment. * add bmp container to allowed list. * add validation to kwargs in run. * fix test error. * update allow list --- host_modules/docker_service.py | 240 ++++++++++++++++------ tests/host_modules/docker_service_test.py | 80 ++++++++ 2 files changed, 255 insertions(+), 65 deletions(-) diff --git a/host_modules/docker_service.py b/host_modules/docker_service.py index f1c7fc8c..a88ed368 100644 --- a/host_modules/docker_service.py +++ b/host_modules/docker_service.py @@ -4,44 +4,108 @@ import docker import signal import errno +import logging MOD_NAME = "docker_service" # The set of allowed containers that can be managed by this service. -# First element is the image name, second element is the container name. -ALLOWED_CONTAINERS = [ - ("docker-syncd-brcm", "syncd"), - ("docker-acms", "acms"), - ("docker-sonic-gnmi", "gnmi"), - ("docker-sonic-telemetry", "telemetry"), - ("docker-snmp", "snmp"), - ("docker-platform-monitor", "pmon"), - ("docker-lldp", "lldp"), - ("docker-dhcp-relay", "dhcp_relay"), - ("docker-router-advertiser", "radv"), - ("docker-teamd", "teamd"), - ("docker-fpm-frr", "bgp"), - ("docker-orchagent", "swss"), - ("docker-sonic-restapi", "restapi"), - ("docker-eventd", "eventd"), - ("docker-database", "database"), -] - - -def is_allowed_container(container): +ALLOWED_CONTAINERS = { + "bgp", + "bmp", + "database", + "dhcp_relay", + "eventd", + "gnmi", + "lldp", + "pmon", + "radv", + "restapi", + "snmp", + "swss", + "syncd", + "teamd", + "telemetry", +} + +# The set of allowed images that can be managed by this service. +ALLOWED_IMAGES = { + "docker-database", + "docker-dhcp-relay", + "docker-eventd", + "docker-fpm-frr", + "docker-lldp", + "docker-orchagent", + "docker-platform-monitor", + "docker-router-advertiser", + "docker-snmp", + "docker-sonic-bmp", + "docker-sonic-gnmi", + "docker-sonic-restapi", + "docker-sonic-telemetry", + "docker-syncd-brcm", + "docker-syncd-cisco", + "docker-teamd", +} + + +def is_allowed_image(image): """ - Check if the container is allowed to be managed by this service. + Check if the image is allowed to be managed by this service. Args: - container (str): The container name. + image (str): The image name. Returns: - bool: True if the container is allowed, False otherwise. + bool: True if the image is allowed, False otherwise. """ - for _, allowed_container in ALLOWED_CONTAINERS: - if container == allowed_container: - return True - return False + image_name = image.split(":")[0] # Remove tag if present + return image_name in ALLOWED_IMAGES + + +def get_sonic_container(container_id): + """ + Get a Sonic docker container by name. If the container is not a Sonic container, raise PermissionError. + """ + client = docker.from_env() + if container_id not in ALLOWED_CONTAINERS: + raise PermissionError( + "Container {} is not allowed to be managed by this service.".format( + container_id + ) + ) + container = client.containers.get(container_id) + return container + + +def validate_docker_run_options(kwargs): + """ + Validate the keyword arguments passed to the Docker container run API. + """ + # Validate the keyword arguments here if needed + # Disallow priviledge mode for security reasons + if kwargs.get("privileged", False): + raise ValueError("Privileged mode is not allowed for security reasons.") + # Disallow sensitive directories to be mounted. + sensitive_dirs = ["/etc", "/var", "/usr"] + for bind in kwargs.get("volumes", {}).keys(): + for sensitive_dir in sensitive_dirs: + if bind.startswith(sensitive_dir): + raise ValueError( + "Mounting sensitive directories is not allowed for security reasons." + ) + # Disallow running containers as root. + if kwargs.get("user", None) == "root": + raise ValueError( + "Running containers as root is not allowed for security reasons." + ) + # Disallow cap_add for security reasons. + if kwargs.get("cap_add", None): + raise ValueError( + "Adding capabilities to containers is not allowed for security reasons." + ) + # Disallow access to sensitive devices. + if kwargs.get("devices", None): + raise ValueError("Access to devices is not allowed for security reasons.") class DockerService(host_service.HostModule): @@ -52,92 +116,138 @@ class DockerService(host_service.HostModule): @host_service.method( host_service.bus_name(MOD_NAME), in_signature="s", out_signature="is" ) - def stop(self, container): + def stop(self, container_id): """ Stop a running Docker container. Args: - container (str): The name or ID of the Docker container. + container_id (str): The name of the Docker container. Returns: tuple: A tuple containing the exit code (int) and a message indicating the result of the operation. """ try: - client = docker.from_env() - if not is_allowed_container(container): - return ( - errno.EPERM, - "Container {} is not allowed to be managed by this service.".format( - container - ), - ) - container = client.containers.get(container) + container = get_sonic_container(container_id) container.stop() return 0, "Container {} has been stopped.".format(container.name) + except PermissionError: + msg = "Container {} is not allowed to be managed by this service.".format( + container_id + ) + logging.error(msg) + return errno.EPERM, msg except docker.errors.NotFound: - return errno.ENOENT, "Container {} does not exist.".format(container) + msg = "Container {} does not exist.".format(container_id) + logging.error(msg) + return errno.ENOENT, msg except Exception as e: - return 1, "Failed to stop container {}: {}".format(container, str(e)) + msg = "Failed to stop container {}: {}".format(container_id, str(e)) + logging.error(msg) + return 1, msg @host_service.method( host_service.bus_name(MOD_NAME), in_signature="si", out_signature="is" ) - def kill(self, container, signal=signal.SIGKILL): + def kill(self, container_id, signal=signal.SIGKILL): """ Kill or send a signal to a running Docker container. Args: - container (str): The name or ID of the Docker container. + container_id (str): The name or ID of the Docker container. signal (int): The signal to send. Defaults to SIGKILL. Returns: tuple: A tuple containing the exit code (int) and a message indicating the result of the operation. """ try: - client = docker.from_env() - if not is_allowed_container(container): - return ( - errno.EPERM, - "Container {} is not allowed to be managed by this service.".format( - container - ), - ) - container = client.containers.get(container) + container = get_sonic_container(container_id) container.kill(signal=signal) return 0, "Container {} has been killed with signal {}.".format( container.name, signal ) + except PermissionError: + msg = "Container {} is not allowed to be managed by this service.".format( + container_id + ) + logging.error(msg) + return errno.EPERM, msg except docker.errors.NotFound: - return errno.ENOENT, "Container {} does not exist.".format(container) + msg = "Container {} does not exist.".format(container_id) + logging.error(msg) + return errno.ENOENT, msg except Exception as e: - return 1, "Failed to kill container {}: {}".format(container, str(e)) + return 1, "Failed to kill container {}: {}".format(container_id, str(e)) @host_service.method( host_service.bus_name(MOD_NAME), in_signature="s", out_signature="is" ) - def restart(self, container): + def restart(self, container_id): """ Restart a running Docker container. Args: - container (str): The name or ID of the Docker container. + container_id (str): The name or ID of the Docker container. + + Returns: + tuple: A tuple containing the exit code (int) and a message indicating the result of the operation. + """ + try: + container = get_sonic_container(container_id) + container.restart() + return 0, "Container {} has been restarted.".format(container.name) + except PermissionError: + return ( + errno.EPERM, + "Container {} is not allowed to be managed by this service.".format( + container_id + ), + ) + except docker.errors.NotFound: + return errno.ENOENT, "Container {} does not exist.".format(container_id) + except Exception as e: + return 1, "Failed to restart container {}: {}".format(container_id, str(e)) + + @host_service.method( + host_service.bus_name(MOD_NAME), in_signature="ssa{sv}", out_signature="is" + ) + def run(self, image, command, kwargs): + """ + Run a Docker container. + + Args: + image (str): The name of the Docker image to run. + command (str): The command to run in the container + kwargs (dict): Additional keyword arguments to pass to the Docker API. Returns: tuple: A tuple containing the exit code (int) and a message indicating the result of the operation. """ try: client = docker.from_env() - if not is_allowed_container(container): + + if not is_allowed_image(image): return ( errno.EPERM, - "Container {} is not allowed to be managed by this service.".format( - container + "Image {} is not allowed to be managed by this service.".format( + image ), ) - container = client.containers.get(container) - container.restart() - return 0, "Container {} has been restarted.".format(container.name) - except docker.errors.NotFound: - return errno.ENOENT, "Container {} does not exist.".format(container) + + if command: + return ( + errno.EPERM, + "Only an empty string command is allowed. Non-empty commands are not permitted by this service.", + ) + + validate_docker_run_options(kwargs) + + # Semgrep cannot detect codes for validating image and command. + # nosemgrep: python.docker.security.audit.docker-arbitrary-container-run.docker-arbitrary-container-run + container = client.containers.run(image, command, **kwargs) + return 0, "Container {} has been created.".format(container.name) + except ValueError as e: + return errno.EINVAL, "Invalid argument.".format(str(e)) + except docker.errors.ImageNotFound: + return errno.ENOENT, "Image {} not found.".format(image) except Exception as e: - return 1, "Failed to restart container {}: {}".format(container, str(e)) + return 1, "Failed to run image {}: {}".format(image, str(e)) diff --git a/tests/host_modules/docker_service_test.py b/tests/host_modules/docker_service_test.py index 0836d826..4152fbc8 100644 --- a/tests/host_modules/docker_service_test.py +++ b/tests/host_modules/docker_service_test.py @@ -207,3 +207,83 @@ def test_docker_restart_fail_api_error(self, MockInit, MockBusName, MockSystemBu assert "API error" in msg, "Message should contain 'API error'" mock_docker_client.containers.get.assert_called_once_with("syncd") mock_docker_client.containers.get.return_value.restart.assert_called_once() + + @mock.patch("dbus.SystemBus") + @mock.patch("dbus.service.BusName") + @mock.patch("dbus.service.Object.__init__") + def test_docker_run_success(self, MockInit, MockBusName, MockSystemBus): + mock_docker_client = mock.Mock() + mock_docker_client.containers.run.return_value.name = "syncd" + + with mock.patch.object(docker, "from_env", return_value=mock_docker_client): + docker_service = DockerService(MOD_NAME) + rc, msg = docker_service.run("docker-syncd-brcm:latest", "", {}) + + assert rc == 0, "Return code is wrong" + mock_docker_client.containers.run.assert_called_once_with( + "docker-syncd-brcm:latest", "", **{} + ) + + @mock.patch("dbus.SystemBus") + @mock.patch("dbus.service.BusName") + @mock.patch("dbus.service.Object.__init__") + def test_docker_run_fail_image_not_found( + self, MockInit, MockBusName, MockSystemBus + ): + mock_docker_client = mock.Mock() + mock_docker_client.containers.run.side_effect = docker.errors.ImageNotFound( + "Image not found" + ) + + with mock.patch.object(docker, "from_env", return_value=mock_docker_client): + docker_service = DockerService(MOD_NAME) + rc, msg = docker_service.run("docker-syncd-brcm:latest", "", {}) + + assert rc == errno.ENOENT, "Return code is wrong" + assert "not found" in msg, "Message should contain 'not found'" + mock_docker_client.containers.run.assert_called_once_with( + "docker-syncd-brcm:latest", "", **{} + ) + + @mock.patch("dbus.SystemBus") + @mock.patch("dbus.service.BusName") + @mock.patch("dbus.service.Object.__init__") + def test_docker_run_fail_api_error(self, MockInit, MockBusName, MockSystemBus): + mock_docker_client = mock.Mock() + mock_docker_client.containers.run.side_effect = docker.errors.APIError( + "API error" + ) + + with mock.patch.object(docker, "from_env", return_value=mock_docker_client): + docker_service = DockerService(MOD_NAME) + rc, msg = docker_service.run("docker-syncd-brcm:latest", "", {}) + + assert rc != 0, "Return code is wrong" + assert "API error" in msg, "Message should contain 'API error'" + mock_docker_client.containers.run.assert_called_once_with( + "docker-syncd-brcm:latest", "", **{} + ) + + @mock.patch("dbus.SystemBus") + @mock.patch("dbus.service.BusName") + @mock.patch("dbus.service.Object.__init__") + def test_docker_run_fail_image_not_allowed( + self, MockInit, MockBusName, MockSystemBus + ): + mock_docker_client = mock.Mock() + with mock.patch.object(docker, "from_env", return_value=mock_docker_client): + docker_service = DockerService(MOD_NAME) + rc, msg = docker_service.run("wrong_image_name", "", {}) + assert rc == errno.EPERM, "Return code is wrong" + + @mock.patch("dbus.SystemBus") + @mock.patch("dbus.service.BusName") + @mock.patch("dbus.service.Object.__init__") + def test_docker_run_fail_non_empty_command( + self, MockInit, MockBusName, MockSystemBus + ): + mock_docker_client = mock.Mock() + with mock.patch.object(docker, "from_env", return_value=mock_docker_client): + docker_service = DockerService(MOD_NAME) + rc, msg = docker_service.run("docker-syncd-brcm:latest", "rm -rf /", {}) + assert rc == errno.EPERM, "Return code is wrong"