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

Support AES encryption of the seda_keys.json file #468

Merged
merged 5 commits into from
Jan 23, 2025

Conversation

Thomasvdam
Copy link
Member

@Thomasvdam Thomasvdam commented Jan 17, 2025

Motivation

As our current implementation requires the keys to be present on the application layer for validator nodes we want to at least enable validator operators to encrypt the seda_keys.json file using AES.

Explanation of Changes

I tried to give the operator various options when generating the SEDA keys file:

  • By default the CLI will generate a random AES key, display it to the user and wait for confirmation, use that key to encrypt the newly generated SEDA keys file.
  • There is a flag --key-file-custom-encryption-key that instead of generating a random key allows the operator to paste/type a custom key through a password prompt. It asks it twice and compares the 2 entries to make errors slightly less likely.
  • When both the SEDA_KEYS_ENCRYPTION_KEY environment variable and the --key-file-custom-encryption-key flag are set the CLI errors.

The node now checks whether the SEDA_KEYS_ENCRYPTION_KEY environment variable is set when starting up and if it's missing the node panics. This behaviour can be overridden by passing the --allow-unencrypted-seda-keys flag to the $BIN start command.

I also updated the CLI commands to allow overwriting the SEDA keys file with a flag, and if the flag is not set it will prompt the user for confirmation if it's ok to overwrite instead of errroring directly.

Came across $BIN query wait-tx while researching the flags and figured it would be useful for our contract deployment script.

Testing

Added unit tests for the key file generation and loading.

Changed pubkey CLI add-seda-keys tx unit test to always use --key-file-no-encryption as I wasn't sure how to test commands that require user input.

Updated scripts to use --key-file-no-encryption in the gentx and create-validator transactions.

Manually tested the tx pubkey add-seda-keys command with a custom encryption key, a manually generated encryption key, and an encryption key from the environment variables.

Tested the contract deployment script by running it manually on a local chain.

Related PRs and Issues

Closes: #368

@gluax
Copy link
Contributor

gluax commented Jan 17, 2025

Not leaving direct code comments yet since it's still a draft, but chatting about the ideas:

We should log a warning if the key is not set, probably at an initial check for it. So at least this way there's some visible warning. It should say hey this warning is okay to ignore if this is not a prod environment. Or at least something along those lines is standard.

Is FlagEncryptionKey for passing in the encryption key from the CLI? or for setting the env variable it should use? The wording on the doc comment makes me think you can change the Env var for it.
And if it is the former we should either:

  • Disallow this. Passing from plain-text is a security issue. Since this would now be stored as plain text in a file, which is what the env var hopes to avoid. Alternatively, I suppose we could warn hey don't use this CLI option in production mode.
  • Or ensure it's from a password prompt, here's a link on why and how this should be done.

Lastly, I think we are fine as far as I could tell looking at the code. But, since this PR's goal is to add in security, we should make sure the env var contents/private keys themselves are never in memory for too long. Or preferably if we have them in memory at all, i.e. more than just as an instant use(although maybe even then), we should use them as byte slices rather than a readable format. It's hard to pick out some bytes among many :). If we do miss it somewhere I imagine this is something a security review would hopefully pick up.

@Thomasvdam Thomasvdam force-pushed the feat/aes-encrypt-seda-keys branch 2 times, most recently from fed7886 to ab4da1b Compare January 20, 2025 19:53
@Thomasvdam Thomasvdam requested a review from a team January 20, 2025 20:05
@Thomasvdam Thomasvdam marked this pull request as ready for review January 20, 2025 20:06
@Thomasvdam Thomasvdam force-pushed the feat/aes-encrypt-seda-keys branch 2 times, most recently from c8c010f to c18c0ea Compare January 22, 2025 16:28
app/utils/seda_signer.go Outdated Show resolved Hide resolved
scripts/deploy_contracts.sh Outdated Show resolved Hide resolved
@Thomasvdam Thomasvdam force-pushed the feat/aes-encrypt-seda-keys branch from c18c0ea to 89117a2 Compare January 22, 2025 19:20
The original implementation was generating old MsgCreateValidator, which
our staking module no longer supports. We're now overriding the method
that returns all the possible operations for the staking module and
simply replace the MsgCreateValidator with our MsgCreateSEDAValidator.
@Thomasvdam Thomasvdam force-pushed the feat/aes-encrypt-seda-keys branch from 4aee165 to 56ecb8e Compare January 23, 2025 12:22
@Thomasvdam Thomasvdam merged commit 56ecb8e into main Jan 23, 2025
17 checks passed
@Thomasvdam Thomasvdam deleted the feat/aes-encrypt-seda-keys branch January 23, 2025 13:02
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.

🔧 Password-based encryption of SEDA key file
4 participants