Skip to content

Charlesmchen/unencrypted headers#439

Merged
robbiehanson merged 11 commits into
yapstudios:masterfrom
signalapp:charlesmchen/unencryptedHeaders
Mar 18, 2018
Merged

Charlesmchen/unencrypted headers#439
robbiehanson merged 11 commits into
yapstudios:masterfrom
signalapp:charlesmchen/unencryptedHeaders

Conversation

@charlesmchen
Copy link
Copy Markdown
Contributor

PTAL @michaelkirk

NOTE: The current approach supplies the database password and key separately. We could supply a database "key spec" instead, but I'm not sure this is a perf hotspot.

Comment thread YapDatabase/YapDatabaseOptions.h Outdated
* You must use the 'YapDatabase/SQLCipher' subspec
* in your Podfile for this option to take effect.
*
* cipherUnencryptedHeaderLength will be ignored if you don't also set cipherKeyBlock and cipherSaltBlock.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I wrote a little essay.

@chrisballinger
Copy link
Copy Markdown
Contributor

Should this also be automatically handling the migration, or including a utility function?

@charlesmchen
Copy link
Copy Markdown
Contributor Author

@chrisballinger Great question. We've got a utility class to handle the migration.

@charlesmchen
Copy link
Copy Markdown
Contributor Author

I'm going to close this PR and re-open it with a few modifications.

@charlesmchen
Copy link
Copy Markdown
Contributor Author

See: signalapp#1

@robbiehanson
Copy link
Copy Markdown
Contributor

Wow, great work on this ! I'm going to use it to convert the database in one of my apps very soon.

@robbiehanson robbiehanson merged commit ec857d7 into yapstudios:master Mar 18, 2018
@robbiehanson
Copy link
Copy Markdown
Contributor

robbiehanson commented Mar 18, 2018

Oh, quick question (since I know I'll get an email from the legal departments of various companies about it shortly after releasing the next version). There's a copyright at the top of YapDatabaseCryptoUtils. Are you cool with integrating this into the YapDatabase project, and having the code fall under the project license (2-clause BSD) ?

@charlesmchen
Copy link
Copy Markdown
Contributor Author

@robbiehanson Hi - thanks for merging this. This PR was closed because I realized that the work wasn't "finished" - I plan/planned to make some additional refinements.

Anyhow, I will hopefully eventually find the time to open a follow-up PR with some of these refinements and improved documentation, etc.

@charlesmchen charlesmchen deleted the charlesmchen/unencryptedHeaders branch March 19, 2018 23:10
@robbiehanson
Copy link
Copy Markdown
Contributor

I've subsequently realized that these migration tests need refinements. For example, the tests block on the YDB database closing. These tests should wait 5 seconds, since YDB can hold on to the cached "registration connection" for five seconds. The tests aren't breaking because they don't register any views, etc.

A more elegant solution may be to make use of the YapDatabaseClosedNotification:

/**
 * This notification is posted when a YapDatabase instance is deallocated,
 * and has thus closed all references to the underlying sqlite files.
 * 
 * If you intend to delete the sqlite file(s) from disk,
 * it's recommended you use this notification as a hook to do so.
 * 
 * More info:
 * The YapDatabase class itself is just a retainer for the filepath, blocks, config, etc.
 * And YapDatabaseConnection(s) open a sqlite connection to the database file,
 * and rely on the blocks & config in the parent YapDatabase class.
 * Thus a YapDatabaseConnection instance purposely retains the YapDatabase instance.
 * This means that in order to fully close all references to the underlying sqlite file(s),
 * you need to deallocate YapDatabase and all associated YapDatabaseConnections.
 * While this may be simple in concept, it's generally difficult to know exactly when all
 * the instances have been deallocated. Especially when there may be a bunch of asynchronous operations going.
 * 
 * Therefore the best approach is to do the following:
 * - destroy your YapDatabase instance (set it to nil)
 * - destroy all YapDatabaseConnection instances
 * - wait for YapDatabaseClosedNotification
 * - use notification as hook to delete all associated sqlite files from disk
 *
 * The userInfo dictionary will look like this:
 * @{
 *     YapDatabasePathKey    : <NSString of full filePath to db.sqlite file>,
 *     YapDatabasePathWalKey : <NSString of full filePath to db.sqlite-wal file>,
 *     YapDatabasePathShmKey : <NSString of full filePath to db.sqlite-shm file>,
 * }
 *
 * This notification is always posted to the main thread.
**/
extern NSString *const YapDatabaseClosedNotification;

extern NSString *const YapDatabasePathKey;
extern NSString *const YapDatabasePathWalKey;
extern NSString *const YapDatabasePathShmKey;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants