Merge pull request #88 from pngmbh/upstream-image-access-check

check image access on `deis pull`

Author: Cryptophobia

rootfs/api/models/app.py

@@ -520,6 +520,9 @@ def deploy(self, release, force_deploy=False, rollback_on_failure=True):  # noqa
         image = settings.SLUGRUNNER_IMAGE if release.build.type == 'buildpack' else release.image
 
         try:
+            # check access to the image, so users can't exploit the k8s image cache
+            # to gain access to other users' images
+            release.check_image_access()
             # create the application config in k8s (secret in this case) for all deploy objects
             self.set_application_config(release)
             # only buildpack apps need access to object storage

rootfs/api/models/release.py

@@ -3,7 +3,7 @@
 from django.conf import settings
 from django.db import models
 
-from registry import publish_release, get_port as docker_get_port, RegistryException
+from registry import publish_release, get_port as docker_get_port, check_access as docker_check_access, RegistryException # noqa
 from api.utils import dict_diff
 from api.models import UuidAuditedModel
 from api.exceptions import DeisException, AlreadyExists
@@ -136,6 +136,13 @@ def publish(self):
         deis_registry = bool(self.build.source_based)
         publish_release(source_image, self.image, deis_registry, self.get_registry_auth())
 
+    def check_image_access(self):
+        try:
+            creds = self.get_registry_auth()
+            docker_check_access(self.image, creds)
+        except Exception as e:
+            raise DeisException(str(e)) from e
+
     def get_port(self):
         """
         Get application port for a given release. If pulling from private registry

rootfs/api/tests/test_app.py

@@ -35,6 +35,7 @@ def _mock_run(*args, **kwargs):
 @requests_mock.Mocker(real_http=True, adapter=adapter)
 @mock.patch('api.models.release.publish_release', lambda *args: None)
 @mock.patch('api.models.release.docker_get_port', mock_port)
+@mock.patch('api.models.release.docker_check_access', lambda *args: None)
 class AppTest(DeisTestCase):
     """Tests creation of applications"""
 

rootfs/api/tests/test_build.py

@@ -23,6 +23,7 @@
 @requests_mock.Mocker(real_http=True, adapter=adapter)
 @mock.patch('api.models.release.publish_release', lambda *args: None)
 @mock.patch('api.models.release.docker_get_port', mock_port)
+@mock.patch('api.models.release.docker_check_access', lambda *args: None)
 class BuildTest(DeisTransactionTestCase):
 
     """Tests build notification from build system"""
@@ -591,6 +592,44 @@ def test_build_image_in_registry_with_auth(self, mock_requests):
         response = self.client.post(url, body)
         self.assertEqual(response.status_code, 201, response.data)
 
+    def test_build_image_no_registry_password(self, mock_requests):
+        """build with image from private registry, but no password given"""
+        app_id = self.create_app()
+
+        # post an image as a build
+        with mock.patch('api.models.release.docker_check_access') as mock_check_access:
+            mock_check_access.side_effect = Exception('no no no')  # let the image access fail
+            url = "/v2/apps/{app_id}/builds".format(**locals())
+            image = 'autotest/example'
+            response = self.client.post(url, {'image': image})
+            self.assertEqual(response.status_code, 400, response.data)
+
+    def test_build_image_wrong_registry_password(self, mock_requests):
+        """build with image from private registry, but wrong password given"""
+        app_id = self.create_app()
+
+        # post an image as a build using registry hostname
+        url = "/v2/apps/{app_id}/builds".format(**locals())
+        image = 'autotest/example'
+        response = self.client.post(url, {'image': image})
+        self.assertEqual(response.status_code, 201, response.data)
+
+        # add the required PORT information
+        url = '/v2/apps/{app_id}/config'.format(**locals())
+        body = {'values': json.dumps({'PORT': '80'})}
+        response = self.client.post(url, body)
+        self.assertEqual(response.status_code, 201, response.data)
+
+        # set some registry information
+        with mock.patch('api.models.release.docker_check_access') as mock_check_access:
+            mock_check_access.side_effect = Exception('no no no')  # let the image access fail
+            url = '/v2/apps/{app_id}/config'.format(**locals())
+            body = {'registry': json.dumps({'username': 'bob', 'password': 'zoomzoom'})}
+            response = self.client.post(url, body)
+            self.assertEqual(response.status_code, 400, response.data)
+            mock_check_access.assert_called_with(
+                image, {'username': 'bob', 'password': 'zoomzoom', 'email': 'autotest@deis.io'})
+
     def test_build_image_in_registry_with_auth_no_port(self, mock_requests):
         """add authentication to the build but with no PORT config"""
         app_id = self.create_app()

rootfs/api/tests/test_config.py

@@ -21,6 +21,7 @@
 @requests_mock.Mocker(real_http=True, adapter=adapter)
 @mock.patch('api.models.release.publish_release', lambda *args: None)
 @mock.patch('api.models.release.docker_get_port', mock_port)
+@mock.patch('api.models.release.docker_check_access', lambda *args: None)
 class ConfigTest(DeisTransactionTestCase):
     """Tests setting and updating config values"""
 

rootfs/api/tests/test_healthchecks.py

@@ -13,6 +13,7 @@
 @requests_mock.Mocker(real_http=True, adapter=adapter)
 @mock.patch('api.models.release.publish_release', lambda *args: None)
 @mock.patch('api.models.release.docker_get_port', mock_port)
+@mock.patch('api.models.release.docker_check_access', lambda *args: None)
 class TestHealthchecks(DeisTransactionTestCase):
     """Tests setting and updating config values"""
 

rootfs/api/tests/test_hooks.py

@@ -50,6 +50,7 @@
 @requests_mock.Mocker(real_http=True, adapter=adapter)
 @mock.patch('api.models.release.publish_release', lambda *args: None)
 @mock.patch('api.models.release.docker_get_port', mock_port)
+@mock.patch('api.models.release.docker_check_access', lambda *args: None)
 class HookTest(DeisTransactionTestCase):
 
     """Tests API hooks used to trigger actions from external components"""

rootfs/api/tests/test_pods.py

@@ -21,6 +21,7 @@
 @requests_mock.Mocker(real_http=True, adapter=adapter)
 @mock.patch('api.models.release.publish_release', lambda *args: None)
 @mock.patch('api.models.release.docker_get_port', mock_port)
+@mock.patch('api.models.release.docker_check_access', lambda *args: None)
 class PodTest(DeisTransactionTestCase):
     """Tests creation of pods on nodes"""
 
@@ -322,7 +323,7 @@ def test_container_str(self, mock_requests):
             self.assertIn(pod['type'], ['web', 'worker'])
             self.assertEqual(pod['release'], 'v2')
             # pod name is auto generated so use regex
-            self.assertRegex(pod['name'], app_id + '-(worker|web)-[0-9]{8,10}-[a-z0-9]{5}')
+            self.assertRegex(pod['name'], app_id + '-(worker|web)-[0-9]{7,10}-[a-z0-9]{5}')
 
     def test_pod_command_format(self, mock_requests):
         # regression test for https://github.com/deisthree/deis/pull/1285

rootfs/api/tests/test_registry.py

@@ -1,5 +1,6 @@
 import json
 import requests_mock
+from unittest import mock
 
 from django.core.cache import cache
 from django.contrib.auth.models import User
@@ -136,7 +137,11 @@ def test_registry_deploy(self, mock_requests):
         self.assertEqual(response.data['registry']['password'], 's3cur3pw1')
 
         # post a new build
-        url = "/v2/apps/{app_id}/builds".format(**locals())
-        body = {'image': 'autotest/example'}
-        response = self.client.post(url, body)
-        self.assertEqual(response.status_code, 201, response.data)
+        with mock.patch('api.models.release.docker_check_access') as mock_check_access:
+            url = "/v2/apps/{app_id}/builds".format(**locals())
+            body = {'image': 'autotest/example'}
+            response = self.client.post(url, body)
+            self.assertEqual(response.status_code, 201, response.data)
+            mock_check_access.assert_called_with(
+                'autotest/example',
+                {'password': 's3cur3pw1', 'username': 'bob', 'email': 'autotest@deis.io'})

rootfs/api/tests/test_release.py

@@ -21,6 +21,7 @@
 @requests_mock.Mocker(real_http=True, adapter=adapter)
 @mock.patch('api.models.release.publish_release', lambda *args: None)
 @mock.patch('api.models.release.docker_get_port', mock_port)
+@mock.patch('api.models.release.docker_check_access', lambda *args: None)
 class ReleaseTest(DeisTransactionTestCase):
 
     """Tests push notification from build system"""

rootfs/registry/__init__.py

@@ -1 +1 @@
-from .dockerclient import publish_release, get_port, RegistryException  # noqa
+from .dockerclient import publish_release, get_port, check_access, RegistryException  # noqa

rootfs/registry/dockerclient.py

@@ -154,6 +154,17 @@ def inspect_image(self, target):
         # inspect the image
         return self.client.inspect_image(target)
 
+    def check_access(self, target, creds=None):
+        """
+        Check access to the docker image, with the given creds (if any).
+        Due to the k8s docker image cache, we can't rely on k8s to do this
+        check - see https://github.com/teamhephy/workflow/issues/78
+        """
+        name, _ = docker.utils.parse_repository_tag(target)
+        self.login(name, creds)
+        self.inspect_image(target)
+        # no exception == success
+
 
 def check_blacklist(repo):
     """Check a Docker repository name for collision with deis/* components."""
@@ -197,3 +208,7 @@ def publish_release(source, target, deis_registry, creds=None):
 
 def get_port(target, deis_registry, creds=None):
     return DockerClient().get_port(target, deis_registry, creds)
+
+
+def check_access(target, creds=None):
+    return DockerClient().check_access(target, creds)