NAS-131168 / 25.04 / harden ix-apps metadata handling (#14510)

This commit makes two minor changes to harden our ix-apps metadata to
avoid crashes due to yaml_loads returning None instead of the expected
dict. One reason this can happen is that the file in question is empty
when read.

1. Switch to using the middleware io util `write_if_changed`
This makes writes to the metadata yaml files de-facto atomic as we
write to temporary file, flush it, then rename over existing file.

Since the files are only used by middlewared I am proactively setting
permissions on them to 0o600.

2. Add a wrapper around yaml.safe_load to return an empty dict instead
of None type if file is empty. Consumers of these methods expect to
receive a dict and so we must handle this edge case.

---------

Co-authored-by: M. Rehan <[email protected]>

src/middlewared/middlewared/plugins/apps/ix_apps/lifecycle.py

@@ -5,6 +5,7 @@
 import yaml
 
 from middlewared.service_exception import CallError
+from middlewared.utils.io import write_if_changed
 
 from .path import (
     get_installed_app_config_path, get_installed_app_rendered_dir_path, get_installed_app_version_path,
@@ -18,7 +19,8 @@ def get_rendered_template_config_of_app(app_name: str, version: str) -> dict:
     for rendered_file in get_rendered_templates_of_app(app_name, version):
         with contextlib.suppress(FileNotFoundError, yaml.YAMLError):
             with open(rendered_file, 'r') as f:
-                rendered_config.update(yaml.safe_load(f.read()))
+                if (data := yaml.safe_load(f)) is not None:
+                    rendered_config.update(data)
 
     return rendered_config
 
@@ -32,13 +34,13 @@ def get_rendered_templates_of_app(app_name: str, version: str) -> list[str]:
 
 
 def write_new_app_config(app_name: str, version: str, values: dict[str, typing.Any]) -> None:
-    with open(get_installed_app_config_path(app_name, version), 'w') as f:
-        f.write(yaml.safe_dump(values))
+    app_config_path = get_installed_app_config_path(app_name, version)
+    write_if_changed(app_config_path, yaml.safe_dump(values), perms=0o600, raise_error=False)
 
 
 def get_current_app_config(app_name: str, version: str) -> dict:
     with open(get_installed_app_config_path(app_name, version), 'r') as f:
-        return yaml.safe_load(f)
+        return yaml.safe_load(f) or {}
 
 
 def render_compose_templates(app_version_path: str, values_file_path: str):
@@ -51,8 +53,8 @@ def render_compose_templates(app_version_path: str, values_file_path: str):
 def update_app_config(app_name: str, version: str, values: dict[str, typing.Any], custom_app: bool = False) -> None:
     write_new_app_config(app_name, version, values)
     if custom_app:
-        with open(get_installed_custom_app_compose_file(app_name, version), 'w') as f:
-            f.write(yaml.safe_dump(values))
+        compose_file_path = get_installed_custom_app_compose_file(app_name, version)
+        write_if_changed(compose_file_path, yaml.safe_dump(values), perms=0o600, raise_error=False)
     else:
         render_compose_templates(
             get_installed_app_version_path(app_name, version), get_installed_app_config_path(app_name, version)

src/middlewared/middlewared/plugins/apps/ix_apps/metadata.py

@@ -3,63 +3,66 @@
 
 import yaml
 
+from middlewared.utils.io import write_if_changed
 from .path import get_collective_config_path, get_collective_metadata_path, get_installed_app_metadata_path
 from .portals import get_portals_and_app_notes
 
 
-def get_app_metadata(app_name: str) -> dict[str, typing.Any]:
+def _load_app_yaml(yaml_path: str) -> dict[str, typing.Any]:
+    """ wrapper around yaml.safe_load that ensure dict always returned """
     try:
-        with open(get_installed_app_metadata_path(app_name), 'r') as f:
-            return yaml.safe_load(f)
+        with open(yaml_path, 'r') as f:
+            if (data := yaml.safe_load(f)) is None:
+                # yaml.safe_load may return None if file empty
+                return {}
+
+            return data
     except (FileNotFoundError, yaml.YAMLError):
         return {}
 
 
+def get_app_metadata(app_name: str) -> dict[str, typing.Any]:
+    return _load_app_yaml(get_installed_app_metadata_path(app_name))
+
+
 def update_app_metadata(
     app_name: str, app_version_details: dict, migrated: bool | None = None, custom_app: bool = False,
 ):
     migrated = get_app_metadata(app_name).get('migrated', False) if migrated is None else migrated
-    with open(get_installed_app_metadata_path(app_name), 'w') as f:
-        f.write(yaml.safe_dump({
+    write_if_changed(get_installed_app_metadata_path(app_name), yaml.safe_dump({
             'metadata': app_version_details['app_metadata'],
             'migrated': migrated,
             'custom_app': custom_app,
             **{k: app_version_details[k] for k in ('version', 'human_version')},
             **get_portals_and_app_notes(app_name, app_version_details['version']),
             # TODO: We should not try to get portals for custom apps for now
-        }))
+        }), perms=0o600, raise_error=False)
 
 
 def update_app_metadata_for_portals(app_name: str, version: str):
     # This should be called after config of app has been updated as that will render compose files
     app_metadata = get_app_metadata(app_name)
-    with open(get_installed_app_metadata_path(app_name), 'w') as f:
-        f.write(yaml.safe_dump({
-            **app_metadata,
-            **get_portals_and_app_notes(app_name, version),
-        }))
+
+    # Using write_if_changed ensures atomicity of the write via writing to a temporary
+    # file then renaming over existing one.
+    write_if_changed(get_installed_app_metadata_path(app_name), yaml.safe_dump({
+        **app_metadata,
+        **get_portals_and_app_notes(app_name, version),
+    }), perms=0o600, raise_error=False)
 
 
 def get_collective_config() -> dict[str, dict]:
-    try:
-        with open(get_collective_config_path(), 'r') as f:
-            return yaml.safe_load(f.read())
-    except FileNotFoundError:
-        return {}
+    return _load_app_yaml(get_collective_config_path())
 
 
 def get_collective_metadata() -> dict[str, dict]:
-    try:
-        with open(get_collective_metadata_path(), 'r') as f:
-            return yaml.safe_load(f.read())
-    except FileNotFoundError:
-        return {}
+    return _load_app_yaml(get_collective_metadata_path())
 
 
 def update_app_yaml_for_last_update(version_path: str, last_update: str):
-    with open(os.path.join(version_path, 'app.yaml'), 'r') as f:
-        app_config = yaml.safe_load(f.read())
+    app_yaml_path = os.path.join(version_path, 'app.yaml')
+
+    app_config = _load_app_yaml(app_yaml_path)
+    app_config['last_update'] = last_update
 
-    with open(os.path.join(version_path, 'app.yaml'), 'w') as f:
-        app_config['last_update'] = last_update
-        f.write(yaml.safe_dump(app_config))
+    write_if_changed(app_yaml_path, yaml.safe_dump(app_config), perms=0o600, raise_error=False)

src/middlewared/middlewared/plugins/apps/ix_apps/setup.py

@@ -3,6 +3,7 @@
 import textwrap
 import yaml
 
+from middlewared.utils.io import write_if_changed
 from .metadata import update_app_yaml_for_last_update
 from .path import get_app_parent_config_path, get_installed_app_version_path
 
@@ -20,9 +21,14 @@ def setup_install_app_dir(app_name: str, app_version_details: dict, custom_app:
 
             This is a custom app where user can use his/her own docker compose file for deploying services.
             '''))
+            f.flush()
 
-        with open(os.path.join(destination, 'app.yaml'), 'w') as f:
-            f.write(yaml.safe_dump(app_version_details['app_metadata']))
+        write_if_changed(
+            os.path.join(destination, 'app.yaml'),
+            yaml.safe_dump(app_version_details['app_metadata']),
+            perms=0o600,
+            raise_error=False
+        )
     else:
         shutil.copytree(app_version_details['location'], destination)