From 26add981765947c9819d43cef29f0b6c3af806a7 Mon Sep 17 00:00:00 2001 From: Richard Wossal Date: Tue, 18 Dec 2018 12:00:53 +0100 Subject: [PATCH 1/2] fix(tests): extend pod name regex --- rootfs/api/tests/test_pods.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rootfs/api/tests/test_pods.py b/rootfs/api/tests/test_pods.py index ecc48ca93..a9902b76e 100644 --- a/rootfs/api/tests/test_pods.py +++ b/rootfs/api/tests/test_pods.py @@ -322,7 +322,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 From 1ce3bc5632b2f5855ba775cc173100d1fa34fda2 Mon Sep 17 00:00:00 2001 From: Richard Wossal Date: Fri, 14 Dec 2018 17:18:46 +0100 Subject: [PATCH 2/2] 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 | 3 +++ 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 +++++++++++ 12 files changed, 81 insertions(+), 6 deletions(-) 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 a9902b76e..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""" 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)