Merge pull request from GHSA-cg54-gpgr-4rm6

use EnvironmentFile to pass environment variables to systemd units

Author: minrk

CHANGELOG.md

@@ -1,5 +1,16 @@
 # Changelog
 
+## v0.15
+
+Fixes vulnerability [GHSA-cg54-gpgr-4rm6](https://github.com/jupyterhub/systemdspawner/security/advisories/GHSA-cg54-gpgr-4rm6) affecting all previous releases.
+
+- Use EnvironmentFile to pass environment variables to units.
+
+## v0.14
+
+- define entrypoints for JupyterHub spawner configuration
+- Fixes for CentOS 7
+
 ## v0.13
 
 ### Bug Fixes

setup.py

@@ -2,7 +2,7 @@
 
 setup(
     name='jupyterhub-systemdspawner',
-    version='0.14',
+    version='0.15.0',
     description='JupyterHub Spawner using systemd for resource isolation',
     long_description='See https://github.com/jupyterhub/systemdspawner for more info',
     url='https://github.com/jupyterhub/systemdspawner',

systemdspawner/systemd.py

@@ -4,8 +4,62 @@
 Contains functions to start, stop & poll systemd services.
 Probably not very useful outside this spawner.
 """
+
 import asyncio
+import os
+import re
 import shlex
+import warnings
+
+# light validation of environment variable keys
+env_pat = re.compile("[A-Za-z_]+")
+
+RUN_ROOT = "/run"
+
+def ensure_environment_directory(environment_file_directory):
+    """Ensure directory for environment files exists and is private"""
+    # ensure directory exists
+    os.makedirs(environment_file_directory, mode=0o700, exist_ok=True)
+    # validate permissions
+    mode = os.stat(environment_file_directory).st_mode
+    if mode & 0o077:
+        warnings.warn(
+            f"Fixing permissions on environment directory {environment_file_directory}: {oct(mode)}",
+            RuntimeWarning,
+        )
+        os.chmod(environment_file_directory, 0o700)
+    else:
+        return
+    # Check again after supposedly fixing.
+    # Some filesystems can have weird issues, preventing this from having desired effect
+    mode = os.stat(environment_file_directory).st_mode
+    if mode & 0o077:
+        warnings.warn(
+            f"Bad permissions on environment directory {environment_file_directory}: {oct(mode)}",
+            RuntimeWarning,
+        )
+
+
+def make_environment_file(environment_file_directory, unit_name, environment_variables):
+    """Make a systemd environment file
+
+    - ensures environment directory exists and is private
+    - writes private environment file
+    - returns path to created environment file
+    """
+    ensure_environment_directory(environment_file_directory)
+    env_file = os.path.join(environment_file_directory, f"{unit_name}.env")
+    env_lines = []
+    for key, value in sorted(environment_variables.items()):
+        assert env_pat.match(key), f"{key} not a valid environment variable"
+        env_lines.append(f"{key}={shlex.quote(value)}")
+    env_lines.append("")  # trailing newline
+    with open(env_file, mode="w") as f:
+        # make the file itself private as well
+        os.fchmod(f.fileno(), 0o400)
+        f.write("\n".join(env_lines))
+
+    return env_file
 
 
 async def start_transient_service(
@@ -20,14 +74,32 @@ async def start_transient_service(
     slice=None,
 ):
     """
-    Start a systemd transient service with given paramters
+    Start a systemd transient service with given parameters
     """
 
     run_cmd = [
         'systemd-run',
         '--unit', unit_name,
     ]
 
+    if properties is None:
+        properties = {}
+    else:
+        properties = properties.copy()
+
+    # ensure there is a runtime directory where we can put our env file
+    # If already set, can be space-separated list of paths
+    runtime_directories = properties.setdefault("RuntimeDirectory", unit_name).split()
+
+    # runtime directories are always resolved relative to `/run`
+    # grab the first item, if more than one
+    runtime_dir = os.path.join(RUN_ROOT, runtime_directories[0])
+    # make runtime directories private by default
+    properties.setdefault("RuntimeDirectoryMode", "700")
+    # preserve runtime directories across restarts
+    # allows `systemctl restart` to load the env
+    properties.setdefault("RuntimeDirectoryPreserve", "restart")
+
     if properties:
         for key, value in properties.items():
             if isinstance(value, list):
@@ -37,10 +109,10 @@ async def start_transient_service(
                 run_cmd.append('--property={}={}'.format(key, value))
 
     if environment_variables:
-        run_cmd += [
-            '--setenv={}={}'.format(key, value)
-            for key, value in environment_variables.items()
-        ]
+        environment_file = make_environment_file(
+            runtime_dir, unit_name, environment_variables
+        )
+        run_cmd.append(f"--property=EnvironmentFile={environment_file}")
 
     # Explicitly check if uid / gid are not None, since 0 is valid value for both
     if uid is not None:
@@ -51,7 +123,7 @@ async def start_transient_service(
 
     if slice is not None:
         run_cmd += ['--slice={}'.format(slice)]
-    
+
     # We unfortunately have to resort to doing cd with bash, since WorkingDirectory property
     # of systemd units can't be set for transient units via systemd-run until systemd v227.
     # Centos 7 has systemd 219, and will probably never upgrade - so we need to support them.

tests/test_systemd.py

@@ -65,23 +65,42 @@ async def test_service_running_fail():
 async def test_env_setting():
     unit_name = 'systemdspawner-unittest-' + str(time.time())
     with tempfile.TemporaryDirectory() as d:
+        os.chmod(d, 0o777)
         await systemd.start_transient_service(
             unit_name,
-            ['/bin/bash'],
-            ['-c', 'env > {}/env'.format(d)],
-            working_dir='/',
+            ["/bin/bash"],
+            ["-c", "pwd; ls -la {0}; env > ./env; sleep 3".format(d)],
+            working_dir=d,
             environment_variables={
-                'TESTING_SYSTEMD_ENV_1': 'TEST_1',
-                'TESTING_SYSTEMD_ENV_2': 'TEST_2'
-            }
+                "TESTING_SYSTEMD_ENV_1": "TEST 1",
+                "TESTING_SYSTEMD_ENV_2": "TEST 2",
+            },
+            # set user to ensure we are testing permission issues
+            properties={
+                "User": "65534",
+            },
         )
+        env_dir = os.path.join(systemd.RUN_ROOT, unit_name)
+        assert os.path.isdir(env_dir)
+        assert (os.stat(env_dir).st_mode & 0o777) == 0o700
 
         # Wait a tiny bit for the systemd unit to complete running
         await asyncio.sleep(0.1)
+        assert await systemd.service_running(unit_name)
+
+        env_file = os.path.join(env_dir, f"{unit_name}.env")
+        assert os.path.exists(env_file)
+        assert (os.stat(env_file).st_mode & 0o777) == 0o400
+        # verify that the env had the desired effect
         with open(os.path.join(d, 'env')) as f:
             text = f.read()
-            assert 'TESTING_SYSTEMD_ENV_1=TEST_1' in text
-            assert 'TESTING_SYSTEMD_ENV_2=TEST_2' in text
+            assert "TESTING_SYSTEMD_ENV_1=TEST 1" in text
+            assert "TESTING_SYSTEMD_ENV_2=TEST 2" in text
+
+        await systemd.stop_service(unit_name)
+        assert not await systemd.service_running(unit_name)
+        # systemd cleans up env file
+        assert not os.path.exists(env_file)
 
 
 @pytest.mark.asyncio