SIP-21: Encrypt keypairs with aes-128 before storing them on disk. #21
Conversation
| 1. for keystore files, we should add a step before we save base64 content to files, | ||
| FileBasedKeystore | ||
| //add aes-128-cbc default encryption | ||
| let encode_data = default_des_128_encode(store.as_bytes()); |
There was a problem hiding this comment.
where does the user set the aes 128 password?
| } | ||
|
|
||
|
|
||
| } |
There was a problem hiding this comment.
we likely won't change the validator config with this change in the sui codebase. this has cascading effect over many of the CI pipelines and adds overhead to validator setup.
instead, you can propose this change as an optional feature.
| but key-pairs content is less used in normal developer, but cli-key tool is widly use for | ||
| dapps developers in their server for deploy dapps. | ||
|
|
||
| ## Specification |
There was a problem hiding this comment.
this section should describe what the AES encoding for keystore do, the code should go to the reference implementation section, or simply a link to your pr).
i.e. describe what's the prefix used for encoding, how is the password set, how does AES encryption work (which standard does it follow with link). example of what a key looks like before and after encryption.
| } | ||
|
|
||
|
|
||
| ## Rationale |
There was a problem hiding this comment.
this section should provide the rationale behind the change such as 1) why encryption 2) why AES 3) why the prefix
|
|
||
|
|
||
|
|
||
| ## Test Cases |
There was a problem hiding this comment.
this should enumerate a few test cases that what the keystore file look like before and after the encryption.
in the PR, please also include test cases for keytool_tests.rs
|
|
||
|
|
||
|
|
||
| ## Security Considerations |
There was a problem hiding this comment.
please include the consideration such as 1) what happens if the password is leaked or lost 2) why this is more secure than plain keystore etc.
|
|
||
| 1. rosetta test : read_prefunded_account | ||
|
|
||
| ## Reference Implementation |
There was a problem hiding this comment.
include your code or just a link to PR here.
|
|
||
| ## Backwards Compatibility | ||
|
|
||
| There are no issues with backwards compatability. |
There was a problem hiding this comment.
if you change the format of a config file, there are compatibility issue that some older sui binary may expect plain base64 but AES encrypted ones are included.
please discuss whether this feature is optional and what error or handling should the user see if there is incompatibility.
|
@oday0311 this looks promising as an option for storing keys differently--is it something you want to push forward with a PR + adjustments to the SIP? |
|
before we can proceed with code review, can you address my comments to the sip? once we resolve these comments and merge the sip we would love to look more into your code. |
|
Thanks for your SIP proposal. In order to process the SIP, the SIP Editor requires write access to the forked repository in order to change the status and add additional information. Could you please grant write access on the forked repository to the following GitHub account: Once this has been completed, we can move the proposal to draft. |
wriches
added
the
new
|
@Eis-D-Z invite you , please check |
Eis-D-Z
changed the title
Eis-D-Z
added
active
Eis-D-Z
changed the title
No description provided.