Add support for SQLeet full database encryption #93

alinz · 2020-06-14T21:10:26Z

This is a PR related to issue #92.

AdamSLevy

Thanks so much for getting the ball rolling! Please let me know if you have any questions about my comments or need more direction or input.

AdamSLevy · 2020-06-15T00:10:54Z

c/sqlite3.c

@@ -1,3 +1,47 @@
+#if (defined(__linux__) || defined(__unix__)) && !defined(_GNU_SOURCE)


I want to support both the latest release of the official SQLite and the latest release of SQLeet, selected using built tags. This will probably require two of these c "dummy" packages: one for the sqleet C source and one for the official SQLite source. Then two separate files in the main sqlite package will include each of the "dummy" packages. Build tags will then target one of these files.

I can push a commit that implements this with the right build tags if you want my help with this aspect. Let me know if you have any questions.

I would need your help on this one as it requires touching c and go. Sqleet is on 3.31.1 version. I was tried to build with new sqlite but got into some rough compile issue in c. I'm using mac and some of the sed command is not working properly and what I ended up was running the code inside a docker.

AdamSLevy · 2020-06-15T05:33:32Z

examples/encryption/main.go

@@ -0,0 +1,64 @@
+package main


I really appreciate that you included an example. However are you familiar with Golang test examples? I'd like to see this example changed into a golang test example. https://blog.golang.org/examples

AdamSLevy · 2020-06-15T05:34:00Z

examples/encryption/main.go

+	dbString := "file:./database.db?key=swordfish&foreign_keys=on"
+
+	poolSize := 10
+
+	dbpool, err := sqlitex.Open(dbString, 0, poolSize)
+	if err != nil {
+		panic(err)
+	}
+
+	// enable all connection to have Foreign key feature
+	for i := 0; i < poolSize; i++ {
+		conn := dbpool.Get(nil)
+		err := sqlitex.Exec(conn, `PRAGMA foreign_keys = ON;`, nil)
+		if err != nil {
+			panic(err)
+		}
+		dbpool.Put(conn)
+	}


Let's keep examples as minimal as possible to demonstrate a feature. Let's not use a pool here, but just a single connection created with sqlite.OpenConn. Foreign key support has nothing to do with encryption so it should not be used or mentioned in this example either.

Another note for your own information regarding enabling foreign keys or other PRAGMA features. Since you already specified it in the URI, this loop to call the PRAGMA is not needed.

I did try using foreign_keys connection string and it did not work. then I tried using PRAGMA and it didn't work until I filed an issue #91 and you answered it.

AdamSLevy · 2020-06-15T05:35:31Z

examples/encryption/main.go

+	// // enable all connection to have Foreign key feature
+	// for i := 0; i < poolSize; i++ {
+	// 	conn := dbpool.Get(nil)
+	// 	err := sqlitex.Exec(conn, `PRAGMA key='swordfish';`, nil)
+	// 	if err != nil {
+	// 		panic(err)
+	// 	}
+	// 	dbpool.Put(conn)
+	// }


I know this is a work in progress but let's avoid keeping commented out code around.

cleaned up the example

AdamSLevy · 2020-06-15T05:37:24Z

examples/encryption/main.go

+	err = sqlitex.Exec(conn, `PRAGMA key=swordfish;`, nil)
+	if err != nil {
+		panic(err)
+	}
+
+	err = sqlite.Crypto(conn, "swordfish2")
+	if err != nil {
+		panic(err)
+	}


These steps (PRAGMA key and sqlite.Crypto()) are redundant and use different keys. Also note that PRAGMAs never return errors, which is why it is generally better to use a programmatic API where possible. We should not use the PRAGMA, and use the programmatic API in examples to guide users to best practices.

Update the example. couple of observation,

if I use connection string key, it does encrypt and decrypt the database properly

if I use sqlite.ApplyKey it does not encrypt database and it errors out: sqlite.Conn.Unlock: SQLITE_AUTH

if I use sqlite.ApplyRekey it does encrypt an unencrypted database or encrypt with the new key. However I have to use connection string to connect back to database.

I think you are missing some nuances to how the encryption API works. I recommend reviewing the documentation for SQLeet, and use examples that exclusively use the API. Don't confuse the issue by using settings in the connection string.

The current flaw with your example, which still persists, is that it does not distinguish between an already encrypted database, and a new, or not yet encrypted, database.

You cannot encrypt an unencrypted or new database as encrypted by using a setting in the connection string. Thus if you open a new database with the URI file:./database.db?key=swordfish, the key param will have no effect and you will still be working with an unencrypted database. Only if the database was already encrypted, and this is the correct key, will it be opened already unlocked.

Anytime you wish to encrypt an un-encrypted or new database file, you MUST use PRAGMA rekey or preferably the API equivalent. This initially will set up the encryption on the file. Thereafter you must use PRAGMA key to unlock the file before reading or writing from it on each connection. This appears to support using the param in the connection string. Only after this occurs, can PRAGMA rekey be successfully called to change the key on an already encrypted database.

I hope this clarifies the correct use of the APIs. Really the example needs a flag that controls whether we are unlocking an encrypted database or creating a new encrypted database. Ultimately though, I want these in golang test examples so they should up in the documentation. There should be two examples for each case I just mentioned.

@alinz I experimented a bit more and I discovered that I was somewhat incorrect about the use of the key param in the connection string. I was not specifying the connection string properly in my own experimentation, which threw me off. You can in fact initialize a new database to be encrypted with a given key using key= in the connection string. The rest of what I said about the use of the API is correct but the SQLeet README describes many advanced settings that can only be set in the URI string. https://github.com/resilar/sqleet/tree/v0.31.1#uri-configuration-interface This is easy to get wrong so in the future I plan to introduce some helper types/functions for building a URI with the correct settings.

Thanks for the update.

AdamSLevy · 2020-06-15T05:54:30Z

sqlite.go

+	ckey := C.CString(key)
+	defer C.free(unsafe.Pointer(ckey))
+
+	// convert CString to void*: https://jamesadam.me/2016/03/26/c-and-go-dealing-with-void-parameters-in-cgo/


I appreciate you documenting the technique, but CGo basics are in the space of assumed knowledge for this package and don't need to be documented.

Understood, removed the comment

AdamSLevy · 2020-06-15T06:19:24Z

sqlite.go

+	defer C.free(unsafe.Pointer(ckey))
+
+	// convert CString to void*: https://jamesadam.me/2016/03/26/c-and-go-dealing-with-void-parameters-in-cgo/
+	res := C.sqlite3_key(conn.conn, unsafe.Pointer(&ckey), 0)


There are two things wrong with this call that were likely causing you some issues. First, the function signature in C is this

int sqlite3_key( sqlite3 *db, /* The connection from sqlite3_open() */ const void *pKey, /* The key */ int nKey /* Number of bytes in the key */ );

First note that by passing ckey is already a pointer. So by passing &ckey you are passing a pointer to a pointer. This is not something C can prevent you from. It has no idea you passed it the wrong thing, but this will produce unexpected behavior.

The next parameter needs to be the length of the string or byte slice. By passing zero you are using an empty key.

Thanks for pointing this out, I don't know what I was thinking. I have cleaned that up and also implemented Rekey. I came up with ApplyKey and ApplyRekey. I would gladly change it if you have a better names.

AdamSLevy · 2020-06-15T06:20:53Z

sqlite.go

+	// convert CString to void*: https://jamesadam.me/2016/03/26/c-and-go-dealing-with-void-parameters-in-cgo/
+	res := C.sqlite3_key(conn.conn, unsafe.Pointer(&ckey), 0)
+	if res != C.SQLITE_OK {
+		return fmt.Errorf("something went wrong")


At minimum errors need to indicate what went wrong or where. In this package we have a function reserr to help format errors. This should be return reserr("Conn.Unlock", "", "", res).

Fixed the error issue

AdamSLevy · 2020-06-15T06:23:00Z

sqlite3.h

@@ -1,3 +1,40 @@
+/*


Similar to the C files we'll have to somehow use build tags to control which header file to use.

I need your help on this

AdamSLevy · 2020-06-15T06:23:42Z

sqlite_test.go

+		// {
+		// 	"",
+		// 	"delete",
+		// 	0,
+		// },


Why did you remove this test? This should be restored.

The test was failing and it was kind of confusing:

--- FAIL: TestJournalMode (0.00s) sqlite_test.go:534: journal_mode not set properly, got: memory, want: delete

I figured out the root cause here. Basically SQLeet uses a different default for temp_store than SQLite3 uses. This caused this to be a different journal_mode than expected for temporary database files. I have a commit coming that addresses this for this test.

In the future if a test is failing please just let the failure be known so it can be addressed.

AdamSLevy

Good improvements. However it's not quite ready so I'm going to push a commit that brings it in line with what I want.

AdamSLevy · 2020-06-21T23:11:42Z

@alinz I pushed a rather large commit that gets everything working as I'd like using build tags. The last things to add are some more package level and README docs about this feature. I'm probably going to add those things next weekend and likely merge it if I don't hear any objections from @crawshaw before then. I'll also deal with the conflicts with master next weekend as well.

Thanks for getting the ball rolling on this. This also lays the groundwork for adding support for SQLCipher as well if someone wants to go that route.

AdamSLevy · 2020-06-22T05:33:17Z

@alinz you don't need to do anything. I already merged master into this branch. It's good to go. Let me know if it doesn't work for you after you pull.

AdamSLevy · 2020-06-22T05:33:55Z

I see you fixed the import. Thanks.

alinz · 2020-06-22T05:35:41Z

@AdamSLevy I will revert my change. This is funny, both of us were trying to fix the same issue 🤣

This reverts commit 6ee59f0.

alinz · 2020-06-22T05:43:57Z

@AdamSLevy There is only one small issue, The version of sqlite3 in sqleet is 3.31.1 but the actual sqlite3 is 3.32.2. I did try to compile sqleet to the latest sqlite3, but wasn't be able to compile and rebuild rekeyvacuum.c. If version difference is not that important, I can defer it to later.

Amazing job, thanks for pushing this forward.

AdamSLevy · 2020-06-22T05:58:01Z

@alinz why did you revert that change? It was needed and you pushed it first. It's all good when that happens. My commit would have looked identical to yours, and if there had been issues they can always be fixed. Anyway I've merge master to this branch again, which contains that fix. So you don't need to do anything. But in the future please don't un-fix things you just fixed :)

The version of sqlite3 in sqleet is 3.31.1 but the actual sqlite3 is 3.32.2.

This is OK. If you run go build it will use the latest release of SQLite3: 3.32.2. If you run go build -tags=sqleet it will build with the latest version of SQLeet: 0.31.1. When SQLeet releases version 0.32.2 then I'll update the SQLeet C code. If you want to build the SQLeet amalgamation C file against the latest SQLite release, you are welcome to maintain that in your own fork, but I'm not going to support that for the main branch.

I did try to compile sqleet to the latest sqlite3, but wasn't be able to compile and rebuild rekeyvacuum.c

I don't understand what you're talking about here. There is no such file on this branch or in this repo.

alinz · 2020-06-22T06:14:12Z

The reason I reverted it was because my change caused an unnecessary merge conflicts. I was using multiple imports, you were using single import.

I had to be more descriptive about the last problem, I was talking about sqleet's main repo. I was trying to build it from source so I could put the amalgamation C file into this project. I guess I will wait until they support it.

AdamSLevy · 2020-06-27T21:32:15Z

So I am afraid after doing more research on updating sqleet to the latest sqlite version I am going to hold off on merging this PR. I'm going to leave it open though as it works for the latest version of sqleet.
resilar/sqleet#38

The reason is that there is currently no way for sqleet to support versions of sqlite3 past 3.32.0 because of some internal changes to sqlite. I don't want to steer users of this package towards adopting an encryption library that is not currently supported.

I am interested in finding the right encryption extension. It's likely that SQLcipher will eventually get a work around to this, but that will take some time. It's possible sqleet will adapt at some point in the future. but I don't want to merge this quite yet. Another promising project is here: https://utelle.github.io/SQLite3MultipleCiphers/

I haven't totally ruled out merging this PR yet, I'm just not ready to pull the trigger. I want to give these other projects some time to adapt to SQLite's latest changes.

I do plan to port some of the work I did in this PR to make it easier to control how SQLite is compiled. It is a goal to allow users of this package to link against a shared system library for SQLite instead of compiling it in. The work in the branch lays the groundwork for some of that.

alinz · 2020-08-27T19:20:25Z

@AdamSLevy I believe SQLite3MultipleCiphers has released first major version which support new version of sqlite interface.

The new release can be found here:
https://github.com/utelle/SQLite3MultipleCiphers/releases/tag/v1.0.0

Let me know if you need any help on my end.

AdamSLevy · 2020-08-27T19:21:52Z

That's awesome. I'm probably not going to have time to work on this few a couple weeks at least. But I will eventually take a look.

alinz · 2020-10-05T19:59:39Z

@AdamSLevy I'd like to help to push this forward. I just need some guidance on your plan.

alinz · 2020-12-31T17:54:04Z

@AdamSLevy happy new year, just wanted to see if you plan to work on this, or anything I can do to help you on this

alinz added 3 commits June 13, 2020 19:40

Add sqleet

1cd04fb

Export sqlite3_key to Go

950b706

Add encryption example

4047d7a

AdamSLevy changed the title ~~Port Sqleet back to v3.31.1~~ Add support for SQLeet full database encryption Jun 14, 2020

AdamSLevy requested changes Jun 15, 2020

View reviewed changes

alinz added 3 commits June 15, 2020 11:36

Update pointer and implement Rekey

3c1146b

uncommented broken test

12fe405

Update example

9e31135

alinz requested a review from AdamSLevy June 15, 2020 16:37

AdamSLevy reviewed Jun 21, 2020

View reviewed changes

Support both SQLite and SQLeet using build tags

9403ac2

AdamSLevy force-pushed the encryption branch from dbc35d1 to 9403ac2 Compare June 21, 2020 23:03

AdamSLevy requested a review from crawshaw June 21, 2020 23:10

AdamSLevy and others added 3 commits June 21, 2020 20:04

Merge branch 'master' into encryption

4eaadc2

Merge branch 'master' into encryption

e9be99f

Added fmt package

6ee59f0

Revert "Added fmt package"

6b391fd

This reverts commit 6ee59f0.

Merge branch 'master' into encryption

89651cb

		@@ -1,3 +1,47 @@
		#if (defined(__linux__) \|\| defined(__unix__)) && !defined(_GNU_SOURCE)

Add support for SQLeet full database encryption #93

Are you sure you want to change the base?

Add support for SQLeet full database encryption #93

Conversation

alinz commented Jun 14, 2020

AdamSLevy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alinz Jun 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alinz Jun 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdamSLevy left a comment

Choose a reason for hiding this comment

AdamSLevy commented Jun 21, 2020 • edited Loading

AdamSLevy commented Jun 22, 2020

AdamSLevy commented Jun 22, 2020

alinz commented Jun 22, 2020 • edited Loading

alinz commented Jun 22, 2020

AdamSLevy commented Jun 22, 2020

alinz commented Jun 22, 2020

AdamSLevy commented Jun 27, 2020

alinz commented Aug 27, 2020

AdamSLevy commented Aug 27, 2020

alinz commented Oct 5, 2020

alinz commented Dec 31, 2020

alinz Jun 15, 2020 •

edited

Loading

alinz Jun 15, 2020 •

edited

Loading

AdamSLevy commented Jun 21, 2020 •

edited

Loading

alinz commented Jun 22, 2020 •

edited

Loading