[Feature]: signatures on WSTS packets need to be verified before feeding them into the state machines

[xoloki]

Feature - Verify WSTS packet signatures

1. Description

Applications which use wsts state machines must verify the signatures on the packets before processing them.

1.1 Context & Purpose

All wsts network packets are signed, to guarantee that they come from the purported source, and have not been tampered with. So they must be verified before processing.

But wsts applications typically run both coordinator and signer state machines, so it’s better to verify them outside the state machines themselves. Also, coordinator selection is external to the state machines.

2. Technical Details:

Call Packet::verify with the current signer and coordinator public keys after receiving packets, before feeding them into the machines. Bad packets should be dropped.

2.1 Acceptance Criteria:

• all packets are verified
• tests added

3. Related Issues and Pull Requests (optional):

[djordon]

I think that this is a duplicate of #296.

[xoloki]

I think that this is a duplicate of #296.

I don’t think so. 296 is about signing bitcoin transactions AFAICT.

[djordon]

Oh yeah you are right. It is not a duplicate.

[djordon]

Oh man, we do here

sbtc/signer/src/transaction_signer.rs

Lines 219 to 223 in 53d43bb

	async fn handle_signer_message(&mut self, msg: &network::Msg) -> Result<(), Error> {
	if !msg.verify() {
	tracing::warn!("unable to verify message");
	return Err(Error::InvalidSignature);
	}

but do not here

sbtc/signer/src/transaction_coordinator.rs

Lines 332 to 377 in c8fa6c5

	async fn relay_messages_to_wsts_state_machine_until_signature_created(
	&mut self,
	bitcoin_chain_tip: &model::BitcoinBlockHash,
	coordinator_state_machine: &mut wsts_state_machine::CoordinatorStateMachine,
	txid: bitcoin::Txid,
	) -> Result<wsts::taproot::SchnorrProof, Error> {
	loop {
	let msg = self.network.receive().await?;
	if &msg.bitcoin_chain_tip != bitcoin_chain_tip {
	tracing::warn!(?msg, "concurrent wsts signing round message observed");
	continue;
	}
	let message::Payload::WstsMessage(wsts_msg) = msg.inner.payload else {
	continue;
	};
	let packet = wsts::net::Packet {
	msg: wsts_msg.inner,
	sig: Vec::new(),
	};
	let (outbound_packet, operation_result) =
	match coordinator_state_machine.process_message(&packet) {
	Ok(val) => val,
	Err(err) => {
	tracing::warn!(?packet, reason = %err, "ignoring packet");
	continue;
	}
	};
	if let Some(packet) = outbound_packet {
	let msg = message::WstsMessage { txid, inner: packet.msg };
	self.send_message(msg, bitcoin_chain_tip).await?;
	}
	match operation_result {
	Some(wsts::state_machine::OperationResult::SignTaproot(signature)) => {
	return Ok(signature)
	}
	None => continue,
	Some(_) => return Err(Error::UnexpectedOperationResult),
	}
	}
	}

Okay, that fell through the cracks. I think the best place to do this is here:

sbtc/signer/src/network/mod.rs

Lines 95 to 113 in 53d43bb

	async fn receive(&mut self) -> Result<Msg, Error> {
	loop {
	tokio::select! {
	_ = self.term.wait_for_shutdown() => {
	return Err(Error::SignerShutdown);
	},
	recv = self.signal_rx.recv() => {
	match recv {
	Ok(SignerSignal::Event(SignerEvent::P2P(P2PEvent::MessageReceived(msg)))) => {
	return Ok(msg);
	},
	// We're only interested in the above messages, so we ignore
	// the rest.
	_ => continue,
	}
	}
	}
	}
	}

That way we won't need to track everything down all the time. Hmmm, this might be straightforward.