fix: Restrict read permissions for config file (#225)

src/service.py

@@ -1,9 +1,10 @@
 """Exporter service helper."""
 
+import os
 from functools import wraps
 from logging import getLogger
 from pathlib import Path
-from typing import Any, Callable, Dict, Tuple
+from typing import Any, Callable, Dict, Optional, Tuple
 
 from charms.operator_libs_linux.v1 import systemd
 from jinja2 import Environment, FileSystemLoader
@@ -55,13 +56,20 @@ def __init__(self, search_path: Path):
         self.config_template = self.environment.get_template(EXPORTER_CONFIG_TEMPLATE)
         self.service_template = self.environment.get_template(EXPORTER_SERVICE_TEMPLATE)
 
-    def _install(self, path: Path, content: str) -> bool:
+    def _install(self, path: Path, content: str, mode: Optional[int] = None) -> bool:
         """Install file."""
         success = True
         try:
             logger.info("Writing file to %s.", path)
-            with open(path, "w", encoding="utf-8") as file:
+            fileobj = (
+                os.fdopen(os.open(path, os.O_CREAT | os.O_WRONLY, mode), "w", encoding="utf-8")
+                if mode
+                # create file with default permissions based on default OS umask
+                else open(path, "w", encoding="utf-8")  # pylint: disable=consider-using-with
+            )
+            with fileobj as file:
                 file.write(content)
+
         except (NotADirectoryError, PermissionError) as err:
             logger.error(err)
             logger.info("Writing file to %s - Failed.", path)
@@ -106,7 +114,7 @@ def render_config(
             REDFISH_PASSWORD=redfish_conn_params.get("password", ""),
             REDFISH_CLIENT_TIMEOUT=redfish_conn_params.get("timeout", ""),
         )
-        return self._install(EXPORTER_CONFIG_PATH, content)
+        return self._install(EXPORTER_CONFIG_PATH, content, mode=0o600)
 
     def render_service(self, charm_dir: str, config_file: str) -> bool:
         """Render and install exporter service file."""

tests/functional/test_charm.py

@@ -533,6 +533,14 @@ async def test_resource_clean_up(self, ops_test, app, unit, required_resources):
 class TestCharm:
     """Perform basic functional testing of the charm without having the actual hardware."""
 
+    async def test_config_file_permissions(self, unit, ops_test):
+        """Check config file permissions are set correctly."""
+        expected_file_mode = "600"
+        cmd = "stat -c '%a' /etc/hardware-exporter-config.yaml"
+        results = await run_command_on_unit(ops_test, unit.name, cmd)
+        assert results.get("return-code") == 0
+        assert results.get("stdout").rstrip("\n") == expected_file_mode
+
     async def test_config_changed_port(self, app, unit, ops_test):
         """Test changing the config option: exporter-port."""
         new_port = "10001"

tests/unit/test_exporter.py

@@ -57,6 +57,10 @@ def setUp(self):
         redfish_client_patcher.start()
         self.addCleanup(redfish_client_patcher.stop)
 
+        os_patcher = mock.patch.object(service, "os")
+        os_patcher.start()
+        self.addCleanup(os_patcher.stop)
+
     @classmethod
     def setUpClass(cls):
         exporter_health_retry_count_patcher = mock.patch("charm.EXPORTER_HEALTH_RETRY_COUNT", 1)
@@ -69,7 +73,7 @@ def setUpClass(cls):
         exporter_health_retry_timeout_patcher.start()
         cls.addClassCleanup(exporter_health_retry_timeout_patcher.stop)
 
-    def test_00_install_okay(self):
+    def test_install_okay(self):
         """Test exporter service is installed when charm is installed - okay."""
         self.harness.begin()
 
@@ -78,7 +82,7 @@ def test_00_install_okay(self):
             mock_open.assert_called()
             self.mock_systemd.daemon_reload.assert_called_once()
 
-    def test_01_install_failed_rendering(self):
+    def test_install_failed_rendering(self):
         """Test exporter service is failed to installed - failed to render."""
         self.harness.begin()
 
@@ -95,7 +99,7 @@ def test_01_install_failed_rendering(self):
             self.mock_systemd.daemon_reload.assert_not_called()
 
     @mock.patch.object(pathlib.Path, "exists", return_value=True)
-    def test_10_uninstall_okay(self, mock_service_exists):
+    def test_uninstall_okay(self, mock_service_exists):
         """Test exporter service is uninstalled when charm is removed - okay."""
         self.harness.begin()
 
@@ -105,7 +109,7 @@ def test_10_uninstall_okay(self, mock_service_exists):
             self.mock_systemd.daemon_reload.assert_called_once()
 
     @mock.patch.object(pathlib.Path, "exists", return_value=True)
-    def test_11_uninstall_failed(self, mock_service_exists):
+    def test_uninstall_failed(self, mock_service_exists):
         """Test exporter service is not uninstalled - failed to remove."""
         self.harness.begin()
 
@@ -116,7 +120,7 @@ def test_11_uninstall_failed(self, mock_service_exists):
             self.mock_systemd.daemon_reload.assert_not_called()
 
     @mock.patch.object(pathlib.Path, "exists", return_value=True)
-    def test_20_start_okay(self, mock_service_installed):
+    def test_start_okay(self, mock_service_installed):
         """Test exporter service started when relation is joined."""
         rid = self.harness.add_relation(EXPORTER_RELATION_NAME, "grafana-agent")
         self.harness.begin()
@@ -127,7 +131,7 @@ def test_20_start_okay(self, mock_service_installed):
         self.mock_systemd.service_enable.assert_called_once()
 
     @mock.patch.object(pathlib.Path, "exists", return_value=False)
-    def test_21_start_failed(self, mock_service_not_installed):
+    def test_start_failed(self, mock_service_not_installed):
         """Test exporter service failed to started when relation is joined."""
         rid = self.harness.add_relation(EXPORTER_RELATION_NAME, "grafana-agent")
         self.harness.begin()
@@ -136,7 +140,7 @@ def test_21_start_failed(self, mock_service_not_installed):
         self.mock_systemd.service_enable.assert_not_called()
 
     @mock.patch.object(pathlib.Path, "exists", return_value=True)
-    def test_22_start_defer_resource_not_ready(self, mock_service_installed):
+    def test_start_defer_resource_not_ready(self, mock_service_installed):
         """Test exporter service started when relation is joined."""
         rid = self.harness.add_relation(EXPORTER_RELATION_NAME, "grafana-agent")
         self.harness.begin()
@@ -147,7 +151,7 @@ def test_22_start_defer_resource_not_ready(self, mock_service_installed):
         self.mock_systemd.service_enable.assert_not_called()
 
     @mock.patch.object(pathlib.Path, "exists", return_value=True)
-    def test_23_start_defer_exporter_not_ready(self, mock_service_installed):
+    def test_start_defer_exporter_not_ready(self, mock_service_installed):
         """Test exporter service started when relation is joined."""
         rid = self.harness.add_relation(EXPORTER_RELATION_NAME, "grafana-agent")
         self.harness.begin()
@@ -158,7 +162,7 @@ def test_23_start_defer_exporter_not_ready(self, mock_service_installed):
         self.mock_systemd.service_enable.assert_not_called()
 
     @mock.patch.object(pathlib.Path, "exists", return_value=True)
-    def test_30_stop_okay(self, mock_service_installed):
+    def test_stop_okay(self, mock_service_installed):
         """Test exporter service is stopped when service is installed and relation is departed."""
         rid = self.harness.add_relation(EXPORTER_RELATION_NAME, "grafana-agent")
         self.harness.begin()
@@ -170,7 +174,7 @@ def test_30_stop_okay(self, mock_service_installed):
         self.mock_systemd.service_disable.assert_called_once()
 
     @mock.patch.object(pathlib.Path, "exists", return_value=False)
-    def test_31_stop_failed(self, mock_service_not_installed):
+    def test_stop_failed(self, mock_service_not_installed):
         """Test exporter service failed to stop when service is not installed."""
         rid = self.harness.add_relation(EXPORTER_RELATION_NAME, "grafana-agent")
         self.harness.begin()
@@ -191,7 +195,7 @@ def test_31_stop_failed(self, mock_service_not_installed):
         ]
     )
     @mock.patch.object(pathlib.Path, "exists", return_value=True)
-    def test_40_check_health(
+    def test_check_health(
         self,
         failed,
         expected_status,
@@ -211,7 +215,7 @@ def test_40_check_health(
         self.assertEqual(self.harness.charm.unit.status, expected_status)
 
     @mock.patch.object(pathlib.Path, "exists", return_value=True)
-    def test_40_check_health_exporter_crash(self, mock_service_installed):
+    def test_check_health_exporter_crash(self, mock_service_installed):
         """Test check_health function when service is installed but exporter crashes."""
         rid = self.harness.add_relation(EXPORTER_RELATION_NAME, "grafana-agent")
         self.harness.begin()
@@ -225,7 +229,7 @@ def test_40_check_health_exporter_crash(self, mock_service_installed):
             self.harness.charm.on.update_status.emit()
 
     @mock.patch.object(pathlib.Path, "exists", return_value=True)
-    def test_50_check_relation_exists(self, mock_service_installed):
+    def test_check_relation_exists(self, mock_service_installed):
         """Test check_relation function when relation exists."""
         rid = self.harness.add_relation(EXPORTER_RELATION_NAME, "grafana-agent")
         self.harness.begin()
@@ -237,7 +241,7 @@ def test_50_check_relation_exists(self, mock_service_installed):
         self.assertEqual(self.harness.charm.unit.status, ActiveStatus("Unit is ready"))
 
     @mock.patch.object(pathlib.Path, "exists", return_value=True)
-    def test_51_check_relation_not_exists(self, mock_service_installed):
+    def test_check_relation_not_exists(self, mock_service_installed):
         """Test check_relation function when relation does not exists."""
         self.harness.begin()
         with mock.patch("builtins.open", new_callable=mock.mock_open) as _:
@@ -249,7 +253,7 @@ def test_51_check_relation_not_exists(self, mock_service_installed):
         )
 
     @mock.patch.object(pathlib.Path, "exists", return_value=True)
-    def test_52_too_many_relations(self, mock_service_installed):
+    def test_too_many_relations(self, mock_service_installed):
         """Test there too many relations."""
         rid_1 = self.harness.add_relation(EXPORTER_RELATION_NAME, "grafana-agent")
         rid_2 = self.harness.add_relation(EXPORTER_RELATION_NAME, "grafana-agent")
@@ -266,7 +270,7 @@ def test_52_too_many_relations(self, mock_service_installed):
         )
 
     @mock.patch.object(pathlib.Path, "exists", return_value=True)
-    def test_60_config_changed_log_level_okay(self, mock_service_installed):
+    def test_config_changed_log_level_okay(self, mock_service_installed):
         """Test on_config_change function when exporter-log-level is changed."""
         rid = self.harness.add_relation(EXPORTER_RELATION_NAME, "grafana-agent")
         self.harness.begin()
@@ -284,7 +288,7 @@ def test_60_config_changed_log_level_okay(self, mock_service_installed):
             )
 
     @mock.patch.object(pathlib.Path, "exists", return_value=True)
-    def test_61_config_changed_not_okay(self, mock_service_installed):
+    def test_invalid_exporter_log_level(self, mock_service_installed):
         """Test on_config_change function when exporter-log-level is changed."""
         rid = self.harness.add_relation(EXPORTER_RELATION_NAME, "grafana-agent")
         self.harness.begin()
@@ -309,8 +313,8 @@ def test_61_config_changed_not_okay(self, mock_service_installed):
 
     @mock.patch("charm.Exporter", return_value=mock.MagicMock())
     @mock.patch.object(pathlib.Path, "exists", return_value=True)
-    def test_62_config_changed_not_okay(self, mock_service_installed, mock_exporter):
-        """Test on_config_change function when exporter-log-level is changed."""
+    def test_render_config_fail(self, mock_service_installed, mock_exporter):
+        """Test on_config_change function when render config fails."""
         rid = self.harness.add_relation(EXPORTER_RELATION_NAME, "grafana-agent")
         self.harness.begin()
 
@@ -361,6 +365,7 @@ def test_render_config(self, mock_get_hw_tool_white_list):
                     REDFISH_USERNAME="",
                     REDFISH_CLIENT_TIMEOUT=10,
                 ),
+                mode=0o600,
             )
 
     @mock.patch(
@@ -393,4 +398,5 @@ def test_render_config_redfish(self, mock_get_hw_tool_white_list):
                     REDFISH_USERNAME="default_user",
                     REDFISH_CLIENT_TIMEOUT="10",
                 ),
+                mode=0o600,
             )