fix(api): remove command escaping from v1 (#822)

This code used to be necessary to prevent systemd abuse. Now that kubernetes handles the escaping
for us, there is no need to be super defensive in our own code.

Author: Matthew Fisher

rootfs/api/models/app.py

@@ -133,37 +133,45 @@ def _get_job_id(self, container_type):
         return "{app}-{version}-{container_type}".format(**locals())
 
     def _get_command(self, container_type):
+        """
+        Return the kubernetes "container arguments" to be sent off to the scheduler.
+
+        In reality this is the command that the user it attempting to run.
+        """
         try:
-            # if this is not procfile-based app, ensure they cannot break out
-            # and run arbitrary commands on the host
             # FIXME: remove slugrunner's hardcoded entrypoint
             release = self.release_set.latest()
             if release.build.dockerfile or not release.build.sha:
-                return "bash -c '{}'".format(release.build.procfile[container_type])
+                return [release.build.procfile[container_type]]
 
-            return 'start {}'.format(container_type)
+            return ['start', container_type]
         # if the key is not present or if a parent attribute is None
         except (KeyError, TypeError, AttributeError):
             # handle special case for Dockerfile deployments
-            return '' if container_type == 'cmd' else 'start {}'.format(container_type)
+            return [] if container_type == 'cmd' else ['start', container_type]
 
-    def _get_command_run(self, command):
-        # SECURITY: shell-escape user input
-        command = command.replace("'", "'\\''")
+    def _get_entrypoint(self, container_type):
+        """
+        Return the kubernetes "container command" to be sent off to the scheduler.
+
+        In this case, it is the entrypoint for the docker image. Because of Heroku compatibility,
+        Any containers that are not from a buildpack are run under /bin/bash.
+        """
+        # handle special case for Dockerfile deployments
+        if container_type == 'cmd':
+            return []
 
         # if this is a procfile-based app, switch the entrypoint to slugrunner's default
         # FIXME: remove slugrunner's hardcoded entrypoint
         release = self.release_set.latest()
         if release.build.procfile and \
            release.build.sha and not \
            release.build.dockerfile:
-            entrypoint = '/runner/init'
-            command = "'{}'".format(command)
+            entrypoint = ['/runner/init']
         else:
-            entrypoint = '/bin/bash'
-            command = "-c '{}'".format(command)
+            entrypoint = ['/bin/bash', '-c']
 
-        return entrypoint, command
+        return entrypoint
 
     def log(self, message, level=logging.INFO):
         """Logs a message in the context of this application.
@@ -436,13 +444,13 @@ def _scale_pods(self, scale_types):
                 'deploy_timeout': deploy_timeout,
             }
 
-            command = self._get_command(scale_type)
             try:
                 self._scheduler.scale(
                     namespace=self.id,
                     name=self._get_job_id(scale_type),
                     image=release.image,
-                    command=command,
+                    entrypoint=self._get_entrypoint(scale_type),
+                    command=self._get_command(scale_type),
                     **kwargs
                 )
             except Exception as e:
@@ -523,6 +531,7 @@ def deploy(self, release, force_deploy=False):
                     namespace=self.id,
                     name=self._get_job_id(scale_type),
                     image=release.image,
+                    entrypoint=self._get_entrypoint(scale_type),
                     command=self._get_command(scale_type),
                     **kwargs
                 )
@@ -674,17 +683,15 @@ def pod_name(size=5, chars=string.ascii_lowercase + string.digits):
             return ''.join(random.choice(chars) for _ in range(size))
 
         """Run a one-off command in an ephemeral app container."""
+        scale_type = 'run'
         release = self.release_set.latest()
         if release.build is None:
             raise DeisException('No build associated with this release to run this command')
 
         # see if the app config has deploy timeout preference, otherwise use global
         deploy_timeout = release.config.values.get('DEIS_DEPLOY_TIMEOUT', settings.DEIS_DEPLOY_TIMEOUT)  # noqa
 
-        # TODO: add support for interactive shell
-        entrypoint, command = self._get_command_run(command)
-
-        name = self._get_job_id('run') + '-' + pod_name()
+        name = self._get_job_id(scale_type) + '-' + pod_name()
         self.log("{} on {} runs '{}'".format(user.username, name, command))
 
         kwargs = {
@@ -703,8 +710,8 @@ def pod_name(size=5, chars=string.ascii_lowercase + string.digits):
                 self.id,
                 name,
                 release.image,
-                entrypoint,
-                command,
+                self._get_entrypoint(scale_type),
+                [command],
                 **kwargs
             )
 

rootfs/api/tests/deployments/test_pods.py

@@ -526,31 +526,31 @@ def test_command_good(self, mock_requests):
         )
 
         # use `start web` for backwards compatibility with slugrunner
-        self.assertEqual(app._get_command('web'), 'start web')
-        self.assertEqual(app._get_command('worker'), 'start worker')
+        self.assertEqual(app._get_command('web'), ['start', 'web'])
+        self.assertEqual(app._get_command('worker'), ['start', 'worker'])
 
         # switch to docker image app
         build.sha = ''
         build.save()
-        self.assertEqual(app._get_command('web'), "bash -c 'node server.js'")
+        self.assertEqual(app._get_command('web'), ["node server.js"])
 
         # switch to dockerfile app
         build.sha = 'european-swallow'
         build.dockerfile = 'dockerdockerdocker'
         build.save()
-        self.assertEqual(app._get_command('web'), "bash -c 'node server.js'")
-        self.assertEqual(app._get_command('cmd'), '')
+        self.assertEqual(app._get_command('web'), ["node server.js"])
+        self.assertEqual(app._get_command('cmd'), [])
 
         # ensure we can override the cmd process type in a Procfile
         build.procfile['cmd'] = 'node server.js'
         build.save()
-        self.assertEqual(app._get_command('cmd'), "bash -c 'node server.js'")
-        self.assertEqual(app._get_command('worker'), "bash -c 'node worker.js'")
+        self.assertEqual(app._get_command('cmd'), ["node server.js"])
+        self.assertEqual(app._get_command('worker'), ["node worker.js"])
 
         # for backwards compatibility if no Procfile is supplied
         build.procfile = {}
         build.save()
-        self.assertEqual(app._get_command('worker'), 'start worker')
+        self.assertEqual(app._get_command('worker'), ['start', 'worker'])
 
     def test_run_command_good(self, mock_requests):
         """Test the run command for each container workflow"""
@@ -587,9 +587,8 @@ def test_run_command_good(self, mock_requests):
         body = {'command': 'echo hi'}
         response = self.client.post(url, body)
         self.assertEqual(response.status_code, 200, response.data)
-        data = App.objects.get(id=app_id)
-        entrypoint, _ = data._get_command_run('echo hi')
-        self.assertEqual(entrypoint, '/bin/bash')
+        app = App.objects.get(id=app_id)
+        self.assertEqual(app._get_entrypoint('web'), ['/bin/bash', '-c'])
 
         # docker image workflow
         build.dockerfile = ''
@@ -599,9 +598,8 @@ def test_run_command_good(self, mock_requests):
         body = {'command': 'echo hi'}
         response = self.client.post(url, body)
         self.assertEqual(response.status_code, 200, response.data)
-        data = App.objects.get(id=app_id)
-        entrypoint, _ = data._get_command_run('echo hi')
-        self.assertEqual(entrypoint, '/bin/bash')
+        app = App.objects.get(id=app_id)
+        self.assertEqual(app._get_entrypoint('cmd'), [])
 
         # procfile workflow
         build.sha = 'somereallylongsha'
@@ -610,9 +608,8 @@ def test_run_command_good(self, mock_requests):
         body = {'command': 'echo hi'}
         response = self.client.post(url, body)
         self.assertEqual(response.status_code, 200, response.data)
-        data = App.objects.get(id=app_id)
-        entrypoint, _ = data._get_command_run('echo hi')
-        self.assertEqual(entrypoint, '/runner/init')
+        app = App.objects.get(id=app_id)
+        self.assertEqual(app._get_entrypoint('web'), ['/runner/init'])
 
     def test_run_not_fail_on_debug(self, mock_requests):
         """
@@ -654,9 +651,8 @@ def test_run_not_fail_on_debug(self, mock_requests):
         body = {'command': 'echo hi'}
         response = self.client.post(url, body)
         self.assertEqual(response.status_code, 200, response.data)
-        data = App.objects.get(id=app_id)
-        entrypoint, _ = data._get_command_run('echo hi')
-        self.assertEqual(entrypoint, '/bin/bash')
+        app = App.objects.get(id=app_id)
+        self.assertEqual(app._get_entrypoint('web'), ['/bin/bash', '-c'])
 
     def test_scaling_does_not_add_run_proctypes_to_structure(self, mock_requests):
         """Test that app info doesn't show transient "run" proctypes."""

rootfs/api/tests/test_pods.py

@@ -524,31 +524,31 @@ def test_command_good(self, mock_requests):
         )
 
         # use `start web` for backwards compatibility with slugrunner
-        self.assertEqual(app._get_command('web'), 'start web')
-        self.assertEqual(app._get_command('worker'), 'start worker')
+        self.assertEqual(app._get_command('web'), ['start', 'web'])
+        self.assertEqual(app._get_command('worker'), ['start', 'worker'])
 
         # switch to docker image app
         build.sha = ''
         build.save()
-        self.assertEqual(app._get_command('web'), "bash -c 'node server.js'")
+        self.assertEqual(app._get_command('web'), ["node server.js"])
 
         # switch to dockerfile app
         build.sha = 'european-swallow'
         build.dockerfile = 'dockerdockerdocker'
         build.save()
-        self.assertEqual(app._get_command('web'), "bash -c 'node server.js'")
-        self.assertEqual(app._get_command('cmd'), '')
+        self.assertEqual(app._get_command('web'), ["node server.js"])
+        self.assertEqual(app._get_command('cmd'), [])
 
         # ensure we can override the cmd process type in a Procfile
         build.procfile['cmd'] = 'node server.js'
         build.save()
-        self.assertEqual(app._get_command('cmd'), "bash -c 'node server.js'")
-        self.assertEqual(app._get_command('worker'), "bash -c 'node worker.js'")
+        self.assertEqual(app._get_command('cmd'), ["node server.js"])
+        self.assertEqual(app._get_command('worker'), ["node worker.js"])
 
         # for backwards compatibility if no Procfile is supplied
         build.procfile = {}
         build.save()
-        self.assertEqual(app._get_command('worker'), 'start worker')
+        self.assertEqual(app._get_command('worker'), ['start', 'worker'])
 
     def test_run_command_good(self, mock_requests):
         """Test the run command for each container workflow"""
@@ -585,9 +585,8 @@ def test_run_command_good(self, mock_requests):
         body = {'command': 'echo hi'}
         response = self.client.post(url, body)
         self.assertEqual(response.status_code, 200, response.data)
-        data = App.objects.get(id=app_id)
-        entrypoint, _ = data._get_command_run('echo hi')
-        self.assertEqual(entrypoint, '/bin/bash')
+        app = App.objects.get(id=app_id)
+        self.assertEqual(app._get_entrypoint('web'), ['/bin/bash', '-c'])
 
         # docker image workflow
         build.dockerfile = ''
@@ -597,9 +596,8 @@ def test_run_command_good(self, mock_requests):
         body = {'command': 'echo hi'}
         response = self.client.post(url, body)
         self.assertEqual(response.status_code, 200, response.data)
-        data = App.objects.get(id=app_id)
-        entrypoint, _ = data._get_command_run('echo hi')
-        self.assertEqual(entrypoint, '/bin/bash')
+        app = App.objects.get(id=app_id)
+        self.assertEqual(app._get_entrypoint('cmd'), [])
 
         # procfile workflow
         build.sha = 'somereallylongsha'
@@ -608,9 +606,8 @@ def test_run_command_good(self, mock_requests):
         body = {'command': 'echo hi'}
         response = self.client.post(url, body)
         self.assertEqual(response.status_code, 200, response.data)
-        data = App.objects.get(id=app_id)
-        entrypoint, _ = data._get_command_run('echo hi')
-        self.assertEqual(entrypoint, '/runner/init')
+        app = App.objects.get(id=app_id)
+        self.assertEqual(app._get_entrypoint('web'), ['/runner/init'])
 
     def test_run_not_fail_on_debug(self, mock_requests):
         """
@@ -652,9 +649,8 @@ def test_run_not_fail_on_debug(self, mock_requests):
         body = {'command': 'echo hi'}
         response = self.client.post(url, body)
         self.assertEqual(response.status_code, 200, response.data)
-        data = App.objects.get(id=app_id)
-        entrypoint, _ = data._get_command_run('echo hi')
-        self.assertEqual(entrypoint, '/bin/bash')
+        app = App.objects.get(id=app_id)
+        self.assertEqual(app._get_entrypoint('web'), ['/bin/bash', '-c'])
 
     def test_scaling_does_not_add_run_proctypes_to_structure(self, mock_requests):
         """Test that app info doesn't show transient "run" proctypes."""