Page MenuHomePhabricator

[web-db] implement crypto functions
ClosedPublic

Authored by kamil on Mar 7 2023, 9:35 AM.
Tags
None
Referenced Files
F3376314: D6995.diff
Tue, Nov 26, 11:19 PM
Unknown Object (File)
Sat, Nov 23, 10:19 AM
Unknown Object (File)
Fri, Nov 15, 1:32 AM
Unknown Object (File)
Mon, Nov 11, 7:25 PM
Unknown Object (File)
Sat, Nov 2, 2:54 AM
Unknown Object (File)
Sat, Nov 2, 12:29 AM
Unknown Object (File)
Sat, Nov 2, 12:29 AM
Unknown Object (File)
Sat, Nov 2, 12:29 AM
Subscribers

Details

Summary

Based on SubtleCrypto. Functions we will need for encryption database.

Depends on D6994

Test Plan

Encryption:

  1. Create data ArrayBuffer
  2. Encrypt
  3. Decrypt
  4. Compare

Crypto Key:

  1. Check if it's properly generated
  2. Verify that can not be extracted using js

Diff Detail

Repository
rCOMM Comm
Branch
db-on-web-2
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

kamil held this revision as a draft.
web/database/utils/worker-crypto-utils.js
26

this is the recommended size source

kamil published this revision for review.Mar 7 2023, 10:10 AM
web/database/utils/worker-crypto-utils.js
10–11

Why aren't these read-only?

rebase, make type read-only

web/database/utils/worker-crypto-utils.js
10–11

oversight, thanks for pointing out

there is more crypto-relatud stuff, making names more exact to easily distinguish

Generally, the code itself looks good to me. But I have a general note regarding naming conventions.
I haven't seen all call sites yet (only the key generation diff is up) so I can't really tell but maybe it's worth revisiting the types and function arguments to have this as clean and consistent as possible.
In my diffs regarding AES encryption, I defined a few naming rules which I stick to (you can find them in this Notion doc). You can take inspiration from them.

If you think there's no point in doing this, just let me know ;)

web/database/utils/worker-crypto-utils.js
5 ↗(On Diff #23697)

What about calling it EncryptedData? See my comment in decryptDatabaseFile

7 ↗(On Diff #23697)

I'd call it ciphertext to be consistent, but up to you

21 ↗(On Diff #23697)

Nit

45–47 ↗(On Diff #23697)

How about connecting cipher and iv by using the EncryptionResult type you defined above (and perhaps renaming the type)? In GCM mode, they always need to be provided together anyways

I haven't seen all call sites yet (only the key generation diff is up) so I can't really tell but maybe it's worth revisiting the types and function arguments to have this as clean and consistent as possible.

I updated the types to match those added in D7101.

Also, I added unit tests so you can check possible callsites: D7104

In my diffs regarding AES encryption, I defined a few naming rules which I stick to (you can find them in this Notion doc). You can take inspiration from them.

Oh, I didn't know those rules exist (btw. good job on those).

If you think there's no point in doing this, just let me know ;)

I think your suggestions make sense, thanks!

This revision is now accepted and ready to land.Mar 21 2023, 4:32 AM
This revision was automatically updated to reflect the committed changes.