Page MenuHomePhabricator

[web-db] implement crypto functions
ClosedPublic

Authored by kamil on Mar 7 2023, 9:35 AM.
Tags
None
Referenced Files
F3528763: D6995.id23513.diff
Tue, Dec 24, 10:11 AM
Unknown Object (File)
Sat, Dec 14, 4:02 PM
Unknown Object (File)
Sat, Dec 14, 4:02 PM
Unknown Object (File)
Sat, Dec 14, 4:02 PM
Unknown Object (File)
Sat, Dec 14, 4:02 PM
Unknown Object (File)
Sat, Dec 14, 4:02 PM
Unknown Object (File)
Sat, Dec 14, 4:02 PM
Unknown Object (File)
Sat, Dec 14, 4:01 PM
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
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 ↗(On Diff #23513)

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 ↗(On Diff #23513)

Why aren't these read-only?

rebase, make type read-only

web/database/utils/worker-crypto-utils.js
10–11 ↗(On Diff #23513)

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.