github-actions[bot]
commented
3 months ago PR Description updated to latest commit (https://github.com/subspace/infra/commit/600cf1e667431f818d93d1e46e15e1806e328647)
Closed DaMandal0rian closed 2 months ago
github-actions[bot]
commented
3 months ago PR Description updated to latest commit (https://github.com/subspace/infra/commit/600cf1e667431f818d93d1e46e15e1806e328647)
github-actions[bot]
commented
3 months ago | ⏱️ Estimated effort to review [1-5] | 3, because the PR introduces new scripts with several functions for generating and storing node keys, which involves external dependencies like `subkey` and `jq`, and interacts with Hashicorp Vault. The logic within the scripts is straightforward, but careful attention is needed to ensure security practices are followed, especially in handling sensitive data like keys and tokens. |
| 🧪 Relevant tests | No |
| 🔍 Possible issues | Possible Security Concern: The script uses `base64 -d <<< ${VAULT_TOKEN}` to decode the Vault token. If the environment variable is not properly secured, this could expose sensitive information. |
| Error Handling: There is minimal error handling for external commands (e.g., `curl`, `subkey generate-node-key`). Failures in these commands may not be caught, leading to incomplete or incorrect execution without clear indication to the user. | |
| 🔒 Security concerns | Sensitive Information Exposure: The scripts handle sensitive information, such as node keys and Vault tokens. It's crucial to ensure that these are securely managed throughout the script's execution, especially when making API calls or storing keys in files. |
| relevant file | scripts/generate_node_keys.sh |
| suggestion | Consider adding error handling for the `curl` command used to install `subkey` and the `subkey generate-node-key` command. This ensures the script halts or takes corrective action if these critical steps fail. [important] |
| relevant line | curl https://getsubstrate.io -sSf | bash -s -- --fast |
| relevant file | scripts/generate_node_keys.sh |
| suggestion | It's recommended to securely handle the output of `base64 -d <<< ${VAULT_TOKEN}` to prevent potential leakage of sensitive information. Consider storing the decoded token in a variable and ensure it's not inadvertently logged or exposed. [important] |
| relevant line | local vault_token=$(base64 -d <<< ${VAULT_TOKEN}) |
| relevant file | scripts/restore_node_keys.sh |
| suggestion | To enhance security, validate the format of the `key_value` retrieved from Hashicorp Vault before proceeding to use it. This can prevent potential command injection or other security issues if the value format is not as expected. [important] |
| relevant line | local key_value=$(echo "${response}" | jq -r '.data.data.key') |
| relevant file | scripts/restore_node_keys.sh |
| suggestion | Add error handling for the `jq` installation command to ensure the script does not proceed if `jq` cannot be installed successfully. This is critical for the correct execution of the script. [medium] |
| relevant line | sudo apt-get install -y jq |
github-actions[bot]
commented
3 months ago | Category | Suggestions |
| Best practice |
Quote variables to prevent globbing and word splitting.___ **It's recommended to quote variables to prevent globbing and word splitting. This isparticularly important in the curl command where ${vault_token} is used. To improve the script's robustness, consider quoting ${vault_token}.**
[scripts/generate_node_keys.sh [37]](https://github.com/subspace/infra/pull/299/files#diff-c93f6283d6b699f02daba82e8259ddbbe11e2356b85438284b9bb5149dee7d56R37-R37)
```diff
-curl -X POST -H "X-Vault-Token: ${vault_token}" -d "{\"data\": {\"key\": \"${key_id}=${key_value}\"}}" "${vault_endpoint}"
+curl -X POST -H "X-Vault-Token: \"${vault_token}\"" -d "{\"data\": {\"key\": \"${key_id}=${key_value}\"}}" "${vault_endpoint}"
```
|
Check if the .env file exists to prevent unintentional overwrites.___ **To ensure that the.env file does not unintentionally expose sensitive information or get overwritten with each execution, it's advisable to check if the file exists and prompt the user before appending to it.** [scripts/restore_node_keys.sh [34-35]](https://github.com/subspace/infra/pull/299/files#diff-4cd0e4d54eda3f354b91149b7f36a906c7c196b2c0e37d11c13059121d899e93R34-R35) ```diff +if [ -f ".env" ]; then + echo ".env file exists. Appending..." +else + echo "Creating .env file..." +fi echo "${node_name}=${key_value}" >> ".env" echo "peer_id=${key_id}" >> ".env" ``` | |
Add error handling for curl command and data retrieval.___ **To improve error handling, consider adding checks for the success of thecurl command and the presence of the expected key_value in the response from Hashicorp Vault. This can prevent the script from proceeding with invalid or empty data.** [scripts/restore_node_keys.sh [21-22]](https://github.com/subspace/infra/pull/299/files#diff-4cd0e4d54eda3f354b91149b7f36a906c7c196b2c0e37d11c13059121d899e93R21-R22) ```diff local response=$(curl -s -H "X-Vault-Token: ${vault_token}" "${vault_endpoint}") +if [ $? -ne 0 ]; then + echo "Failed to retrieve data from Vault." + exit 1 +fi local key_value=$(echo "${response}" | jq -r '.data.data.key') +if [ -z "${key_value}" ]; then + echo "No key_value found in response." + exit 1 +fi ``` | |
| Enhancement |
Improve portability by handling differences in the base64 command.___ **The script usesbase64 -d to decode the VAULT_TOKEN, which might not work on macOS due to differences in the base64 command options. To make the script more portable, consider using an if-statement to detect the operating system and adjust the command accordingly.** [scripts/generate_node_keys.sh [35]](https://github.com/subspace/infra/pull/299/files#diff-c93f6283d6b699f02daba82e8259ddbbe11e2356b85438284b9bb5149dee7d56R35-R35) ```diff -local vault_token=$(base64 -d <<< ${VAULT_TOKEN}) +if [[ "$OSTYPE" == "darwin"* ]]; then + local vault_token=$(base64 -D <<< ${VAULT_TOKEN}) +else + local vault_token=$(base64 -d <<< ${VAULT_TOKEN}) +fi ``` |
| Security |
Enhance security by inspecting scripts before execution.___ **Instead of usingcurl to pipe a script directly into bash for installing subkey, consider downloading the script first, inspecting it, and then executing it. This approach enhances security by allowing review before execution.** [scripts/generate_node_keys.sh [7]](https://github.com/subspace/infra/pull/299/files#diff-c93f6283d6b699f02daba82e8259ddbbe11e2356b85438284b9bb5149dee7d56R7-R7) ```diff -curl https://getsubstrate.io -sSf | bash -s -- --fast +curl https://getsubstrate.io -sSf -o get_substrate.sh +# Inspect the script or perform any necessary actions +bash get_substrate.sh --fast ``` |
User description
We currently generate node keys manually, and store in flat files which is cumbersome especially as the network expands.
These scripts do the following:
Type
enhancement
Description
generate_node_keys.shfor generating and storing node keys in Hashicorp Vault.restore_node_keys.shfor retrieving node keys from Hashicorp Vault and storing them in a.envfile.Changes walkthrough
generate_node_keys.sh
Automate Node Key Generation and Storage in Hashicorp Vaultscripts/generate_node_keys.sh
subkeyand store them in Hashicorp Vault.
subkeyinstallation, generate nodekeys, and add them to Hashicorp Vault.
domain, farmer, nova-bootstrap).
restore_node_keys.sh
Automate Restoration of Node Keys from Hashicorp Vaultscripts/restore_node_keys.sh
them in a
.envfile.jqinstallation, retrieve key-valuepairs from Hashicorp Vault, and store them in
.envfile.