feat: CRP-2911 Modify AES-GCM encryption to be more robust #224
Conversation
randombit
force-pushed
the
jack/crp-2911
branch
from
ef18570 to
fccdf0d
Compare
|
This is now ready for a new review |
There was a problem hiding this comment.
Thanks for addressing the comments, @randombit, and for adding the respective changes also in Typescript.
| assert_eq!( | ||
| dkm.decrypt_message(&ctext, domain_sep, aad).unwrap(), | ||
| test_message, | ||
| ); |
There was a problem hiding this comment.
Would it make sense to add a test ensuring that decrypting with a wrong aad fails? (Same for Typescript).
There was a problem hiding this comment.
IICU, these are only formatting changes.
There was a problem hiding this comment.
Oops, yes. I think we might want to fix these in a different PR...
There was a problem hiding this comment.
We should also include the changes of this PR in the CHANGELOGs (for both the TS and the Rust library).
|
|
||
| static fromCryptoKey(cryptokey: CryptoKey): DerivedKeyMaterial { | ||
| return new DerivedKeyMaterial(cryptokey); | ||
| static async fromCryptoKey(raw: CryptoKey): Promise<DerivedKeyMaterial> { |
There was a problem hiding this comment.
This looks like a breaking change.
My understanding is that if someone has stored the CryptoKey returned from DerivedKeyMaterial::getCryptoKey with the current library version in an IndexedDB, then passing that to DerivedKeyMaterial::fromCryptoKey with the new library version won't work.
Not sure yet what the best way is to fix it. Do we also have to make this backward-compatible?
There was a problem hiding this comment.
It should not be a breaking change, or at least was not intended to be. fromCryptoKey and getCryptoKey return the CryptoKey which wrapping the raw VetKey bytes. Then fromCryptoKey performs an additional HKDF
There was a problem hiding this comment.
It is possible to store CryptoKey objects in an IndexedDB in web browsers (example from encrypted notes dapp). Consider a dapp that (for whatever reason) stores the CryptoKeys returned by @dfinity/vetkeys -0.4.0's DerivedKeyMaterial::getCryptoKey in an IndexedDB.
If that dapp is now updated to a version that contains this change here, IIUC, importing the CryptoKeys (that are fetched from the IndexedDB) using the new DerivedKeyMaterial::fromCryptoKey would fail in line 590 with an InvalidAccessError because the raw key doesn't allow the deriveBits usage because the old library version didn't set that usage.
There was a problem hiding this comment.
Thinking about this more, even if importing CryptoKeys that were exported with an old library version fails, it doesn't seem strictly necessary to make this backwards-compatible because developers work around this on their end by wiping and re-creating their (e.g, IndexedDB) cache. This would require re-fetching the underlying vetKey, but that seems reasonable. So maybe all we need here is a brief explanation in the changelog.
| * The CryptoKey is not exportable | ||
| */ | ||
| async deriveAesGcmCryptoKey( | ||
| private async deriveAesGcmCryptoKey( |
There was a problem hiding this comment.
If we make this private, we should mention it in the CHANGELOG as breaking change.
Changes include
DerivedKeyMaterialis, instead of being just the raw VetKey treated as an HKDF key, is first hashed. This prevents working backwards from aDerivedKeyMaterialto the originalVetKey.These changes are not backwards compatible. Backwards compatability could be added in the future if required (eg if the header is unknown, attempt to decrypt using the old scheme - if it works, return the recovered plaintext). The hope is that this is not required.