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

password-protected keystore with sc_keystore #3320

Closed
Hugo-Trentesaux opened this issue Feb 14, 2024 · 8 comments · Fixed by #3334
Closed

password-protected keystore with sc_keystore #3320

Hugo-Trentesaux opened this issue Feb 14, 2024 · 8 comments · Fixed by #3334

Comments

@Hugo-Trentesaux
Copy link
Contributor

I tried to use a password-protected keystore with LocalKeystore from sc_keystore with this naive approach:

let suri = rpassword::prompt_password("SURI: ")?;
let keypair = pair_from_str(&suri)?;
let password = rpassword::prompt_password("Password: ")?;
let password = secrecy::Secret::new(password);
let keystore =
	sc_keystore::LocalKeystore::open(project_dir.data_dir(), Some(password))?;
keystore.insert(KeyTypeId(*b"user"), &suri, &keypair.public())?;

But when looking in the keystore file, I get the SURI in clear:

cat .local/share/mytool/7573657296723102a2e339276775717781cbf445953362f7cc65df7e9be5bce5ee21c570
"drill pledge public bunker involve nose vanish buddy day puppy coast patrol"%   

If this is the intended behavior, I think it's worth documenting in https://docs.rs/sc-keystore/latest/sc_keystore/struct.LocalKeystore.html#method.open

@bkchr
Copy link
Member

bkchr commented Feb 14, 2024

But when looking in the keystore file, I get the SURI in clear:

If you load this SURI, you will find out that this is not the SURI of your private key. The actual SURI is drill pledge public bunker involve nose vanish buddy day puppy coast patrol///YOUR_PASSWORD.

/// - the path may be followed by `///`, in which case everything after the `///` is treated
/// as a password.

@bkchr bkchr closed this as completed Feb 14, 2024
@Hugo-Trentesaux
Copy link
Contributor Author

I still think that the doc of the open method of LocalKeystore could be more than one line (https://docs.rs/sc-keystore/latest/src/sc_keystore/local.rs.html#49). In particular, the password argument is not mentioned.

@davxy
Copy link
Member

davxy commented Feb 15, 2024

@Hugo-Trentesaux what is the implementation of your pair_from_str?

Because if you generated the keypair without the password (as I suspect) you end up with a problem.
Ergo: the password you use to open the keystore should match the password you used to generate the key.

If you try to sign using the pair you manually generated, it will not correspond to the one that your keystore will try to find.

Example

		let suri = "drill pledge public bunker involve nose vanish buddy day puppy coast patrol";
		let (keypair, _): (sr25519::Pair, _) = Pair::from_phrase(&suri, None).unwrap(); // No password specified for key gen
		let password = String::from("pass");
		let keystore = LocalKeystore::open(Path::new("/tmp/password"), Some(password.clone().into())).unwrap();

		let keytype = KeyTypeId(*b"user");
		keystore.insert(keytype, &suri, &keypair.public()).unwrap();

		let res = keystore.sr25519_sign(keytype, &keypair.public(), b"message");
		assert!(res.is_err());   // `keypair.public()` is not the same as the one the keystore will try to use (which is that one augmented with the pass)

If you indend to use a password for your keystore you need to construct your secret in one of these two ways:

Option 1 - Generate the key directly using the keystore methods

No need to insert() the key as it is directly generated within the keystore

		let public = keystore.sr25519_generate_new(keytype, None).unwrap();
		let sig = keystore.sr25519_sign(keytype, &public, b"message").unwrap().unwrap();
		assert!(sr25519::Pair::verify(&sig, b"message", &public));

Option 2 - Generate the key ouside the keystore but with the password

The password you use should match the one used when you open the keystore

		let (keypair, _): (sr25519::Pair, _) = Pair::from_phrase(&suri, Some(&password)).unwrap();

@bkchr
Copy link
Member

bkchr commented Feb 15, 2024

Because if you generated the keypair without the password (as I suspect) you end up with a problem.

I have totally overseen this 🙈 Maybe the best would be we return an error in insert if a password is set?

@bkchr
Copy link
Member

bkchr commented Feb 15, 2024

I mean it is not the best solution at all, but we also don't have that many possibilities.

@davxy
Copy link
Member

davxy commented Feb 15, 2024

Maybe the best would be we return an error in insert if a password is set?

Basically we have two options:

  • option 1) fail the insert if a password is set in the keystore. But this is not super ideal as the user is allowed to generate the key outside the keystore AND with a password. But if you don't plan to insert it in the keystore doesn't make much sense to have generated this key with the password in the first place.
  • option 2) Keep the things as is but document insert() such that if you manually insert a key then this should have been generated using the same password used by the keystore when has been opened

github-merge-queue bot pushed a commit that referenced this issue Feb 15, 2024
Close: #3320

@Hugo-Trentesaux are these docs better for explaining the internals?
@bkchr
Copy link
Member

bkchr commented Feb 15, 2024

  • option 2) Keep the things as is but document insert() such that if you manually insert a key then this should have been generated using the same password used by the keystore when has been opened

The problem is that if a node operator is using the author_insertKey RPC they will not find out about this that easily 🙈

@Hugo-Trentesaux
Copy link
Contributor Author

@Hugo-Trentesaux what is the implementation of your pair_from_str?

You are right, I forgot to mention that. I do not want to override the password:

Sr25519Pair::from_string(secret, None)

The problem is that if a node operator [...]

I also forgot to mention the context: I intended to use this crate not in the context of a node, but in a wallet. That's why I need the user to be able to insert exactly the mnemonic he wants, including derivation scheme.

The general idea is:

  • high entropy → mnemonic → KDF → keypair
  • mnemonic + low entropy user password → KDF → (other) keypair

My intent was to store the high entropy mnemonic in a file encrypted with low entropy user password as you would do with ssh.

@Hugo-Trentesaux are these docs better for explaining the internals?

Yes, with this doc I was able to identify that this crate was not what I was looking for.


(bonus)

If you are curious of what I finally used, I chose rage crate which is a beta Rust implementation of AGE. With armor feature, I come to something like this:

$ cat ~/.local/share/vault/5DfhGyQdFobKM8NsWvEeAKk5EQQgYe9AydgJ7rMB6E1EqRzV
-----BEGIN AGE ENCRYPTED FILE-----
YWdlLWVuY3J5cHRpb24ub3JnL3YxCi0+IHNjcnlwdCBscTQxMCsyWGpRU00zS1Qx
dDJKQVVRIDEzCk4ySWdubXljMkdPQlJERWpuZUpEekg4NGdtSmVHZWhpVCs3Ky8r
QWpnRTAKLS0tIHRQMFA3ZG9LRENtSncyTTkrQVFPZjJyTW9DZzFDdFVCckN3VDE0
UkJYMEUKcGlmGWxGDOkfsdqL2JuN8thkQPpMJItmkAs3JfzGt/s=
-----END AGE ENCRYPTED FILE-----

And this can be encrypted using ssh keys, which is convenient.

github-merge-queue bot pushed a commit that referenced this issue Feb 16, 2024
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this issue Mar 25, 2024
Close: paritytech#3320

@Hugo-Trentesaux are these docs better for explaining the internals?
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this issue Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants