From 4476dca00786adef5da3bcf74699e0b217f8ffa6 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Wed, 24 Mar 2021 11:33:04 -0400 Subject: [PATCH] Repair pysqlcipher and use sqlcipher3 The ``pysqlcipher`` dialect now imports the ``sqlcipher3`` module for Python 3 by default. Regressions have been repaired such that the connection routine was not working. To better support the post-connection steps of the pysqlcipher dialect, a new hook Dialect.on_connect_url() is added, which supersedes Dialect.on_connect() and is passed the URL object. The dialect now pulls the passphrase and other cipher args from the URL directly without including them in the "connect" args. This will allow any user-defined extensibility to connecting to work as it would for other dialects. The commit also builds upon the extended routines in sqlite/provisioning.py to better support running tests against multiple simultaneous SQLite database files. Additionally enables backend for test_sqlite which was skipping everything for aiosqlite too, fortunately everything there is passing. Fixes: #5848 Change-Id: I43f53ebc62298a84a4abe149e1eb699a027b7915 --- doc/build/changelog/unreleased_14/5848.rst | 14 +++ doc/build/dialects/sqlite.rst | 3 + lib/sqlalchemy/dialects/sqlite/provision.py | 66 +++++++---- lib/sqlalchemy/dialects/sqlite/pysqlcipher.py | 109 ++++++++++-------- lib/sqlalchemy/engine/create.py | 2 +- lib/sqlalchemy/engine/interfaces.py | 71 ++++++++++++ lib/sqlalchemy/testing/requirements.py | 2 +- setup.cfg | 3 + test/dialect/test_sqlite.py | 16 ++- tox.ini | 6 +- 10 files changed, 221 insertions(+), 71 deletions(-) create mode 100644 doc/build/changelog/unreleased_14/5848.rst diff --git a/doc/build/changelog/unreleased_14/5848.rst b/doc/build/changelog/unreleased_14/5848.rst new file mode 100644 index 0000000000..e8fd341021 --- /dev/null +++ b/doc/build/changelog/unreleased_14/5848.rst @@ -0,0 +1,14 @@ +.. change:: + :tags: bug, sqlite, regression + :tickets: 5848 + + Repaired the ``pysqlcipher`` dialect to connect correctly which had + regressed in 1.4, and added test + CI support to maintain the driver + in working condition. The dialect now imports the ``sqlcipher3`` module + for Python 3 by default before falling back to ``pysqlcipher3`` which + is documented as now being unmaintained. + + .. seealso:: + + :ref:`pysqlcipher` + diff --git a/doc/build/dialects/sqlite.rst b/doc/build/dialects/sqlite.rst index e9a84e3388..6d40daf5fe 100644 --- a/doc/build/dialects/sqlite.rst +++ b/doc/build/dialects/sqlite.rst @@ -49,6 +49,9 @@ Aiosqlite .. automodule:: sqlalchemy.dialects.sqlite.aiosqlite + +.. _pysqlcipher: + Pysqlcipher ----------- diff --git a/lib/sqlalchemy/dialects/sqlite/provision.py b/lib/sqlalchemy/dialects/sqlite/provision.py index d0d12695da..e5b17e8294 100644 --- a/lib/sqlalchemy/dialects/sqlite/provision.py +++ b/lib/sqlalchemy/dialects/sqlite/provision.py @@ -1,9 +1,12 @@ import os +import re +from ... import exc from ...engine import url as sa_url from ...testing.provision import create_db from ...testing.provision import drop_db from ...testing.provision import follower_url_from_main +from ...testing.provision import generate_driver_url from ...testing.provision import log from ...testing.provision import post_configure_engine from ...testing.provision import run_reap_dbs @@ -11,21 +14,38 @@ from ...testing.provision import stop_test_class_outside_fixtures from ...testing.provision import temp_table_keyword_args -# likely needs a generate_driver_url() def here for the --dbdriver part to -# work +# TODO: I can't get this to build dynamically with pytest-xdist procs +_drivernames = {"pysqlite", "aiosqlite", "pysqlcipher"} -_drivernames = set() + +@generate_driver_url.for_db("sqlite") +def generate_driver_url(url, driver, query_str): + if driver == "pysqlcipher" and url.get_driver_name() != "pysqlcipher": + if url.database: + url = url.set(database=url.database + ".enc") + url = url.set(password="test") + url = url.set(drivername="sqlite+%s" % (driver,)) + try: + url.get_dialect() + except exc.NoSuchModuleError: + return None + else: + return url @follower_url_from_main.for_db("sqlite") def _sqlite_follower_url_from_main(url, ident): url = sa_url.make_url(url) + if not url.database or url.database == ":memory:": return url else: - _drivernames.add(url.get_driver_name()) + + m = re.match(r"(.+?)\.(.+)$", url.database) + name, ext = m.group(1, 2) + drivername = url.get_driver_name() return sa_url.make_url( - "sqlite+%s:///%s.db" % (url.get_driver_name(), ident) + "sqlite+%s:///%s_%s.%s" % (drivername, drivername, ident, ext) ) @@ -81,7 +101,6 @@ def stop_test_class_outside_fixtures(config, db, cls): if files: db.dispose() - # some sqlite file tests are not cleaning up well yet, so do this # just to make things simple for now for file_ in files: @@ -102,19 +121,22 @@ def _reap_sqlite_dbs(url, idents): for ident in idents: # we don't have a config so we can't call _sqlite_drop_db due to the # decorator - for path in ( - [ - "%s.db" % ident, - ] - + [ - "%s_test_schema.db" % (drivername,) - for drivername in _drivernames - ] - + [ - "%s_%s_test_schema.db" % (ident, drivername) - for drivername in _drivernames - ] - ): - if os.path.exists(path): - log.info("deleting SQLite database file: %s" % path) - os.remove(path) + for ext in ("db", "db.enc"): + for path in ( + ["%s.%s" % (ident, ext)] + + [ + "%s_%s.%s" % (drivername, ident, ext) + for drivername in _drivernames + ] + + [ + "%s_test_schema.%s" % (drivername, ext) + for drivername in _drivernames + ] + + [ + "%s_%s_test_schema.%s" % (ident, drivername, ext) + for drivername in _drivernames + ] + ): + if os.path.exists(path): + log.info("deleting SQLite database file: %s" % path) + os.remove(path) diff --git a/lib/sqlalchemy/dialects/sqlite/pysqlcipher.py b/lib/sqlalchemy/dialects/sqlite/pysqlcipher.py index 659043366d..8f0f46acb0 100644 --- a/lib/sqlalchemy/dialects/sqlite/pysqlcipher.py +++ b/lib/sqlalchemy/dialects/sqlite/pysqlcipher.py @@ -8,32 +8,43 @@ """ .. dialect:: sqlite+pysqlcipher :name: pysqlcipher - :dbapi: pysqlcipher + :dbapi: sqlcipher 3 or pysqlcipher :connectstring: sqlite+pysqlcipher://:passphrase/file_path[?kdf_iter=] - :url: https://pypi.python.org/pypi/pysqlcipher - ``pysqlcipher`` is a fork of the standard ``pysqlite`` driver to make - use of the `SQLCipher `_ backend. + Dialect for support of DBAPIs that make use of the + `SQLCipher `_ backend. - ``pysqlcipher3`` is a fork of ``pysqlcipher`` for Python 3. This dialect - will attempt to import it if ``pysqlcipher`` is non-present. - - .. versionadded:: 1.1.4 - added fallback import for pysqlcipher3 - - .. versionadded:: 0.9.9 - added pysqlcipher dialect Driver ------ -The driver here is the -`pysqlcipher `_ -driver, which makes use of the SQLCipher engine. This system essentially +Current dialect selection logic is: + +* If the :paramref:`_sa.create_engine.module` parameter supplies a DBAPI module, + that module is used. +* Otherwise for Python 3, choose https://pypi.org/project/sqlcipher3/ +* If not available, fall back to https://pypi.org/project/pysqlcipher3/ +* For Python 2, https://pypi.org/project/pysqlcipher/ is used. + +.. warning:: The ``pysqlcipher3`` and ``pysqlcipher`` DBAPI drivers are no + longer maintained; the ``sqlcipher3`` driver as of this writing appears + to be current. For future compatibility, any pysqlcipher-compatible DBAPI + may be used as follows:: + + import sqlcipher_compatible_driver + + from sqlalchemy import create_engine + + e = create_engine( + "sqlite+pysqlcipher://:password@/dbname.db", + module=sqlcipher_compatible_driver + ) + +These drivers make use of the SQLCipher engine. This system essentially introduces new PRAGMA commands to SQLite which allows the setting of a -passphrase and other encryption parameters, allowing the database -file to be encrypted. +passphrase and other encryption parameters, allowing the database file to be +encrypted. -`pysqlcipher3` is a fork of `pysqlcipher` with support for Python 3, -the driver is the same. Connect Strings --------------- @@ -82,7 +93,7 @@ from __future__ import absolute_import from .pysqlite import SQLiteDialect_pysqlite from ... import pool -from ...engine import url as _url +from ... import util class SQLiteDialect_pysqlcipher(SQLiteDialect_pysqlite): @@ -92,13 +103,18 @@ class SQLiteDialect_pysqlcipher(SQLiteDialect_pysqlite): @classmethod def dbapi(cls): - try: - from pysqlcipher import dbapi2 as sqlcipher - except ImportError as e: + if util.py3k: try: - from pysqlcipher3 import dbapi2 as sqlcipher + import sqlcipher3 as sqlcipher except ImportError: - raise e + pass + else: + return sqlcipher + + from pysqlcipher3 import dbapi2 as sqlcipher + + else: + from pysqlcipher import dbapi2 as sqlcipher return sqlcipher @@ -106,34 +122,37 @@ class SQLiteDialect_pysqlcipher(SQLiteDialect_pysqlite): def get_pool_class(cls, url): return pool.SingletonThreadPool - def connect(self, *cargs, **cparams): - passphrase = cparams.pop("passphrase", "") + def on_connect_url(self, url): + super_on_connect = super( + SQLiteDialect_pysqlcipher, self + ).on_connect_url(url) - pragmas = dict((key, cparams.pop(key, None)) for key in self.pragmas) + # pull the info we need from the URL early. Even though URL + # is immutable, we don't want any in-place changes to the URL + # to affect things + passphrase = url.password or "" + url_query = dict(url.query) - conn = super(SQLiteDialect_pysqlcipher, self).connect( - *cargs, **cparams - ) - conn.exec_driver_sql('pragma key="%s"' % passphrase) - for prag, value in pragmas.items(): - if value is not None: - conn.exec_driver_sql('pragma %s="%s"' % (prag, value)) + def on_connect(conn): + cursor = conn.cursor() + cursor.execute('pragma key="%s"' % passphrase) + for prag in self.pragmas: + value = url_query.get(prag, None) + if value is not None: + cursor.execute('pragma %s="%s"' % (prag, value)) + cursor.close() - return conn + if super_on_connect: + super_on_connect(conn) + + return on_connect def create_connect_args(self, url): - super_url = _url.URL( - url.drivername, - username=url.username, - host=url.host, - database=url.database, - query=url.query, + plain_url = url._replace(password=None) + plain_url = plain_url.difference_update_query(self.pragmas) + return super(SQLiteDialect_pysqlcipher, self).create_connect_args( + plain_url ) - c_args, opts = super( - SQLiteDialect_pysqlcipher, self - ).create_connect_args(super_url) - opts["passphrase"] = url.password - return c_args, opts dialect = SQLiteDialect_pysqlcipher diff --git a/lib/sqlalchemy/engine/create.py b/lib/sqlalchemy/engine/create.py index 789030f2bc..682d0dd5d3 100644 --- a/lib/sqlalchemy/engine/create.py +++ b/lib/sqlalchemy/engine/create.py @@ -646,7 +646,7 @@ def create_engine(url, **kwargs): engine = engineclass(pool, dialect, u, **engine_args) if _initialize: - do_on_connect = dialect.on_connect() + do_on_connect = dialect.on_connect_url(url) if do_on_connect: def on_connect(dbapi_connection, connection_record): diff --git a/lib/sqlalchemy/engine/interfaces.py b/lib/sqlalchemy/engine/interfaces.py index 010abcc24f..24e0e5b0d1 100644 --- a/lib/sqlalchemy/engine/interfaces.py +++ b/lib/sqlalchemy/engine/interfaces.py @@ -746,6 +746,67 @@ class Dialect(object): """ + def on_connect_url(self, url): + """return a callable which sets up a newly created DBAPI connection. + + This method is a new hook that supersedes the + :meth:`_engine.Dialect.on_connect` method when implemented by a + dialect. When not implemented by a dialect, it invokes the + :meth:`_engine.Dialect.on_connect` method directly to maintain + compatibility with existing dialects. There is no deprecation + for :meth:`_engine.Dialect.on_connect` expected. + + The callable should accept a single argument "conn" which is the + DBAPI connection itself. The inner callable has no + return value. + + E.g.:: + + class MyDialect(default.DefaultDialect): + # ... + + def on_connect_url(self, url): + def do_on_connect(connection): + connection.execute("SET SPECIAL FLAGS etc") + + return do_on_connect + + This is used to set dialect-wide per-connection options such as + isolation modes, Unicode modes, etc. + + This method differs from :meth:`_engine.Dialect.on_connect` in that + it is passed the :class:`_engine.URL` object that's relevant to the + connect args. Normally the only way to get this is from the + :meth:`_engine.Dialect.on_connect` hook is to look on the + :class:`_engine.Engine` itself, however this URL object may have been + replaced by plugins. + + .. note:: + + The default implementation of + :meth:`_engine.Dialect.on_connect_url` is to invoke the + :meth:`_engine.Dialect.on_connect` method. Therefore if a dialect + implements this method, the :meth:`_engine.Dialect.on_connect` + method **will not be called** unless the overriding dialect calls + it directly from here. + + .. versionadded:: 1.4.3 added :meth:`_engine.Dialect.on_connect_url` + which normally calls into :meth:`_engine.Dialect.on_connect`. + + :param url: a :class:`_engine.URL` object representing the + :class:`_engine.URL` that was passed to the + :meth:`_engine.Dialect.create_connect_args` method. + + :return: a callable that accepts a single DBAPI connection as an + argument, or None. + + .. seealso:: + + :meth:`_engine.Dialect.on_connect` + + """ + return self.on_connect() + def on_connect(self): """return a callable which sets up a newly created DBAPI connection. @@ -776,6 +837,12 @@ class Dialect(object): for the first connection of a dialect. The on_connect hook is still called before the :meth:`_engine.Dialect.initialize` method however. + .. versionchanged:: 1.4.3 the on_connect hook is invoked from a new + method on_connect_url that passes the URL that was used to create + the connect args. Dialects can implement on_connect_url instead + of on_connect if they need the URL object that was used for the + connection in order to get additional context. + If None is returned, no event listener is generated. :return: a callable that accepts a single DBAPI connection as an @@ -786,6 +853,10 @@ class Dialect(object): :meth:`.Dialect.connect` - allows the DBAPI ``connect()`` sequence itself to be controlled. + :meth:`.Dialect.on_connect_url` - supersedes + :meth:`.Dialect.on_connect` to also receive the + :class:`_engine.URL` object in context. + """ return None diff --git a/lib/sqlalchemy/testing/requirements.py b/lib/sqlalchemy/testing/requirements.py index 208ba00919..46844803b4 100644 --- a/lib/sqlalchemy/testing/requirements.py +++ b/lib/sqlalchemy/testing/requirements.py @@ -18,9 +18,9 @@ to provide specific inclusion/exclusions. import platform import sys -from sqlalchemy.pool.impl import QueuePool from . import exclusions from .. import util +from ..pool import QueuePool class Requirements(object): diff --git a/setup.cfg b/setup.cfg index 82251e38fa..7e328d5b5f 100644 --- a/setup.cfg +++ b/setup.cfg @@ -77,6 +77,8 @@ aiomysql = aiosqlite = %(asyncio)s aiosqlite;python_version>="3" +sqlcipher = + sqlcipher3_binary;python_version>="3" [egg_info] tag_build = dev @@ -142,6 +144,7 @@ sqlite = sqlite:///:memory: aiosqlite = sqlite+aiosqlite:///:memory: sqlite_file = sqlite:///querytest.db aiosqlite_file = sqlite+aiosqlite:///async_querytest.db +pysqlcipher_file = sqlite+pysqlcipher://:test@/querytest.db.enc postgresql = postgresql://scott:tiger@127.0.0.1:5432/test asyncpg = postgresql+asyncpg://scott:tiger@127.0.0.1:5432/test asyncpg_fallback = postgresql+asyncpg://scott:tiger@127.0.0.1:5432/test?async_fallback=true diff --git a/test/dialect/test_sqlite.py b/test/dialect/test_sqlite.py index 00bf109c02..7fd7ac333c 100644 --- a/test/dialect/test_sqlite.py +++ b/test/dialect/test_sqlite.py @@ -74,6 +74,8 @@ class TestTypes(fixtures.TestBase, AssertsExecutionResults): __only_on__ = "sqlite" + __backend__ = True + def test_boolean(self, connection, metadata): """Test that the boolean only treats 1 as True""" @@ -288,6 +290,7 @@ class JSONTest(fixtures.TestBase): __requires__ = ("json_type",) __only_on__ = "sqlite" + __backend__ = True @testing.requires.reflects_json_type def test_reflection(self, connection, metadata): @@ -445,6 +448,7 @@ class TimeTest(fixtures.TestBase, AssertsCompiledSQL): class DefaultsTest(fixtures.TestBase, AssertsCompiledSQL): __only_on__ = "sqlite" + __backend__ = True def test_default_reflection(self, connection, metadata): @@ -579,6 +583,7 @@ class DialectTest( ): __only_on__ = "sqlite" + __backend__ = True def test_3_7_16_warning(self): with expect_warnings( @@ -771,10 +776,11 @@ class DialectTest( class AttachedDBTest(fixtures.TestBase): __only_on__ = "sqlite" + __backend__ = True def _fixture(self): meta = self.metadata - # self.conn = self.engine.connect() + Table("created", meta, Column("foo", Integer), Column("bar", String)) Table("local_only", meta, Column("q", Integer), Column("p", Integer)) @@ -1331,6 +1337,7 @@ class InsertTest(fixtures.TestBase, AssertsExecutionResults): """Tests inserts and autoincrement.""" __only_on__ = "sqlite" + __backend__ = True # empty insert was added as of sqlite 3.3.8. @@ -1499,6 +1506,7 @@ class MatchTest(fixtures.TestBase, AssertsCompiledSQL): __only_on__ = "sqlite" __skip_if__ = (full_text_search_missing,) + __backend__ = True @classmethod def setup_test_class(cls): @@ -1686,6 +1694,7 @@ class AutoIncrementTest(fixtures.TestBase, AssertsCompiledSQL): class ReflectHeadlessFKsTest(fixtures.TestBase): __only_on__ = "sqlite" + __backend__ = True def setup_test(self): exec_sql(testing.db, "CREATE TABLE a (id INTEGER PRIMARY KEY)") @@ -1709,6 +1718,7 @@ class ReflectHeadlessFKsTest(fixtures.TestBase): class KeywordInDatabaseNameTest(fixtures.TestBase): __only_on__ = "sqlite" + __backend__ = True @testing.fixture def db_fixture(self, connection): @@ -1732,6 +1742,7 @@ class KeywordInDatabaseNameTest(fixtures.TestBase): class ConstraintReflectionTest(fixtures.TestBase): __only_on__ = "sqlite" + __backend__ = True @classmethod def setup_test_class(cls): @@ -2364,6 +2375,7 @@ class SavepointTest(fixtures.TablesTest): """test that savepoints work when we use the correct event setup""" __only_on__ = "sqlite" + __backend__ = True @classmethod def define_tables(cls, metadata): @@ -2435,6 +2447,7 @@ class FutureSavepointTest(fixtures.FutureEngineMixin, SavepointTest): class TypeReflectionTest(fixtures.TestBase): __only_on__ = "sqlite" + __backend__ = True def _fixed_lookup_fixture(self): return [ @@ -2662,6 +2675,7 @@ class RegexpTest(fixtures.TestBase, testing.AssertsCompiledSQL): class OnConflictTest(fixtures.TablesTest): __only_on__ = ("sqlite >= 3.24.0",) + __backend__ = True @classmethod def define_tables(cls, metadata): diff --git a/tox.ini b/tox.ini index e01b82372f..7831405024 100644 --- a/tox.ini +++ b/tox.ini @@ -25,6 +25,7 @@ deps= sqlite: .[aiosqlite] sqlite_file: .[aiosqlite] + sqlite_file: .[sqlcipher]; python_version >= '3' postgresql: .[postgresql] postgresql: .[postgresql_asyncpg]; python_version >= '3' postgresql: .[postgresql_pg8000]; python_version >= '3' @@ -38,6 +39,9 @@ deps= mssql: .[mssql] + dbapimaster-sqlite: git+https://github.com/omnilib/aiosqlite.git#egg=aiosqlite + dbapimaster-sqlite: git+https://github.com/coleifer/sqlcipher3.git#egg=sqlcipher3 + dbapimaster-postgresql: git+https://github.com/psycopg/psycopg2.git@master#egg=psycopg2 dbapimaster-postgresql: git+https://github.com/MagicStack/asyncpg.git#egg=asyncpg dbapimaster-postgresql: git+https://github.com/tlocke/pg8000.git#egg=pg8000 @@ -88,7 +92,7 @@ setenv= sqlite: SQLITE={env:TOX_SQLITE:--db sqlite} sqlite_file: SQLITE={env:TOX_SQLITE_FILE:--db sqlite_file} py3{,5,6,7,8,9,10,11}-sqlite: EXTRA_SQLITE_DRIVERS={env:EXTRA_SQLITE_DRIVERS:--dbdriver sqlite --dbdriver aiosqlite} - py3{,5,6,7,8,9,10,11}-sqlite_file: EXTRA_SQLITE_DRIVERS={env:EXTRA_SQLITE_DRIVERS:--dbdriver sqlite --dbdriver aiosqlite} + py3{,5,6,7,8,9,10,11}-sqlite_file: EXTRA_SQLITE_DRIVERS={env:EXTRA_SQLITE_DRIVERS:--dbdriver sqlite --dbdriver aiosqlite --dbdriver pysqlcipher} postgresql: POSTGRESQL={env:TOX_POSTGRESQL:--db postgresql} py2{,7}-postgresql: POSTGRESQL={env:TOX_POSTGRESQL_PY2K:{env:TOX_POSTGRESQL:--db postgresql}} -- 2.47.2