fission-codes / keystore-idb

In-browser key management with IndexedDB and the Web Crypto API
Apache License 2.0
57 stars 8 forks source link

String operations #27

Closed icidasset closed 4 years ago

icidasset commented 4 years ago

As we discussed earlier, I extracted the string methods from the KeyStore classes so they can be used without a KeyStore instance. My plan is to use eccOperations.verify and rsaOperations.verify to verify a string signed by a DID (in the SDK).

icidasset commented 4 years ago

Not sure what to do to get full test coverage.

icidasset commented 4 years ago

@dholms Sounds good, how do I do that? Add a cryptoMethod({ ... }) entry to test/rsa.test.ts?

dholms commented 4 years ago

yup! one for each method in RSA & one for each in ECC.

Let me know if it makes any sense. There was a ton of repeat test code so I abstracted it away in that cryptoMethod function. But I'm not sure how immediately understandable it is :thinking:

dholms commented 4 years ago

Hey also last week, I started messing around with separating crypto methods out into their own package. It isn't ready to go yet, so we should definitely add these methods here for now and we can wrap up that new package when one of us gets some spare cycles.

But, I changed the functions around so they could take any form of input

encrypt(msg: Msg, key.......)
// Msg is a type alias for
type Msg = string | ArrayBuffer | Uint8Array | Uint16Array

We then just normalize that to an ArrayBuffer & perform whatever operations on it.

Check out some of the code here: https://github.com/fission-suite/webcrypto/blob/master/src/aes/operations.ts

I think this makes the package quite a bit more flexible. What do you think?

icidasset commented 4 years ago

Oh cool, I hadn't seen that yet. Yeah that sounds good 👍 I do however feel like it should be only string | Uint8Array though, not sure why we have both ArrayBuffer and Uint8Array? Also, where does Uint16Array come in?

dholms commented 4 years ago

not sure why we have both ArrayBuffer and Uint8Array They're very closely related. It's not a huge deal to normalize between them in package vs in the caller. Right now the crypto functions take an ArrayBuffer so I was thinking we'd leave that and just extend it to also take string & Uint8Array

Also, where does Uint16Array come in? :thinking: honestly not quite sure what I was thinking when I included this, I don't think we use it anywhere haha so yeah let's drop it

dholms commented 4 years ago

Heads up @icidasset

I split the normalize function into normalizeUtf16ToBuf (used for plaintext) and normalizeBase64ToBuf (used for base64 ie ciphertext) as well as normalizeToBuf which both those functions use that just takes a given msg & a string conversion function

icidasset commented 4 years ago

@dholms I refactored the rsa and ecc modules to use your normalize functions and added more tests, so that pretty much everything is covered (except some utils)

icidasset commented 4 years ago

😻

I cleared the cache on Travis and now it passed 👍