Skip to content

Commit da154e7

Browse files
wpaulinowpaulino
committed
Remove NodeSigner::get_node_secret
Secrets should not be exposed in-memory at the interface level as it would be impossible the implement it against a hardware security module/secure element.
1 parent e7d0893 commit da154e7

File tree

14 files changed

+85
-151
lines changed

14 files changed

+85
-151
lines changed

‎fuzz/src/chanmon_consistency.rs

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -189,19 +189,21 @@ impl EntropySource for KeyProvider {
189189
}
190190

191191
impl NodeSigner for KeyProvider {
192-
fn get_node_secret(&self, _recipient: Recipient) -> Result<SecretKey, ()> {
193-
Ok(self.node_secret.clone())
194-
}
195-
196192
fn get_node_id(&self, recipient: Recipient) -> Result<PublicKey, ()> {
197-
let secp_ctx = Secp256k1::signing_only();
198-
Ok(PublicKey::from_secret_key(&secp_ctx, &self.get_node_secret(recipient)?))
193+
let node_secret = match recipient {
194+
Recipient::Node => Ok(&self.node_secret),
195+
Recipient::PhantomNode => Err(())
196+
}?;
197+
Ok(PublicKey::from_secret_key(&Secp256k1::signing_only(), node_secret))
199198
}
200199

201200
fn ecdh(&self, recipient: Recipient, other_key: &PublicKey, tweak: Option<&Scalar>) -> Result<SharedSecret, ()> {
202-
let mut node_secret = self.get_node_secret(recipient)?;
201+
let mut node_secret = match recipient {
202+
Recipient::Node => Ok(self.node_secret.clone()),
203+
Recipient::PhantomNode => Err(())
204+
}?;
203205
if let Some(tweak) = tweak {
204-
node_secret = node_secret.mul_tweak(tweak).unwrap();
206+
node_secret = node_secret.mul_tweak(tweak).map_err(|_| ())?;
205207
}
206208
Ok(SharedSecret::new(other_key, &node_secret))
207209
}
@@ -234,7 +236,6 @@ impl SignerProvider for KeyProvider {
234236
let id = channel_keys_id[0];
235237
let keys = InMemorySigner::new(
236238
&secp_ctx,
237-
self.get_node_secret(Recipient::Node).unwrap(),
238239
SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 4, self.node_secret[31]]).unwrap(),
239240
SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 5, self.node_secret[31]]).unwrap(),
240241
SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 6, self.node_secret[31]]).unwrap(),
@@ -251,7 +252,7 @@ impl SignerProvider for KeyProvider {
251252
fn read_chan_signer(&self, buffer: &[u8]) -> Result<Self::Signer, DecodeError> {
252253
let mut reader = std::io::Cursor::new(buffer);
253254

254-
let inner: InMemorySigner = ReadableArgs::read(&mut reader, self.get_node_secret(Recipient::Node).unwrap())?;
255+
let inner: InMemorySigner = Readable::read(&mut reader)?;
255256
let state = self.make_enforcement_state_cell(inner.commitment_seed);
256257

257258
Ok(EnforcingSigner {

‎fuzz/src/full_stack.rs

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ use lightning::util::errors::APIError;
4848
use lightning::util::events::Event;
4949
use lightning::util::enforcing_trait_impls::{EnforcingSigner, EnforcementState};
5050
use lightning::util::logger::Logger;
51-
use lightning::util::ser::{ReadableArgs, Writeable};
51+
use lightning::util::ser::{Readable, Writeable};
5252

5353
use crate::utils::test_logger;
5454
use crate::utils::test_persister::TestPersister;
@@ -293,19 +293,21 @@ impl EntropySource for KeyProvider {
293293
}
294294

295295
impl NodeSigner for KeyProvider {
296-
fn get_node_secret(&self, _recipient: Recipient) -> Result<SecretKey, ()> {
297-
Ok(self.node_secret.clone())
298-
}
299-
300296
fn get_node_id(&self, recipient: Recipient) -> Result<PublicKey, ()> {
301-
let secp_ctx = Secp256k1::signing_only();
302-
Ok(PublicKey::from_secret_key(&secp_ctx, &self.get_node_secret(recipient)?))
297+
let node_secret = match recipient {
298+
Recipient::Node => Ok(&self.node_secret),
299+
Recipient::PhantomNode => Err(())
300+
}?;
301+
Ok(PublicKey::from_secret_key(&Secp256k1::signing_only(), node_secret))
303302
}
304303

305304
fn ecdh(&self, recipient: Recipient, other_key: &PublicKey, tweak: Option<&Scalar>) -> Result<SharedSecret, ()> {
306-
let mut node_secret = self.get_node_secret(recipient)?;
305+
let mut node_secret = match recipient {
306+
Recipient::Node => Ok(self.node_secret.clone()),
307+
Recipient::PhantomNode => Err(())
308+
}?;
307309
if let Some(tweak) = tweak {
308-
node_secret = node_secret.mul_tweak(tweak).unwrap();
310+
node_secret = node_secret.mul_tweak(tweak).map_err(|_| ())?;
309311
}
310312
Ok(SharedSecret::new(other_key, &node_secret))
311313
}
@@ -341,7 +343,6 @@ impl SignerProvider for KeyProvider {
341343
EnforcingSigner::new_with_revoked(if inbound {
342344
InMemorySigner::new(
343345
&secp_ctx,
344-
self.node_secret.clone(),
345346
SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, ctr]).unwrap(),
346347
SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 2, ctr]).unwrap(),
347348
SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3, ctr]).unwrap(),
@@ -354,7 +355,6 @@ impl SignerProvider for KeyProvider {
354355
} else {
355356
InMemorySigner::new(
356357
&secp_ctx,
357-
self.node_secret.clone(),
358358
SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 7, ctr]).unwrap(),
359359
SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 8, ctr]).unwrap(),
360360
SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 9, ctr]).unwrap(),
@@ -368,7 +368,7 @@ impl SignerProvider for KeyProvider {
368368
}
369369

370370
fn read_chan_signer(&self, mut data: &[u8]) -> Result<EnforcingSigner, DecodeError> {
371-
let inner: InMemorySigner = ReadableArgs::read(&mut data, self.node_secret.clone())?;
371+
let inner: InMemorySigner = Readable::read(&mut data)?;
372372
let state = Arc::new(Mutex::new(EnforcementState::new()));
373373

374374
Ok(EnforcingSigner::new_with_revoked(
@@ -452,7 +452,7 @@ pub fn do_test(data: &[u8], logger: &Arc<dyn Logger>) {
452452
// keys subsequently generated in this test. Rather than regenerating all the messages manually,
453453
// it's easier to just increment the counter here so the keys don't change.
454454
keys_manager.counter.fetch_sub(3, Ordering::AcqRel);
455-
let our_id = PublicKey::from_secret_key(&Secp256k1::signing_only(), &keys_manager.get_node_secret(Recipient::Node).unwrap());
455+
let our_id = &keys_manager.get_node_id(Recipient::Node).unwrap();
456456
let network_graph = Arc::new(NetworkGraph::new(genesis_block(network).block_hash(), Arc::clone(&logger)));
457457
let gossip_sync = Arc::new(P2PGossipSync::new(Arc::clone(&network_graph), None, Arc::clone(&logger)));
458458
let scorer = FixedPenaltyScorer::with_penalty(0);
@@ -462,7 +462,7 @@ pub fn do_test(data: &[u8], logger: &Arc<dyn Logger>) {
462462
chan_handler: channelmanager.clone(),
463463
route_handler: gossip_sync.clone(),
464464
onion_message_handler: IgnoringMessageHandler {},
465-
}, our_network_key, 0, &[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 15, 0], Arc::clone(&logger), IgnoringMessageHandler{}, keys_manager.clone()));
465+
}, 0, &[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 15, 0], Arc::clone(&logger), IgnoringMessageHandler{}, keys_manager.clone()));
466466

467467
let mut should_forward = false;
468468
let mut payments_received: Vec<PaymentHash> = Vec::new();

‎fuzz/src/onion_message.rs

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -100,17 +100,19 @@ impl EntropySource for KeyProvider {
100100
}
101101

102102
impl NodeSigner for KeyProvider {
103-
fn get_node_secret(&self, _recipient: Recipient) -> Result<SecretKey, ()> {
104-
Ok(self.node_secret.clone())
105-
}
106-
107103
fn get_node_id(&self, recipient: Recipient) -> Result<PublicKey, ()> {
108-
let secp_ctx = Secp256k1::signing_only();
109-
Ok(PublicKey::from_secret_key(&secp_ctx, &self.get_node_secret(recipient)?))
104+
let node_secret = match recipient {
105+
Recipient::Node => Ok(&self.node_secret),
106+
Recipient::PhantomNode => Err(())
107+
}?;
108+
Ok(PublicKey::from_secret_key(&Secp256k1::signing_only(), node_secret))
110109
}
111110

112111
fn ecdh(&self, recipient: Recipient, other_key: &PublicKey, tweak: Option<&Scalar>) -> Result<SharedSecret, ()> {
113-
let mut node_secret = self.get_node_secret(recipient)?;
112+
let mut node_secret = match recipient {
113+
Recipient::Node => Ok(self.node_secret.clone()),
114+
Recipient::PhantomNode => Err(())
115+
}?;
114116
if let Some(tweak) = tweak {
115117
node_secret = node_secret.mul_tweak(tweak).map_err(|_| ())?;
116118
}

‎fuzz/src/utils/test_node_signer.rs

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -23,14 +23,6 @@ impl TestNodeSigner {
2323
}
2424

2525
impl NodeSigner for TestNodeSigner {
26-
fn get_node_secret(&self, recipient: Recipient) -> Result<SecretKey, ()> {
27-
let node_secret = match recipient {
28-
Recipient::Node => Ok(self.node_secret.clone()),
29-
Recipient::PhantomNode => Err(())
30-
}?;
31-
Ok(node_secret)
32-
}
33-
3426
fn get_inbound_payment_key_material(&self) -> lightning::chain::keysinterface::KeyMaterial {
3527
unreachable!()
3628
}

‎lightning-background-processor/src/lib.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -615,7 +615,7 @@ mod tests {
615615
use bitcoin::network::constants::Network;
616616
use lightning::chain::{BestBlock, Confirm, chainmonitor};
617617
use lightning::chain::channelmonitor::ANTI_REORG_DELAY;
618-
use lightning::chain::keysinterface::{InMemorySigner, Recipient, EntropySource, KeysManager, NodeSigner};
618+
use lightning::chain::keysinterface::{InMemorySigner, EntropySource, KeysManager};
619619
use lightning::chain::transaction::OutPoint;
620620
use lightning::get_event_msg;
621621
use lightning::ln::channelmanager::{BREAKDOWN_TIMEOUT, ChainParameters, ChannelManager, SimpleArcChannelManager};
@@ -786,7 +786,7 @@ mod tests {
786786
let p2p_gossip_sync = Arc::new(P2PGossipSync::new(network_graph.clone(), Some(chain_source.clone()), logger.clone()));
787787
let rapid_gossip_sync = Arc::new(RapidGossipSync::new(network_graph.clone()));
788788
let msg_handler = MessageHandler { chan_handler: Arc::new(test_utils::TestChannelMessageHandler::new()), route_handler: Arc::new(test_utils::TestRoutingMessageHandler::new()), onion_message_handler: IgnoringMessageHandler{}};
789-
let peer_manager = Arc::new(PeerManager::new(msg_handler, keys_manager.get_node_secret(Recipient::Node).unwrap(), 0, &seed, logger.clone(), IgnoringMessageHandler{}, keys_manager.clone()));
789+
let peer_manager = Arc::new(PeerManager::new(msg_handler, 0, &seed, logger.clone(), IgnoringMessageHandler{}, keys_manager.clone()));
790790
let node = Node { node: manager, p2p_gossip_sync, rapid_gossip_sync, peer_manager, chain_monitor, persister, tx_broadcaster, network_graph, logger, best_block, scorer };
791791
nodes.push(node);
792792
}

‎lightning-net-tokio/src/lib.rs

Lines changed: 3 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -614,14 +614,6 @@ mod tests {
614614
}
615615

616616
impl NodeSigner for TestNodeSigner {
617-
fn get_node_secret(&self, recipient: Recipient) -> Result<SecretKey, ()> {
618-
let node_secret = match recipient {
619-
Recipient::Node => Ok(self.node_secret.clone()),
620-
Recipient::PhantomNode => Err(())
621-
}?;
622-
Ok(node_secret)
623-
}
624-
625617
fn get_inbound_payment_key_material(&self) -> lightning::chain::keysinterface::KeyMaterial {
626618
unreachable!()
627619
}
@@ -753,7 +745,7 @@ mod tests {
753745
chan_handler: Arc::clone(&a_handler),
754746
route_handler: Arc::clone(&a_handler),
755747
onion_message_handler: Arc::new(lightning::ln::peer_handler::IgnoringMessageHandler{}),
756-
}, a_key.clone(), 0, &[1; 32], Arc::new(TestLogger()), Arc::new(lightning::ln::peer_handler::IgnoringMessageHandler{}), Arc::new(TestNodeSigner::new(a_key))));
748+
}, 0, &[1; 32], Arc::new(TestLogger()), Arc::new(lightning::ln::peer_handler::IgnoringMessageHandler{}), Arc::new(TestNodeSigner::new(a_key))));
757749

758750
let (b_connected_sender, mut b_connected) = mpsc::channel(1);
759751
let (b_disconnected_sender, mut b_disconnected) = mpsc::channel(1);
@@ -768,7 +760,7 @@ mod tests {
768760
chan_handler: Arc::clone(&b_handler),
769761
route_handler: Arc::clone(&b_handler),
770762
onion_message_handler: Arc::new(lightning::ln::peer_handler::IgnoringMessageHandler{}),
771-
}, b_key.clone(), 0, &[2; 32], Arc::new(TestLogger()), Arc::new(lightning::ln::peer_handler::IgnoringMessageHandler{}), Arc::new(TestNodeSigner::new(b_key))));
763+
}, 0, &[2; 32], Arc::new(TestLogger()), Arc::new(lightning::ln::peer_handler::IgnoringMessageHandler{}), Arc::new(TestNodeSigner::new(b_key))));
772764

773765
// We bind on localhost, hoping the environment is properly configured with a local
774766
// address. This may not always be the case in containers and the like, so if this test is
@@ -821,7 +813,7 @@ mod tests {
821813
chan_handler: Arc::new(lightning::ln::peer_handler::ErroringMessageHandler::new()),
822814
onion_message_handler: Arc::new(lightning::ln::peer_handler::IgnoringMessageHandler{}),
823815
route_handler: Arc::new(lightning::ln::peer_handler::IgnoringMessageHandler{}),
824-
}, a_key, 0, &[1; 32], Arc::new(TestLogger()), Arc::new(lightning::ln::peer_handler::IgnoringMessageHandler{}), Arc::new(TestNodeSigner::new(a_key))));
816+
}, 0, &[1; 32], Arc::new(TestLogger()), Arc::new(lightning::ln::peer_handler::IgnoringMessageHandler{}), Arc::new(TestNodeSigner::new(a_key))));
825817

826818
// Make two connections, one for an inbound and one for an outbound connection
827819
let conn_a = {

‎lightning/src/chain/channelmonitor.rs

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4185,7 +4185,6 @@ mod tests {
41854185
SecretKey::from_slice(&[41; 32]).unwrap(),
41864186
SecretKey::from_slice(&[41; 32]).unwrap(),
41874187
SecretKey::from_slice(&[41; 32]).unwrap(),
4188-
SecretKey::from_slice(&[41; 32]).unwrap(),
41894188
[41; 32],
41904189
0,
41914190
[0; 32],

0 commit comments

Comments
 (0)