refactor: move authentication outside the `Client` #718

tomyrd · 2025-02-05T21:51:16Z

closes #637

This PR refactors authentication out of the Client. The main idea is that now key tracking is responsability of the authenticator and the addition of new keys is done by the user of the client and not by the Client itself. The Client will only have access to the TransactionAuthenticator::get_signature function that will be used to sign transactions when needed. If the keys were not added by the client user to the authenticator then this will fail.

Authenticator

The authenticator now uses a new KeyStore instead of the client's Store to get the secret key. This keystore now is created outside the client and is responsability of the client's user to keep it updated with every new key.

Keystore

The PR adds two implementations for the keystore:

FilesystemKeyStore: based on the filesystem. It stores keys as individual files where the name is the public key and the contents are a serialized AuthSecretKey. It's used by the CLI and by the tests.
WebKeySTore: based on the browser local storage. It stores keys as entries using the Web Storage API. It's used in the web client.

Other changes

The CLI and integration tests were changed to use the new authenticator. They add keys to the authenticator when creating accounts.
The retrieval of authentication information was removed from the Store.
The web client now also uses the new authenticator. A lot of extra code was removed because keys were dealt differently in the web client (they were cached so that the authenticator could be synchronous).

TODOs:

Fix all the integration tests
Add account authentication in the CLI
Investigate posible alternative implementations for the web client. The current one doesn't work for wasm.
Remove TODOs in code.
Add documentation.
Clean account auth from stores
Add top level module documentation

…e file

igamigo

Nice! Cool that this results in a net loss of lines of code, too.
I left some minor comments, related to docs and naming mostly. I also wonder if we should generalize the CliAuthenticator into a Keystore struct to which we are implementing TransactionAuthenticator. This is because it seems like it handles more responsibilities than just authenticating, and also we might want to add new functionalities to it in the future (such as listing keys, removing them, encrypting/decrypting them, etc.).

igamigo · 2025-02-10T19:31:48Z

bin/miden-cli/src/commands/export.rs

+    Word::from(
+        account
+            .storage()
+            .get_item(1)


Wouldn't this be index 0 for any non-faucet account?

igamigo · 2025-02-10T19:37:21Z

bin/miden-cli/src/commands/import.rs

-            &account_data.auth_secret_key,
-            overwrite,
-        )
+        .add_account(&account_data.account, account_data.account_seed, overwrite)
        .await?;


This is fine for now but maybe we could add a TODO here to address potential cases where adding the secret key succeeds, but adding the account fails.

Alternatively, maybe in the future we want to have some sort of "keystore" command that adds/removes keys separately from accounts.

Created an issue for this

igamigo · 2025-02-10T19:38:36Z

bin/miden-cli/src/commands/new_account.rs

+        authenticator
+            .add_key(AuthSecretKey::RpoFalcon512(key_pair))
+            .map_err(CliError::Authentication)?;


igamigo · 2025-02-10T19:39:31Z

bin/miden-cli/src/config.rs

@@ -22,6 +22,8 @@ pub struct CliConfig {
    pub rpc: RpcConfig,
    /// Path to the sqlite store file.


nit: Not from this PR but while we are at this, let's write this line as follows

Suggested change

/// Path to the sqlite store file.

/// Path to the SQLite store file.

igamigo · 2025-02-10T19:41:22Z

bin/miden-cli/src/config.rs

@@ -22,6 +22,8 @@ pub struct CliConfig {
    pub rpc: RpcConfig,
    /// Path to the sqlite store file.
    pub store_filepath: PathBuf,
+    /// Path to secret keys file.


nit: Would this be better?

Suggested change

/// Path to secret keys file.

/// Path to the directory that contains the secret key files.

igamigo · 2025-02-10T19:43:22Z

bin/miden-cli/src/errors.rs

@@ -21,6 +21,9 @@ pub enum CliError {
    #[error("asset error")]
    #[diagnostic(code(cli::asset_error))]
    Asset(#[source] AssetError),
+    #[error("authentication error")]


nit: Maybe I'd generalize this as key store error, since this is not always authentication-related (for instance, it also manages adding keys to the filesystem)

igamigo · 2025-02-10T19:54:57Z

crates/rust-client/src/authenticator/client_authenticator.rs

+}
+
+impl<R: Rng> ClientAuthenticator<R> {
+    /// Creates a new instance of the authenticator.


nit: We should mention that this creates the directory if it does not exist (or alternatively we could do it on the doc comments for pub struct ClientAuthenticator<R>.

igamigo · 2025-02-10T20:01:58Z

crates/rust-client/src/authenticator/client_authenticator.rs

+};
+use rand::Rng;
+
+/// An account authenticator that stores keys in the filesystem.


Could we expand this doc comment mentioning the way that it stores and looks up files? For instance, saying that it's one file per AuthSecretKey, with a filename that corresponds to the pub key for easy lookups when signatures are requested, etc.

igamigo · 2025-02-10T20:05:40Z

crates/rust-client/src/authenticator/web_authenticator.rs

+use web_sys::wasm_bindgen::JsValue;
+
+#[derive(Debug, Clone)]
+pub struct WebAuthenticator<R> {


nit: A brief description of what this authenticator uses to store files would be cool.

Also, I think eventually we probably want to add password encryption to the keystore so we might want to add an issue to discuss this (or maybe it's more in the domain of the wallet itself? cc @dagarcia7)

bobbinth · 2025-02-12T01:00:35Z

Should we rebase this PR from the latest next? Seems like it may be including some extraneous changes.

tomyrd · 2025-02-12T14:12:19Z

I reviewed the diff again just to be sure but I couldn't find the extraneous changes you mention. There are a lot of changes that were necessary because of the shift in responsibility in public key tracking. Mainly the tests had to be updated so that they have access to the authenticator in case they need to add new keys (which some of them need).

dagarcia7 · 2025-02-13T06:01:32Z

crates/web-client/test.html

minor: we can get rid of this file now. It used to be how I tested things, but now with more formal integration tests for the web client, this is obsolete 👍

dagarcia7 · 2025-02-13T06:34:42Z

Looks great! 💯 Thanks for taking care of the web client changes. I have a quick question. Are there any plans to encrypt the Secret Key before it's put in local storage? And subsequently handle the decryption when retrieving it? I was also wondering if local storage was the best way to store the keys on the web side, but I think given the limitation that the get_signature call is still only synchronous, this may be the only way beyond what we were doing before with the call to cache the auth first before making a transaction call. In the future, if get_signature ends up supporting async, you can play around with this and create different types of WebKeyStore that maybe store things differently -- say, a separate IndexedDB store, or chrome extension storage, etc.

tomyrd · 2025-02-13T12:24:37Z

Are there any plans to encrypt the Secret Key before it's put in local storage? And subsequently handle the decryption when retrieving it?

I think in the future we will probably make this upgrades. This PR is meant to be a first PoC to see how we could move the keystore responsability out of the client. To upgrade we would only need to change the KeyStore which would be much easier.

I was also wondering if local storage was the best way to store the keys on the web side, but I think given the limitation that the get_signature call is still only synchronous

Before this PR we used pollster to wait on the async calls as the SqliteStore had also been changed to make it async. We can change the web storage method to an IndexDB in the future if it's better.

tomyrd added 6 commits February 5, 2025 14:30

refactor: remove Store from authenticator

42c9609

fix: build

879c45a

refactor: remove authentication from client

4a0b9c9

fix: unit tests

2c334cf

fix: integration tests

d0f9dea

fix: cli

7e67c79

tomyrd changed the title ~~refactor: remove authentication from Client~~ refactor: move authentication outside the Client Feb 6, 2025

tomyrd force-pushed the tomyrd-authentication-refactor branch from 2d0d0d2 to 7e67c79 Compare February 6, 2025 16:24

tomyrd added 9 commits February 6, 2025 15:19

refactor: change ClientAuthenticator to store each key in a separat…

000b5dc

…e file

fix: CLI export

0509596

fix: check auth procedure for account capabilities

8f83d7a

fix: remove TODOs

839eb60

refactor: create account directory

63d8c6d

add new authenticator for web client

7fde53a

fix: docs

7d1e46c

chore: improve documentation

9195555

Merge branch 'next' into tomyrd-authentication-refactor

4e0b856

tomyrd marked this pull request as ready for review February 10, 2025 17:22

fix: clean up stores

8b87cef

tomyrd force-pushed the tomyrd-authentication-refactor branch from dad3b1b to 8b87cef Compare February 10, 2025 17:26

tomyrd requested review from igamigo, SantiagoPittella and bobbinth February 10, 2025 17:35

igamigo requested review from dagarcia7 and julian-demox February 10, 2025 18:11

igamigo approved these changes Feb 10, 2025

View reviewed changes

tomyrd added 3 commits February 10, 2025 17:54

Merge branch 'next' into tomyrd-authentication-refactor

b443a31

fix: lint

592ff68

remove unused dependency

6a99d5c

tomyrd mentioned this pull request Feb 12, 2025

feat: ensure that both the account and public key get inserted successfully in the CLI #731

Open

tomyrd added 3 commits February 12, 2025 18:18

refactor: move key managment to KeyStore

d15a62b

fix: sqlite to SQLite

41e48ad

fix: add failing test and fix regular account importing

82a7f31

tomyrd force-pushed the tomyrd-authentication-refactor branch from 194f903 to 82a7f31 Compare February 12, 2025 21:18

dagarcia7 reviewed Feb 13, 2025

View reviewed changes

julian-demox removed their request for review February 13, 2025 17:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: move authentication outside the `Client` #718

refactor: move authentication outside the `Client` #718

tomyrd commented Feb 5, 2025 •

edited

Loading

igamigo left a comment

igamigo Feb 10, 2025

igamigo Feb 10, 2025

tomyrd Feb 12, 2025

igamigo Feb 10, 2025

igamigo Feb 10, 2025

igamigo Feb 10, 2025

igamigo Feb 10, 2025

igamigo Feb 10, 2025

igamigo Feb 10, 2025

igamigo Feb 10, 2025

igamigo Feb 10, 2025

bobbinth commented Feb 12, 2025

tomyrd commented Feb 12, 2025

dagarcia7 Feb 13, 2025

dagarcia7 commented Feb 13, 2025

tomyrd commented Feb 13, 2025

		@@ -22,6 +22,8 @@ pub struct CliConfig {
		pub rpc: RpcConfig,
		/// Path to the sqlite store file.

	/// Path to secret keys file.
	/// Path to the directory that contains the secret key files.

refactor: move authentication outside the Client #718

Are you sure you want to change the base?

refactor: move authentication outside the Client #718

Conversation

tomyrd commented Feb 5, 2025 • edited Loading

Authenticator

Keystore

Other changes

igamigo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobbinth commented Feb 12, 2025

tomyrd commented Feb 12, 2025

Choose a reason for hiding this comment

dagarcia7 commented Feb 13, 2025

tomyrd commented Feb 13, 2025

refactor: move authentication outside the `Client` #718

refactor: move authentication outside the `Client` #718

tomyrd commented Feb 5, 2025 •

edited

Loading