feat: Doc encryption with symmetric key #2731

islamaliev · 2024-06-17T16:31:17Z

Relevant issue(s)

Description

This change introduces doc encryption. Upon creation of a document the user can pass the encryption flag which will signal to db to create a new symmetric key using AES-GCM and will store it locally.
With the encryption flag all doc fields (deltas) being stored in the DAG encrypted. The datastore will still store data as plain text, as for it's encryption we can use encryption-at-rest.
Decryption is not used at the moment, as it is relevant only p2p environment and we don't have yet key exchange mechanism. All peers sync encrypted data.

This PR also adds 2 new parameters to GraphQL create_ mutation:

inputs: a list of documents to be created
encrypt: flag that indicates if document(s) need to be encrypted.

cli/collection_create.go

datastore/multi.go

internal/db/db.go

internal/encryption/context.go

fredcarle

Overall I think you're going in the right direction. I have 2 main critiques at this stage. The first, as mentioned in an other comment, is the passing of a key on document create. I think we should avoid doing this and let Defra create a new key when needed. The second is the use of the context for the encryption key and store. It feels like it's making it more complex than it needs to be. We can discuss it some more in the standup or in a separate call if you want.

codecov · 2024-06-24T12:04:52Z

Codecov Report

Attention: Patch coverage is 76.28032% with 88 lines in your changes missing coverage. Please review.

Project coverage is 78.69%. Comparing base (ea77dc2) to head (8bc56c7).

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2731      +/-   ##
===========================================
- Coverage    78.99%   78.69%   -0.30%     
===========================================
  Files          315      318       +3     
  Lines        23873    24128     +255     
===========================================
+ Hits         18858    18987     +129     
- Misses        3649     3772     +123     
- Partials      1366     1369       +3

Flag	Coverage Δ
all-tests	`78.69% <76.28%> (-0.30%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
client/document.go	`70.50% <ø> (-1.30%)`	⬇️
client/request/mutation.go	`100.00% <ø> (ø)`
datastore/multi.go	`92.00% <100.00%> (+1.09%)`	⬆️
http/client.go	`55.97% <100.00%> (+0.18%)`	⬆️
internal/core/block/block.go	`90.65% <100.00%> (-0.57%)`	⬇️
internal/db/collection.go	`72.02% <100.00%> (-0.41%)`	⬇️
internal/db/context.go	`100.00% <100.00%> (ø)`
internal/db/fetcher/fetcher.go	`79.18% <100.00%> (-0.63%)`	⬇️
internal/db/fetcher/versioned.go	`68.46% <100.00%> (ø)`
internal/encryption/context.go	`100.00% <100.00%> (ø)`
... and 24 more

... and 12 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea77dc2...8bc56c7. Read the comment docs.

nasdf

Looks good! Just a few small things to clear up.

http/client_collection.go

http/handler_collection.go

http/client.go

cli/collection_create.go

client/document.go

AndrewSisley

Part way through my review but need to get some food - looks good so far :)

tests/integration/encryption/utils.go

tests/integration/utils2.go

internal/planner/create.go

cli/collection_create.go

tests/integration/encryption/query_test.go

tests/integration/encryption/peer_test.go

tests/integration/encryption/commit_test.go

AndrewSisley

Looks really good, just a handful of smaller comments for you before merge :)

AndrewSisley · 2024-07-03T17:06:29Z

internal/planner/create.go

+	if len(n.docs) == 1 {
+		err = n.collection.Create(n.p.ctx, n.docs[0])
+	} else {
+		err = n.collection.CreateMany(n.p.ctx, n.docs)


todo: I do disagree with creating docs in Start - it feels like it is very much in the wrong place and may trip us up in the future (if not by being technically annoying for some future change, it seems likely to cause 'surprise').

Please either move this call into Next, or discuss in in discord/standup (for greater visibility).

I understand your concern. I had similar feelings but thought it was highly unlikely and having a nicer code was worth it. If you think otherwise, I will move it to Next. No problem.

Thanks Islam, sorry about the bother, and I understand it is a bit 'abstract' in this case, but I do think it is in the wrong place atm. Ideally we would remove the Start/Init funcs anyway, and conceptually I very much think the 'creating' part of a 'create' iterator should be done in the iteration funcs (next/value).

AndrewSisley · 2024-07-03T17:14:23Z

internal/core/block/block.go

@@ -103,6 +103,8 @@ type Block struct {
 	Delta crdt.CRDT
 	// Links are the links to other blocks in the DAG.
 	Links []DAGLink
+	// IsEncrypted is a flag that indicates if the block's delta is encrypted.
+	IsEncrypted *bool


todo: Please don't use a pointer for a boolean, it feels very wasteful and confusing - suggesting that it's pointerness may be used for odd mutation reasons. If you want it to be optional, please use immutable.Option instead, it is much cheaper, less confusing, and wont contribute to GC pressure.

If you have a very specific reason for using a pointer here, please document it.

datastore/store.go

AndrewSisley

LGTM Islam :) And thanks for the random little cleanups you made along the way :)

islamaliev self-assigned this Jun 17, 2024

islamaliev added security Related to security area/datastore Related to the datastore / storage engine system area/collections Related to the collections system labels Jun 17, 2024

islamaliev added this to the DefraDB v0.12 milestone Jun 17, 2024