Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow Database Connection Configuration #508

Conversation

michaelkirk-signal
Copy link
Contributor

SQLCipher4.0.1 added a new pragma cipher_compatibility which can be helpful for folks upgrading from an older version of SQLCipher.

I'm aware that SQLCipher4 support is somewhat in-motion (#481) so it might not make sense to consider this PR yet, but I wanted to get it on your radar since it would be a breaking change, so it might make more sense to merge before the GRDB4 release.

from: https://www.zetetic.net/sqlcipher/sqlcipher-api/#cipher_compatibility

Calling this PRAGMA and passing in 1, 2, or 3 will cause SQLCipher to operate with default settings consistent with that major version number for the current connection, e.g. the following will cause SQLCipher to treat the current database as a SQLCipher 3.x database

PRAGMA cipher_compatibility = 3;

This is an easier/safer way to set pageSize/kdfIterations in a way consistent with previous major versions.

Because it doesn't make sense to use cipher_compatibility while also specifying custom pageSize/kdfIterations I've reworked the Configuration process to ensure that only one or the other can be specified, pulling out a CipherConfiguration struct in the process.

Pull Request Checklist

  • [ NO] This pull request is submitted against the development branch.

As this is a breaking change, I targeted the GRDB-4.0 branch, but let me know if you'd prefer otherwise.

  • Inline documentation has been updated.
  • README.md or another dedicated guide has been updated.
  • [ NO] Changes are tested, for all flavors of GRDB, GRDBCipher, and GRDBCustomSQLite.

SQLCipher4 tests aren't yet passing - I believe that's part of #481. If there's something I can do to help there, LMK. /cc @darrenclark

@michaelkirk-signal michaelkirk-signal force-pushed the mkirk/GRDB-4.0-sqlcipher-cipher_compatibility branch 2 times, most recently from bb2513f to c1206b1 Compare March 31, 2019 23:31
@groue
Copy link
Owner

groue commented Apr 1, 2019

Hello @michaelkirk-signal,

Thanks for this PR!

I have just closed #481, which was stuck. I may have to ship GRDB 4 with CocoaPods-only support for SQLCipher. And that's just OK: it can be improved in the future.

Before we move forward, May I ask you for your opinion on #497? I think @Marus had a lot of good ideas there. And I also provided some guidance in #497 (comment). We'd better synchronize our efforts.

@michaelkirk-signal
Copy link
Contributor Author

michaelkirk-signal commented Apr 1, 2019

Before we move forward, May I ask you for your opinion on #497? I think @Marus had a lot of good ideas there. And I also provided some guidance in #497 (comment). We'd better synchronize our efforts.

Thanks for the link - I didn't realize that the custom cipher params were so recently added. Agreed that we should synchronize!

from @groue in #497

Unless I'm mistaken, there are two transition options: 1. Don't change the database and "downgrade" SQLCipher 4 configuration so that it can deal with SQLCipher 3 defaults. 2. Dump the content of the old SQLCipher 3 database into a new SQLCipher 4 database.

I think that's directionally correct. We can either (1) specify the old defaults or (2) convert the database to use the new defaults.

As for (2) converting the the database to the new defaults, I'd recommend an approach based on PRAGMA cipher_migrate, but that's not included in this PR.

In terms of (1) how to specify the old defaults, as @Marus mentioned in #497, we'd need to specify a couple of other parameters. This can get a bit hairy:

SQLCipher 3

PRAGMA cipher_page_size = 1024;
PRAGMA kdf_iter = 64000;
PRAGMA cipher_hmac_algorithm = HMAC_SHA1;
PRAGMA cipher_kdf_algorithm = PBKDF2_HMAC_SHA1;

SQLCipher 2

PRAGMA cipher_page_size = 1024;
PRAGMA kdf_iter = 4000;
PRAGMA cipher_hmac_algorithm = HMAC_SHA1;
PRAGMA cipher_kdf_algorithm = PBKDF2_HMAC_SHA1;

SQLCipher 1

PRAGMA cipher_page_size = 1024;
PRAGMA kdf_iter = 4000;
PRAGMA cipher_hmac_algorithm = HMAC_SHA1;
PRAGMA cipher_kdf_algorithm = PBKDF2_HMAC_SHA1;
PRAGMA cipher_use_hmac = OFF;

Alternatively, what this PR leverages, you can call:

SQLCipher 3

PRAGMA cipher_compatibility = 3;

SQLCipher 2

PRAGMA cipher_compatibility = 2;

SQLCipher 1

PRAGMA cipher_compatibility = 1;

So in Swift, that's something like:

var config = Configuration()
config.passphrase = "secret"
config.cipherPageSize = 1204
config.kdfIterations = 6400

# API for these 2 don't exist yet
config.cipherHMACAlgorithm = "HMAC_SHA1"
config.cipherKDFAlgorithm = "PBKDF2_HMAC_SHA1"

vs.

var config = Configuration()
var cipherConfig = CipherConfiguration(passphrase: "secret")
cipherConfig.cipherParameters = .compatibility(version: 3)
config.cipherConfiguration = cipherConfiguration

Even if you do choose to merge this cipher_compatibility feature, note that I kept the work @Marus did in case users want to fine tune their cipher parameters. I did juggle things around a bit so that it's intentionally not possible to specify both cipher compatibility mode and custom cipher params. In my opinion the user will almost always want one or the other, so hopefully this helps them make the right decision.

As for the upgrade documentation, once there's a "Migrating to GRDB4" doc, I'd recommend including something like:

GRDB4 Migration Guide

[ other stuff here]

SQLCipher 4

GRDB now supports SQLCipher 4. If you have existing databases, there are some special considerations.

In all cases, the configuration syntax to use SQLCipher has changed slightly.

Previously, to use the default cipher configuration, you might have something like this:

var config = Configuration()
var config.passphrase = "secret"

Now that would be written like this:

var config = Configuration()
var cipherConfig = CipherConfiguration(passphrase: "secret")
config.cipherConfiguration = cipherConfig

Dealing with existing SQLCipher 3 databases

SQLCipher 4 introduced some changes to their default cipher configurations. This means that a database created with SQLCipher 3 defaults will not be readable using SQLCipher 4 unless you explicitly tell it to use the SQLCipher 3 defaults:

var config = Configuration()
var cipherConfig = CipherConfiguration(passphrase: "secret")

# use default cipher params from SQLCipher 3
cipherConfig.cipherParams = .compatibility(version: 3)

config.cipherConfiguration = cipherConfig

If you were using non-default params, you'll need to explicitly specify them like this:

var config = Configuration()
var cipherConfig = CipherConfiguration(passphrase: "secret")

# use custom cipher params
cipherConfig.cipherParams = .custom(pageSize: 1024, kdfIterations: 128000)

config.cipherConfiguration = cipherConfig

What do you think @Marus, @groue ?

@groue
Copy link
Owner

groue commented Apr 1, 2019

Hi again @michaelkirk-signal,

Thanks for joining the discussion!

(In all I'll be writing below, remember that I'm no SQLCipher expert, and not even an SQLCipher user. I just ship a library that is supposed to provide the necessary tools for people who are actual experts or users of SQLCipher. I mean that I may be wrong sometimes due to my lack of experience.)

First, I'm happy that we all agree about the three main use cases!

  1. Create a new SQLCipher 4 db with all recommended defaults.
  2. Open an existing SQLCipher 3 db with SQLCipher 4.
  3. Migrate an existing SQLCipher 3 db into a new SQLCipher 4 db.

I'm glad you have mentioned SQLCipher 1 and 2, even if GRDB has never explicitly supported them, never documented anything about them, and no user has ever asked about them. I suppose the existing support for raw SQL pragmas have provided the necessary support.

Generally speaking, we don't have to provide high-level APIs when existing low-level ones are sufficient. This is especially true for our two last use cases: if a user asks Google, Stack Overflow, or the SQLCipher forums how to deal with his SQLCipher 3 database, he will be told to set a few options and run a few pragmas. Now he just expects GRDB to allow exactly that. He won't look for a high-level API that may, or not, apply the recipe he wants to run.

I admit I have somewhat over-simplified things in the last two paragraphs: @Marus has shown in #497 that database pools need special care. They create several database connections, and each one of them need to be configured before it can access an SQLCipher database that is not configured with default options.

@Marus has exposed some necessary pragmas as GRDB.Configuration properties. This is quite a valid approach. We'll see below that we may be even happier with an alternative take on the subject.

But please follow me in your comment:

In terms of (1) how to specify the old defaults, as @Marus mentioned in #497, we'd need to specify a couple of other parameters. This can get a bit hairy:

SQLCipher 3

PRAGMA cipher_page_size = 1024;
PRAGMA kdf_iter = 64000;
PRAGMA cipher_hmac_algorithm = HMAC_SHA1;
PRAGMA cipher_kdf_algorithm = PBKDF2_HMAC_SHA1;

...

Well, is it really hairy?

If you're an expert, you know what it means. And if you're not an expert, this just means that you are applying some recipe found somewhere on the internet. In both cases, you are glad and relieved when you find those options.

I have turned very confident about this way of thinking about expert options since #498, an eye-opener for me. A GRDB API which was "swifty" and well typed... completely missed its target because it did not look like the raw option the user was looking for.

Let's keep on:

Even if you do choose to merge this cipher_compatibility feature, note that I kept the work @Marus did in case users want to fine tune their cipher parameters. I did juggle things around a bit so that it's intentionally not possible to specify both cipher compatibility mode and custom cipher params.

Yes, I see your Parameters enum:

public enum Parameters {
    case defaultParameters
    case compatibility(version: UInt)
    case custom(cipherPageSize: Int?, kdfIteration: Int?)
}

I totally understand what you want to do, and what you want to prevent. Yes, the compatibility pragmas don't mix well with individual options.

This means your PR is correct. This is the best quality a PR can aim for! 👍

But I also worry we are missing the point 😓

Why? Because it is difficult for experts to find their way in a complex hierarchy of GRDB types and values that more or less look like their raw SQLCipher counterpart. And non-expert users will suffer a double punishment: first they'll fight Google in order to find a solution for their problem, and then they'll fight the GRDB API and documentation until it does what they hope it should do.


What do you think @Marus, @groue ?

What about the following:

  1. We revert Add additional SQLCipher configuration options #497 (!!)

  2. We introduce a new Configuration option: a closure whose general purpose is to configure all database connections created by a Database Queue, Pool, and Snapshot.

    var configuration = Configuration()
    configuration.prepareDatabase { db in
        try db.execute(sql: "PRAGMA cipher_compatibility = 3")
    }
    let dbPool = try DatabasePool("/path/to/db", configuration: configuration)

    (I'm not sure about the actual method or property name).

  3. We put most of our energy in writing a handy guide that answers the most common questions about SQLCipher transition. This guide will use the new configuration closure above, and fill it with raw sql queries because we don't really need more.

This is a somewhat drastic change. I hope it doesn't sound too bold, and above all that it allows what we want to do!

@michaelkirk-signal
Copy link
Contributor Author

Thanks for thoughtful followup @groue.

I think the approach you've outlined sounds great!

Let me know how I can help.

@groue
Copy link
Owner

groue commented Apr 2, 2019

I'm glad I could make sense, @michaelkirk-signal :-)

I'm looking right now at the detailed database opening code. It is currently in two steps: Database.init(), and Database.setup(). Database pool adds a third one, DatabasePool.setupDatabase(_:), which has little to do with our topic.

PRAGMA cipher_page_size and PRAGMA kdf_iter (and, I presume, cipher_hmac_algorithm, cipher_kdf_algorithm, and cipher_compatibility) must be run before any other db access is done. Since Database.init currently does a database access (it calls Database.validateDatabaseFormat), @Marus, in #497, had to run those pragmas right from Database.init.

And because those pragmas run before the Database Swift object is fully initialized, @Marus could not use regular db.execute("PRAGMA ...") GRDB apis. He had to calls raw sqlite3_prepare_v2 and sqlite3_step C functions.

Only later happens further setup and configuration such as format validation, trace, foreign keys, busy mode, observation setup, and generally everything GRDB users take for granted.

Consequence: it looks like my suggestion, the one which lets the user give a { db in try db.execute(...) } closure, requires some refactoring of the database initialization process. This closure must run after Database.init: nearly all initialization steps would thus have to be moved from Database.init() to Database.setup().

Does this look like something you would like to do? Tests added by #497 will be a perfect validation for this refactoring.

@groue
Copy link
Owner

groue commented Apr 2, 2019

But... you'll need SQLCipher 4, and #481 has been discarded. How did you use SQLCipher 4 in this PR?

@groue
Copy link
Owner

groue commented Apr 2, 2019

But... you'll need SQLCipher 4, and #481 has been discarded. How did you use SQLCipher 4 in this PR?

The answer is in the SQLCipher-4 branch.

Any SQLCipher-related PR should be submitted against this branch, which will eventually be merged in GRDB-4.

The SQLCipher-4 branch is at a very preliminary phase. I did the bare minimum until you could run the GRDBCipher tests for OSX and iOS. A few tests fail, you can ignore them.

I lack the Xcode skills that would let me keep on the work started in #481, so I have integrated SQLCipher as a CocoaPods. After you clone this branch, run pod install before you open the GRDB workspace and run the GRDBCipher(OSX/iOS) tests.

@michaelkirk-signal michaelkirk-signal force-pushed the mkirk/GRDB-4.0-sqlcipher-cipher_compatibility branch from c1206b1 to 3ea41d5 Compare April 2, 2019 16:46
@michaelkirk-signal michaelkirk-signal force-pushed the mkirk/GRDB-4.0-sqlcipher-cipher_compatibility branch from 3ea41d5 to 99065a4 Compare April 2, 2019 16:47
@michaelkirk-signal
Copy link
Contributor Author

Does this look like something you would like to do? Tests added by #497 will be a perfect validation for this refactoring.

I've updated my PR (force-pushed) in an attempt to reflect this approach.

If it looks directionally correct, I'll follow up with some documentation updates.

The tests are passing and this works for my cipher_compatibility needs.

But... you'll need SQLCipher 4, and #481 has been discarded. How did you use SQLCipher 4 in this PR?

I simply bumped the Podspec SQLCipher constraint and was able to build against SQLCipher4. I'll have a look at rebasing on the SQLCipher-4 branch you've set up.

@groue
Copy link
Owner

groue commented Apr 3, 2019

But... you'll need SQLCipher 4, and #481 has been discarded. How did you use SQLCipher 4 in this PR?

I simply bumped the Podspec SQLCipher constraint and was able to build against SQLCipher4. I'll have a look at rebasing on the SQLCipher-4 branch you've set up.

No, no, don't rebase. I rushed this message. As a matter of fact, your PR is now just OK with SQLCipher 3 too! Our new approach has removed any dependency on the SQLCipher version :-)

Does this look like something you would like to do? Tests added by #497 will be a perfect validation for this refactoring.

I've updated my PR (force-pushed) in an attempt to reflect this approach.

If it looks directionally correct, I'll follow up with some documentation updates.

The tests are passing and this works for my cipher_compatibility needs.

This looks great. I'll have a detailed look and answer your questions shortly 👍

Copy link
Owner

@groue groue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much @michaelkirk-signal! Please look at the few requests for changes, and we'll be good to go!

GRDB/Core/Database.swift Outdated Show resolved Hide resolved
GRDB/Core/Database.swift Outdated Show resolved Hide resolved
GRDB/Core/Database.swift Outdated Show resolved Hide resolved
GRDB/Core/Database.swift Outdated Show resolved Hide resolved
GRDB/Core/SerializedDatabase.swift Show resolved Hide resolved
GRDBCipher.podspec Outdated Show resolved Hide resolved
GRDB/Core/Configuration.swift Outdated Show resolved Hide resolved
@michaelkirk-signal
Copy link
Contributor Author

OK @groue, thanks for the feedback so far.

I've taken another pass at this. Can you please take another look?

@groue
Copy link
Owner

groue commented Apr 5, 2019

👍 Thanks @michaelkirk-signal ! I'll look at your changes on the beginning of the next week.

@groue groue added this to the GRDB 4.0.0 milestone Apr 5, 2019
@groue groue changed the title Add support for #PRAGMA cipher_compatibility Allow Database Connection Configuration Apr 5, 2019
@groue groue mentioned this pull request Apr 5, 2019
26 tasks
@groue
Copy link
Owner

groue commented Apr 9, 2019

Thank you very much @michaelkirk-signal, the PR is now ready, and I'm very happy of it 👏

Let's wait for Travis tests, and we'll merge this ASAP. Welcome to the GRDB contributors :-)

@groue
Copy link
Owner

groue commented Apr 9, 2019

@Marus: I'm pretty confident you'll be happy with this PR, as it extends #497 in a pretty natural way. And we get free support for the SQLCipher 4 pragmas cipher_kdf_algorithm and cipher_hmac_algorithm :-) Without you, the whole context would have been much less clear 👍

@michaelkirk-signal
Copy link
Contributor Author

michaelkirk-signal commented Apr 10, 2019 via email

@groue groue merged commit be95016 into groue:GRDB-4.0 Apr 10, 2019
@groue
Copy link
Owner

groue commented Apr 10, 2019

Merged 🎉

@groue
Copy link
Owner

groue commented Apr 18, 2019

@michaelkirk I forgot to propose you a push access to the repository. Just drop me a line if you are interested!

@groue groue mentioned this pull request Aug 6, 2019
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants