This is an automated email from the ASF dual-hosted git repository. rusackas pushed a commit to branch fix/database-connection-identity-check in repository https://gitbox.apache.org/repos/asf/superset.git
commit 87a528051229e7751af99c726d595efab3a62aa6 Author: Evan Rusackas <[email protected]> AuthorDate: Mon Sep 7 13:23:14 2026 -0700 fix(database): don't reattach stored connection secrets when the effective destination changes test_connection, validate_parameters, database update, and database import all decide whether to reattach a stored secret (password, encrypted_extra, or SSH tunnel credential) by comparing one visible field against its masked/stored form, without checking whether other request-controlled fields also feed into the actual connection destination: - extra.engine_params (in particular engine_params.connect_args.host/ .port) is merged into the DBAPI connect() kwargs by SQLAlchemy and can override the host/port carried in the URI itself. - The SSH tunnel server_address/server_port can change independently of the URI. - (update only) the URI's own host/port can change directly while its password segment is left masked, with nothing comparing it against the stored host first. A principal who can reach one of these endpoints for a given database can redirect its real stored credential to a destination they control. For the update path this is a persistent change: the real credential is sent to the new destination on the *next* use of the database by anyone, not just the requester. Also scopes DatabaseDAO.get_database_by_name to the same DatabaseFilter object-visibility boundary find_by_id/get_connection already apply to this resource; it was previously an unfiltered lookup. Shared comparison logic factored into superset/commands/database/utils.py (engine_params_changed, ssh_tunnel_endpoint_changed, ssh_tunnel_rebind_unsafe, uri_identity_changed) and reused across all four call sites, extending the identity-check policy PR #43393 established for database import's URI-host case to also cover extra.engine_params, and applying an equivalent check to database update, which previously had none. Regression tests added per site (engine_params variant, SSH-tunnel- endpoint variant, and for update also the plain URI-host variant), each verified to fail without its corresponding fix and pass with it. Legitimate flows (unchanged destination, or a deliberate move with a freshly supplied credential) are covered and unaffected. Co-Authored-By: Claude Sonnet 5 <[email protected]> --- superset/commands/database/exceptions.py | 29 +++ superset/commands/database/importers/v1/utils.py | 60 +++---- superset/commands/database/test_connection.py | 37 +++- superset/commands/database/update.py | 55 ++++++ superset/commands/database/utils.py | 96 ++++++++++ superset/commands/database/validate.py | 44 ++++- superset/daos/database.py | 20 ++- tests/unit_tests/commands/databases/update_test.py | 197 +++++++++++++++++++++ .../unit_tests/commands/databases/validate_test.py | 123 +++++++++++++ .../databases/commands/importers/v1/import_test.py | 43 +++++ 10 files changed, 658 insertions(+), 46 deletions(-) diff --git a/superset/commands/database/exceptions.py b/superset/commands/database/exceptions.py index b8877270fd5..c971a029e9d 100644 --- a/superset/commands/database/exceptions.py +++ b/superset/commands/database/exceptions.py @@ -44,6 +44,26 @@ class DatabaseExistsValidationError(ValidationError): ) +class DatabaseUpdateUnsafeRebindError(ValidationError): + """ + Marshmallow validation error for an update that would change a + database's effective connection destination while leaving the stored + password/encrypted_extra/SSH tunnel credential masked. + """ + + def __init__(self) -> None: + super().__init__( + _( + "This update would change the connection's effective " + "destination (host/port, engine parameters, or SSH tunnel " + "endpoint) while reusing the stored credential. Provide " + "the real password (or SSH tunnel credential) to confirm " + "a connection move." + ), + field_name="sqlalchemy_uri", + ) + + class DatabaseRequiredFieldValidationError(ValidationError): def __init__(self, field_name: str) -> None: super().__init__( @@ -174,6 +194,15 @@ class DatabaseSecurityUnsafeError(CommandInvalidError): message = _("Stopped an unsafe database connection") +class DatabaseTestConnectionUnsafeRebindError(CommandInvalidError): + message = _( + "Testing this connection would change its effective destination " + "(engine parameters or SSH tunnel endpoint) while reusing the stored " + "password. Provide the real password to test a connection whose " + "destination has changed." + ) + + class DatabaseTestConnectionDriverError(CommandInvalidError): message = _("Could not load database driver") diff --git a/superset/commands/database/importers/v1/utils.py b/superset/commands/database/importers/v1/utils.py index 973aaad8b80..5357500c197 100644 --- a/superset/commands/database/importers/v1/utils.py +++ b/superset/commands/database/importers/v1/utils.py @@ -22,7 +22,12 @@ from flask import current_app as app from superset import db, security_manager from superset.commands.database.exceptions import DatabaseInvalidError -from superset.commands.database.utils import add_permissions +from superset.commands.database.utils import ( + add_permissions, + engine_params_changed, + ssh_tunnel_rebind_unsafe, + uri_identity_changed, +) from superset.commands.exceptions import ImportFailedError from superset.constants import PASSWORD_MASK from superset.databases.ssh_tunnel.models import SSHTunnel @@ -41,14 +46,19 @@ logger = logging.getLogger(__name__) def _connection_identity_changed(existing: Database, config: dict[str, Any]) -> bool: """Whether the import points the database at a different endpoint.""" - try: - stored = make_url_safe(existing.sqlalchemy_uri)._replace(password=None) - incoming = make_url_safe(config["sqlalchemy_uri"])._replace(password=None) - except DatabaseInvalidError: - # An unparseable URI cannot be compared: treat it as a change so - # stored secrets never survive onto it. + if uri_identity_changed(existing.sqlalchemy_uri, config.get("sqlalchemy_uri")): return True - return stored != incoming + + # The URI's host/port aren't the whole story: `extra.engine_params` + # (e.g. `connect_args.host`/`port`) is merged into the actual DBAPI + # connect kwargs and can override them. An import that opens a live + # connection (`add_permissions` -> `get_all_catalog_names`) with a + # rehydrated stored password must not do so against a destination this + # field silently redirected. + submitted_extra = config.get("extra") + if isinstance(submitted_extra, dict): + submitted_extra = json.dumps(submitted_extra) + return engine_params_changed(existing.extra, submitted_extra) def _refuse_stored_secret_reuse(existing: Database, config: dict[str, Any]) -> None: @@ -77,33 +87,13 @@ def _refuse_stored_secret_reuse(existing: Database, config: dict[str, Any]) -> N "connection to confirm the change." ) - if ssh_tunnel := config.get("ssh_tunnel"): - existing_tunnel = existing.ssh_tunnel - if existing_tunnel and ( - ssh_tunnel.get("server_address") != existing_tunnel.server_address - or ssh_tunnel.get("server_port") != existing_tunnel.server_port - ): - has_fresh_credential = any( - ssh_tunnel.get(field) not in (None, PASSWORD_MASK) - for field in ("password", "private_key") - ) - # A passphrase-protected private key's stored passphrase is a - # secret in its own right: if the existing tunnel had one, a - # repoint that supplies a fresh private_key but leaves - # private_key_password masked/absent would keep the old - # passphrase attached to the new key rather than requiring the - # importer to confirm it too. - stale_private_key_password = ( - existing_tunnel.private_key_password is not None - and ssh_tunnel.get("private_key_password") in (None, PASSWORD_MASK) - ) - if not has_fresh_credential or stale_private_key_password: - raise ImportFailedError( - f"Import would change the SSH tunnel endpoint of database " - f"'{existing.database_name}' without providing new tunnel " - "credentials. Re-enter the SSH tunnel credentials to " - "confirm the change." - ) + if ssh_tunnel_rebind_unsafe(existing.ssh_tunnel, config.get("ssh_tunnel")): + raise ImportFailedError( + f"Import would change the SSH tunnel endpoint of database " + f"'{existing.database_name}' without providing new tunnel " + "credentials. Re-enter the SSH tunnel credentials to " + "confirm the change." + ) def import_database( # noqa: C901 diff --git a/superset/commands/database/test_connection.py b/superset/commands/database/test_connection.py index 1247a2dee5a..6932a991ff5 100644 --- a/superset/commands/database/test_connection.py +++ b/superset/commands/database/test_connection.py @@ -26,13 +26,18 @@ from superset.commands.database.exceptions import ( DatabaseSecurityUnsafeError, DatabaseTestConnectionDriverError, DatabaseTestConnectionUnexpectedError, + DatabaseTestConnectionUnsafeRebindError, ) from superset.commands.database.ssh_tunnel.exceptions import ( SSHTunnelDatabasePortError, SSHTunnelHostKeyVerificationError, SSHTunnelingNotEnabledError, ) -from superset.commands.database.utils import ping +from superset.commands.database.utils import ( + engine_params_changed, + ping, + ssh_tunnel_endpoint_changed, +) from superset.daos.database import DatabaseDAO from superset.databases.utils import make_url_safe from superset.errors import ErrorLevel, SupersetErrorType @@ -65,6 +70,7 @@ class TestConnectionDatabaseCommand(BaseCommand): _model: Optional[Database] = None _context: dict[str, Any] _uri: str + _identity_changed: bool def __init__(self, data: dict[str, Any]): self._properties = data.copy() @@ -73,8 +79,24 @@ class TestConnectionDatabaseCommand(BaseCommand): self._model = DatabaseDAO.get_database_by_name(database_name) uri = self._properties.get("sqlalchemy_uri", "") - if self._model and uri == self._model.safe_sqlalchemy_uri(): - uri = self._model.sqlalchemy_uri_decrypted + self._identity_changed = False + if (model := self._model) is not None: + # A stored password (and, below, encrypted_extra / SSH tunnel + # credentials) must never be rehydrated onto a connection whose + # final effective destination the requester can change. The + # visible `sqlalchemy_uri` is only one part of that destination: + # `extra.engine_params` (merged into the DBAPI connect kwargs, + # e.g. `connect_args.host`/`port`) and the SSH tunnel endpoint + # can both override it after this decision is made. + self._identity_changed = engine_params_changed( + model.extra, self._properties.get("extra", "{}") + ) or ssh_tunnel_endpoint_changed( + model.ssh_tunnel, self._properties.get("ssh_tunnel") + ) + if uri == model.safe_sqlalchemy_uri(): + if self._identity_changed: + raise DatabaseTestConnectionUnsafeRebindError() + uri = model.sqlalchemy_uri_decrypted url = make_url_safe(uri) @@ -102,7 +124,7 @@ class TestConnectionDatabaseCommand(BaseCommand): "masked_encrypted_extra", "{}", ) - if self._model: + if self._model and not self._identity_changed: serialized_encrypted_extra = ( self._model.db_engine_spec.unmask_encrypted_extra( self._model.encrypted_extra, @@ -112,7 +134,12 @@ class TestConnectionDatabaseCommand(BaseCommand): # collect SSH tunnel info ssh_tunnel_properties = self._properties.get("ssh_tunnel") - if ssh_tunnel_properties and self._model and self._model.ssh_tunnel: + if ( + ssh_tunnel_properties + and self._model + and self._model.ssh_tunnel + and not self._identity_changed + ): # unmask password while allowing for updated values ssh_tunnel_properties = unmask_password_info( ssh_tunnel_properties, diff --git a/superset/commands/database/update.py b/superset/commands/database/update.py index 124bf11c87b..260892b1556 100644 --- a/superset/commands/database/update.py +++ b/superset/commands/database/update.py @@ -30,10 +30,18 @@ from superset.commands.database.exceptions import ( DatabaseInvalidError, DatabaseNotFoundError, DatabaseUpdateFailedError, + DatabaseUpdateUnsafeRebindError, MissingOAuth2TokenError, ) from superset.commands.database.sync_permissions import SyncPermissionsCommand +from superset.commands.database.utils import ( + engine_params_changed, + ssh_tunnel_rebind_unsafe, + uri_identity_changed, +) +from superset.constants import PASSWORD_MASK from superset.daos.database import DatabaseDAO +from superset.databases.utils import make_url_safe from superset.exceptions import OAuth2RedirectError from superset.models.core import Database from superset.utils import json @@ -180,3 +188,50 @@ class UpdateDatabaseCommand(BaseCommand): database_name, ): raise DatabaseInvalidError(exceptions=[DatabaseExistsValidationError()]) + + if self._model: + self._check_no_unsafe_secret_rebind() + + def _check_no_unsafe_secret_rebind(self) -> None: + """ + Refuse an update that changes the connection's effective destination + (URI host/port, `extra.engine_params`, or the SSH tunnel endpoint) + while leaving the corresponding stored secret masked. + + Without this, an editor could silently redirect the real stored + password/encrypted_extra/SSH tunnel credential to a different + destination -- and since an update persists, every subsequent use of + the database (by any user) would send the real secret there, not + just the editor's own request. + """ + model = self._model + assert model is not None + + connection_identity_changed = False + submitted_password: str | None = None + + if "sqlalchemy_uri" in self._properties: + submitted_uri = self._properties["sqlalchemy_uri"] or "" + connection_identity_changed = uri_identity_changed( + model.sqlalchemy_uri, submitted_uri + ) + try: + submitted_password = make_url_safe(submitted_uri).password + except DatabaseInvalidError: + submitted_password = None + + if "extra" in self._properties and engine_params_changed( + model.extra, self._properties["extra"] + ): + connection_identity_changed = True + + if connection_identity_changed and submitted_password in ( + None, + PASSWORD_MASK, + ): + raise DatabaseInvalidError(exceptions=[DatabaseUpdateUnsafeRebindError()]) + + if "ssh_tunnel" in self._properties and ssh_tunnel_rebind_unsafe( + model.ssh_tunnel, self._properties["ssh_tunnel"] + ): + raise DatabaseInvalidError(exceptions=[DatabaseUpdateUnsafeRebindError()]) diff --git a/superset/commands/database/utils.py b/superset/commands/database/utils.py index 0c25f8173c2..0d40516419c 100644 --- a/superset/commands/database/utils.py +++ b/superset/commands/database/utils.py @@ -19,6 +19,7 @@ from __future__ import annotations import logging import sqlite3 from contextlib import closing +from typing import Any from flask import current_app as app from flask_appbuilder.security.sqla.models import ( @@ -30,14 +31,109 @@ from sqlalchemy.engine import Engine from sqlalchemy.orm import Session from superset import security_manager +from superset.commands.database.exceptions import DatabaseInvalidError +from superset.constants import PASSWORD_MASK +from superset.databases.ssh_tunnel.models import SSHTunnel +from superset.databases.utils import make_url_safe from superset.db_engine_specs.base import GenericDBException from superset.models.core import Database from superset.security.manager import SupersetSecurityManager +from superset.utils import json from superset.utils.core import timeout logger = logging.getLogger(__name__) +def uri_identity_changed(existing_uri: str | None, submitted_uri: str | None) -> bool: + """ + Whether two SQLAlchemy URIs differ once their password is stripped -- + i.e. whether the effective connection destination (driver, host, port, + database, username, query params) changed. + """ + try: + stored = make_url_safe(existing_uri or "")._replace(password=None) + incoming = make_url_safe(submitted_uri or "")._replace(password=None) + except DatabaseInvalidError: + # An unparseable URI cannot be compared: treat it as a change so a + # stored secret never survives onto it. + return True + return stored != incoming + + +def engine_params_changed( + existing_extra: str | None, submitted_extra: str | None +) -> bool: + """ + Whether ``submitted_extra`` carries different ``engine_params`` than + ``existing_extra``. + + ``engine_params`` (in particular ``engine_params.connect_args``) is + merged into the actual DBAPI connect kwargs, so it can override the + host/port/etc. carried in the SQLAlchemy URI itself. Any caller that + conditionally reattaches a stored secret (password, encrypted_extra, SSH + tunnel credentials) based on the URI being unchanged must also check + this, or the destination can be silently redirected while the real + secret rides along. + """ + + def _engine_params(serialized_extra: str | None) -> dict[str, Any]: + try: + return json.loads(serialized_extra or "{}").get("engine_params", {}) + except (json.JSONDecodeError, AttributeError): + # Unparseable/non-dict `extra` cannot be compared: treat it as a + # change so a stored secret never rides along with input that + # can't be verified to leave the connection identity untouched. + return {"__unparseable__": True} + + return _engine_params(submitted_extra) != _engine_params(existing_extra) + + +def ssh_tunnel_endpoint_changed( + existing_tunnel: SSHTunnel | None, submitted_tunnel: dict[str, Any] | None +) -> bool: + """ + Whether a submitted SSH tunnel config points at a different endpoint + than the stored tunnel it would otherwise inherit credentials from. + """ + if not submitted_tunnel or not existing_tunnel: + return False + return bool( + submitted_tunnel.get("server_address") != existing_tunnel.server_address + or submitted_tunnel.get("server_port") != existing_tunnel.server_port + ) + + +def ssh_tunnel_rebind_unsafe( + existing_tunnel: SSHTunnel | None, submitted_tunnel: dict[str, Any] | None +) -> bool: + """ + Whether a submitted SSH tunnel config repoints the tunnel at a + different endpoint without supplying credentials fresh enough to + justify it -- i.e. whether carrying the stored tunnel secrets over + onto this submission would be unsafe. + """ + if not ssh_tunnel_endpoint_changed(existing_tunnel, submitted_tunnel): + return False + + assert submitted_tunnel is not None + assert existing_tunnel is not None + + has_fresh_credential = any( + submitted_tunnel.get(field) not in (None, PASSWORD_MASK) + for field in ("password", "private_key") + ) + # A passphrase-protected private key's stored passphrase is a secret in + # its own right: if the existing tunnel had one, a repoint that + # supplies a fresh private_key but leaves private_key_password + # masked/absent would keep the old passphrase attached to the new key + # rather than requiring the caller to confirm it too. + stale_private_key_password = ( + existing_tunnel.private_key_password is not None + and submitted_tunnel.get("private_key_password") in (None, PASSWORD_MASK) + ) + return not has_fresh_credential or stale_private_key_password + + def ping(engine: Engine) -> bool: try: time_delta = app.config["TEST_DATABASE_CONNECTION_TIMEOUT"] diff --git a/superset/commands/database/validate.py b/superset/commands/database/validate.py index df7b4c0f363..450a90655a5 100644 --- a/superset/commands/database/validate.py +++ b/superset/commands/database/validate.py @@ -27,6 +27,10 @@ from superset.commands.database.exceptions import ( InvalidEngineError, InvalidParametersError, ) +from superset.commands.database.utils import ( + engine_params_changed, + ssh_tunnel_endpoint_changed, +) from superset.daos.database import DatabaseDAO from superset.databases.utils import make_url_safe from superset.db_engine_specs import get_engine_spec @@ -90,11 +94,25 @@ class ValidateDatabaseParametersCommand(BaseCommand): event_logger.log_with_context(action="validation_error", engine=engine) raise InvalidParametersError(errors) + # A stored password/encrypted_extra/SSH tunnel credential must never + # be rehydrated onto a connection whose final effective destination + # the caller can change. `parameters` only covers what feeds into + # `sqlalchemy_uri` here -- `extra.engine_params` (merged into the + # actual DBAPI connect kwargs, e.g. `connect_args.host`/`port`) and + # the SSH tunnel endpoint can both override it independently. + identity_changed = False + if (model := self._model) is not None: + identity_changed = engine_params_changed( + model.extra, self._properties.get("extra", "{}") + ) or ssh_tunnel_endpoint_changed( + model.ssh_tunnel, self._properties.get("ssh_tunnel") + ) + serialized_encrypted_extra = self._properties.get( "masked_encrypted_extra", "{}", ) - if self._model: + if self._model and not identity_changed: serialized_encrypted_extra = engine_spec.unmask_encrypted_extra( self._model.encrypted_extra, serialized_encrypted_extra, @@ -110,13 +128,35 @@ class ValidateDatabaseParametersCommand(BaseCommand): encrypted_extra, ) if self._model and sqlalchemy_uri == self._model.safe_sqlalchemy_uri(): + if identity_changed: + raise InvalidParametersError( + [ + SupersetError( + message=__( + "Testing this connection would change its " + "effective destination (engine parameters " + "or SSH tunnel endpoint) while reusing the " + "stored password. Provide the real " + "password to test a connection whose " + "destination has changed." + ), + error_type=SupersetErrorType.GENERIC_DB_ENGINE_ERROR, + level=ErrorLevel.ERROR, + ) + ] + ) sqlalchemy_uri = self._model.sqlalchemy_uri_decrypted # Forward the SSH tunnel into the connection test so that # tunnel-only databases are reached through the tunnel rather # than directly, mirroring the existing test_connection flow. ssh_tunnel_properties = self._properties.get("ssh_tunnel") - if ssh_tunnel_properties and self._model and self._model.ssh_tunnel: + if ( + ssh_tunnel_properties + and self._model + and self._model.ssh_tunnel + and not identity_changed + ): ssh_tunnel_properties = unmask_password_info( ssh_tunnel_properties, self._model.ssh_tunnel, diff --git a/superset/daos/database.py b/superset/daos/database.py index 6d84a695138..81730cdca1e 100644 --- a/superset/daos/database.py +++ b/superset/daos/database.py @@ -23,6 +23,7 @@ from urllib.parse import unquote import requests from flask import current_app as app +from flask_appbuilder.models.sqla.interface import SQLAInterface from sqlalchemy.orm import joinedload try: @@ -179,11 +180,22 @@ class DatabaseDAO(BaseDAO[Database]): @staticmethod def get_database_by_name(database_name: str) -> Database | None: - return ( - db.session.query(Database) - .filter(Database.database_name == database_name) - .one_or_none() + """ + Look up a database by name, scoped to the requesting user's object-level + visibility (the same ``DatabaseFilter`` boundary ``find_by_id``/ + ``get_connection`` already apply). An unfiltered lookup would let any + principal with class-level ``can_write`` reference an arbitrary + existing database by name -- including one they have no catalog, + schema, datasource, or database access to -- and ride along with + whatever secret-rehydration behavior callers apply to the result. + """ + query = db.session.query(Database).filter( + Database.database_name == database_name + ) + query = DatabaseFilter("id", SQLAInterface(Database, db.session)).apply( + query, None ) + return query.one_or_none() @staticmethod def build_db_for_connection_test( diff --git a/tests/unit_tests/commands/databases/update_test.py b/tests/unit_tests/commands/databases/update_test.py index 07663df2450..187083a9328 100644 --- a/tests/unit_tests/commands/databases/update_test.py +++ b/tests/unit_tests/commands/databases/update_test.py @@ -17,10 +17,13 @@ from unittest.mock import MagicMock +import pytest from pytest_mock import MockerFixture from superset import db +from superset.commands.database.exceptions import DatabaseInvalidError from superset.commands.database.update import UpdateDatabaseCommand +from superset.constants import PASSWORD_MASK from superset.extensions import security_manager from superset.utils import json from tests.conftest import with_config @@ -667,3 +670,197 @@ def test_update_broken_connection(mocker: MockerFixture) -> None: UpdateDatabaseCommand(1, {}).run() update_catalog_attribute.assert_called_once_with(1, "main") + + +def test_update_host_change_requires_new_credentials(mocker: MockerFixture) -> None: + """ + An update that repoints an existing database at a different host while + the URI's password stays masked must not silently reuse the stored + password: the update persists, so every subsequent use of the database + (by any user) would send the real credential to the new host. + """ + existing = mocker.MagicMock() + existing.sqlalchemy_uri = "postgresql://user:XXXXXXXXXX@host1" + existing.extra = "{}" + existing.ssh_tunnel = None + + database_dao = mocker.patch("superset.commands.database.update.DatabaseDAO") + database_dao.find_by_id.return_value = existing + + with pytest.raises(DatabaseInvalidError): + UpdateDatabaseCommand( + 1, + { + "sqlalchemy_uri": ( + "postgresql://user:[email protected]:5432/prod" + ) + }, + ).run() + + database_dao.update.assert_not_called() + + +def test_update_engine_params_change_requires_new_credentials( + mocker: MockerFixture, +) -> None: + """ + An update that changes `extra.engine_params` (e.g. + `connect_args.host`/`port`, merged into the actual DBAPI connect kwargs + ahead of anything in `sqlalchemy_uri`) while the URI's password stays + masked must not silently reuse the stored password. + """ + existing = mocker.MagicMock() + existing.sqlalchemy_uri = "postgresql://user:XXXXXXXXXX@host1" + existing.extra = "{}" + existing.ssh_tunnel = None + + database_dao = mocker.patch("superset.commands.database.update.DatabaseDAO") + database_dao.find_by_id.return_value = existing + + with pytest.raises(DatabaseInvalidError): + UpdateDatabaseCommand( + 1, + { + "extra": json.dumps( + { + "engine_params": { + "connect_args": { + "host": "attacker.example.com", + "port": 15432, + } + } + } + ) + }, + ).run() + + database_dao.update.assert_not_called() + + +def test_update_ssh_tunnel_host_change_requires_new_credentials( + mocker: MockerFixture, +) -> None: + """ + An update that repoints an existing database's SSH tunnel at a + different server must not silently reuse the stored tunnel password. + """ + existing = mocker.MagicMock() + existing.sqlalchemy_uri = "postgresql://user:XXXXXXXXXX@host1" + existing.extra = "{}" + tunnel = mocker.MagicMock() + tunnel.server_address = "10.0.0.1" + tunnel.server_port = 22 + existing.ssh_tunnel = tunnel + + database_dao = mocker.patch("superset.commands.database.update.DatabaseDAO") + database_dao.find_by_id.return_value = existing + + with pytest.raises(DatabaseInvalidError): + UpdateDatabaseCommand( + 1, + { + "ssh_tunnel": { + "server_address": "attacker.example.com", + "server_port": 22, + "username": "tunnel_user", + "password": PASSWORD_MASK, + } + }, + ).run() + + database_dao.update.assert_not_called() + + +def test_update_ssh_tunnel_private_key_password_not_carried_over( + mocker: MockerFixture, +) -> None: + """ + An update that repoints the SSH tunnel and supplies a fresh + private_key but omits private_key_password must not silently keep the + old, real passphrase attached to the new key. + """ + tunnel = mocker.MagicMock() + tunnel.server_address = "10.0.0.1" + tunnel.server_port = 22 + tunnel.private_key_password = "original-passphrase" # noqa: S105 + + existing = mocker.MagicMock() + existing.sqlalchemy_uri = "postgresql://user:XXXXXXXXXX@host1" + existing.extra = "{}" + existing.ssh_tunnel = tunnel + + database_dao = mocker.patch("superset.commands.database.update.DatabaseDAO") + database_dao.find_by_id.return_value = existing + + with pytest.raises(DatabaseInvalidError): + UpdateDatabaseCommand( + 1, + { + "ssh_tunnel": { + "server_address": "attacker.example.com", + "server_port": 22, + "username": "tunnel_user", + "private_key": "-----BEGIN PRIVATE KEY-----\nNew\n-----END-----", + # private_key_password omitted entirely + } + }, + ).run() + + database_dao.update.assert_not_called() + + +def test_update_host_change_with_new_credentials(mocker: MockerFixture) -> None: + """ + A deliberate connection move is still possible when the update supplies + a fresh password for the new destination. + """ + old_database = mocker.MagicMock(allow_multi_catalog=False) + old_database.sqlalchemy_uri = "postgresql://user:XXXXXXXXXX@host1" + old_database.extra = "{}" + old_database.ssh_tunnel = None + old_database.get_default_catalog.return_value = "prod" + old_database.id = 1 + + new_database = mocker.MagicMock(allow_multi_catalog=False) + new_database.get_default_catalog.return_value = "prod" + + database_dao = mocker.patch("superset.commands.database.update.DatabaseDAO") + database_dao.find_by_id.return_value = old_database + database_dao.update.return_value = new_database + + mocker.patch("superset.commands.database.update.SyncPermissionsCommand") + mocker.patch.object(UpdateDatabaseCommand, "_update_catalog_attribute") + + UpdateDatabaseCommand( + 1, + {"sqlalchemy_uri": "postgresql://user:newpass@host2:5432/prod"}, + ).run() + + database_dao.update.assert_called_once() + + +def test_update_unrelated_fields_still_work(mocker: MockerFixture) -> None: + """ + An update that doesn't touch `sqlalchemy_uri`, `extra`, or `ssh_tunnel` + at all must not be blocked by the new destination-change check. + """ + old_database = mocker.MagicMock(allow_multi_catalog=False) + old_database.sqlalchemy_uri = "postgresql://user:XXXXXXXXXX@host1" + old_database.extra = "{}" + old_database.ssh_tunnel = None + old_database.get_default_catalog.return_value = "prod" + old_database.id = 1 + + new_database = mocker.MagicMock(allow_multi_catalog=False) + new_database.get_default_catalog.return_value = "prod" + + database_dao = mocker.patch("superset.commands.database.update.DatabaseDAO") + database_dao.find_by_id.return_value = old_database + database_dao.update.return_value = new_database + + mocker.patch("superset.commands.database.update.SyncPermissionsCommand") + mocker.patch.object(UpdateDatabaseCommand, "_update_catalog_attribute") + + UpdateDatabaseCommand(1, {"expose_in_sqllab": False}).run() + + database_dao.update.assert_called_once() diff --git a/tests/unit_tests/commands/databases/validate_test.py b/tests/unit_tests/commands/databases/validate_test.py index 1d507bd944a..981a56def4e 100644 --- a/tests/unit_tests/commands/databases/validate_test.py +++ b/tests/unit_tests/commands/databases/validate_test.py @@ -24,7 +24,9 @@ from superset.commands.database.exceptions import ( InvalidParametersError, ) from superset.commands.database.validate import ValidateDatabaseParametersCommand +from superset.constants import PASSWORD_MASK from superset.errors import ErrorLevel, SupersetError, SupersetErrorType +from superset.utils import json def test_command(mocker: MockerFixture) -> None: @@ -648,6 +650,127 @@ def test_validate_ssh_tunnel_feature_disabled_via_parameters_ssh( ) +def test_validate_engine_params_change_requires_new_credentials( + mocker: MockerFixture, +) -> None: + """ + Validating an existing database's parameters (unchanged host/port/etc, + so `sqlalchemy_uri` matches the stored masked URI) must not silently + reuse the stored password when `extra.engine_params` would redirect the + actual DBAPI connect kwargs elsewhere. + """ + existing = mocker.MagicMock() + existing.safe_sqlalchemy_uri.return_value = "postgresql://u:XXXXXXXXXX@host1/d" + existing.sqlalchemy_uri_decrypted = "postgresql://u:realpass@host1/d" + existing.encrypted_extra = "{}" + existing.extra = "{}" + existing.ssh_tunnel = None + + DatabaseDAO = mocker.patch( # noqa: N806 + "superset.commands.database.validate.DatabaseDAO" + ) + DatabaseDAO.find_by_id.return_value = existing + DatabaseDAO.validate_update_uniqueness.return_value = True + + mocker.patch( + "superset.commands.database.validate.get_engine_spec", + return_value=mocker.MagicMock( + validate_parameters=mocker.MagicMock(return_value=[]), + build_sqlalchemy_uri=mocker.MagicMock( + return_value="postgresql://u:XXXXXXXXXX@host1/d" + ), + unmask_encrypted_extra=mocker.MagicMock(return_value="{}"), + ), + ) + + properties = { + "id": 1, + "engine": "postgresql", + "parameters": { + "host": "host1", + "port": 5432, + "username": "u", + "database": "d", + }, + "extra": json.dumps( + { + "engine_params": { + "connect_args": {"host": "attacker.example.com", "port": 15432} + } + } + ), + } + command = ValidateDatabaseParametersCommand(properties) + with pytest.raises(InvalidParametersError): + command.run() + + DatabaseDAO.build_db_for_connection_test.assert_not_called() + + +def test_validate_ssh_tunnel_host_change_requires_new_credentials( + mocker: MockerFixture, +) -> None: + """ + Validating with an unchanged host/parameters set must not silently + reuse the stored SSH tunnel password when the tunnel endpoint itself + is being redirected. + """ + mocker.patch( + "superset.commands.database.validate.is_feature_enabled", + return_value=True, + ) + + tunnel = mocker.MagicMock() + tunnel.server_address = "10.0.0.1" + tunnel.server_port = 22 + + existing = mocker.MagicMock() + existing.safe_sqlalchemy_uri.return_value = "postgresql://u:XXXXXXXXXX@host1/d" + existing.sqlalchemy_uri_decrypted = "postgresql://u:realpass@host1/d" + existing.encrypted_extra = "{}" + existing.extra = "{}" + existing.ssh_tunnel = tunnel + + DatabaseDAO = mocker.patch( # noqa: N806 + "superset.commands.database.validate.DatabaseDAO" + ) + DatabaseDAO.find_by_id.return_value = existing + DatabaseDAO.validate_update_uniqueness.return_value = True + + mocker.patch( + "superset.commands.database.validate.get_engine_spec", + return_value=mocker.MagicMock( + validate_parameters=mocker.MagicMock(return_value=[]), + build_sqlalchemy_uri=mocker.MagicMock( + return_value="postgresql://u:XXXXXXXXXX@host1/d" + ), + unmask_encrypted_extra=mocker.MagicMock(return_value="{}"), + ), + ) + + properties = { + "id": 1, + "engine": "postgresql", + "parameters": { + "host": "host1", + "port": 5432, + "username": "u", + "database": "d", + }, + "ssh_tunnel": { + "server_address": "attacker.example.com", + "server_port": 22, + "username": "tunnel_user", + "password": PASSWORD_MASK, + }, + } + command = ValidateDatabaseParametersCommand(properties) + with pytest.raises(InvalidParametersError): + command.run() + + DatabaseDAO.build_db_for_connection_test.assert_not_called() + + def test_ssh_tunnel_missing_message_is_interpolated( mocker: MockerFixture, ) -> None: diff --git a/tests/unit_tests/databases/commands/importers/v1/import_test.py b/tests/unit_tests/databases/commands/importers/v1/import_test.py index 2ccf0cc1fb2..27c2e4c6f69 100644 --- a/tests/unit_tests/databases/commands/importers/v1/import_test.py +++ b/tests/unit_tests/databases/commands/importers/v1/import_test.py @@ -593,6 +593,49 @@ def test_import_database_host_change_with_new_credentials( assert database.password == "newpass" # noqa: S105 +def test_import_database_engine_params_change_requires_new_credentials( + mocker: MockerFixture, session: Session +) -> None: + """ + An overwrite that changes `extra.engine_params` (e.g. + `connect_args.host`/`port`, which the DBAPI merges into the actual + connection target ahead of anything carried in `sqlalchemy_uri`) must + not silently reuse the stored password: the submitted URI can still + match the stored host, while the DBAPI actually connects elsewhere. + """ + from superset import security_manager + from superset.commands.database.importers.v1.utils import import_database + from superset.models.core import Database + from tests.integration_tests.fixtures.importexport import database_config + + mocker.patch.object(security_manager, "can_access", return_value=True) + mocker.patch("superset.commands.database.importers.v1.utils.add_permissions") + + engine = db.session.get_bind() + Database.metadata.create_all(engine) # pylint: disable=no-member + + config = copy.deepcopy(database_config) + database = import_database(config) + assert database.password == "pass" # noqa: S105 + + hostile = copy.deepcopy(database_config) + hostile["sqlalchemy_uri"] = f"postgresql://user:{PASSWORD_MASK}@host1" + hostile["extra"] = { + "engine_params": { + "connect_args": {"host": "attacker.example.com", "port": 15432} + } + } + with pytest.raises(ImportFailedError): + import_database(hostile, overwrite=True) + + # the same engine_params (a normal re-import of an exported bundle) still works + unchanged = copy.deepcopy(database_config) + unchanged["sqlalchemy_uri"] = f"postgresql://user:{PASSWORD_MASK}@host1" + unchanged["password"] = "pass" # noqa: S105 + database = import_database(unchanged, overwrite=True) + assert database.password == "pass" # noqa: S105 + + def test_import_database_unparseable_uri_treated_as_change( mocker: MockerFixture, session: Session ) -> None:
