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

feat(wallet): add change mnemonic password rpc #2317

Open
wants to merge 19 commits into
base: dev
Choose a base branch
from

Conversation

borngraced
Copy link
Member

@borngraced borngraced commented Jan 10, 2025

implement functionality to update the password for seed storage in the wallet in kdf with it's rpc method

Example JSON Request

{
       "method": "update_seed_storage_password",
	"userpass": "rpc_password",
	"mmrpc": "2.0",
	"params": {
		"current_password": "old_password123",
		"new_password": "new_password456"
	}
 }

Example JSON Response

{
    "result": "success"
}

Comment on lines 590 to 593
let encrypted_data = encrypt_mnemonic(&mnemonic, &req.new_password)?;
// save new encrypted mnemonic data with new password
save_encrypted_passphrase(&ctx, &wallet_name, &encrypted_data).await?;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could u actually verify whether this would play well in wasm?
this call eventually does table.add_item, which if the item already exists will error ConstraintError (as per this).

We should use table.replace_item to be able to supported updates.

Copy link
Member Author

@borngraced borngraced Jan 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done thank you 9e50c9f

@laruh
Copy link
Member

laruh commented Jan 10, 2025

Why the result is bool and not Ok if password updated successfully or Err if failed?
image

mm2src/mm2_main/src/lp_wallet.rs Outdated Show resolved Hide resolved
mm2src/mm2_main/src/lp_wallet.rs Outdated Show resolved Hide resolved
@borngraced borngraced changed the title feat(wallet): add support for updating seed storage password feat(wallet): add update seed storage password rpc Jan 17, 2025
@shamardy
Copy link
Collaborator

One thing I need @CharlVS's opinion on regarding this PR: Should we update the password using an RPC or during KDF initialization? I think the GUI will provide users the option to update the password before starting KDF. Also, some CLI users will not want to pass the encryption password through RPC at all. Can we have the two modes if possible @borngraced?

@CharlVS
Copy link
Member

CharlVS commented Jan 23, 2025

@shamardy

using an RPC or during KDF initialization?

For the GUI, I'd strongly prefer it to be via RPC. Doing via startup-config for GUI is not ideal because:

  1. The user would have to sign out to change their password or change their password before signing in, which goes against what they'd expect based on the typical flow in other apps.
  2. The user would have to shut down their trading bot and also wait for swaps to complete.

This is outside my domain, but I'd expect it to be a non-issue for CLI users since they face a similar issue with private key RPCs. Those concerned about this would/should be using HTTPS mode. However, the damage from a single coin's leaked private key could be far less than that from a leaked password.

If you implement startup-config password changes, it's worth considering either terminating KDF after the password is updated, or make the fields named password and old_password so that it's idempotent. (Ignore old_password if password is valid)

@borngraced
Copy link
Member Author

borngraced commented Jan 24, 2025

One thing I need @CharlVS's opinion on regarding this PR: Should we update the password using an RPC or during KDF initialization? I think the GUI will provide users the option to update the password before starting KDF. Also, some CLI users will not want to pass the encryption password through RPC at all. Can we have the two modes if possible @borngraced?

Also, some CLI users will not want to pass the encryption password through RPC at all

seems to be a good reason why we would want to have the both.

make the fields named password and old_password so that it's idempotent. (Ignore old_password if password is valid)

  • If password is valid -> nothing is needed to be done
  • Else if old_password valid -> update password to new password
  • Else -> reject

cc. @CharlVS @shamardy

@mariocynicys
Copy link
Collaborator

Should we update the password using an RPC or during KDF initialization? I think the GUI will provide users the option to update the password before starting KDF.

Why would the GUI want to provide such a feat considering that RPC comm is internal on the same device (well if you want a GUI to re-launch KDF with different config, this means that GUI runs KDF internally).

Also, some CLI users will not want to pass the encryption password through RPC at all

If KDF is on the same device that shouldn't be an issue for a CLI user. If it's on a different device, they could log into the device/machine hosting KDF and do the RPC there (well if they wanted to go with changing the config route, this means they must have access to that device hosting KDF).
That's assuming that the hosting device isn't compromised. If it's compromised then having the password plainly in the config file is already dangerous.

mariocynicys
mariocynicys previously approved these changes Jan 24, 2025
Copy link
Collaborator

@mariocynicys mariocynicys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM
non-blocking comments inline.

@@ -217,6 +217,7 @@ async fn dispatcher_v2(request: MmRpcRequest, ctx: MmArc) -> DispatcherResult<Re
"trade_preimage" => handle_mmrpc(ctx, request, trade_preimage_rpc).await,
"trezor_connection_status" => handle_mmrpc(ctx, request, trezor_connection_status).await,
"update_nft" => handle_mmrpc(ctx, request, update_nft).await,
"update_seed_storage_password" => handle_mmrpc(ctx, request, update_seed_storage_password_rpc).await,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we better drop the word storage here.

storage makes it feel as if the whole seed store (multiple seeds) is encrypted using a single pass and not just the single seed in question.
better alternatives imo: update_seed_password or update_seedphrase/passphrase_password

that's a non-blocking comment though since im not a certified linguist.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use the word mnemonic not seed since we have get_mnemonic RPC. How about change_mnemonic_password

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aha better

))))?;
// encrypt mnemonic with new passphrase.
let encrypted_data = encrypt_mnemonic(&mnemonic, &req.new_password)?;
// save new encrypted mnemonic data::default() with new password
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

data::default() what?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo, thanks for the catch

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well actually this is blocking 🤦‍♂️

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@shamardy
Copy link
Collaborator

Why would the GUI want to provide such a feat considering that RPC comm is internal on the same device

I thought that updating password flow will be like some websites where you do it without logging in, but @CharlVS cleared that.

If it's compromised then having the password plainly in the config file is already dangerous.

It's shouldn't be in the config file, it should be passed as args with the start KDF command.

@shamardy
Copy link
Collaborator

One thing I need @CharlVS's opinion on regarding this PR: Should we update the password using an RPC or during KDF initialization? I think the GUI will provide users the option to update the password before starting KDF. Also, some CLI users will not want to pass the encryption password through RPC at all. Can we have the two modes if possible @borngraced?

Also, some CLI users will not want to pass the encryption password through RPC at all

seems to be a good reason why we would want to have the both.

make the fields named password and old_password so that it's idempotent. (Ignore old_password if password is valid)

  • If password is valid -> nothing is needed to be done
  • Else if old_password valid -> update password to new password
  • Else -> reject

cc. @CharlVS @shamardy

No need to, let's have the RPC command only for now like it is now.

Copy link
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments from my side!

mm2src/mm2_main/src/lp_wallet.rs Outdated Show resolved Hide resolved
mm2src/mm2_main/src/lp_wallet.rs Outdated Show resolved Hide resolved
mm2src/mm2_main/src/rpc/dispatcher/dispatcher.rs Outdated Show resolved Hide resolved
laruh
laruh previously approved these changes Jan 24, 2025
Copy link
Member

@laruh laruh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

shamardy
shamardy previously approved these changes Jan 24, 2025
Copy link
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Only one nit.

Can you please add test coverage for the new RPC?

mm2src/mm2_main/src/lp_wallet.rs Outdated Show resolved Hide resolved
@borngraced borngraced changed the title feat(wallet): add update seed storage password rpc feat(wallet): add change mnemonic password rpc Jan 24, 2025
@borngraced borngraced dismissed stale reviews from shamardy and laruh via d9647de January 24, 2025 14:00
#[display(fmt = "Invalid request error: {}", _0)]
InvalidRequest(String),
#[display(fmt = "Wallets storage error: {}", _0)]
WalletsStorageError(String),
#[display(fmt = "Internal error: {}", _0)]
Internal(String),
#[display(fmt = "Invalid password error: {}", _0)]
#[from_stringify("MnemonicError")]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have MnemonicError and now MnemonicsError, which are too similar.
Maybe MnemonicsError -> ApiMnemonicError?

Copy link
Member Author

@borngraced borngraced Jan 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be MnemonicRpcError... thanks

update: done c782372

@borngraced
Copy link
Member Author

borngraced commented Jan 26, 2025

LGTM! Only one nit.

Can you please add test coverage for the new RPC?

done c782372 and c782372

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants