fix(controller): check image access when creating builds

Due to the nature of k8s' docker image cache, users can acccess other
users' images in some situations (when they land on the same k8s node).

That is, once user A has deployed privateregistry.example.com/image-a
(with the help of `deis registry:set password=... username=...`), user B
can just do `deis pull privateregistry.example.com/image-a` and that
will (sometimes) work, since the image is in the k8s cache and thus not
pulled again.

This commit changes things so that on a `deis pull` (aka `deis
build:create`) the controller tries to access the image (with the
configured registry credentials, if any) - and then refuses to deploy it
to k8s if that fails.

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': '[email protected]'})
+
     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"""
 

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': '[email protected]'})

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)