From 964e64abc207cdb5ff8f66c3630b7fb5a30e2605 Mon Sep 17 00:00:00 2001 From: Cryptophobia Date: Thu, 13 Jun 2019 23:02:22 -0400 Subject: [PATCH 1/5] fix(controller): Revert check image access when creating builds This reverts commit 1ce3bc5632b2f5855ba775cc173100d1fa34fda2. --- rootfs/api/models/release.py | 9 +------ rootfs/api/tests/test_app.py | 1 - rootfs/api/tests/test_build.py | 39 --------------------------- rootfs/api/tests/test_config.py | 1 - rootfs/api/tests/test_healthchecks.py | 1 - rootfs/api/tests/test_hooks.py | 1 - rootfs/api/tests/test_pods.py | 1 - rootfs/api/tests/test_registry.py | 13 +++------ rootfs/api/tests/test_release.py | 1 - rootfs/registry/__init__.py | 2 +- rootfs/registry/dockerclient.py | 15 ----------- 11 files changed, 6 insertions(+), 78 deletions(-) diff --git a/rootfs/api/models/release.py b/rootfs/api/models/release.py index 150ca2e5b..bf755ed21 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, check_access as docker_check_access, RegistryException # noqa +from registry import publish_release, get_port as docker_get_port, RegistryException from api.utils import dict_diff from api.models import UuidAuditedModel from api.exceptions import DeisException, AlreadyExists @@ -136,13 +136,6 @@ 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 453bad743..5b4b3bf0f 100644 --- a/rootfs/api/tests/test_app.py +++ b/rootfs/api/tests/test_app.py @@ -35,7 +35,6 @@ 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 900cfadc4..965741c4e 100644 --- a/rootfs/api/tests/test_build.py +++ b/rootfs/api/tests/test_build.py @@ -23,7 +23,6 @@ @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""" @@ -592,44 +591,6 @@ 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 7d88816b5..00077b1c7 100644 --- a/rootfs/api/tests/test_config.py +++ b/rootfs/api/tests/test_config.py @@ -21,7 +21,6 @@ @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 e24fbd0f5..3e23f6f57 100644 --- a/rootfs/api/tests/test_healthchecks.py +++ b/rootfs/api/tests/test_healthchecks.py @@ -13,7 +13,6 @@ @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 88686ae6f..656ac89a9 100644 --- a/rootfs/api/tests/test_hooks.py +++ b/rootfs/api/tests/test_hooks.py @@ -50,7 +50,6 @@ @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 1943d6045..ec3a70ea9 100644 --- a/rootfs/api/tests/test_pods.py +++ b/rootfs/api/tests/test_pods.py @@ -21,7 +21,6 @@ @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""" diff --git a/rootfs/api/tests/test_registry.py b/rootfs/api/tests/test_registry.py index 928dab6c5..e20dc0aec 100644 --- a/rootfs/api/tests/test_registry.py +++ b/rootfs/api/tests/test_registry.py @@ -1,6 +1,5 @@ import json import requests_mock -from unittest import mock from django.core.cache import cache from django.contrib.auth.models import User @@ -137,11 +136,7 @@ def test_registry_deploy(self, mock_requests): self.assertEqual(response.data['registry']['password'], 's3cur3pw1') # post a new build - 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'}) + 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) diff --git a/rootfs/api/tests/test_release.py b/rootfs/api/tests/test_release.py index 6c42b5974..cdd5eedb4 100644 --- a/rootfs/api/tests/test_release.py +++ b/rootfs/api/tests/test_release.py @@ -21,7 +21,6 @@ @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 8856561c4..1f247b155 100644 --- a/rootfs/registry/__init__.py +++ b/rootfs/registry/__init__.py @@ -1 +1 @@ -from .dockerclient import publish_release, get_port, check_access, RegistryException # noqa +from .dockerclient import publish_release, get_port, RegistryException # noqa diff --git a/rootfs/registry/dockerclient.py b/rootfs/registry/dockerclient.py index 1bb63bcf7..128205c14 100644 --- a/rootfs/registry/dockerclient.py +++ b/rootfs/registry/dockerclient.py @@ -154,17 +154,6 @@ 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.""" @@ -208,7 +197,3 @@ 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) From 1114ea5e628e9f15e7e3e58dc79692fd29182772 Mon Sep 17 00:00:00 2001 From: Cryptophobia Date: Thu, 13 Jun 2019 23:06:37 -0400 Subject: [PATCH 2/5] fix(controller): Revert check_image_access only when using docker This reverts commit b3a741da4ea78c8c4cebfe71915c7451743af7b0. --- rootfs/api/models/app.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/rootfs/api/models/app.py b/rootfs/api/models/app.py index c2ef88140..d4874a710 100644 --- a/rootfs/api/models/app.py +++ b/rootfs/api/models/app.py @@ -520,15 +520,14 @@ 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 - # only docker apps need check access to the image, so users can't exploit the k8s - # image cache to gain access to other users' images if release.build.type == 'buildpack': self.create_object_store_secret() - else: - release.check_image_access() # gather all proc types to be deployed tasks = [ From 5bb969a0476681960ad56c1ddaa44cf167960390 Mon Sep 17 00:00:00 2001 From: Cryptophobia Date: Thu, 13 Jun 2019 23:37:28 -0400 Subject: [PATCH 3/5] Revert "fix(controller): check image access when creating builds" This reverts commit 1ce3bc5632b2f5855ba775cc173100d1fa34fda2. --- rootfs/api/models/app.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/rootfs/api/models/app.py b/rootfs/api/models/app.py index d4874a710..d6ad92697 100644 --- a/rootfs/api/models/app.py +++ b/rootfs/api/models/app.py @@ -520,9 +520,6 @@ 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 From 598d1e8811b8b74c3e81d6c62a102ecbd5a01036 Mon Sep 17 00:00:00 2001 From: Cryptophobia Date: Thu, 13 Jun 2019 23:38:39 -0400 Subject: [PATCH 4/5] Revert "fix(controller): check_image_access only when using docker" This reverts commit b3a741da4ea78c8c4cebfe71915c7451743af7b0. --- rootfs/api/models/app.py | 3 +++ 1 file changed, 3 insertions(+) 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 From 13dd3db61933ab1c7b4ae9abd97ba6a8761179b0 Mon Sep 17 00:00:00 2001 From: Cryptophobia Date: Thu, 13 Jun 2019 23:58:24 -0400 Subject: [PATCH 5/5] fix(controller): revert release.check_image_access for now --- rootfs/api/models/app.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/rootfs/api/models/app.py b/rootfs/api/models/app.py index d4874a710..d6ad92697 100644 --- a/rootfs/api/models/app.py +++ b/rootfs/api/models/app.py @@ -520,9 +520,6 @@ 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