Skip to content

Commit

Permalink
feat(composer)! avoid holding private key in env var (#1074)
Browse files Browse the repository at this point in the history
## 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.
  • Loading branch information
Fraser999 authored May 17, 2024
1 parent 8953669 commit 8f261a4
Show file tree
Hide file tree
Showing 30 changed files with 216 additions and 192 deletions.
16 changes: 2 additions & 14 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,9 @@ bytes = "1"
celestia-tendermint = "0.32.1"
celestia-types = "0.1.1"
clap = "4.5.4"
ed25519-consensus = "2.1.0"
ed25519-consensus = { version = "2.1.0", default-features = false, features = [
"std",
] }
ethers = "2.0.11"
futures = "0.3"
hex = "0.4"
Expand Down
2 changes: 1 addition & 1 deletion charts/evm-rollup/Chart.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ type: application
# This is the chart version. This version number should be incremented each time you make changes
# to the chart and its templates, including the app version.
# Versions are expected to follow Semantic Versioning (https://semver.org/)
version: 0.16.1
version: 0.16.2

# This is the version number of the application being deployed. This version number should be
# incremented each time you make changes to the application. Versions are not expected to
Expand Down
17 changes: 15 additions & 2 deletions charts/evm-rollup/templates/configmap.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,12 @@ data:
OTEL_EXPORTER_OTLP_HEADERS: "{{ .Values.config.rollup.otel.otlpHeaders }}"
OTEL_EXPORTER_OTLP_TRACE_HEADERS: "{{ .Values.config.rollup.otel.traceHeaders }}"
OTEL_SERVICE_NAME: "{{ tpl .Values.config.rollup.otel.serviceNamePrefix . }}-composer"
{{- if not .Values.global.dev }}
{{- if not .Values.secretProvider.enabled }}
ASTRIA_COMPOSER_PRIVATE_KEY: "{{ .Values.config.sequencer.privateKey }}"
ASTRIA_COMPOSER_PRIVATE_KEY: "{{ .Values.config.sequencer.privateKey.devContent }}"
{{- end }}
{{- if not .Values.global.dev }}
{{- else }}
ASTRIA_COMPOSER_PRIVATE_KEY_FILE: "/var/secrets/{{ .Values.config.sequencer.privateKey.secret.filename }}"
{{- end }}
---
apiVersion: v1
Expand Down Expand Up @@ -259,3 +260,15 @@ metadata:
data:
VISUALIZER__SERVER__GRPC__ENABLED: "false"
VISUALIZER__SERVER__HTTP__ADDR: "0.0.0.0:8151"
---
{{- if not .Values.secretProvider.enabled }}
apiVersion: v1
kind: ConfigMap
metadata:
namespace: {{ include "rollup.namespace" . }}
name: sequencer-private-key
data:
{{ .Values.config.sequencer.privateKey.secret.filename }}: |
{{ .Values.config.sequencer.privateKey.devContent }}
---
{{- end }}
29 changes: 11 additions & 18 deletions charts/evm-rollup/templates/statefulsets.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,8 @@ spec:
{{- else }}
- --state.scheme=path
{{- end }}
{{ if .Values.config.rollup.metrics.enabled }}
- --metrics
{{ if .Values.config.rollup.metrics.enabled }}
- --metrics
- --metrics.addr=0.0.0.0
- --metrics.port={{ .Values.ports.metrics }}
{{- end }}
Expand All @@ -79,12 +79,12 @@ spec:
name: ws-rpc
- containerPort: {{ .Values.ports.executionGRPC }}
name: execution-grpc
{{- if .Values.config.rollup.metrics.enabled }}
{{- if .Values.config.rollup.metrics.enabled }}
- containerPort: {{ .Values.ports.metrics }}
name: geth-metr
{{- end }}
resources:
{{- toYaml .Values.resources.geth | trim | nindent 12 }}
{{- toYaml .Values.resources.geth | trim | nindent 12 }}
- name: composer
image: {{ include "composer.image" . }}
command: [ "/usr/local/bin/astria-composer" ]
Expand All @@ -93,20 +93,10 @@ spec:
envFrom:
- configMapRef:
name: {{ .Values.config.rollup.name }}-composer-env
{{- if .Values.secretProvider.enabled }}
env:
- name: ASTRIA_COMPOSER_PRIVATE_KEY
valueFrom:
secretKeyRef:
name: sequencer-private-key
key: {{ .Values.secretProvider.secrets.sequencerPrivateKey.key }}
{{- end }}
volumeMounts:
{{- if .Values.secretProvider.enabled }}
- mountPath: "/var/secrets"
name: sequencer-private-key
{{- end }}
{{- if .Values.config.rollup.metrics.enabled }}
{{- if .Values.config.rollup.metrics.enabled }}
ports:
- containerPort: {{ .Values.ports.composerMetrics }}
name: composer-metr
Expand All @@ -123,7 +113,7 @@ spec:
name: {{ .Values.config.rollup.name }}-conductor-env
resources:
{{- toYaml .Values.resources.conductor | trim | nindent 12 }}
{{- if .Values.config.rollup.metrics.enabled }}
{{- if .Values.config.rollup.metrics.enabled }}
ports:
- containerPort: {{ .Values.ports.conductorMetrics }}
name: conductor-metr
Expand All @@ -140,14 +130,17 @@ spec:
{{- else }}
emptyDir: {}
{{- end }}
{{- if .Values.secretProvider.enabled }}
- name: sequencer-private-key
{{- if .Values.secretProvider.enabled }}
csi:
driver: secrets-store.csi.k8s.io
readOnly: true
volumeAttributes:
secretProviderClass: sequencer-private-key
{{- end }}
{{- else }}
configMap:
name: sequencer-private-key
{{- end }}
---
{{- if .Values.config.blockscout.enabled }}
apiVersion: apps/v1
Expand Down
6 changes: 5 additions & 1 deletion charts/evm-rollup/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,11 @@ config:
# Private key which is used for wrapping txs for sequencer submission
# Note: When secretProvider.enabled is true the secret provided by
# `sequencerPrivateKey` is used instead of this value.
privateKey: "2bd806c97f0e00af1a1fc3328fa763a9269723c8db8fac4f93af71db186d6e90"
privateKey:
devContent: "2bd806c97f0e00af1a1fc3328fa763a9269723c8db8fac4f93af71db186d6e90"
secret:
filename: "key.hex"
resourceName: "projects/$PROJECT_ID/secrets/sequencerPrivateKey/versions/latest"

celestia:
# if config.rollup.executionLevel is NOT 'SoftOnly' AND celestia-node is not enabled
Expand Down
6 changes: 2 additions & 4 deletions crates/astria-cli/src/commands/sequencer.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use astria_core::{
crypto::SigningKey,
primitive::v1::asset,
protocol::transaction::v1alpha1::{
action::{
Expand Down Expand Up @@ -31,10 +32,7 @@ use color_eyre::{
Context,
},
};
use ed25519_consensus::{
SigningKey,
VerificationKeyBytes,
};
use ed25519_consensus::VerificationKeyBytes;
use rand::rngs::OsRng;

use crate::cli::sequencer::{
Expand Down
2 changes: 1 addition & 1 deletion crates/astria-composer/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ telemetry = { package = "astria-telemetry", path = "../astria-telemetry", featur
] }

pin-project-lite = "0.2.13"
secrecy = { version = "0.8", features = ["serde"] }
tonic-health = "0.10.2"

async-trait = { workspace = true }
Expand Down Expand Up @@ -70,6 +69,7 @@ test_utils = { package = "astria-test-utils", path = "../astria-test-utils", fea
"geth",
] }
insta = { workspace = true, features = ["json"] }
tempfile = { workspace = true }
tokio-test = { workspace = true }
astria-core = { path = "../astria-core", features = ["client"] }
tendermint-rpc = { workspace = true }
Expand Down
6 changes: 3 additions & 3 deletions crates/astria-composer/local.env.example
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,9 @@ ASTRIA_COMPOSER_SEQUENCER_CHAIN_ID="astria-dev-1"
# names are sha256 hashed and used as the `rollup_id` in `SequenceAction`s
ASTRIA_COMPOSER_ROLLUPS="astriachain::ws://127.0.0.1:8545"

# Private key for the sequencer account used for signing transactions
# Must be a hex-encoded 32-byte array (64-character hex string)
ASTRIA_COMPOSER_PRIVATE_KEY="2bd806c97f0e00af1a1fc3328fa763a9269723c8db8fac4f93af71db186d6e90"
# The path to the file storing the private key for the sequencer account used for signing
# transactions. The file should contain a hex-encoded Ed25519 secret key.
ASTRIA_COMPOSER_PRIVATE_KEY_FILE=/path/to/priv_sequencer_key.json

# Block time in milliseconds, used to force submitting of finished bundles.
# Should match the sequencer node configuration for 'timeout_commit', as
Expand Down
2 changes: 1 addition & 1 deletion crates/astria-composer/src/composer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ impl Composer {
let (executor, executor_handle) = executor::Builder {
sequencer_url: cfg.sequencer_url.clone(),
sequencer_chain_id: cfg.sequencer_chain_id.clone(),
private_key: cfg.private_key.clone(),
private_key_file: cfg.private_key_file.clone(),
block_time_ms: cfg.block_time_ms,
max_bytes_per_bundle: cfg.max_bytes_per_bundle,
bundle_queue_capacity: cfg.bundle_queue_capacity,
Expand Down
27 changes: 2 additions & 25 deletions crates/astria-composer/src/config.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,8 @@
use std::net::SocketAddr;

use secrecy::{
zeroize::ZeroizeOnDrop,
ExposeSecret as _,
SecretString,
};
use serde::{
Deserialize,
Serialize,
Serializer,
};

// this is a config, may have many boolean values
Expand All @@ -31,9 +25,8 @@ pub struct Config {
/// A list of <rollup_name>::<url> pairs
pub rollups: String,

/// Private key for the sequencer account used for signing transactions
#[serde(serialize_with = "serialize_private_key")]
pub private_key: SecretString,
/// Path to private key for the sequencer account used for signing transactions
pub private_key_file: String,

/// Sequencer block time in milliseconds
#[serde(alias = "max_submit_interval_ms")]
Expand Down Expand Up @@ -69,22 +62,6 @@ impl config::Config for Config {
const PREFIX: &'static str = "ASTRIA_COMPOSER_";
}

impl ZeroizeOnDrop for Config {}

fn serialize_private_key<S>(key: &SecretString, s: S) -> Result<S::Ok, S::Error>
where
S: Serializer,
{
use serde::ser::Error as _;
let mut raw_key = key.expose_secret().clone().into_bytes();
if let Some(sub_slice) = raw_key.get_mut(4..) {
sub_slice.fill(b'#');
}
let sanitized_key = std::str::from_utf8(&raw_key)
.map_err(|_| S::Error::custom("private key hex contained non-ascii characters"))?;
s.serialize_str(sanitized_key)
}

#[cfg(test)]
mod tests {
use super::Config;
Expand Down
43 changes: 23 additions & 20 deletions crates/astria-composer/src/executor/builder.rs
Original file line number Diff line number Diff line change
@@ -1,21 +1,18 @@
use std::time::Duration;
use std::{
fs,
path::Path,
time::Duration,
};

use astria_core::{
crypto::SigningKey,
primitive::v1::Address,
protocol::transaction::v1alpha1::action::SequenceAction,
};
use astria_eyre::{
use astria_eyre::eyre::{
self,
eyre,
eyre::{
eyre,
WrapErr as _,
},
};
use ed25519_consensus::SigningKey;
use secrecy::{
ExposeSecret,
SecretString,
Zeroize,
WrapErr as _,
};
use tokio::sync::watch;
use tokio_util::sync::CancellationToken;
Expand All @@ -28,7 +25,7 @@ use crate::{
pub(crate) struct Builder {
pub(crate) sequencer_url: String,
pub(crate) sequencer_chain_id: String,
pub(crate) private_key: SecretString,
pub(crate) private_key_file: String,
pub(crate) block_time_ms: u64,
pub(crate) max_bytes_per_bundle: usize,
pub(crate) bundle_queue_capacity: usize,
Expand All @@ -40,7 +37,7 @@ impl Builder {
let Self {
sequencer_url,
sequencer_chain_id,
private_key,
private_key_file,
block_time_ms,
max_bytes_per_bundle,
bundle_queue_capacity,
Expand All @@ -49,12 +46,10 @@ impl Builder {
let sequencer_client = sequencer_client::HttpClient::new(sequencer_url.as_str())
.wrap_err("failed constructing sequencer client")?;
let (status, _) = watch::channel(Status::new());
let mut private_key_bytes: [u8; 32] = hex::decode(private_key.expose_secret())
.wrap_err("failed to decode private key bytes from hex string")?
.try_into()
.map_err(|_| eyre!("invalid private key length; must be 32 bytes"))?;
let sequencer_key = SigningKey::from(private_key_bytes);
private_key_bytes.zeroize();

let sequencer_key = read_signing_key_from_file(&private_key_file).wrap_err_with(|| {
format!("failed reading signing key from file at path `{private_key_file}`")
})?;

let sequencer_address = Address::from_verification_key(sequencer_key.verification_key());

Expand All @@ -78,3 +73,11 @@ impl Builder {
))
}
}

fn read_signing_key_from_file<P: AsRef<Path>>(path: P) -> eyre::Result<SigningKey> {
let private_key_hex = fs::read_to_string(path)?;
let private_key_bytes: [u8; 32] = hex::decode(private_key_hex.trim())?
.try_into()
.map_err(|_| eyre!("invalid private key length; must be 32 bytes"))?;
Ok(SigningKey::from(private_key_bytes))
}
Loading

0 comments on commit 8f261a4

Please sign in to comment.