Skip to content
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

[Task] Improve Stronghold constructor and did_create impl #836

Open
9 tasks
PhilippGackstatter opened this issue May 5, 2022 · 0 comments
Open
9 tasks
Labels
Breaking change A change to the API that requires a major release. Part of "Changed" section in changelog Rust Related to the core Rust code. Becomes part of the Rust changelog.

Comments

@PhilippGackstatter
Copy link
Contributor

PhilippGackstatter commented May 5, 2022

Description

After #787 we should make some quality of life and security improvements to our Stronghold wrapper. Some of these will be breaking changes.

  • Increase PBKDF2_HMAC_SHA512 iterations to 120,000.
    • Note that this breaks using older stronghold snapshot files, so it can be considered a more severe breaking change than changing an API.
  • Implement transaction-like behaviour in did_create. Investigate if this is necessary for other functions as well. This should perhaps also be documented and/or recommended behaviour.
    • For instance, Someone re-trying to create an identity from an existing keypair might fail every time, because the DID was added to the index, but the client syncing later failed.
  • Add a create flag for Stronghold that, if true, will create a snapshot file if it doesn't exist, or, if false, throw an error if the file doesn't exist.

This last point warrants re-thinking the Stronghold constructor.

The broad options are (copied from @cycraig):

  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?;

I prefer option 1, because the other options leave the possibility of a user not providing a password and path which are required, requiring us to introduce an error variant (à la MissingRequiredField). This can of course be further discussed.

Motivation

Improve security and provide users a way to better express their expectations (create flag).

Resources

Change checklist

Add an x to the boxes that are relevant to your changes, and delete any items that are not.

  • The feature or fix is implemented in Rust and across all bindings whereas possible.
  • The feature or fix has sufficient testing coverage
  • All tests and examples build and run locally as expected
  • Every piece of code has been document according to the documentation guidelines.
  • If conceptual documentation (mdbook) and examples highlighting the feature exist, they are properly updated.
  • If the feature is not currently documented, a documentation task Issue has been opened to address this.
@PhilippGackstatter PhilippGackstatter added Breaking change A change to the API that requires a major release. Part of "Changed" section in changelog Rust Related to the core Rust code. Becomes part of the Rust changelog. labels May 5, 2022
@PhilippGackstatter PhilippGackstatter changed the title [Task] Improve Stronghold constructor did_create impl [Task] Improve Stronghold constructor and did_create impl May 5, 2022
@cycraig cycraig added this to the v0.7 Features milestone Jun 23, 2022
@eike-hass eike-hass removed this from the v0.7 Features milestone Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking change A change to the API that requires a major release. Part of "Changed" section in changelog Rust Related to the core Rust code. Becomes part of the Rust changelog.
Projects
Status: Product Backlog
Development

No branches or pull requests

3 participants