Draft: Added support for the different SecretFormat within the Vault

changed the description

Good idea to use the filename to store the format.

An other option would be to add a proper format (eventually by adding a .<format> to the name) and have an internal format with more options like:

a name
crypto type
...

This way we could have something like alice.keyformat which is encrypted file of a json containing:

{
  "name": "alice",
  "crypto_type": "ed25519",
  "secret_type": "mnemonic",
  "secret": "<mnemonic>",
  "more": 123
}

Which would allow to make some command like:

# suggestion of command
$ vault use alice
account 5GrwvaEF5zXb26Fz9rcQpDWS57CtERHpNehXCPcNoHGKutQY selected

(similar to what was suggested in #36)

I wonder if such format already exists so that we do not have to re-develop one.

Just to avoid confusion in this discussion:

Address: SS58 Substrate Public Key
Identity: Optional username linked to an Address
VaultItemName: Optional vault item name linked to an Address

I read #36 and for that one; I wonder how to avoid potential confusion between a VaultItemName and an optional Identity name - In my test cases above, I have several Address that don't have any Identity.

But if all the commands using that VaultItemName explicitly mention something like "<vault_item_name> or <Address>" for a command such as gcli vault use ... I guess it could be fine.

And having something more standardised/structured for the vault entries would be a good idea as well; we just have to think of what can work with all current usages + the ones we want to add.

For now, a vault entry is selected through the current Address - which is the only thing that should always exist for any kind of vault entry.

1. completely encrypted Vault entry

Let's say we now use vault filenames named one of

<VaultItemName>.keyformat
<Address>.keyformat

With encrypted content

{
  #If it's encrypted; that one is useless to retrieve the correct Vault entry 
  #I expect this one to be VaultItemName - not Identity which can change independently
  "name": "Alice",
  "crypto_type": "ed25519",
  "secret_type": "mnemonic",
  "secret": "<mnemonic>",
  "more": 123
}

1.1 Using VaultItemName

In the case we had <VaultItemName>.keyformat for say Alice.keyformat

Commands using -a <Address> will not be able to find a Vault entry even if it was for the proper Address of Alice

Unless we keep a part of the vault entry un-enctrypted and with an extra Address property (point 2)

Or we need to format file names more strictly - say one of

<Address>.keyformat
<Address>_<VaultItemName>.keyformat

Which means if searching for VaultItemName we need to parse all file names until we have a match

Also tricky to prevent using the same VaultItemName more than once

2. Partially encrypted Vault entry

Content

{
  # Un-encrypted value of VaultItemName
  "name": "Alice",
  # Un-encrypted value of Address
  "address": "5GrwvaEF5zXb26Fz9rcQpDWS57CtERHpNehXCPcNoHGKutQY",
  # value could be encrypted
  "crypto_type": "ed25519",
  # value could be encrypted
  "secret_type": "mnemonic",
  # value MUST be encrypted - and still need to support dual secret
  "secret": "<mnemonic>",
  # value could be encrypted
  "more": 123
}

In this case I don't even think the file name is still relevant, because unless we put both the Address and the VaultItemName in it (filename), we won't be able to find it back with the other one.

So I would keep the original naming + extension

<Address>.keyformat

And we would have to parse all existing vault entries at the start of the gcli command and keep maps of

VaultItemName => Address
Address => vault_item_filepath

Which would also allow us to prevent naming a vault entry with a name already used in the map

... I'll stop here for those first thoughts; maybe you can check those and give me your feedback

Extra idea for the VaultItemName to Address mapping & filename.

Still use the base file names

<Address>.keyformat

And one extra special file - say

vault_item_name.mapping

That could have a json structure like

{
  "Alice": "5GrwvaEF5zXb26Fz9rcQpDWS57CtERHpNehXCPcNoHGKutQY",
  "Nicolas": "5...",
}

So only one file to parse at startup of every gcli command - and we can keep the "keyformat" files fully encrypted...

The single ".mapping" file could be a .sqlite file with the same structure as Tikka's. This would allow to manage derivations and so on. The scope is larger than this MR but it's nice to think ahead to know the direction we are going!

You're right that -a would not be able to find account if the name of the encrypted file is simply name.xxx. But this option is not very intuitive anyway, so we might prefer to have some kind of -v <name> option to tell which vault to use.

This moves from stateless commands to stateful commands where the state contains some context (like the account to target / to use) within the config file.

I pushed a new commit with support for sqlite (I didn't change anything about the vault key files for now).

I added support of mapping Address <=> VaultName and added some extra commands in Vault.

It's normally also possible to use -a <Address> or -v VaultName interchangeably

I also left 2 points for review in there (//TODO & //FIXME).

Since it's my first time using databases with rust; I looked a bit at creates and went with sea-orm as I wanted to have support of entity mapping (not being forced to write the table creation sql).

Can you (or some other dev on the project) review that; and then we can re-discuss the best approach for the vault key files themselves. Since we would now have the database anyway; do we want to store that in there or still on the file system ?

I checked in Tikka for the way they mapped the keys and found 3 entities for it:

https://git.duniter.org/clients/python/tikka/-/blob/main/tikka/domains/entities/account.py?ref_type=heads

https://git.duniter.org/clients/python/tikka/-/blob/main/tikka/domains/entities/password.py?ref_type=heads

https://git.duniter.org/clients/python/tikka/-/blob/main/tikka/domains/entities/wallet.py?ref_type=heads

Although I don't clearly understand why we would need to have 2 persistence of key in "password" and "wallet"... (apparently, a password entry is only ever created when it's for root account...)

Edit: In fact "password" entity persists the password itself and encrypts it with the keypair (for when user adds derivation keys without a password - fetching the one of the root key) I don't think we want that for GCli - also I don't see the point of persisting several wallets neither - we only actually need the root mnemonic/seed(SR25519)/seed(ED25519)

Also, I have a question regarding non-mnemonic keys; it is possible to use derivation path when working with cesium key (2 secrets) or from a private seed directly ?

If I understand this part correctly; derivations can also be created from the private seed (called mini-secret)

https://polkadot.js.org/docs/keyring/start/suri/

@HugoTrentesaux

Actually, it seems it's possible to make derivations for a cesium v1 key (I made a test for it as well as all SecretFormat derivations):

/// Test which verifies that it's possible to derive a key coming from a cesium v1 id & password
///
/// Using subkey command with **ed25519** scheme to show we can derive a key from a seed
/// and to retrieve expected values.
///
/// ##### Without derivation (using seed from the test)
/// ```
/// subkey inspect --scheme ed25519
/// URI:
/// Secret Key URI `0x2101d2bc68de9ad149c06293bfe489c8608de576c88927aa5439a81be17aae84` is account:
///   Network ID:        substrate
///   Secret seed:       0x2101d2bc68de9ad149c06293bfe489c8608de576c88927aa5439a81be17aae84
///   Public key (hex):  0x697f6bd16ddebf142384e503fd3f3efc39fe5c7be7c693bd98d982403bb6eb74
///   Account ID:        0x697f6bd16ddebf142384e503fd3f3efc39fe5c7be7c693bd98d982403bb6eb74
///   Public key (SS58): 5ET2jhgJFoNQUpgfdSkdwftK8DKWdqZ1FKm5GKWdPfMWhPr4
///   SS58 Address:      5ET2jhgJFoNQUpgfdSkdwftK8DKWdqZ1FKm5GKWdPfMWhPr4
/// ```
///
/// ##### With derivation '//0' (using seed from the test)
/// ```
/// subkey inspect --scheme ed25519
/// URI:
/// Secret Key URI `0x2101d2bc68de9ad149c06293bfe489c8608de576c88927aa5439a81be17aae84//0` is account:
///   Network ID:        substrate
///   Secret seed:       0x916e95359a49c82e3d84269b90551433352c433eb9bb270fb8cb86e8a6c9ec85
///   Public key (hex):  0x1658ce32f039dff26d83b5282f611cfed8c71296a311417ef737db4e016194de
///   Account ID:        0x1658ce32f039dff26d83b5282f611cfed8c71296a311417ef737db4e016194de
///   Public key (SS58): 5Ca1HrNxQ4hiekd92Z99fzhfdSAqPy2rUkLBmwLsgLCjeSQf
///   SS58 Address:      5Ca1HrNxQ4hiekd92Z99fzhfdSAqPy2rUkLBmwLsgLCjeSQf
/// ```
#[test]
fn test_cesium_v1_key_derivation() {
  let cesium_id = "test_cesium_id".to_string();
  let cesium_pwd = "test_cesium_pwd".to_string();

  let cesium_keypair = pair_from_cesium(cesium_id, cesium_pwd);

  println!("Nacl keypair: pkey:'0x{}' skey:'0x{}'", hex::encode(cesium_keypair.pkey), hex::encode(cesium_keypair.skey));

  let cesium_v1_pubkey = bs58::encode(cesium_keypair.pkey).into_string();
  println!("Cesium v1 Pubkey: '{}'", cesium_v1_pubkey);
  let cesium_v1_address: AccountId = cesium_keypair.pkey.into();
  println!("SS58 Address: '{}'", cesium_v1_address);

  //ed25519 seed **seems** to be the first 32 bytes of the secret key from nacl keypair
  let mut seed: [u8; 32] = [0; 32];
  seed.copy_from_slice(&cesium_keypair.skey[0..32]);
  println!();
  println!("ed25519 seed: '0x{}'", hex::encode(seed));

  let ed25519_pair = sp_core::ed25519::Pair::from_seed(&seed);

  println!();
  println!("ed25519 keypair: public:'0x{}' raw_vec:'0x{}'", hex::encode(ed25519_pair.public().0), hex::encode(ed25519_pair.to_raw_vec().as_slice()));
  let ed25519_address: AccountId = ed25519_pair.public().into();
  println!("ed25519 keypair: public SS58 Address:'{}'", ed25519_address);
  assert_eq!(cesium_v1_address, ed25519_address);

  // Tested derivation manually with `subkey` command using adapted suri: `0x2101d2bc68de9ad149c06293bfe489c8608de576c88927aa5439a81be17aae84//0`
  let expected_ss58_address_derivation_0 = "5Ca1HrNxQ4hiekd92Z99fzhfdSAqPy2rUkLBmwLsgLCjeSQf".to_string();

  let (derived_ed25519_pair, _seed) = ed25519_pair.derive(Some(sp_core::DeriveJunction::hard(0)).into_iter(), None).unwrap();
  println!();
  println!("ed25519 derived keypair: public:'0x{}' raw_vec:'0x{}'", hex::encode(derived_ed25519_pair.public().0), hex::encode(derived_ed25519_pair.to_raw_vec().as_slice()));
  let account_id32 = subxt::utils::AccountId32::from(derived_ed25519_pair.public());
  println!("ed25519 derived keypair: public SS58 Address:'{}'", account_id32);
  assert_eq!(expected_ss58_address_derivation_0, account_id32.to_string());
}

But this requires to use sp_core::ed25519::Pair instead of nacl::sign::Keypair (unless it's also supported in nacl; but I didn't find it).

So my question is: would it be ok to change the code for cesium so that it uses sp_core::ed25519::Pair everywhere (except for retrieving the seed from cesium id & secret using nacl::sign::Keypair) ?

That would also mean:

removing custom CesiumSigner to use what is provided by sp_core::ed25519::Pair
in the Vault, we could only encrypt the seed for cesium accounts instead of both id & secret
...

the best approach for the vault key files themselves

The reason I wanted age-encrypted secrets is for portability. Example using rage (https://github.com/str4d/rage):

> gcli vault where
/home/hugo/.local/share/gcli
> rage --decrypt ~/.local/share/gcli/5GrwvaEF5zXb26Fz9rcQpDWS57CtERHpNehXCPcNoHGKutQY
bottom drive obey lake curtain smoke basket hold race lonely fit walk//Alice

I checked in Tikka for the way they mapped the keys

You can discuss Tikka with vit on the forum.

password entry is only ever created when it's for root account

This can be discussed here: https://forum.duniter.org/t/utilisation-dune-derivation-avec-gcli/12761. The idea of HD wallet is that with known derivation scheme, we can use a single secret to generate multiple keys. The choice of encrypting only one secret or every secret with derivation independantly depends on what we want to achieve. Since gcli does not have persistent memory secret between two runs, I think it does not make sense here as you write below.

So my question is: would it be ok to change the code for cesium so that it uses sp_core::ed25519::Pair everywhere (except for retrieving the seed from cesium id & secret using nacl::sign::Keypair) ?

The point of "Cesium secret" is to be compatible with legacy software. Using something else as nacl would break this, so there is no point doing this. Think of "non-mnemonic keys" as "non secure" since the entropy level is so low that a computer can quickly break this just by brute-forcing all accounts pubkeys.

In the Vault, we could only encrypt the seed

That is an other feature which could be useful. But with each seed you have to know the key derivation function to use if you want to produce the same keypair. And the standard way of producing a high entropy seed is to use standard implementations that usually expose... a mnemonic! (instead of the bare seed or "mini secret").

You can apply cargo fmt on your code to format it. We have a check in Duniter CI, but gcli does not have. You can also use cargo clippy to see what could be improved in your code (for example here an unnecessary use of to_string).

Having the format inside the file would prevent from having to pass the format here as an argument. And note that "address" is a string, so you could simply wrap this function by giving format!("{}-{}", address, format_str) as an argument to get_vault_key_path function, allowing to use the same function if the filename is something like alice.json or so.

changed this line in version 2 of the diff

dirty

Indeed; but I'll let it on the side until we decide on the vault item filename & content in the first comment as it will be impacted anyway.

It's not unlikely that this function will at some point be used on a file which contains a single line. At this point we would rather return a gcli error telling that vault file does not match expected format rather than panic ^^

changed this line in version 6 of the diff

I did a quick review with some non-trivial suggested changes. Do not hesitate to tell me if it looks like a too big task for now!

Thanks for the thorough and detailed review, I'll look at it in details when I have some free time

requested changes

marked this merge request as draft

added 1 commit

97abd338 - * Modified VaultItem to only contain relevant data necessary to create the vault entry.

Compare with previous version

added 1 commit

9f712d7b - * Added sea-orm dependency to allow having DB entity mappings and use a local sqlite file database

Compare with previous version

added 1 commit

6d1be328 - * Added test for cesium v1 key derivation

Compare with previous version

added 1 commit

18e061cd - * Moved cesium derivation test into keys.rs

Compare with previous version

Can you describe your suggestions of architecture on the forum? That would allow to collect more feedback before sticking to an implementation with blind spots ;)

I'll check later to create a post on the forum, referencing the new (cleaner) MR !41 (merged)

Some questions for that, is it better to do it in English or French? (I'm not sure what languages the different developers speak)?

Added post in forum asking for review: https://forum.duniter.org/t/request-for-review-adding-db-persistence-for-all-secretformat-of-vault-keys-as-well-as-supporting-derivations/12829

I almost have something testable now; still need to do some cleanup (and some extra manual testing might be useful as well).

For now; I kept the same encrypt & decrypt functions; but only persists "substrate uris" for the root vault accounts.

This is in the form of

sr25519 mnemonic: "bottom drive obey lake curtain smoke basket hold race lonely fit walk"
sr25519 seed: "0xfac7959dbfe72f052e5a0c3c8d6530f202b02fd8f9f5ca3580ec8deb7797479e" (seed of the mnemonic above)
ed25519 seed: "0x916e95359a49c82e3d84269b90551433352c433eb9bb270fb8cb86e8a6c9ec85" (seed retrieved from nacl key, which is retrieved from cesium id & secret)

added 1 commit

e5dbb383 - * Lot of changes to add vault_account and vault_derivation to persist everything.

Compare with previous version

I retrieved a patch of this branch and re-phrased that in one commit on a new branch that is directly on the original repository so I could also rebase with latest changes from master (I didn't have developer access before):

!41 (merged)

We should probably close this MR

mentioned in merge request !41 (merged)

Because !40 (closed) is merged, we can close this one.

see !41 (comment 46152)

closed

mentioned in issue #36

 		keys::Secret::DualSecret(id, pwd) => {
 			//Making a simple separation of the 2 parts with a newline character
 			let secret = format!("{id}\n{pwd}");
 			file.write_all(&encrypt(secret.as_bytes(), password).map_err(|e| anyhow!(e))?[..])?;
+		}
+	}
 	Ok(address)
+}
 /// try get secret in keystore
 pub fn try_fetch_secret(data: &Data, address: AccountId) -> Result<Option<String>, GcliError> {
 	let path = data.project_dir.data_dir().join(address.to_string());
 fn get_vault_key_path(data: &Data, secret_format: SecretFormat, address: String) -> PathBuf {
 	let format_str: &'static str = From::from(secret_format);
 	data.project_dir.data_dir().join(format!("{}-{}", address, format_str))

 		secret_format = SecretFormat::Seed;
 		path = get_vault_key_path(data, secret_format, address.to_string());
+	}
 	if !path.exists() {
 		secret_format = SecretFormat::Cesium;
 		path = get_vault_key_path(data, secret_format, address.to_string());
+	}
 	if !path.exists() {
 		secret_format = SecretFormat::Predefined;
 		path = get_vault_key_path(data, secret_format, address.to_string());
+	}
 	if path.exists() {
 		Ok(Some((secret_format, path)))
 	} else {
 		Ok(None)
+	}

 		let secret_vec = decrypt(&cypher, password).map_err(|e| GcliError::Input(e.to_string()))?;
 		let secret = String::from_utf8(secret_vec).map_err(|e| anyhow!(e))?;
 		//Still need to handle different secret formats
 		match secret_format {
 			SecretFormat::Substrate => {
 				Ok(Some(pair_from_str(&secret)?.into()))
+			}
 			SecretFormat::Seed => {
 				Ok(Some(pair_from_seed(&secret)?.into()))
+			}
 			SecretFormat::Cesium => {
 				let mut lines = secret.lines();
 				//Un-wrapping the 2 secrets from each line
 				let id = lines.next().unwrap();
 				let pwd = lines.next().unwrap();

Draft: Added support for the different SecretFormat within the Vault

Performed manual tests using accounts:

Saving the different key types in the vault

Using the different vaults for transfers

Activity

1. completely encrypted Vault entry

1.1 Using VaultItemName

2. Partially encrypted Vault entry

Draft: Added support for the different SecretFormat within the Vault

Performed manual tests using accounts:

Saving the different key types in the vault

Using the different vaults for transfers

Merge request reports

Activity

1. completely encrypted Vault entry

1.1 Using VaultItemName

2. Partially encrypted Vault entry