Refactor KeyManager #204

diehuxx · 2024-05-13T23:11:16Z

Partially addresses #158

Context

The KeyManager and KeyStore traits are fuzzy abstractions -- the conceptual boundary between a "store" and a "manager" is not clear to me. KeyManager currently seems to be a wrapper around KeyStore with little-to-no added functionality. Also, the traits themselves have methods export_private_keys() and import_private_keys() which ought not to be coupled to the notion of "management" which entails storing private keys, signing, and getting public keys. See #158 for more.

Design

Remove the KeyStore trait entirely. Move logic from struct InMemoryKeyStore into LocalKeyManager.
Pare down KeyManager to just three methods: generate_private_key(), sign(), and get_public_key().
Add trait KeyImporter with one required method import_with_alias() and one provided method import() which generates its own alias.

This PR does NOT add the KeyExporter trait described in #158 because I do not see an immediate use case in web5-rs. Exporting private keys is risky stuff. Until we have a clear need for a standard interface for exporting, we ought to omit it.

Open questions

What is the purpose of the .alias() method in the Key trait? Is it a unique digest? Whatever it is, we should probably rename the method.
What is the expected behavior of KeyImporter when a key alias is overwritten with a new private key? Should that be an error? Is it up to each individual impl of KeyImporter? I lean towards the former.

KendallWeihe · 2024-05-14T13:21:15Z

crates/keys/src/key_manager/local_key_manager.rs

@@ -31,41 +38,81 @@ impl KeyManager for LocalKeyManager {
        curve: Curve,
        key_alias: Option<String>,
    ) -> Result<String, KeyManagerError> {
-        let key_alias = self.key_store.generate_new(curve, key_alias)?;
+        let private_key = Arc::new(match curve {


FYI #205 (not necessary for this PR, just raising awareness)

crates/keys/src/key_manager/local_key_manager.rs

KendallWeihe · 2024-05-14T13:25:47Z

crates/keys/src/key_manager/mod.rs

-    #[error(transparent)]
-    KeyStoreError(#[from] KeyStoreError),
+    #[error("{0}")]
+    InternalKeyStoreError(String),


thoughts?

Suggested change

InternalKeyStoreError(String),

KeyStoreError(String),

KendallWeihe · 2024-05-14T13:28:18Z

crates/keys/src/key_manager/mod.rs

+    fn import(&self, private_key: Arc<dyn PrivateKey>) -> Result<String, KeyManagerError> {
+        let key_alias = private_key.alias()?;
+        self.import_with_alias(private_key, &key_alias)?;
+        Ok(key_alias)


this is clever 👏

KendallWeihe · 2024-05-14T13:43:45Z

Alright a handful of things...

This is amazing work. I've ✅'d but I propose we let it sit for a couple days at least to give people time to chime in if they choose.
I respect taking the discretion to remove the KeyStore and I approve. We have a guiding principal here in rust core to "prefer modularity over convenience" but in this case, we don't have a concrete use case (that we can even think of) where decoupling the abstractions is necessary, so yeah I think it's wise to remove.
I appreciate the named functions import_with_alias and import rather than using a single function with Option param. I think this is the ideal pattern here in our rust initiative -- it's slightly more verbose than what we're accustomed to with other languages, but it'll pay dividends IMO as we start bindings things.
💯 on not implementing the KeyExporter trait. I think what I'll probably do is eventually close Move import & export functions to new trait called UnsafeKeyManager #158 and create a new ticket for the export functionality, which hopefully just sits there forever. I'll handle that.

What is the purpose of the .alias() method in the Key trait? Is it a unique digest? Whatever it is, we should probably rename the method.

It's a good point... the way I think about it is, the key alias is akin to a primary key in a db, so it's self referential with respect to a key. It's slightly confusing though because we enable callers of key manager methods to override the key alias, so it's a matter which is conceptualized at the key manager level, but at the same time we implement the alias for the Jwk which is the thumbprint. 🤔 created #206

What is the expected behavior of KeyImporter when a key alias is overwritten with a new private key? Should that be an error? Is it up to each individual impl of KeyImporter? I lean towards the former.

Yeah that should be an error. So the requirement is: key's cannot be imported which have a key alias which pre-exist in the key manager. Created #207 @diehuxx feel free to go ahead and do that here or not and we can follow up, your call.

mistermoe · 2024-05-15T12:17:13Z

How is a portable did created without a key exporter?

KendallWeihe · 2024-05-15T12:49:17Z

How is a portable did created without a key exporter?

It can't. But, TBD. This is a hot topic, so perhaps I'll open a ticket to discuss the full merits. I know this has been discussed at length a while back, but it's appropriate to take a bit of time to reopen the matter with the rust initiative.

KendallWeihe · 2024-05-15T14:15:08Z

Added #211, we can close #158 upon merging this PR

* main: implement vc verification (#182) Remove unused credentials/tests, rename vc.rs to verifiable_credential.rs (#214) Refactor KeyManager (#204) Rename 'method' module to 'methods' (plural) (#210)

diehuxx requested review from KendallWeihe, nitro-neal, kirahsapong, shamilovtim, decentralgabe and frankhinek as code owners May 13, 2024 23:11

diehuxx force-pushed the key-importer branch from 248f808 to 1a125b9 Compare May 13, 2024 23:51

KendallWeihe reviewed May 14, 2024

View reviewed changes

crates/keys/src/key_manager/local_key_manager.rs Show resolved Hide resolved

KendallWeihe reviewed May 14, 2024

View reviewed changes

KendallWeihe approved these changes May 14, 2024

View reviewed changes

This was referenced May 14, 2024

Consider removing or renaming Key trait's alias() method #206

Open

Key's should not be imported which have a key alias which pre-exists in the key manager #207

Open

KendallWeihe mentioned this pull request May 15, 2024

Add serialization of private key material (KeyExporter) #211

Open

Diane Huxley added 2 commits May 15, 2024 20:36

Refactor KeyManager

061a552

Lint and fix

040cd90

KendallWeihe force-pushed the key-importer branch from 1a125b9 to 040cd90 Compare May 16, 2024 00:39

KendallWeihe merged commit 807067e into main May 16, 2024
6 checks passed

KendallWeihe deleted the key-importer branch May 16, 2024 00:41

KendallWeihe mentioned this pull request May 16, 2024

Move import & export functions to new trait called UnsafeKeyManager #158

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor KeyManager #204

Refactor KeyManager #204

diehuxx commented May 13, 2024

KendallWeihe May 14, 2024

KendallWeihe May 14, 2024

KendallWeihe May 14, 2024

KendallWeihe commented May 14, 2024

mistermoe commented May 15, 2024

KendallWeihe commented May 15, 2024 •

edited

Loading

KendallWeihe commented May 15, 2024

Refactor KeyManager #204

Refactor KeyManager #204

Conversation

diehuxx commented May 13, 2024

Context

Design

Open questions

KendallWeihe May 14, 2024

Choose a reason for hiding this comment

KendallWeihe May 14, 2024

Choose a reason for hiding this comment

KendallWeihe May 14, 2024

Choose a reason for hiding this comment

KendallWeihe commented May 14, 2024

mistermoe commented May 15, 2024

KendallWeihe commented May 15, 2024 • edited Loading

KendallWeihe commented May 15, 2024

KendallWeihe commented May 15, 2024 •

edited

Loading