Add Diffie-Hellman key exchange for encryption to Account

[HenriqueNogara]

Description of change

Add Diffie-Hellman key exchange to Account

Links to any relevant issues

closes #756

Type of change

• Bug fix (a non-breaking change which fixes an issue)
• Enhancement (a non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation Fix

How the change has been tested

New key_exchange example.

Change checklist

• I have followed the contribution guidelines for this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

[cycraig]

Preliminary review: I think we need to remove key_exchange (from the Storage interface) and move the Diffie-Hellman operation into the encrypt/decrypt functions. We also need to be more explicit about which encryption algorithm is to be used in both Account and Storage. Lastly, we need to support the A256KW key wrapping algorithm for ECDH-1PU required by DIDComm.

See:

• https://identity.foundation/didcomm-messaging/spec/#curves-and-content-encryption-algorithms
• https://identity.foundation/didcomm-messaging/spec/#key-wrapping-algorithms
• https://datatracker.ietf.org/doc/html/draft-madden-jose-ecdh-1pu-04#section-2

[cycraig]

I'm not sure how useful this is until we get support for key wrapping and key concatenation in Stronghold:

• [Request] Support AES key wrapping stronghold.rs#338
• [Request] Add a procedure to concatenate secrets stronghold.rs#339

We can only really support ECDH-ES with no key wrapping right now.

[olivereanderson]

Users can always generate the ephemeral key themselves, and we could perhaps support inputting a pre-generated keypair then. That would also sufficiently address the use-case of using a single epk with multiple recipients.

Supporting a pre-generated keypair (as an Option) sounds like a really sensible thing to do!

Overall the Account should focus on providing necessary cryptographic functionality for DID/DIDComm operations, not scope-creep to become a generic key management system.

Agreed.

[eike-hass]

should be okay again after a merging dev

[cycraig]

To clarify, it's disabled on dev too.

[PhilippGackstatter]

And it will be for a while, it looks like 😕: iotaledger/stronghold.rs#353 (comment)

[cycraig]

Can we use an async RwLock over the Stronghold storage implementation to prevent this bug in the meantime then?

[PhilippGackstatter]

I guess we can, but this issue is supposed to be prioritized now, so perhaps @felsweg-iota can give an update on the current status.

[felsweg-iota]

The current problem with the implementation of the standard interface to Stronghold is, that it internally uses a std::sync::RwLock, since every function in the interface isn't supposed to be async. This actually brings in a problem while working with async code.

There are two solutions to this, and i am just naming them here for reference, @PhilippGackstatter already pointed out the link to the issue:

• we either change the current standard library locking mechanism to their async counterparts, which will effectively change the interface to operate as pseudo-async,
• the STM based approach will be finalized and will replace all existing locks.

[PhilippGackstatter]

I think I'd prefer if we try the RwLock solution in our code to fix it temporarily (if that works), and that could avoid you having to do temporary work if the STM will eventually be the proper solution.

[PhilippGackstatter]

Looks pretty good. The MemStore modifications need some changes. Other than that mostly quality of life stuff, docs, typos, etc. To solve the tuple problem in Wasm, I think you can just make the ephemeral public key part of EncryptedData.

Not sure about the naming, i.e. EcdhEs vs. ECDH_ES, but if it's not too much work to change, I'd prefer the latter, and we can just follow JWA naming in the future, preventing further naming debates 😛.

Edit: Extending the StorageTestSuite is probably the most important outstanding part so both MemStore and Stronghold are tested, ideally in Rust and Wasm.

[PhilippGackstatter]

Great work, thanks for addressing all the previous comments so thoroughly!

A few more minor comments.

[PhilippGackstatter]

One more thing: The StorageTestSuite::encryption test should also be exposed in Wasm (wasm/src/account/storage/test_suite.rs), and called in stronghold-nodejs/examples/src/tests/storage_test_suite.ts.

I'm fine with not implementing the encryption functions for the TS MemStore because that seems non-trivial to do without exposing all the primitives from the Rust library, and I'd rather get this PR merged soon. Realistically, this means it won't be implemented in the TS MemStore for quite a while.

[cycraig]

Great to see how far this PR has come.

Most issues have already been addressed, minor comments.