solana-labs / solana-program-library

A collection of Solana programs maintained by Solana Labs
https://solanalabs.com
Apache License 2.0
3.46k stars 2.03k forks source link

token-cli: issues when sending keypair data via STDIN #2668

Closed sdeoras closed 2 years ago

sdeoras commented 2 years ago

This is a follow up from https://github.com/solana-labs/solana/issues/21811 where the intent is to pass keypair data via STDIN to spl-token CLI. It seems that the behavior or keypair coming in from STDIN is not as seamless as that coming from a file leading to a workflow that breaks during spl-token mint.

If the config file, named stdin.yaml, is defined as follows, indicating keypair coming in from STDIN.

json_rpc_url: "http://localhost:8899"
websocket_url: ""
keypair_path: "stdin:"
address_labels:
  "11111111111111111111111111111111": System Program
commitment: confirmed

and let's assume that a keypair exists in a file called id.json, then

└─ $ ▶ cat ~/.config/solana/id.json | solana-keygen pubkey --config ~/.config/solana/cli/stdin.yaml
FTS7wJf5NPqFBqbmBCeCpP8pGZoQkRxG6ug22C8wj8Zv

token creation went fine after explicitly passing --mint-authority

└─ $ ▶ cat ~/.config/solana/id.json | spl-token create-token --mint-authority=FTS7wJf5NPqFBqbmBCeCpP8pGZoQkRxG6ug22C8wj8Zv --config ~/.config/solana/cli/stdin.yaml
Creating token F8qXgouoUUdsoQF2iDT7JvEozaoJs3rq9icZUKhukNUf

Signature: 5YKg5uRFSiXe6Xq1xtd1N4cPCX63E5NcXJtd8wkTfsfmPW6zfjCyje1ktFQhLLcUWDsmdCE8WoMcA1EBVeohFiCT

but minting failed

└─ $ ▶ cat ~/.config/solana/id.json | spl-token mint F8qXgouoUUdsoQF2iDT7JvEozaoJs3rq9icZUKhukNUf 100 --config ~/.config/solana/cli/stdin.yaml
error: EOF while parsing a value at line 1 column 0

explicitly providing --mint-authority leads to another error as follows:

└─ $ ▶ cat ~/.config/solana/id.json | spl-token mint F8qXgouoUUdsoQF2iDT7JvEozaoJs3rq9icZUKhukNUf 100 --config ~/.config/solana/cli/stdin.yaml --mint-authority=FTS7wJf5NPqFBqbmBCeCpP8pGZoQkRxG6ug22C8wj8Zv
error: missing signature for supplied pubkey: FTS7wJf5NPqFBqbmBCeCpP8pGZoQkRxG6ug22C8wj8Zv

It seems that spl-token is not honoring keypair input via STDIN properly. I would have expected it to behave similarly to the case when keypair input is from a file. In particular, when keypair input is from a file, spl-token create-token and spl-token mint commands work without requiring additional flags.

Thanks in advance for support on this issue. Allowing STDIN input for keypair enabled encrypted wallets and prevents persisting of keys on the disk in plaintext.

t-nelson commented 2 years ago

What's the output of spl-token --version ?

sdeoras commented 2 years ago
$ spl-token --version
spl-token-cli 2.0.15
jmalcic commented 2 years ago

So I've investigated this a bit, and the issue is that there are situations where the CLI tries to read from STDIN more than once, and because the system buffer is (unavoidably) consumed the first time, this gives EOF errors on subsequent read attempts. This is the cause of solana-labs/solana#21811 as well, as noted there (https://github.com/solana-labs/solana/issues/21811#issuecomment-1000984039). I've looked at the file in the monorepo where this happens and there are a number of public functions that can collectively only be called once by a CLI command where STDIN is used as input, else you'd get multiple attempted reads. Here are the signatures, where indentation shows what calls what:

pub fn signer_from_path_with_config(
    matches: &ArgMatches,
    path: &str,
    keypair_name: &str,
    wallet_manager: &mut Option<Arc<RemoteWalletManager>>,
    config: &SignerFromPathConfig,
) -> Result<Box<dyn Signer>, Box<dyn error::Error>>;

    pub fn signer_from_path_with_config(
        &self,
        matches: &ArgMatches,
        wallet_manager: &mut Option<Arc<RemoteWalletManager>>,
        config: &SignerFromPathConfig,
    ) -> Result<Box<dyn Signer>, Box<dyn std::error::Error>>;

    pub fn signer_from_path(
        matches: &ArgMatches,
        path: &str,
        keypair_name: &str,
        wallet_manager: &mut Option<Arc<RemoteWalletManager>>,
    ) -> Result<Box<dyn Signer>, Box<dyn error::Error>>;

        pub fn signer_from_path(
            &self,
            matches: &ArgMatches,
            wallet_manager: &mut Option<Arc<RemoteWalletManager>>,
        ) -> Result<Box<dyn Signer>, Box<dyn std::error::Error>>;

            pub fn generate_unique_signers(…)  -> …;

        pub fn pubkey_from_path(…) -> …;

pub fn resolve_signer_from_path(…) -> …;

pub fn keypair_from_path(…) -> …;

In the token CLI in this repo, here are the publicly accessible direct usages for these—ignoring differences between the different commands, main() is making up to three calls.

pub fn signer_from_path_with_config(
    matches: &ArgMatches,
    path: &str,
    keypair_name: &str,
    wallet_manager: &mut Option<Arc<RemoteWalletManager>>,
    config: &SignerFromPathConfig,
) -> Result<Box<dyn Signer>, Box<dyn error::Error>>;

    // token/cli/src/config.rs

    pub(crate) fn signer_or_default(…) -> …;

         // token/cli/src/main.rs

         // clap commands (handled by process_command(…))
         "authorize";
         "transfer";
         "burn";
         "mint";
         "freeze";
         "thaw";
         "wrap";
         "unwrap";
         "approve";
         "revoke";
         "close";
         "gc";

pub fn signer_from_path(
    matches: &ArgMatches,
    path: &str,
    keypair_name: &str,
    wallet_manager: &mut Option<Arc<RemoteWalletManager>>,
) -> Result<Box<dyn Signer>, Box<dyn error::Error>>;

    // token/cli/src/main.rs

    // clap commands
    "create-token";
    "create-account";
    "create-multisig";

    fn main() -> Result<(), Error>; // (two calls)

    pub fn signers_of(…) -> …;

          fn main() -> Result<(), Error>;

There are also some indirect usages I can see (there might be more obviously).

pub fn pubkey_from_path(…) -> …;

    // in the monorepo: clap-utils/src/input_parsers.rs

    pub fn pubkey_of_signer(…) -> …;

        // token/cli/src/main.rs

        // clap commands
        "create-account";
        "authorize"; // (×2)
        "authorize"; // (×3)
        "burn"; // (×2)
        "mint";
        "freeze"; // (×2)
        "thaw"; // (×2)
        "unwrap";
        "approve"; // (×3)
        "revoke"; // (×2)
        "supply";
        "accounts";
        "address";
        "multisig";

        fn main() -> Result<(), Error>;

    pub fn pubkeys_of_multiple_signers(…) -> …;

        // token/cli/src/main.rs

        // clap commands
        "create-multisig";

Is the idea is to be able to send keypair data via STDIN without restriction in principle for all of this stuff? It seems like it would be good to try to use something to guard against multiple reads if so, else it's pretty hard to effectively prevent from happening somewhere with indirection and so on.

joncinque commented 2 years ago

The idea is definitely for this use case to work, where someone defines their keypair path to be stdin: in their config file. It looks like we're abusing pubkey_from_path by using it more than once. If we've read the Signer, we can the pubkey directly from that rather than dipping back into the args.

Looks like a lot of the duplicate calls are coming from the Config struct when handling defaults. That logic has always been really tricky, and the best option to me is to explicitly pass in the fallback value to signer_or_default to avoid calling signer_from_path_with_config more than once, and instead read the keypair in once at the beginning. It may be tricky, but it'll save us a lot of headaches in the long run. What do you think?