Repository: mamba-org/quetz (python)

Issue:
Quetz should not log sensitive information
One example I just noticed logs the database password if that is part of the DB url, as is common:
https://github.com/mamba-org/quetz/blob/f4c30fc5e1dbe8d7359cac326600e5655eb305b9/quetz/cli.py#LL318C36-L318C36

Pull request:
enhancement: sanitize DB urls before printing
This PR adds a mechanism for sanitzing database URLs before logging them to avoid leaks of sensitive information.

Closes #630

Patch:
diff --git a/docs/source/deploying/configuration.rst b/docs/source/deploying/configuration.rst
index 9cca604..3c5f852 100644
--- a/docs/source/deploying/configuration.rst
+++ b/docs/source/deploying/configuration.rst
@@ -19,10 +19,33 @@ Quetz can be run with SQLlite or PostgreSQL as database backends (PostgreSQL is
 .. code::
 
    [sqlalchemy]
+   # The URL to the database to use.
    database_url = "postgresql://postgres:mysecretpassword@localhost:5432/quetz"
 
+   # Undocumented setting, unknown use
+   database_plugin_path = ""
+
+   # Passed directly to the "echo" argument of sqlalchemy.create_engine
+   echo_sql = false
+
+   # The pool size for sqlalchemy engine connections to postgres DBs
+   # see https://docs.sqlalchemy.org/en/latest/core/pooling.html
+   postgres_pool_size = 100
+
+   # The maximal number of overflow connections beyond the pool size
+   # see https://docs.sqlalchemy.org/en/latest/core/pooling.html
+   postgres_max_overflow = 100
+
 :database_url: URL of the database (may contain user credentials) prefixed with either ``sqlite://`` or ``postgresql://``.
 
+:database_plugin_path: Undocumented setting, unknown use, default: `""`.
+
+:echo_sql: Passed directly to the "echo" argument of sqlalchemy.create_engine, default: `false`.
+
+:postgres_pool_size: The pool size for sqlalchemy engine connections to postgres DBs. See `sqlalchemy docs <https://docs.sqlalchemy.org/en/latest/core/pooling.html>`_. Default: `100`
+
+:postgres_max_overflow: The maximal number of overflow connections beyond the pool size. See `sqlalchemy docs <https://docs.sqlalchemy.org/en/latest/core/pooling.html>`_. Default: `100`
+
 ``github`` section
 ^^^^^^^^^^^^^^^^^^
 
diff --git a/quetz/cli.py b/quetz/cli.py
index a81ab7f..de705b1 100644
--- a/quetz/cli.py
+++ b/quetz/cli.py
@@ -29,7 +29,7 @@ from quetz.config import (
     configure_logger,
     create_config,
 )
-from quetz.database import get_session
+from quetz.database import get_session, sanitize_db_url
 from quetz.db_models import (
     ApiKey,
     Channel,
@@ -315,7 +315,7 @@ def _is_deployment(base_dir: Path):
             if not database_exists(config.sqlalchemy_database_url):
                 logger.error(
                     "Cannot verify that specified database exists. "
-                    + config.sqlalchemy_database_url
+                    + sanitize_db_url(config.sqlalchemy_database_url)
                 )
                 return False
             else:
diff --git a/quetz/config.py b/quetz/config.py
index 562a7e9..c7d989a 100644
--- a/quetz/config.py
+++ b/quetz/config.py
@@ -106,6 +106,8 @@ class Config:
                 ConfigEntry("database_url", str),
                 ConfigEntry("database_plugin_path", str, default="", required=False),
                 ConfigEntry("echo_sql", bool, default=False, required=False),
+                ConfigEntry("postgres_pool_size", int, default=100, required=False),
+                ConfigEntry("postgres_max_overflow", int, default=100, required=False),
             ],
         ),
         ConfigSection(
diff --git a/quetz/database.py b/quetz/database.py
index 6fb50e0..00d53ab 100644
--- a/quetz/database.py
+++ b/quetz/database.py
@@ -1,11 +1,14 @@
 # Copyright 2020 QuantStack
 # Distributed under the terms of the Modified BSD License.
 import logging
+import re
 from contextlib import contextmanager
 from typing import Callable
 
 from sqlalchemy import create_engine, event
 from sqlalchemy.engine import Engine
+from sqlalchemy.engine.url import make_url
+from sqlalchemy.exc import ArgumentError
 from sqlalchemy.orm import sessionmaker
 from sqlalchemy.orm.session import Session
 from sqlalchemy.pool import StaticPool
@@ -26,7 +29,7 @@ def set_metrics(*args):
     DATABASE_CONNECTIONS_USED.set(checked_out)
 
 
-def get_engine(db_url, echo: bool = False, reuse_engine=True, **kwargs) -> Engine:
+def get_engine(db_url, reuse_engine=True, **kwargs) -> Engine:
     if db_url.startswith('sqlite'):
         kwargs.setdefault('connect_args', {'check_same_thread': False})
 
@@ -38,15 +41,12 @@ def get_engine(db_url, echo: bool = False, reuse_engine=True, **kwargs) -> Engin
     global engine
 
     if not engine or not reuse_engine:
-        # TODO make configurable!
         if db_url.startswith('postgres'):
-            engine = create_engine(
-                db_url, echo=echo, pool_size=200, max_overflow=100, **kwargs
-            )
+            engine = create_engine(db_url, **kwargs)
             for event_name in ['connect', 'close', 'checkin', 'checkout']:
                 event.listen(engine, event_name, set_metrics)
         else:
-            engine = create_engine(db_url, echo=echo, **kwargs)
+            engine = create_engine(db_url, **kwargs)
 
         def on_connect(dbapi_conn, conn_record):
             logger.debug("connection opened: %s", engine.pool.status())
@@ -64,22 +64,55 @@ def get_session_maker(engine) -> Callable[[], Session]:
     return sessionmaker(autocommit=False, autoflush=True, bind=engine)
 
 
-def get_session(db_url: str, echo: bool = False, **kwargs) -> Session:
+def get_session(db_url: str, **kwargs) -> Session:
     """Get a database session.
 
     Important note: this function is mocked during tests!
 
     """
-    return get_session_maker(get_engine(db_url, echo, **kwargs))()
+    return get_session_maker(get_engine(db_url, **kwargs))()
 
 
 @contextmanager
 def get_db_manager():
     config = Config()
-    database_url = config.sqlalchemy_database_url
-    db = get_session(database_url, echo=config.sqlalchemy_echo_sql)
+    db = get_session(
+        db_url=config.sqlalchemy_database_url,
+        echo=config.sqlalchemy_echo_sql,
+        pool_size=config.sqlalchemy_postgres_pool_size,
+        max_overflow=config.sqlalchemy_postgres_max_overflow,
+    )
 
     try:
         yield db
     finally:
         db.close()
+
+
+def sanitize_db_url(db_url: str) -> str:
+    """
+    Sanitizes the DB url so it is safe to print.
+
+    If the URL is parseable with sqlalchemy's make_url,
+    it is parsed and the password is replaced.
+    If not, we try to replace everything between ":" and "@",
+    if those characters are present.
+
+    If neither method succeeds, we give up and return the
+    full initial URL.
+    """
+
+    # Attempt 1: Actual parsing, this is ideal but may fail
+    try:
+        parsed_url = make_url(db_url)
+        if parsed_url.password:
+            return db_url.replace(parsed_url.password, "***")
+    except ArgumentError:
+        pass
+
+    # Attempt 2: Poor man's parsing: Just replacing everything between ":" and "@"
+    if ":" in db_url and "@" in db_url:
+        return re.sub(":[^:@]*@", ":***@", db_url)
+
+    # Fallback: We don't understand the URL format, so we do nothing
+    return db_url


Test patch:
diff --git a/quetz/tests/test_database.py b/quetz/tests/test_database.py
new file mode 100644
index 0000000..3c7be92
--- /dev/null
+++ b/quetz/tests/test_database.py
@@ -0,0 +1,22 @@
+import pytest
+
+from quetz.database import sanitize_db_url
+
+
+@pytest.mark.parametrize(
+    "input_url,expected_output_url",
+    (
+        (
+            "sqlite:///./quetz.sqlite",
+            "sqlite:///./quetz.sqlite",
+        ),  # No password, no effect
+        (
+            "postgresql+psycopg2://postgres_user:postgres_password@localhost:5432/postgres",  # noqa: E501
+            "postgresql+psycopg2://postgres_user:***@localhost:5432/postgres",
+        ),
+        ("A:B@C:1111/DB", "A:***@C:1111/DB"),
+        ("THISISNOTAURL", "THISISNOTAURL"),
+    ),
+)
+def test_sanitize_db_url(input_url, expected_output_url):
+    assert sanitize_db_url(input_url) == expected_output_url