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

Reading mnemonic from keychain #45

Open
0xlaine opened this issue Apr 28, 2022 · 5 comments
Open

Reading mnemonic from keychain #45

0xlaine opened this issue Apr 28, 2022 · 5 comments
Labels
enhancement New feature or request

Comments

@0xlaine
Copy link
Contributor

0xlaine commented Apr 28, 2022

Idea

In thread with #39 I was thinking whether support should be added for reading the signer's mnemonic from the local device's keychain, rather than hardcoded config or ENV vars.

Keychains on Linux, Mac OS and Windows provides encrypted storage with access-gating by the logged in user, and should thus be a safer storage option than exporting ENV or saving static config files.

It should be opt-in and not the default option, to ensure that this is not a breaking change to terrain

Implementation

Expand on wallet:new (if #41 is accepted and merged) so it can --store-to-keychain=<name>. Name would be the name of the secret to register in the keychain, and it could default to signer_mnemonic or so.

Additionally a wallet:store command can be added to save either the content of a file (with a mnemonic) or via standard-in, into the keychain.

Finally, adding support for setting an env-var like SECRET_FROM_KEYCHAIN_NAME=<name> which would supercede all other secret vars or config, and instruct terrain to read the mnemonic from the keychain.

The node-js package keytar from https://github.com/atom/node-keytar works cross platform and has a very easy to use async interface for writing and reading keychain data. I have just verified it working on Ubuntu, mac OS and Windows without hassle.

@0xlaine
Copy link
Contributor Author

0xlaine commented Apr 28, 2022

@octalmage @emidev98 Thoughts - love it, hate it?

I'll be happy to provide a PR :)

@emidev98
Copy link
Contributor

emidev98 commented May 2, 2022

Hey, I like the approach that the mnemonics are stored safely and at the same time I like the approach of using a unique library that will fix the platform compatibility issue. I will check with the team and come back to you. Thanks!

@emidev98 emidev98 added the enhancement New feature or request label May 2, 2022
@octalmage
Copy link
Contributor

I would love this! I'm a big fan of node-keytar, I attempted to implement node-keytar in a way that could read from the same keystore that terrad uses. I think this would be ideal, but I was struggling to get it working.

@0xlaine
Copy link
Contributor Author

0xlaine commented May 3, 2022

I would love this! I'm a big fan of node-keytar, I attempted to implement node-keytar in a way that could read from the same keystore that terrad uses. I think this would be ideal, but I was struggling to get it working.

Cool, I will give it a stab and submit a PR when I have something working 👍

@0xlaine
Copy link
Contributor Author

0xlaine commented May 4, 2022

@octalmage @emidev98

Okay so I have been digging into how the terrad cli is storing the keys into the keychain (and the rabbit hole goes very deep now 😅 ).

From what I can gather the terra-money/cosmos-sdk keys command is Amino encoding it's storage... Clues:

There's also a terra-money/amino-js project however this is archived, and the amino-structs for the LocalInfo are outdated and therefore unable to decode (I have tried already, decoding throws byte-length errors).

A way forward:
Either terra-money resurrects the amino-js project and we update the structs, rebuild and publish a v4 of the package (as I assume the struct updates is a breaking change), or I fork it and create a separate package we can consume. I like the first approach best.

That is, if we wish to read and store in a terrad compatible way.

I will try with a local updated version of the amino-js library to verify that this path would indeed work, before we pursue any library resurrections.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants