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

Support for SQLite Encryption Extension (SEE) #509

Closed
wants to merge 18 commits into from

Conversation

barnettben
Copy link
Collaborator

@barnettben barnettben commented Apr 3, 2019

This pull request adds code to support the SQLite Encryption Extension.
(see #506)

The changes are almost all additive and mostly involve adding functions to set/change a key.

All code for this is behind the conditional compilation flag GRDB_SQLITE_SEE.

Notes/questions:

  • The SEE code needs a custom build of SQLite to be used. I intend to submit patches to SQLiteLib to make it easier to support this.

  • It can't be used at the same time as GRDBCipher and should probably throw a compiler error if both GRDB_SQLITE_SEE and GRDBCIPHER are defined. I haven't implemented this. Where would be the best place to put this?

  • You need a paid license from Hwaci to use the SEE code. This means that many people won't be able to run the code or tests. I have run them and it builds/tests ok here.

  • Documentation hasn't been updated yet. I will submit changes to the custom builds guide before asking for a merge.

  • This is my first contribution to a well-used open source project and would be grateful for any suggestions for improvement

Only expose SQLCipher validation code if we are using SQLCipher and not another encryption extension.
It’s all hidden behind a new flag called GRDB_SQLITE_SEE. This must be defined along with SQLITE_HAS_CODEC.
Also renamed existing SQLCipher tests to make distinction clear.
@groue
Copy link
Owner

groue commented Apr 3, 2019

Thank you very much @BenRB!

All code for this is behind the conditional compilation flag GRDB_SQLITE_SEE.

Notes/questions:

  • The SEE code needs a custom build of SQLite to be used. I intend to submit patches to SQLiteLib to make it easier to support this.

All right. This can be done independently from this pull request And I'll be happy merging a future PR with an updated SQLiteLib.

  • It can't be used at the same time as GRDBCipher and should probably throw a compiler error if both GRDB_SQLITE_SEE and GRDBCIPHER are defined. I haven't implemented this. Where would be the best place to put this?

Do you think it's a mistake we should expect people to do, and thus plan for with an explicit error? If not, I don't think this is really necessary.

  • You need a paid license from Hwaci to use the SEE code. This means that many people won't be able to run the code or tests. I have run them and it builds/tests ok here.

Yes. I hope you will tell us when a GRDB change inadvertently breaks SEE builds 😅. I'll take care of avoiding the most obvious mistakes. Consider SEE as an officially supported platform now 👍

  • Documentation hasn't been updated yet. I will submit changes to the custom builds guide before asking for a merge.

Thank you! A documentation update will be very much appreciated.

  • This is my first contribution to a well-used open source project and would be grateful for any suggestions for improvement

I'll have a detailed look very shortly. What I see looks like a very fine first contribution 🤝

Missing from last commit
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.

This is a very good pull request. And thank you for fitting in the existing code base, this is really appreciated. I suggest a few changes that I hope you will agree on.

GRDB/Core/DatabasePool.swift Outdated Show resolved Hide resolved
GRDB/Core/Database.swift Outdated Show resolved Hide resolved
GRDB/Core/Database.swift 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/Database.swift Outdated Show resolved Hide resolved
GRDBCustom.xcodeproj/project.pbxproj Show resolved Hide resolved
GRDB/Core/Configuration.swift Outdated Show resolved Hide resolved
Tests/GRDBTests/SQLiteEncryptionExtensionTests.swift Outdated Show resolved Hide resolved
@barnettben
Copy link
Collaborator Author

Thank you for taking the time to provide feedback, it's much appreciated. I will implement the changes and push updates to this branch.

EncryptionType -> Algorithm
passphrase -> key
Tidy enum values
@groue
Copy link
Owner

groue commented Apr 4, 2019

Thanks for the changes!

What should we do now? Keep this pull request open until the SQLiteLib update, so that we can update the documentation with accurate information?

@groue groue changed the title Sqlite see support Support for SQLite Encryption Extension (SEE) Apr 4, 2019
@barnettben
Copy link
Collaborator Author

I've updated the CustomSQLiteBuilds documentation and noted that Swift 5 is required to use SEE.

I've also submitted a pull request to SQLiteLib: swiftlyfalling/SQLiteLib#40

Hopefully that will be accepted and all the pieces will be in place to allow this to be merged and closed.

@groue
Copy link
Owner

groue commented Apr 5, 2019

OK, let's wait for @swiftlyfalling!

@groue groue added this to the GRDB 4.0.0 milestone Apr 5, 2019
@groue groue mentioned this pull request Apr 5, 2019
26 tasks
@groue
Copy link
Owner

groue commented Apr 10, 2019

@BenRB, I've just merged #508, which unfortunately brings conflicts with your pull request. Tell me if you have any trouble merging or rebasing GRDB-4.0 in your branch: I'll easily overcome the difficulty since I'm familiar with both pull requests.

I hope @swiftlyfalling will soon look at swiftlyfalling/SQLiteLib#40. I know how to contact him by email, but I think we should wait a couple more days before I do.

Only expose SQLCipher validation code if we are using SQLCipher and not another encryption extension.
It’s all hidden behind a new flag called GRDB_SQLITE_SEE. This must be defined along with SQLITE_HAS_CODEC.
Also renamed existing SQLCipher tests to make distinction clear.
Missing from last commit
EncryptionType -> Algorithm
passphrase -> key
Tidy enum values
Don’t bother testing for error descriptions, these might change.

Just check error codes and messages from SQLite as these remain more constant.
… sqlite-see-support

# Conflicts:
#	GRDB/Core/Configuration.swift
#	GRDB/Core/Database.swift
#	Tests/GRDBTests/SQLiteEncryptionExtensionTests.swift
@barnettben
Copy link
Collaborator Author

I've rebased my changes onto the new GRDB-4.0 branch and checked that the tests pass

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.

Thanks for rebasing your changes, @BenRB!

Something went off, though, because your pull request does not just add commits to GRDB-4.0. It looks like it was forked from some previous commit. This is not a big problem, but this has hidden some issues in the rebase:

Your pull requests adds five methods that are no longer used and should be removed:

  • static func activateExtendedCodes
  • static func validateSQLCipher
  • static func set(passphrase: forConnection:)
  • static func set(cipherPageSize: forConnection:)
  • static func set(kdfIterations: forConnection:)

The GitHub diff does not show this very clearly, because of the weird rebasing. But I was able to spot it very clearly by checking out your branch, and diffing from the head of GRDB-4.0.

Meanwhile, I contact @swiftlyfalling.

GRDB/Core/Database.swift Outdated Show resolved Hide resolved
@groue
Copy link
Owner

groue commented Apr 17, 2019

I still have no news from @swiftlyfalling.

I don't see myself fork SQLiteLib and commit to its maintenance. I'm not comfortable enough with low-level Xcode project configuration. GRDB support for custom SQLite builds would become a pain point, and this is the highest danger for a free open source library with a very very little core team (read: me).

So we may have to ship GRDB 4 without SEE support, depending on my own schedule.

I'm not very worried about your particular setup, @BenRB, because you show a great autonomy. But I would be pleased to add SEE to the "officially" supported platforms.

@barnettben
Copy link
Collaborator Author

I have made the requested changes and removed the extra code left by the bad rebase.

If it is OK with you, I am happy to wait for SQLiteLib to be updated (assuming @swiftlyfalling is OK with my change!). Until then, I'm fine to use my own fork of GRDB for my projects and keep updating this PR as needed.

@groue groue removed this from the GRDB 4.0.0 milestone May 27, 2019
@groue
Copy link
Owner

groue commented May 27, 2019

@BenRB, do you have any update on this PR?

@swiftlyfalling
Copy link
Collaborator

@groue, @BenRB: Apologies for not posting an update here as well - swiftlyfalling/SQLiteLib#40 was merged a little while ago, so should be all set there.

@groue
Copy link
Owner

groue commented May 29, 2019

Hello @swiftlyfalling, it's good to hear from you :-) Let's wait for @BenRB, there is no rush. And thanks for SQLite 3.28 in swiftlyfalling/SQLiteLib#41 👍

@groue groue closed this Jun 13, 2019
@groue
Copy link
Owner

groue commented Jun 13, 2019

Oops, I did not intend to close this PR, but I deleted the target GRDB-4.0 branch. @BenRB, if you intend to complete the support for SEE, please reopen and target the development branch.

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