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

feat(composer)! avoid holding private key in env var #1074

Merged
merged 6 commits into from
May 17, 2024

Conversation

Fraser999
Copy link
Contributor

Summary

This PR changes the configuration env vars for composer to not hold the contents of the private key for signing sequencer transactions.

Background

It's undesirable to have the contents of a private key in an env var.

Changes

Changed the existing env var to hold a path to a hex-encoded private key file.

Testing

Modified existing tests, and ran smoke test using local build of composer.

Breaking Changelist

  • Removed ASTRIA_COMPOSER_PRIVATE_KEY
  • Added ASTRIA_COMPOSER_PRIVATE_KEY_FILE

Related Issues

Closes #993.

@Fraser999 Fraser999 requested review from a team as code owners May 15, 2024 10:26
@github-actions github-actions bot added composer pertaining to composer cd labels May 15, 2024
Copy link
Member

@SuperFluffy SuperFluffy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with the changes.

Can we also put the SigningKey behind a https://docs.rs/secrecy/0.8.0/secrecy/struct.Secret.html? So that we don't accidentally leak it if someone decides to debug-print executor or the submitfut? Right now SigningKey implements Debug and happily leaks its contents.

crates/astria-composer/local.env.example Outdated Show resolved Hide resolved
crates/astria-composer/src/executor/builder.rs Outdated Show resolved Hide resolved
@joroshiba joroshiba added the docker-build used to trigger docker builds on PRs label May 15, 2024
@github-actions github-actions bot added conductor pertaining to the astria-conductor crate sequencer pertaining to the astria-sequencer crate sequencer-relayer pertaining to the astria-sequencer-relayer crate labels May 17, 2024
@Fraser999
Copy link
Contributor Author

@SuperFluffy - as agreed, I put the signing key in a newtype in 3515ac0 and replaced all usages of the original type.

crates/astria-core/src/crypto.rs Outdated Show resolved Hide resolved
crates/astria-core/src/crypto.rs Show resolved Hide resolved
crates/astria-core/src/crypto.rs Show resolved Hide resolved
crates/astria-sequencer-relayer/src/validator.rs Outdated Show resolved Hide resolved
crates/astria-sequencer-relayer/src/validator.rs Outdated Show resolved Hide resolved
@Fraser999 Fraser999 enabled auto-merge May 17, 2024 14:50
@Fraser999 Fraser999 added this pull request to the merge queue May 17, 2024
Merged via the queue into main with commit 8f261a4 May 17, 2024
36 checks passed
@Fraser999 Fraser999 deleted the fraser/composer-private-key branch May 17, 2024 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cd composer pertaining to composer conductor pertaining to the astria-conductor crate docker-build used to trigger docker builds on PRs sequencer pertaining to the astria-sequencer crate sequencer-relayer pertaining to the astria-sequencer-relayer crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace ASTRIA_COMPOSER_PRIVATE_KEY env var with ASTRIA_COMPOSER_PRIVATE_KEY_FILE
4 participants