feat(security): use files rather than pass by arg#1818
Conversation
Signed-off-by: Giles Cope <gilescope@gmail.com>
| [..]Executing generate-intent[..] | ||
| [..]Executing maintain command | ||
| [..]Executing ../toolkit-js/dist/bin.js with arguments: ["maintain", "contract", "-c", "[CWD]/../toolkit-js/test/contract/contract.config.ts", "--network", "undeployed", "--coin-public", "aa0d72bb77ea46f986a800c66d75c4e428a95bd7e1244f1ed059374e6266eb98", "--input", "[CWD]/test-data/contract/counter/contract_state.mn", "--output", "[CWD]/out/intent.bin", "--signing", "0000000000000000000000000000000000000000000000000000000000000001", "3102ba67572345ef8bc5cd238bff10427b4533e376b4aaed524c2f1ef5eca806", "0000000000000000000000000000000000000000000000000000000000000002"]... | ||
| [..]Executing ../toolkit-js/dist/bin.js with arguments: ["maintain", "contract", "-c", "[CWD]/../toolkit-js/test/contract/contract.config.ts", "--network", "undeployed", "--coin-public", "aa0d72bb77ea46f986a800c66d75c4e428a95bd7e1244f1ed059374e6266eb98", "--input", "[CWD]/test-data/contract/counter/contract_state.mn", "--output", "[CWD]/out/intent.bin", "--signing-file", "[..]", "3102ba67572345ef8bc5cd238bff10427b4533e376b4aaed524c2f1ef5eca806", "--new-authority-file", "[..]"]... |
There was a problem hiding this comment.
Not printing file path in logs is security via obscurity. If adversary has access to the machine with seeds files, then he has access to the files regardless if these files path is printed here or not.
| "error when reading AURA seed file at {seed_file}. Error: {e}" | ||
| )) | ||
| })?; | ||
| let seed = read_seed_file(seed_file, "AURA")?; |
There was a problem hiding this comment.
Talking about security: we depend on polkadot-sdk and keystore files. Is this code really indispensable? Supporting reading from two sources will always be at most as secure as the less secure of two ways.
LGLO
left a comment
LGLO
left a comment
There was a problem hiding this comment.
There are few bits that are smells, but in essence problems existed before.
| fields are also `Debug`-redacted (`SecretString`) so `--dry-run` info logs | ||
| can't leak them. | ||
|
|
||
| PR: |
| `midnight-node-ledger-helpers` so the toolkit can reuse it for its new | ||
| `--*-file` secret arguments; the node re-exports it at the old path. | ||
|
|
||
| PR: |
| } | ||
| } | ||
|
|
||
| /// Hand a secret to toolkit-js via an owner-only temp file instead of argv |
There was a problem hiding this comment.
owner-only is better, but still not great -
Perhaps a better solution is piping via stdin - only the parent and child can see the data that way - but this is a bigger change that can be left to a separate PR
3 participants
Overview
Passing secrets by arg isn't the safest way to do things. Midnight toolkit is a testing toolkit but still it's good to provide people with the safest way to do things.
🗹 TODO before merging
📌 Submission Checklist
git commit -s) for the DCO🧪 Testing Evidence
Please describe any additional testing aside from CI:
🔱 Fork Strategy
Links