Security improvements (#1227)

* Security improvements

* Update pattern escape

Co-authored-by: Mher Movsisyan <mher@users.noreply.github.com>

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:

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')

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")

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)

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:

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"))