Closed xoloki closed 2 days ago
I think that this is a duplicate of https://github.com/stacks-network/sbtc/issues/296.
I think that this is a duplicate of #296.
I don’t think so. 296 is about signing bitcoin transactions AFAICT.
Oh yeah you are right. It is not a duplicate.
Oh man, we do here https://github.com/stacks-network/sbtc/blob/53d43bb9f1bd7797626779b16127e09d3d453d87/signer/src/transaction_signer.rs#L219-L223 but do not here https://github.com/stacks-network/sbtc/blob/c8fa6c52341725820c005d6e7e72486c14066a87/signer/src/transaction_coordinator.rs#L332-L377
Okay, that fell through the cracks. I think the best place to do this is here: https://github.com/stacks-network/sbtc/blob/53d43bb9f1bd7797626779b16127e09d3d453d87/signer/src/network/mod.rs#L95-L113 That way we won't need to track everything down all the time. Hmmm, this might be straightforward.
Edit: Oh maybe not. We also need to validate the public key against the list of accepted public keys. I wonder if we can skip this entirely, since the networking layer also does this.
I wonder if we can skip this entirely, since the networking layer also does this.
It does not at present -- that's where I raised the Q about which messages should be guarded and not (since a pending/joining node may still need to be able to communicate with the network in some capacity). Right now we assume every message is a SignedMessage
, so we could do this in the networking layer if we wanted to, at least until we (maybe) introduce some other network-management messages where we'd need to adjust the impl.
We could also just implement it for wsts messages in the networking layer (for now).
We could also just implement it for wsts messages in the networking layer (for now).
I've been looking over this, and while we can verify messages at the network layer, it won't solve the problem this issue was created to solve.
ecdsa::Signed<message::SignerMessage>
wraps the message, signature, and pubkey, so we can call Signed<>::verify
. This will at least show that the pub key was used to sign the message, and it has not been altered in transport since being signed.
But wsts
needs more than this. wsts::net::Packet
contains a wsts::net::Message
, and the subtypes of the latter usually contain a signer_id
. The signer_id
allows us to actually check authentication, not just integrity, and later authorization. So we need to be able to see signer_id
, and map it to a PublicKey
.
The wsts
state machines do have this knowledge; the SignerStateMachine
has a PublicKeys
field, a the CoordinatorStateMachine
s have a Config
field with similar information.
So it probably makes more sense for the purposes of this issue to do the checks inside TxSignerEventLoop::relay_message
Actually, it looks like TxSignerEventLoop ::handle_wsts_message
already cracks open the messages themselves, so we would have access to signer_id
with no refactoring necessary.
Here's a draft PR which does this for public and private shares: https://github.com/stacks-network/sbtc/pull/723
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:
3. Related Issues and Pull Requests (optional):