Stronghold Storage Implementation #1157
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
abdulmth
added
Added
abdulmth
changed the title
JwkStorage
abdulmth
force-pushed
the
feat/rust-stronghold
branch
from
0d3feef to
a5258e5
Compare
PhilippGackstatter
approved these changes
May 2, 2023
Co-authored-by: Philipp Gackstatter <philipp.gackstatter@iota.org>
6 tasks
# Conflicts: # bindings/wasm/docs/api-reference.md # bindings/wasm/src/credential/jwt_credential_validation/jwt_credential_validator.rs # bindings/wasm/src/credential/jwt_credential_validation/mod.rs # bindings/wasm/src/credential/jwt_credential_validation/options.rs # bindings/wasm/src/credential/mod.rs # bindings/wasm/src/did/jws_verification_options.rs # bindings/wasm/src/did/wasm_core_document.rs # bindings/wasm/src/error.rs # bindings/wasm/src/iota/iota_document.rs # bindings/wasm/src/jose/jws_header.rs # bindings/wasm/src/jose/mod.rs # bindings/wasm/src/storage/jwk_storage.rs # bindings/wasm/src/storage/signature_options.rs # bindings/wasm/src/verification/custom_verification.rs # bindings/wasm/tests/jwk_storage.ts # bindings/wasm/tests/storage.ts # identity_core/src/error.rs # identity_credential/src/credential/credential.rs # identity_credential/src/credential/jwt_serialization.rs # identity_credential/src/credential/mod.rs # identity_credential/src/validator/vc_jwt_validation/credential_jwt_validator.rs # identity_credential/src/validator/vc_jwt_validation/mod.rs # identity_document/src/document/core_document.rs # identity_document/src/error.rs # identity_document/src/verifiable/jws_verification_options.rs # identity_iota/Cargo.toml # identity_iota_core/src/document/iota_document.rs # identity_iota_core/src/error.rs # identity_jose/examples/jws_encoding_decoding.rs # identity_jose/src/jws/custom_verification/jws_verifier.rs # identity_jose/src/jws/decoder.rs # identity_jose/src/jws/encoding/mod.rs # identity_jose/src/jws/encoding/utils.rs # identity_jose/src/tests/rfc7515.rs # identity_jose/src/tests/rfc8037.rs # identity_jose/src/tests/roundtrip.rs # identity_storage/Cargo.toml # identity_storage/src/key_id_storage/method_digest.rs # identity_storage/src/key_storage/memstore.rs # identity_storage/src/lib.rs # identity_storage/src/storage/error.rs # identity_storage/src/storage/mod.rs # identity_storage/src/storage/signature_options.rs # identity_storage/src/storage/tests/api.rs # identity_storage/src/storage/tests/credential_validation.rs # identity_storage/src/storage/tests/mod.rs # identity_verification/Cargo.toml # identity_verification/src/verification_method/method.rs # libjose/src/jwk/key_params.rs # libjose/tests/jwe.rs # libjose/tests/jws.rs
PhilippGackstatter
suggested changes
Aug 14, 2023
examples/0_basic/8_stronghold.rs
Outdated
There was a problem hiding this comment.
I don't think Stronghold should be its own example. We already use stronghold for the client in every example and we should encourage using it in identity too by showing stronghold for identity usage in every example. That's how we always did it in the past and I think that was good.
identity_storage/Cargo.toml
Outdated
| @@ -21,12 +21,15 @@ identity_document = { version = "=0.7.0-alpha.6", path = "../identity_document", | |||
| identity_iota_core = { version = "=0.7.0-alpha.6", path = "../identity_iota_core", default-features = false, optional = true } | |||
| identity_verification = { version = "=0.7.0-alpha.6", path = "../identity_verification", default_features = false } | |||
| iota-crypto = { version = "0.23", default-features = false, features = ["blake2b", "ed25519", "random"], optional = true } | |||
| iota-sdk = { version = "1.0", default-features = false, features = ["tls", "client", "stronghold"], optional = true } | |||
| iota_stronghold = { version = "2.0", optional = true } | |||
There was a problem hiding this comment.
Let's please follow our codebase convention and use default-features = false and activate what we need.
| pub(crate) static IDENTITY_CLIENT_PATH: &[u8] = b"iota_identity_client"; | ||
|
|
||
| /// The Ed25519 key type. | ||
| #[allow(dead_code)] |
There was a problem hiding this comment.
If this is correctly exported, the lint should not be necessary, right? Why is this here again?
Comment on lines
52
to
53
| let keytype: ProceduresKeyType = match key_type.to_string().to_lowercase().as_str() { | ||
| "ed25519" => Ok(ProceduresKeyType::Ed25519), |
There was a problem hiding this comment.
We have the StrongholdKeyType enum to make this case easy to handle. Not sure why we need to lowercase convert anything here and cause another allocation.
Suggested change
| let keytype: ProceduresKeyType = match key_type.to_string().to_lowercase().as_str() { | |
| "ed25519" => Ok(ProceduresKeyType::Ed25519), | |
| let keytype: ProceduresKeyType = match key_type { | |
| StrongholdKeyType::Ed25519 => Ok(ProceduresKeyType::Ed25519), |
| .write_secret(location, zeroize::Zeroizing::from(secret_key.to_bytes().to_vec())) | ||
| .map_err(|err| { | ||
| KeyStorageError::new(KeyStorageErrorKind::Unspecified) | ||
| .with_custom_message("stronghold client error") |
There was a problem hiding this comment.
Suggested change
| .with_custom_message("stronghold client error") | |
| .with_custom_message("stronghold write secret failed") |
| } | ||
|
|
||
| /// Shared reference to the inner [`SecretManager`]. | ||
| pub fn inner(&self) -> Arc<SecretManager> { |
There was a problem hiding this comment.
Suggested change
| pub fn inner(&self) -> Arc<SecretManager> { | |
| pub fn as_secret_manager(&self) -> Arc<SecretManager> { |
Follow rust convention with as_ and be more specific about what this fn does.
Actually, does this need to return Arc<SecretManager>? Wouldn't &SecretManager be sufficient?
| } | ||
|
|
||
| ///Acquire lock of the inner [`Stronghold`]. | ||
| pub async fn get_stronghold(&self) -> MutexGuard<'_, Stronghold> { |
There was a problem hiding this comment.
Suggested change
| pub async fn get_stronghold(&self) -> MutexGuard<'_, Stronghold> { | |
| pub(crate) async fn get_stronghold(&self) -> MutexGuard<'_, Stronghold> { |
This can be pub(crate) if I'm not mistaken.
| self.0.clone() | ||
| } | ||
|
|
||
| ///Acquire lock of the inner [`Stronghold`]. |
There was a problem hiding this comment.
Suggested change
| ///Acquire lock of the inner [`Stronghold`]. | |
| /// Acquire lock of the inner [`Stronghold`]. |
| use tokio::sync::MutexGuard; | ||
|
|
||
| /// Wrapper around `SecretManager` that implements the storage interfaces. | ||
| #[derive(Clone)] |
There was a problem hiding this comment.
Can we derive Debug as well?
| use std::sync::Arc; | ||
| use tokio::sync::MutexGuard; | ||
|
|
||
| /// Wrapper around `SecretManager` that implements the storage interfaces. |
There was a problem hiding this comment.
Suggested change
| /// Wrapper around `SecretManager` that implements the storage interfaces. | |
| /// Wrapper around a `StrongholdSecretManager` that implements the [`KeyIdStorage`](todo:link) and [`JwkStorage`](todo:link) interfaces. |
Yes, technically it wraps a SecretManager, but conceptually it's a StrongholdSecretManager and that is what matters to the user.
|
Maybe we can change the examples in a separate PR. |
PhilippGackstatter
approved these changes
Aug 15, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of change
Stronghold storage implementation.
Still to do:
KeyIdStorageLinks to any relevant issues
Be sure to reference any related issues by adding
fixes issue #.Type of change