Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
112 changes: 83 additions & 29 deletions crates/config/src/keys.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use sha2::Sha256;
use std::sync::Arc;
use tn_types::{
construct_proof_of_possession_message, Address, BlsKeypair, BlsPublicKey, BlsSignature,
BlsSigner, DefaultHashFunction, NetworkKeypair, NetworkPublicKey, Signer,
BlsSigner, DefaultHashFunction, NetworkKeypair, NetworkPublicKey, Signer, WorkerId,
};
use zeroize::Zeroizing;

Expand Down Expand Up @@ -208,15 +208,27 @@ fn warn_if_key_permissions_are_loose(
) {
}

#[derive(Debug)]
/// Private key material and derivation inputs shared by a key manager.
struct KeyConfigInner {
// DO NOT expose the private key to other code. Tests that need this will provide a primary
// key. Use the BlsSigner trait for signing for the primary.
/// DO NOT expose the private key to other code. Tests provide their own primary key.
/// Use the BlsSigner trait for signing for the primary.
primary_keypair: BlsKeypair,
// Derived from the primary_keypair.
/// Derived from the primary keypair.
primary_network_keypair: NetworkKeypair,
// Derived from the primary_keypair.
worker_network_keypair: NetworkKeypair,
/// Seed string for worker network keypairs. Per-worker keypairs are derived on demand from
/// the primary keypair and this seed; see [`KeyConfig::worker_network_keypair`].
worker_network_seed: String,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The derived Debug on KeyConfigInner renders the worker network seed, and this PR dropped the positive assertion that kept the worker side of the leak test honest.

KeyConfigInner is #[derive(Debug)] (keys.rs:211) and now holds worker_network_seed: String on this line. KeyConfig is #[derive(Debug, Clone)] (:234) over Arc<KeyConfigInner>, and Arc's Debug forwards to the inner value — so format!("{:?}", key_config) does render the seed.

This is informational rather than urgent, for two reasons. The seed is a domain-separation label, not key material: it's operator-configurable, read from node_keys_path().join(WORKER_NETWORK_SEED_FILE) at keys.rs:356-358. And no production log path debug-prints a KeyConfig — I grepped every tracing macro plus format!, dbg! and println! and found zero sites.

Still worth closing, since the type is one ?key_config away from leaking, and the swap to a stored seed removed the only thing that would have caught a regression. A manual Debug for KeyConfigInner rendering the seed as "<redacted>" handles the first part.

For the second: the diff replaces the worker positive anchor in debug_does_not_leak_key_material with a comment explaining that the worker side has no anchor now. That's true as written, but it leaves the worker half of the test with no assertion at all. A negative assertion would restore coverage and would fail if the seed ever starts rendering:

assert!(
    !rendered.contains("worker network keypair"),
    "worker network seed should not be rendered: {rendered}"
);

That pins the redaction directly, rather than relying on the absence of a field that could come back.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added the manual Debug implementation in 1748aba, rendering the stored seed as [REDACTED]. The leak test now explicitly rejects the seed and checks positive anchors for redaction and public fields.

}

impl std::fmt::Debug for KeyConfigInner {
fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
formatter
.debug_struct("KeyConfigInner")
.field("primary_keypair", &self.primary_keypair)
.field("primary_network_keypair", &self.primary_network_keypair)
.field("worker_network_seed", &"[REDACTED]")
.finish()
}
}

/// Basic implementation of a key manager. This version will read a BLS key
Expand All @@ -225,7 +237,7 @@ struct KeyConfigInner {
/// It should NOT expose the BLS private key, even though it is currently read
/// from a file this will not always be the case and all code needing signatures
/// MUST go through KeyConfig.
/// NOTE: The two network keys (primary and worker) are derived from the BLS key
/// NOTE: The network keys (primary and per-worker) are derived from the BLS key
/// and are exposed to other code. This is required to work with libp2p which
/// wants the actual private key. This method of deriving the key is an attempt
/// to provide some protection to the key- even though it will exist in memory it
Expand Down Expand Up @@ -363,12 +375,11 @@ impl KeyConfig {
};
let primary_network_keypair =
Self::generate_network_keypair(&primary_keypair, &primary_seed);
let worker_network_keypair = Self::generate_network_keypair(&primary_keypair, &worker_seed);
Ok(Self {
inner: Arc::new(KeyConfigInner {
primary_keypair,
primary_network_keypair,
worker_network_keypair,
worker_network_seed: worker_seed,
}),
})
}
Expand Down Expand Up @@ -429,7 +440,6 @@ impl KeyConfig {
let worker_seed = "worker network keypair";
let primary_network_keypair =
Self::generate_network_keypair(&primary_keypair, primary_seed);
let worker_network_keypair = Self::generate_network_keypair(&primary_keypair, worker_seed);
// Make sure we have the validator dir, owner-only.
// Don't error out if path exists.
create_keys_dir(&tn_datadir.node_keys_path())?;
Expand Down Expand Up @@ -458,7 +468,7 @@ impl KeyConfig {
inner: Arc::new(KeyConfigInner {
primary_keypair,
primary_network_keypair,
worker_network_keypair,
worker_network_seed: worker_seed.to_string(),
}),
})
}
Expand All @@ -467,13 +477,11 @@ impl KeyConfig {
pub fn new_with_testing_key(primary_keypair: BlsKeypair) -> Self {
let primary_network_keypair =
Self::generate_network_keypair(&primary_keypair, "primary network keypair");
let worker_network_keypair =
Self::generate_network_keypair(&primary_keypair, "worker network keypair");
Self {
inner: Arc::new(KeyConfigInner {
primary_keypair,
primary_network_keypair,
worker_network_keypair,
worker_network_seed: "worker network keypair".to_string(),
}),
}
}
Expand All @@ -494,15 +502,30 @@ impl KeyConfig {
self.primary_network_keypair().public().clone().into()
}

/// Provide the keypair (with private key) for the worker network.
/// Provide the keypair (with private key) for the network of `worker_id`.
/// Allows building the libp2p worker network.
pub fn worker_network_keypair(&self) -> &NetworkKeypair {
&self.inner.worker_network_keypair
///
/// Worker 0 derives from the stored seed exactly as before per-worker swarms existed. This
/// keeps worker 0's PeerId stable for deployed nodes: that network identity is advertised
/// on-chain and cached in peers' kad stores, so it must not change. Worker ids above 0
/// append the id to the seed to get a distinct keypair per swarm.
pub fn worker_network_keypair(&self, worker_id: WorkerId) -> NetworkKeypair {
if worker_id == 0 {
Self::generate_network_keypair(
&self.inner.primary_keypair,
&self.inner.worker_network_seed,
)
} else {
Self::generate_network_keypair(
&self.inner.primary_keypair,
&format!("{} {worker_id}", self.inner.worker_network_seed),
)
}
}

/// The [NetworkPublicKey] for the worker network.
pub fn worker_network_public_key(&self) -> NetworkPublicKey {
self.worker_network_keypair().public().into()
/// The [NetworkPublicKey] for the network of `worker_id`.
pub fn worker_network_public_key(&self, worker_id: WorkerId) -> NetworkPublicKey {
self.worker_network_keypair(worker_id).public().into()
}

/// Creates a proof that the authority account address is owned by the
Expand Down Expand Up @@ -989,7 +1012,22 @@ mod tests {
let config = KeyConfig::new_with_testing_key(keypair);
let rendered = format!("{config:?}");

assert!(rendered.contains("[REDACTED]"), "BLS private half must be redacted: {rendered}");
assert!(
rendered.contains("private: \"[REDACTED]\""),
"BLS private half must be redacted: {rendered}"
);
assert!(
rendered.contains(&format!("public: {:?}", config.primary_public_key())),
"BLS public key should still be shown: {rendered}"
);
assert!(
!rendered.contains(&config.inner.worker_network_seed),
"worker network seed must not be shown"
);
assert!(
rendered.contains("worker_network_seed: \"[REDACTED]\""),
"worker network seed field must remain present and redacted: {rendered}"
);

let assert_secret_absent = |bytes: &[u8], what: &str| {
assert!(!rendered.contains(&hex::encode(bytes)), "{what} leaked as hex");
Expand Down Expand Up @@ -1017,14 +1055,18 @@ mod tests {
ed25519_secret(config.primary_network_keypair()).as_ref(),
"primary network secret",
);
// Per-worker network keypairs are derived on demand from the primary key and the stored
// seed (#555), so `KeyConfigInner` stores no worker keypair. Worker 0 is the legacy
// derivation; check it in case a future field caches derived keypairs.
assert_secret_absent(
ed25519_secret(config.worker_network_keypair()).as_ref(),
ed25519_secret(&config.worker_network_keypair(0)).as_ref(),
"worker network secret",
);

// Positive anchors: the network fields must actually render their public halves,
// Positive anchor: the primary network field must actually render its public half,
// otherwise the negative checks above pass vacuously once `KeyConfigInner`'s Debug
// stops printing the network keypairs at all.
// stops printing the network keypair at all. The worker seed's redacted field is
// anchored above; no worker keypair is stored.
let ed25519_public_rendered = |net: &NetworkKeypair| {
let ed25519: libp2p::identity::ed25519::Keypair =
net.clone().try_into().expect("network keypairs are ed25519");
Expand All @@ -1034,9 +1076,21 @@ mod tests {
rendered.contains(&ed25519_public_rendered(config.primary_network_keypair())),
"primary network public key should still be shown: {rendered}"
);
assert!(
rendered.contains(&ed25519_public_rendered(config.worker_network_keypair())),
"worker network public key should still be shown: {rendered}"
);
}

/// Worker 0 must keep the legacy bare-seed derivation (its PeerId is advertised on-chain),
/// worker 1 must get a distinct keypair, and derivation must be deterministic per id.
#[test]
fn test_worker_network_keypair_per_id_derivation() {
let kc = KeyConfig::new_with_testing_key(random_keypair());
let legacy: NetworkPublicKey = KeyConfig::generate_network_keypair(
&kc.inner.primary_keypair,
"worker network keypair",
)
.public()
.into();
assert_eq!(kc.worker_network_public_key(0), legacy);
assert_ne!(kc.worker_network_public_key(1), kc.worker_network_public_key(0));
assert_eq!(kc.worker_network_public_key(1), kc.worker_network_public_key(1));
}
}
2 changes: 1 addition & 1 deletion crates/network-libp2p/src/consensus.rs
Original file line number Diff line number Diff line change
Expand Up @@ -365,7 +365,7 @@ where
external_addr: Multiaddr,
rpc: Option<RpcInfo>,
) -> NetworkResult<Self> {
let network_key = key_config.worker_network_keypair().clone();
let network_key = key_config.worker_network_keypair(worker_id);
Self::new(
network_config,
event_stream,
Expand Down
Loading
Loading