Page MenuHomePhabricator

Enable webcrypto usage in keyserver codebase and unify aes crypto stack across web and the keyserver
ClosedPublic

Authored by marcin on Jul 17 2023, 6:44 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, May 7, 10:13 PM
Unknown Object (File)
Tue, May 7, 10:12 PM
Unknown Object (File)
Tue, May 7, 10:12 PM
Unknown Object (File)
Sat, May 4, 4:58 PM
Unknown Object (File)
Sat, May 4, 4:58 PM
Unknown Object (File)
Sat, May 4, 4:58 PM
Unknown Object (File)
Fri, May 3, 9:46 AM
Unknown Object (File)
Wed, May 1, 5:13 AM
Subscribers

Details

Summary

This differential brings WebCrypt API typing to keyserver and enables AES crypto stack implemented for web to be used on the keyserver as well

Test Plan

Use aes-crypto-utils.js to encrypt and decrypt simple string on the keyserver. Ensure that encryption and decryption is succesfull and flow does not complain.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

keyserver/src/utils/aes-crypto-utils.js
13 ↗(On Diff #28756)

An alternative would be to implement AES encryption on the keyserver from scratch using the part of crypto API that comes typed in flow. I thought about this and it has two disadvantages:

  1. We would repeat implementation of the same functionality twice, while technically nothing prevents us from having one common API. It reduces code quailty and maintainability. Every change to the way we handle AES encryption would require to remember about more places in the codebase.
  2. Even though some crypto API is typed in flow it is still not complete. In particular the function to generate a secret key for particular algorithm is missing in flow typed. Therefore we would still need to write our own types and probably do casting to any.
web/flow-typed/web-crypto.js
283 ↗(On Diff #28756)

An alternative would be to move this entire file to lib and insert webcrypto into global on the keyserver so that the statement declare var crypto: Crypto; is true. However, the crypto on the keyserver is a library and we don't just insert libraries into global so that they don't need to be imported. Therefore I moved only the types to the lib and left global declaration on web.

web/media/aes-crypto-utils.js
10 ↗(On Diff #28756)

I could remove this file and use function from lib directly, but I didn't want to affect to much code and leave this layer of abstraction unchanged.

keyserver/src/utils/aes-crypto-utils.js
13 ↗(On Diff #28756)

I agree we should not reimplement it. This any-cast is safe, since you immediately cast it to another type

15–31 ↗(On Diff #28756)

In all three of these cases, your use of async / await is not necessary, and results in unnecessary computation. Instead, you can simplify forward the Promise

bartek added 1 blocking reviewer(s): kamil.

Looks good to me. Letting @kamil take a look

keyserver/src/utils/aes-crypto-utils.js
13 ↗(On Diff #28756)

I'm okay with any-cast too

web/media/aes-crypto-utils.js
10 ↗(On Diff #28756)

It would be good to do this in a separate diff. It creates a redundant layer of abstraction.

This revision is now accepted and ready to land.Jul 19 2023, 5:28 AM

Remove web/media/aes-crypto-utils.js from the scope of this diff, so that it can be handled in separate diff