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

Relates to #674. Add suri_path and password_path options to CLI #1134

Closed
wants to merge 5 commits into from

Conversation

ltfschoen
Copy link

@ltfschoen ltfschoen commented May 27, 2023

Proposed changes
This PR relates to issue #674 and #1106 and updates the CLI options available to allow loading the suri and password from plain text .txt files so users can avoid having to enter values in the terminal. It could be changed so they can read from any file even if it doesn't have an extension.

Usage

  • Run a substrate-contracts-node on ws://127.0.0.1:9944 (e.g. this codebase allows you to run it in a Docker container https://github.com/ltfschoen/InkTemplate, and if necessary delete the chain db and restart the node by running docker exec -it ink /app/docker/reset.sh)

  • Run the following to deploy a Flipper contract, where the contract upload command reads the suri and password from associated text files with file extension .txt.

touch my-suri.txt && echo "//Alice" > my-suri.txt
touch my-password.txt && echo "" > my-password.txt
cargo build
cargo run -p cargo-contract contract new flipper
  • Move the ./flipper folder out of the workspace
mv ./flipper ../
cargo run -p cargo-contract contract build --manifest-path /path_to_flipper/Cargo.toml
cargo run -p cargo-contract contract upload --suri-path my-suri.txt --password-path my-password.txt --manifest-path /path_to_flipper/Cargo.toml

Important note: The purpose of this PR is so the user can run CLI commands that read sensitive information from a file, where they'd open a text editor and enter the sensitive information in them that way and then read from the files with CLI commands, and then delete those files after using them. That way they can avoid entering the suri and password in the terminal window where hackers may gain access to it from terminal history logs, but only for demonstration purposes I'm the example suri and password via the terminal with echo "//Alice" > my-suri.txt and echo "" > my-password.txt.

Verified it works with the following functionality:

touch my-suri.txt && echo "//Alice" > my-suri.txt
touch my-password.txt && echo "" > my-password.txt
cargo build
cargo run -p cargo-contract contract new flipper
cargo run -p cargo-contract contract build --manifest-path ./flipper/Cargo.toml
cargo run -p cargo-contract contract upload --suri-path my-suri.txt --password-path my-password.txt --manifest-path ./flipper/Cargo.toml

then it outputs the following:

      Result Success!
   Code hash "0x______"
     Deposit ___
Your upload call has not been executed.
To submit the transaction and execute the call on chain, add -x/--execute flag to the command.
  • If you do the same as in the previous step but you change the contents of my-suri.txt to be //Alice\n instead of //Alice to simulate what would happen if we weren't applying .trim() when reading the file contents, then it outputs the following since it thinks it's dealing with a different account that wouldn't have sufficient funds to upload. Likewise, if you change the my-password.txt file from not having a password (default for Alice's built-in account) to having a password, then it also outputs the following:
Result ModuleError: Contracts::StorageDepositNotEnoughFunds: ["Origin doesn't have enough balance to pay the required storage deposits."]
  • If you provide both --suri and --suri-path it outputs the following since in the code we've set multiple(false) to the ArgGroup called "surigroup"
error: the argument '--suri <suri>' cannot be used with '--suri-path <suri-path>'

Usage: cargo contract upload --suri <suri> --password-path <password-path> --manifest-path <MANIFEST_PATH> [FILE]
  • If you provide both --password and --password-path it outputs the following since in the code we've set multiple(false) to the ArgGroup called "passgroup"
error: the argument '--password <password>' cannot be used with '--password-path <password-path>'

Usage: cargo contract upload --password <password> --suri-path <suri-path> --manifest-path <MANIFEST_PATH> [FILE]
  • If you provide a file containing the password for --password-path but it doesn't have an extension, then it'll output error:
ERROR: suri data not provided. password path has no extension, expected `.txt`
  • If you provide a file containing the suri for --suri-path but it doesn't have an extension, then it'll output error:
ERROR: suri data not provided. suri path has no extension, expected `.txt`

Further considerations:

  • Something worth considering is whether to change it from:

    • --suri-path to --suri-file-path
    • --password-path to --password-file-path
  • There are numerous TODO's, so any feedback on how to approach those in addition to general review would be appreciated.

  • Maybe the Suri and Password struct types could be used better.

@ltfschoen ltfschoen marked this pull request as ready for review May 28, 2023 13:38
@ltfschoen ltfschoen changed the title WIP: Relates to #674. Add suri_path and password_path options to CLI Relates to #674. Add suri_path and password_path options to CLI May 28, 2023
@SkymanOne
Copy link
Contributor

Can you please merge the master branch, and I will review the PR?

@cmichi
Copy link
Collaborator

cmichi commented Feb 27, 2024

I'm closing the PR due to staleness and the cargo-contract master having moved on significantly since the PR was created. Feel free to reopen based on the latest master in case you're interested in continuing work on it!

@cmichi cmichi closed this Feb 27, 2024
@ltfschoen
Copy link
Author

ltfschoen commented Apr 18, 2024

I'm closing the PR due to staleness and the cargo-contract master having moved on significantly since the PR was created. Feel free to reopen based on the latest master in case you're interested in continuing work on it!

Yes, i'd be happy to try and update it, but are there enough resources to review and merge after i finish the changes? last time it took a couple months and by that stage master had changed too much

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 this pull request may close these issues.

3 participants