-
Notifications
You must be signed in to change notification settings - Fork 63
fix: caching cmm export and material #186
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: caching cmm export and material #186
Conversation
* WebCryptoDecryptionMaterial do not have an unencrypted data key This is because the CrypoKey offers better security, and some unwrapping algorithms can directly return a CryptoKey without exposing the unencrypted data key. Update test for WebCryptoDecryptionMaterial. * Caching CMMs need to be able to create a local cryptographic materials cache export the function.
const material = new WebCryptoDecryptionMaterial(webCryptoSuite, { some: 'context' }) | ||
.setUnencryptedDataKey(udk128, trace) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this change fixes the cloning so that it handles the WebCryptoDecryptionMaterials case, but is setting an unencryptedDataKey on WebCryptoDecryptionMaterials something that should be allowed in the first place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be true. I'd have to think about it. But it would also be a VERY large change. Having it should not hurt anything...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is a potential for this being a sharp edge, can we either document this somewhere (maybe CryptoKey or WebCryptoDecryptionMaterial) or create an issue for further investigation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline, we do not see this as a sharp edge. Investigation for how to make this easy to maintain is part of #74
@@ -14,3 +14,4 @@ | |||
*/ | |||
|
|||
export * from './caching_materials_manager_browser' | |||
export { getLocalCryptographicMaterialsCache } from '@aws-crypto/cache-material' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are these exports being used? Should they be included in a PR containing changes which include them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can not create a local cache easily without these. The should always have been exported... I found this "bug" in writing the examples. I can put it somewhere else, but I felt like they were in the same world.
WebCryptoDecryptionMaterial do not have an unencrypted data key
This is because the CrypoKey offers better security,
and some unwrapping algorithms can directly return a CryptoKey
without exposing the unencrypted data key.
Update test for WebCryptoDecryptionMaterial.
Caching CMMs need to be able to create a local cryptographic materials cache
export the function.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Check any applicable: