diff --git a/rootfs/api/models/app.py b/rootfs/api/models/app.py index d6ad92697..d4874a710 100644 --- a/rootfs/api/models/app.py +++ b/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 diff --git a/rootfs/api/models/release.py b/rootfs/api/models/release.py index bf755ed21..150ca2e5b 100644 --- a/rootfs/api/models/release.py +++ b/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 diff --git a/rootfs/api/tests/test_app.py b/rootfs/api/tests/test_app.py index 5b4b3bf0f..453bad743 100644 --- a/rootfs/api/tests/test_app.py +++ b/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""" diff --git a/rootfs/api/tests/test_build.py b/rootfs/api/tests/test_build.py index 965741c4e..900cfadc4 100644 --- a/rootfs/api/tests/test_build.py +++ b/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() diff --git a/rootfs/api/tests/test_config.py b/rootfs/api/tests/test_config.py index 00077b1c7..7d88816b5 100644 --- a/rootfs/api/tests/test_config.py +++ b/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""" diff --git a/rootfs/api/tests/test_healthchecks.py b/rootfs/api/tests/test_healthchecks.py index 3e23f6f57..e24fbd0f5 100644 --- a/rootfs/api/tests/test_healthchecks.py +++ b/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""" diff --git a/rootfs/api/tests/test_hooks.py b/rootfs/api/tests/test_hooks.py index 656ac89a9..88686ae6f 100644 --- a/rootfs/api/tests/test_hooks.py +++ b/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""" diff --git a/rootfs/api/tests/test_pods.py b/rootfs/api/tests/test_pods.py index ecc48ca93..f3285d6e2 100644 --- a/rootfs/api/tests/test_pods.py +++ b/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 diff --git a/rootfs/api/tests/test_registry.py b/rootfs/api/tests/test_registry.py index e20dc0aec..928dab6c5 100644 --- a/rootfs/api/tests/test_registry.py +++ b/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'}) diff --git a/rootfs/api/tests/test_release.py b/rootfs/api/tests/test_release.py index cdd5eedb4..6c42b5974 100644 --- a/rootfs/api/tests/test_release.py +++ b/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""" diff --git a/rootfs/registry/__init__.py b/rootfs/registry/__init__.py index 1f247b155..8856561c4 100644 --- a/rootfs/registry/__init__.py +++ b/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 diff --git a/rootfs/registry/dockerclient.py b/rootfs/registry/dockerclient.py index 01aaafa85..78270c8bf 100644 --- a/rootfs/registry/dockerclient.py +++ b/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)