From d91fbbf338044c0668cf053ccf1c5a33c6454f0e Mon Sep 17 00:00:00 2001 From: Doug Goldstein Date: Thu, 7 Jul 2022 15:59:38 -0500 Subject: [PATCH 1/3] use GitHub Actions to build Docker container (#1181) Start using GitHub Actions to build the Docker container for this project. Builds all tags and master as the latest container tag. Builds for amd64 and arm64. --- .github/workflows/docker.yml | 45 ++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) create mode 100644 .github/workflows/docker.yml diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml new file mode 100644 index 000000000..424b7a862 --- /dev/null +++ b/.github/workflows/docker.yml @@ -0,0 +1,45 @@ +name: docker + +on: + push: + branches: + - master + tags: + - '*' + pull_request: + branches: + - master + +jobs: + docker: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + + - uses: docker/setup-qemu-action@v1 + + - uses: docker/setup-buildx-action@v1 + + - id: meta + uses: docker/metadata-action@v3 + with: + images: mher/flower + tags: | + type=ref,event=branch + type=ref,event=pr + type=semver,pattern={{version}} + type=semver,pattern={{major}}.{{minor}} + + - uses: docker/login-action@v1 + if: github.event_name != 'pull_request' + with: + username: ${{ secrets.DOCKERHUB_USERNAME }} + password: ${{ secrets.DOCKERHUB_TOKEN }} + + - uses: docker/build-push-action@v2 + with: + context: . + platforms: linux/amd64,linux/arm64 + push: ${{ github.event_name != 'pull_request' }} + tags: ${{ steps.meta.outputs.tags }} + labels: ${{ steps.meta.outputs.labels }} From cd6a3c9282868acc70c2024752a57d5a49e4de41 Mon Sep 17 00:00:00 2001 From: mher <691049+mher@users.noreply.github.com> Date: Fri, 15 Jul 2022 13:23:58 -0400 Subject: [PATCH 2/3] Security improvements (#1227) * Security improvements * Update pattern escape Co-authored-by: Mher Movsisyan --- docs/config.rst | 2 ++ flower/command.py | 7 ++++++ flower/options.py | 3 ++- flower/views/__init__.py | 11 +++++----- flower/views/auth.py | 28 ++++++++++++++++++++---- tests/unit/views/test_auth.py | 40 ++++++++++++++++++++++++++++++++++- 6 files changed, 80 insertions(+), 11 deletions(-) diff --git a/docs/config.rst b/docs/config.rst index 61e8e77ac..ca919a498 100644 --- a/docs/config.rst +++ b/docs/config.rst @@ -65,6 +65,8 @@ auth ~~~~ Enables authentication. `auth` is a regexp of emails to grant access. +For security reasons `auth` only supports a basic regex syntax: single email (`user@example.com`), +wildcard (`.*@example.com`) or list of emails separated by pipes (`one@example.com|two@example.com`). For more info see :ref:`authentication`. .. _auto_refresh: diff --git a/flower/command.py b/flower/command.py index 6a64010cd..cb2e552a8 100644 --- a/flower/command.py +++ b/flower/command.py @@ -18,6 +18,7 @@ from .urls import settings from .utils import abs_path, prepend_url from .options import DEFAULT_CONFIG_FILE, default_options +from .views.auth import validate_auth_option logger = logging.getLogger(__name__) ENV_VAR_PREFIX = 'FLOWER_' @@ -135,6 +136,10 @@ def extract_settings(): if options.ca_certs: settings['ssl_options']['ca_certs'] = abs_path(options.ca_certs) + if options.auth and not validate_auth_option(options.auth): + logger.error("Invalid '--auth' option: %s", options.auth) + sys.exit(1) + def is_flower_option(arg): name, _, _ = arg.lstrip('-').partition("=") @@ -168,3 +173,5 @@ def print_banner(app, ssl): pformat(sorted(app.tasks.keys())) ) logger.debug('Settings: %s', pformat(settings)) + if not (options.basic_auth or options.auth): + logger.warning('Running without authentication') diff --git a/flower/options.py b/flower/options.py index f69ffecb0..39e29c30c 100644 --- a/flower/options.py +++ b/flower/options.py @@ -1,4 +1,5 @@ import types +from secrets import token_urlsafe from prometheus_client import Histogram from tornado.options import define @@ -52,7 +53,7 @@ help="refresh dashboards", type=bool) define("purge_offline_workers", default=None, type=int, help="time (in seconds) after which offline workers are purged from dashboard") -define("cookie_secret", type=str, default=None, +define("cookie_secret", type=str, default=token_urlsafe(64), help="secure cookie secret") define("conf", default=DEFAULT_CONFIG_FILE, help="configuration file") diff --git a/flower/views/__init__.py b/flower/views/__init__.py index ffef204b3..50946d25e 100644 --- a/flower/views/__init__.py +++ b/flower/views/__init__.py @@ -16,11 +16,12 @@ class BaseHandler(tornado.web.RequestHandler): def set_default_headers(self): - self.set_header("Access-Control-Allow-Origin", "*") - self.set_header("Access-Control-Allow-Headers", - "x-requested-with,access-control-allow-origin,authorization,content-type") - self.set_header('Access-Control-Allow-Methods', - ' PUT, DELETE, OPTIONS, POST, GET, PATCH') + if not (self.application.options.basic_auth or self.application.options.auth): + self.set_header("Access-Control-Allow-Origin", "*") + self.set_header("Access-Control-Allow-Headers", + "x-requested-with,access-control-allow-origin,authorization,content-type") + self.set_header('Access-Control-Allow-Methods', + ' PUT, DELETE, OPTIONS, POST, GET, PATCH') def options(self, *args, **kwargs): self.set_status(204) diff --git a/flower/views/auth.py b/flower/views/auth.py index 66ea43ac9..1256ab490 100644 --- a/flower/views/auth.py +++ b/flower/views/auth.py @@ -14,6 +14,26 @@ from ..views import BaseHandler +def authenticate(pattern, email): + if '|' in pattern: + return email in pattern.split('|') + elif '*' in pattern: + pattern = re.escape(pattern).replace('\.\*', "[A-Za-z0-9!#$%&'*+/=?^_`{|}~.\-]*") + return re.fullmatch(pattern, email) + else: + return pattern == email + + +def validate_auth_option(pattern): + if pattern.count('*') > 1: + return False + if '*' in pattern and '|' in pattern: + return False + if '*' in pattern.rsplit('@', 1)[-1]: + return False + return True + + class GoogleAuth2LoginHandler(BaseHandler, tornado.auth.GoogleOAuth2Mixin): _OAUTH_SETTINGS_KEY = 'oauth' @@ -49,7 +69,7 @@ def _on_auth(self, user): raise tornado.web.HTTPError(403, 'Google auth failed: %s' % e) email = json.loads(response.body.decode('utf-8'))['email'] - if not re.match(self.application.options.auth, email): + if not authenticate(self.application.options.auth, email): message = ( "Access denied to '{email}'. Please use another account or " "ask your admin to add your email to flower --auth." @@ -129,7 +149,7 @@ def _on_auth(self, user): 'User-agent': 'Tornado auth'}) emails = [email['email'].lower() for email in json.loads(response.body.decode('utf-8')) - if email['verified'] and re.match(self.application.options.auth, email['email'])] + if email['verified'] and authenticate(self.application.options.auth, email['email'])] if not emails: message = ( @@ -209,7 +229,7 @@ def _on_auth(self, user): raise tornado.web.HTTPError(403, 'GitLab auth failed: %s' % e) user_email = json.loads(response.body.decode('utf-8'))['email'] - email_allowed = re.match(self.application.options.auth, user_email) + email_allowed = authenticate(self.application.options.auth, user_email) # Check user's groups against list of allowed groups matching_groups = [] @@ -323,7 +343,7 @@ def _on_auth(self, access_token_response): email = (decoded_body.get('email') or '').strip() email_verified = ( decoded_body.get('email_verified') and - re.match(self.application.options.auth, email) + authenticate(self.application.options.auth, email) ) if not email_verified: diff --git a/tests/unit/views/test_auth.py b/tests/unit/views/test_auth.py index 92420ad01..844e05887 100644 --- a/tests/unit/views/test_auth.py +++ b/tests/unit/views/test_auth.py @@ -1,5 +1,5 @@ from tests.unit import AsyncHTTPTestCase - +from flower.views.auth import authenticate, validate_auth_option class BasicAuthTests(AsyncHTTPTestCase): def test_with_single_creds(self): @@ -21,3 +21,41 @@ def test_with_multiple_creds(self): self.assertEqual(200, r.code) r = self.fetch('/', auth_username='user1', auth_password='pswd2') self.assertEqual(401, r.code) + + +class AuthTests(AsyncHTTPTestCase): + def test_validate_auth_option(self): + self.assertTrue(validate_auth_option("mail@example.com")) + self.assertTrue(validate_auth_option(".*@example.com")) + self.assertTrue(validate_auth_option("one.*@example.com")) + self.assertTrue(validate_auth_option("one.*two@example.com")) + self.assertFalse(validate_auth_option(".*@.*example.com")) + self.assertFalse(validate_auth_option("one@domain1.com|.*@domain2.com")) + self.assertTrue(validate_auth_option("one@example.com|two@example.com")) + self.assertFalse(validate_auth_option("mail@.*example.com")) + self.assertFalse(validate_auth_option(".*example.com")) + + def test_authenticate_single_email(self): + self.assertTrue(authenticate("mail@example.com", "mail@example.com")) + self.assertFalse(authenticate("mail@example.com", "foo@example.com")) + self.assertFalse(authenticate("mail@example.com", "long.mail@example.com")) + self.assertFalse(authenticate("mail@example.com", "")) + self.assertFalse(authenticate("me@gmail.com", "me@gmail.com.attacker.com")) + self.assertFalse(authenticate("me@gmail.com", "*")) + + def test_authenticate_email_list(self): + self.assertTrue(authenticate("one@example.com|two@example.net", "one@example.com")) + self.assertTrue(authenticate("one@example.com|two@example.net", "two@example.net")) + self.assertFalse(authenticate("one@example.com|two@example.net", "two@example.com")) + self.assertFalse(authenticate("one@example.com|two@example.net", "one@example.net")) + self.assertFalse(authenticate("one@example.com|two@example.net", "mail@gmail.com")) + self.assertFalse(authenticate("one@example.com|two@example.net", "")) + self.assertFalse(authenticate("one@example.com|two@example.net", "*")) + + def test_authenticate_wildcard_email(self): + self.assertTrue(authenticate(".*@example.com", "one@example.com")) + self.assertTrue(authenticate("one.*@example.com", "one@example.com")) + self.assertTrue(authenticate("one.*@example.com", "one.two@example.com")) + self.assertFalse(authenticate(".*@example.com", "attacker@example.com.attacker.com")) + self.assertFalse(authenticate(".*@corp.example.com", "attacker@corpZexample.com")) + self.assertFalse(authenticate(".*@corp\.example\.com", "attacker@corpZexample.com")) From 30b5a975b763843bc7ff27d5515f33f97f986bce Mon Sep 17 00:00:00 2001 From: Steve Kowalik Date: Sat, 16 Jul 2022 03:24:23 +1000 Subject: [PATCH 3/3] Remove use of mock (#1228) As of Python 3.3, mock is now included in the standard library, and since the lowest version of Python supported is below that, we can remove the dependency. Switch to using unittest.mock everywhere, cleaning up some imports as a side benefit. --- requirements/test.txt | 2 +- tests/unit/__init__.py | 4 ++-- tests/unit/api/test_control.py | 5 ++--- tests/unit/api/test_tasks.py | 3 +-- tests/unit/api/test_workers.py | 3 +-- tests/unit/test_command.py | 3 +-- tests/unit/utils/test_broker.py | 3 +-- tests/unit/views/test_dashboard.py | 7 +------ 8 files changed, 10 insertions(+), 20 deletions(-) diff --git a/requirements/test.txt b/requirements/test.txt index 932a8957f..8b1378917 100644 --- a/requirements/test.txt +++ b/requirements/test.txt @@ -1 +1 @@ -mock + diff --git a/tests/unit/__init__.py b/tests/unit/__init__.py index 07bcd17ab..ef66fe098 100644 --- a/tests/unit/__init__.py +++ b/tests/unit/__init__.py @@ -1,3 +1,4 @@ +from unittest.mock import patch from urllib.parse import urlencode import tornado.testing @@ -6,7 +7,6 @@ from tornado.concurrent import Future import celery -import mock from flower.app import Flower from flower.urls import handlers @@ -37,4 +37,4 @@ def post(self, url, **kwargs): return self.fetch(url, method='POST', **kwargs) def mock_option(self, name, value): - return mock.patch.object(options.mockable(), name, value) + return patch.object(options.mockable(), name, value) diff --git a/tests/unit/api/test_control.py b/tests/unit/api/test_control.py index a83d9ad6a..aa7bac0ec 100644 --- a/tests/unit/api/test_control.py +++ b/tests/unit/api/test_control.py @@ -1,5 +1,4 @@ -from mock import MagicMock -import mock +from unittest.mock import MagicMock, patch from flower.api.control import ControlHandler from tests.unit import AsyncHTTPTestCase @@ -174,7 +173,7 @@ def test_terminate_signal(self): class ControlAuthTests(WorkerControlTests): def test_auth(self): - with mock.patch.object(options.mockable(), 'basic_auth', ['user1:password1']): + with patch.object(options.mockable(), 'basic_auth', ['user1:password1']): app = self._app.capp app.control.broadcast = MagicMock() r = self.post('/api/worker/shutdown/test', body={}) diff --git a/tests/unit/api/test_tasks.py b/tests/unit/api/test_tasks.py index c38278dc7..dcee141da 100644 --- a/tests/unit/api/test_tasks.py +++ b/tests/unit/api/test_tasks.py @@ -1,4 +1,4 @@ -from mock import Mock, patch +from unittest.mock import Mock, patch, PropertyMock from datetime import datetime, timedelta from celery.result import AsyncResult @@ -16,7 +16,6 @@ class ApplyTests(AsyncHTTPTestCase): def test_apply(self): - from mock import patch, PropertyMock import json result = 'result' diff --git a/tests/unit/api/test_workers.py b/tests/unit/api/test_workers.py index 6b62c13ab..831fa0cfd 100644 --- a/tests/unit/api/test_workers.py +++ b/tests/unit/api/test_workers.py @@ -1,6 +1,5 @@ import json - -import mock +from unittest import mock from flower.api.control import ControlHandler from flower.inspector import Inspector diff --git a/tests/unit/test_command.py b/tests/unit/test_command.py index 4e90e63ae..2e7b98c7b 100644 --- a/tests/unit/test_command.py +++ b/tests/unit/test_command.py @@ -5,7 +5,6 @@ import subprocess from unittest.mock import Mock, patch -import mock from prometheus_client import Histogram from flower.command import apply_options, warn_about_celery_args_used_in_flower_command, apply_env_options @@ -49,7 +48,7 @@ def test_autodiscovery(self): - create flower command """ celery_app = self._get_celery_app() - with mock.patch.object(celery_app, '_autodiscover_tasks') as autodiscover: + with patch.object(celery_app, '_autodiscover_tasks') as autodiscover: celery_app.autodiscover_tasks() self.get_app(capp=celery_app) diff --git a/tests/unit/utils/test_broker.py b/tests/unit/utils/test_broker.py index f495af46b..53990c7a2 100644 --- a/tests/unit/utils/test_broker.py +++ b/tests/unit/utils/test_broker.py @@ -1,6 +1,5 @@ import unittest - -from mock import MagicMock +from unittest.mock import MagicMock from flower.utils import broker from flower.utils.broker import RabbitMQ, Redis, RedisBase, RedisSocket, Broker, RedisSentinel diff --git a/tests/unit/views/test_dashboard.py b/tests/unit/views/test_dashboard.py index 571257daa..de673c2c8 100644 --- a/tests/unit/views/test_dashboard.py +++ b/tests/unit/views/test_dashboard.py @@ -1,17 +1,12 @@ import time import unittest +from unittest.mock import patch import sys from tests.unit import AsyncHTTPTestCase from tests.unit.utils import task_succeeded_events, task_failed_events from tests.unit.utils import HtmlTableParser -if sys.version_info >= (2, 7): - from mock import patch -else: - from unittest.mock import patch - - from celery.events import Event from celery.utils import uuid