-
Notifications
You must be signed in to change notification settings - Fork 87
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
Implement ECDH-ES+A256KW
for Storage
encryption
#867
Conversation
f1b743c
to
fed679a
Compare
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.
Minor comments, only one serious issue (encrypted_cek
potential panic).
The Wasm bindings structs CekAlgorithm
, EncryptionAlgorithm
should be updated with the new variants, despite not being implemented for the custom Wasm MemStore
example yet.
identity-account-storage/Cargo.toml
Outdated
[dependencies.iota-crypto] | ||
version = ">=0.7, <0.10" | ||
default-features = false | ||
features = ["hmac", "pbkdf", "sha", "std", "aes"] | ||
|
||
[dependencies.iota_stronghold] | ||
git = "https://github.com/iotaledger/stronghold.rs" | ||
rev = "629466da83a677925904b2e733ef7bfad5a42864" | ||
optional = true | ||
features = ["hmac", "pbkdf", "sha", "std", "aes", "aes-kw"] |
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.
Aside: it would be nice to feature-gate the default MemStore
implementation too, to cut down on dependencies for the Stronghold Napi bindings and those not using it. The Storage
implementations (MemStore
, Stronghold
) could even be placed in sub-crates to better separate their dependencies and make deployment more difficult for us.
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.
It would be nice, but that sounds a lot like something for another PR :).
Still suffers from not being testable against fixed test vectors, see #868. Important to match existing implementations (as raised on Discord). We would need to allow inputting the ephemeral key to test encryption. Not sure if we can even test it without fixing the nonce/s used too, however... |
Description of change
Implements
ECDH-ES+A256KW
in theStorage
encryption API, for use inDIDComm
.Links to any relevant issues
Part of #841
Type of change
Add an
x
to the boxes that are relevant to your changes.How the change has been tested
StorageTestSuite
was modified to also test the newly addedCekAlgorithm
.Change checklist
Add an
x
to the boxes that are relevant to your changes.