Upgrade to new Stronghold interface

[PhilippGackstatter]

Description of change

Reimplements our Stronghold wrapper with the new stronghold interface.

Removes the old wrapper completely.

For now, the Stronghold wrapper takes care of creating the parent directories of a stronghold snapshot file before attempting to persist it. Stronghold should probably handle this itself, but it currently doesn't.

TODO:

• change workflow include in build-and-test.yml:207 back to dev

Open Questions

• Should Stronghold::new only take path and password, without dropsave? Should we then add a second method Stronghold::new_with_options that takes a StrongholdOptions struct where users can set the dropsave? This would make things easily extendable in the future, but so far it doesn't look like we need a second option, so it's arguably premature optimization.
  • One possible second useful option might be for users to explicitly state whether the file path they provide is supposed to point to an existing stronghold, or if it should be created if it doesn't exist. Just like std::fs::OpenOptions::create. E.g. if it doesn't exist, then an error can be returned.
  • This would technically make this PR a breaking change.
• Do we have to roll back changes to a client if the entire did_create "transaction" doesn't complete successfully? Otherwise, someone re-trying to create an identity from an existing keypair might fail every time, because the DID was added to the index, but e.g. the client syncing later failed. Generally, we don't go to those lengths, so this might be fine?

Links to any relevant issues

fixes #785

Type of change

Add an x to the boxes that are relevant to your changes.

• 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

• The storage test suite was added for stronghold.
• Two tests were added to test the custom functionality of the Stronghold wrapper, such as mutate_client and an error case.
• A local test was executed with the current dev version and the current PR to ensure the storage layout from the dev stronghold snapshot files can continue to be used with the new Stronghold wrapper.
  • That test ran a modified example from dev that added a key and a service and wrote to a stronghold file. On the current PR, an example was run to ensure the DID could be loaded, sign with the added key, and the service could be removed.
• identity_account::tests::util::storages now also returns a Stronghold, so all update tests for the account run once with a MemStore and once with a Stronghold.

Change checklist

Add an x to the boxes that are relevant to your changes.

• 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

[PhilippGackstatter]

The multiple identities example using Napi Stronghold returns a lock acquisition error, that is tracked (in part) here: iotaledger/stronghold.rs#353.

[cycraig]

The significant simplification and reduction of code and dependencies is much appreciated.

I'm still not that familiar with Stronghold, and the code is pretty meticulous, so my feedback is mostly nits.

[cycraig]

Aside: still not happy with how we use derive_encryption_key/PBKDF2_HMAC_SHA512 with a low number of iterations and fixed salt.

Given the hassle it would be for Stronghold to take on this responsibility, for example by storing the unencrypted salt in the snapshot associated data and running the PBKDF for us (or allowing us to store arbitrary unencrypted associated data ourselves), can we at least increase the iterations? Should be around 120,000, per OWASP. It's been quite a few years since NIST recommended a minimum of 10,000 iterations, and we're currently doing 100. Brute-forcing the snapshot password should take at least some effort.

[cycraig]

This change would be breaking, so if this PR is really just a patch it should be done elsewhere.

[cycraig]

If RevokeData is idempotent, is it worth checking record_exists first at all? I guess it's the difference between acquiring read vs write locks internally? I wouldn't expect this to be a frequent or slow operation though.

Happy to leave it as-is too.

[PhilippGackstatter]

Agree with what you're saying, but if we don't check for existence first, then we cannot accurately return whether the function deleted the key or not (the bool return parameter). So to comply with our interface, I'd leave it as-is.

[cycraig]

Nit: the error message doesn't really match the variant name, perhaps Password might be more appropriate unless we throw Memory errors elsewhere? Maybe Password isn't the best either since a different error is thrown if the password is incorrect.

Probably fine.

[cycraig]

Should Stronghold::new only take path and password, without dropsave? Should we then add a second method Stronghold::new_with_options that takes a StrongholdOptions struct where users can set the dropsave? [...]

I was opposed to changing this because, as you noted, dropsave is the only optional parameter. However, I really like the idea of a create flag to indicate whether an existing snapshot is expected.

If this is up for debate, the broad options are:

1. Stronghold::new and Stronghold::new_with_options with a StrongholdOptions (would have to be a builder to avoid breaking changes).

   Stronghold::new_with_options("./snapshot.hodl", "password", StrongholdOptions::new().dropsave(false)).await?;

2. Introducing a StrongholdBuilder, e.g.,

   Stronghold::builder()
     .path("./snapshot.hodl")
     .password("password")
     .dropsave(false)
     .create(false)
     .build()
     .await?;

3. Placing everything in a non-exhaustive StrongholdOptions struct, embracing the TypeScript approach:

   Stronghold::new(StrongholdOptions {
     path: "./snapshot.hold".into(),
     password: "some-password".into(),
     create: false,
     ..Default::default()
   }),await?;

Do we have to roll back changes to a client if the entire did_create "transaction" doesn't complete successfully? [...]

Ideally, yes. We should probably restore the old index and purge both the temporary and DID clients if an error occurs. There are unfortunately many places that can error there... I would be fine leaving a TODO/FIXME there for now though.

The multiple identities example using Napi Stronghold returns a lock acquisition error, that is tracked (in part) here: iotaledger/stronghold.rs#353.

Looks like the Ubuntu example is throwing that error too, do we want to exclude that example as an edge-case for now or delay this PR until it's fixed?

[PhilippGackstatter]

Opened #836 to address the comments from @cycraig, as they are non-essential or constitute breaking changes. The motivation is to get this PR merged sooner than later.

Similarly, I've disabled the multiple identites examples in Rust and stronghold-nodejs, because of iotaledger/stronghold.rs#353, and disabled the musl build (opened #837 for the required changes). Both will need to be addressed before the next release.

[felsweg-iota]

Integration of Stronghold looks good to me. Check the remarks, if the PrivateKey-type can be replaced, or would require to much effort. The current solution is Ok.

[felsweg-iota]

Is there a way to leverage the KeyStore or Stronghold's NC types in favor of PrivateKey ? It somehow feels wasted to convert the private key to a vec, to just be zeroized and dropped afterwards.

[PhilippGackstatter]

Appreciate the idea! Unfortunately, this would "leak" the stronghold dependency to our Account which is a no-go for Wasm. If you could factor out these NC types in a separate small crate that is also Wasm compatible, we could reconsider 😉. Perhaps this would improve Wasm memory security, too.

[felsweg-iota]

ultra-nit:

Suggested change

	pub fn set_dropsave(&mut self, dropsave: bool) {
	pub fn dropsave_mut(&mut self, dropsave: bool) {

[PhilippGackstatter]

Mh, this is just a setter, and does not return a mutable reference to dropsave, so the naming matches Rust conventions. clippy would probably complain about dropsave_mut, too.

[HenriqueNogara]

Not all procedures will be dealing with KeyLocation, since this is either an Ed25519 or an X25519 Key for now. For example, when dealing with the procedures related to shared secrets the type is neither. However, we are dealing with random locations in that case. Should we change KeyLocation or is it fine to have an empty location in these cases?

[cycraig]

Would changing it to Vec<String> account for shared secrets? I think Location alone doesn't have a string representation.

[PhilippGackstatter]

If the need arises to return errors that include non-KeyLocations, then Craig's suggestion is great. Though, I think for debugging it should be helpful enough to include the KeyLocations, as the other, temporary locations will probably be completely random or static, and we'll be able to trace the issue based on the procedure name alone, I imagine.

[HenriqueNogara]

temporary locations will probably be completely random or static, and we'll be able to trace the issue based on the procedure name alone, I imagine.

I thought so too. For now, I won't change the type, but will change it to String in case I need it.