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
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 1 addition & 11 deletions Cargo.lock

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

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
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 be a hex-encoded Ed25519 secret key.
Fraser999 marked this conversation as resolved.
Show resolved Hide resolved
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
27 changes: 15 additions & 12 deletions crates/astria-composer/src/executor/builder.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
use std::time::Duration;
use std::{
fs,
time::Duration,
};

use astria_core::{
primitive::v1::Address,
Expand All @@ -12,11 +15,6 @@ use astria_eyre::{
},
};
use ed25519_consensus::SigningKey;
use secrecy::{
ExposeSecret,
SecretString,
Zeroize,
};
use tokio::sync::watch;
use tokio_util::sync::CancellationToken;

Expand All @@ -28,7 +26,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 +38,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 +47,17 @@ 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")?
let private_key_hex = fs::read_to_string(&private_key_file)
.wrap_err_with(|| format!("failed to read private key at `{private_key_file}`"))?;
let private_key_bytes: [u8; 32] = hex::decode(private_key_hex.trim())
.wrap_err_with(|| {
format!("failed to hex-decode private key bytes in `{private_key_file}`")
})?
.try_into()
.map_err(|_| eyre!("invalid private key length; must be 32 bytes"))?;
.map_err(|_| {
eyre!("invalid private key length in `{private_key_file}`; must be 32 bytes")
})?;
let sequencer_key = SigningKey::from(private_key_bytes);
private_key_bytes.zeroize();
Fraser999 marked this conversation as resolved.
Show resolved Hide resolved

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

Expand Down
13 changes: 0 additions & 13 deletions crates/astria-composer/src/executor/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ use futures::{
};
use pin_project_lite::pin_project;
use prost::Message as _;
use secrecy::Zeroize as _;
use sequencer_client::{
tendermint_rpc::endpoint::broadcast::tx_sync,
Address,
Expand Down Expand Up @@ -146,12 +145,6 @@ impl Handle {
}
}

impl Drop for Executor {
fn drop(&mut self) {
self.sequencer_key.zeroize();
}
}

#[derive(Debug)]
pub(super) struct Status {
is_connected: bool,
Expand Down Expand Up @@ -537,12 +530,6 @@ pin_project! {
state: SubmitState,
bundle: SizedBundle,
}

impl PinnedDrop for SubmitFut {
fn drop(this: Pin<&mut Self>) {
this.project().signing_key.zeroize();
}
}
}

pin_project! {
Expand Down
Loading
Loading