refactor!: don't send public key with signature #4518

mversic · 2024-04-26T13:50:18Z

Description

Linked issue

Closes #4393 #4410

Benefits

Checklist

I've read CONTRIBUTING.md
I've used the standard signed-off commit format (or will squash just before merging)
All applicable CI checks pass (or I promised to make them pass later)
(optional) I've written unit tests for the code changes
I replied to all comments after code review, marking all implemented changes with thumbs up

data_model/src/block.rs

core/src/sumeragi/network_topology.rs

core/src/block.rs

core/src/sumeragi/view_change.rs

client/src/config/user/boilerplate.rs

core/src/smartcontracts/isi/query.rs

Erigara · 2024-05-23T07:13:46Z

core/src/block.rs

+                    if additional_leader_signatures.next().is_some() {
+                        return Err(SignatureVerificationError::DuplicateSignatures {
+                            signatory: leader_index,
+                        });
+                    }


I have concern about this check, can't malicious peer insert wrong index and valid signature (for random key pair)?
So that it looks like leader submitted multiple signatures.

This way malicous peer can't prevent safety of blockchain, but can violate liveliness.

Maybe we can require that at least one signature should be from leader?

malicious peer can send all sorts of invalid blocks. What you're saying is true, but is no less dangerous than sending other types of invalid blocks. We just reject a block sent from this peer, there is no harm in that IMO

I think what @Erigara is saying is that the block validation will globally and repeatedly fail unless it has a mechanism to identify and ignore (or expel from the validator set) the peer that spoof index zero with its signature.
The problem here would be trusting the index without checking it against the signature

I think what @Erigara is saying is that the block validation will globally and repeatedly fail unless it has a mechanism to identify and ignore (or expel from the validator set) the peer that spoof index zero with its signature.

spoofing 0 index is just one way in which malicious peer can affect consensus. Worst case, malicious peer can withhold sending any messages to other peers. It will only fail consensus repeatedly until the malicious peer becomes observing peer. 0 index spoofing is no special case and I don't think we should be more forgiving in this case

Ok, at least this is not a problem specific to this PR. It used to trust public keys instead of indices

core/src/block.rs

core/src/tx.rs

core/src/sumeragi/network_topology.rs

core/src/sumeragi/view_change.rs

data_model/src/query/mod.rs

data_model/src/block.rs

core/src/sumeragi/main_loop.rs

Erigara · 2024-05-24T06:14:44Z

core/src/sumeragi/main_loop.rs

+            (BlockMessage::BlockSigned(BlockSigned { signatures }), Role::ProxyTail) => {
+                info!(
+                    peer_id=%self.peer_id,
+                    role=%self.role(),
+                    "Received block signatures"
+                );

-                let roles: &[Role] = if current_view_change_index >= 1 {
+                let roles: &[Role] = if view_change_index >= 1 {
                    &[Role::ValidatingPeer, Role::ObservingPeer]
                } else {
                    &[Role::ValidatingPeer]
                };
-                let valid_signatures =
-                    current_topology.filter_signatures_by_roles(roles, &signatures);
-
-                if let Some(voted_block) = voting_block.as_mut() {
-                    let voting_block_hash = voted_block.block.as_ref().hash_of_payload();
-
-                    if hash == voting_block_hash {
-                        add_signatures::<true>(voted_block, valid_signatures);
-                    } else {
-                        debug!(%voting_block_hash, "Received signatures are not for the current block");
-                    }
+                let valid_signatures = self
+                    .topology
+                    .filter_signatures_by_roles(roles, &signatures)
+                    .cloned();
+
+                if let Some(mut voted_block) = voting_block.take() {
+                    add_signatures::<true>(&mut voted_block, valid_signatures, &self.topology);
+                    *voting_block = self.try_commit_block(voted_block, is_genesis_peer);
                } else {
                    // NOTE: Due to the nature of distributed systems, signatures can sometimes be received before
                    // the block (sent by the leader). Collect the signatures and wait for the block to be received
                    voting_signatures.extend(valid_signatures);
                }
            }


By looking at this code and try_commit it look like we don't check that signature indeed belong to the peer.
So any peer (including malicious leader itself) can send BlockSigned message with random block matching signatures for enough validating/observing peers and block will be committed.

Another problem is that on this path signatures are not checked for duplication.
So as before peer can submit BlockSigned message with the same index and signature and block will be committed.

ValidBlock is guaranteed to have all valid signatures except for the ProxyTail signature

valid_bloc.commit will check that ProxyTail signature is valid

try_commit is only invoked by the ProxyTail. You can see there is an assert statement inside

Erigara · 2024-05-24T06:25:04Z

core/src/sumeragi/main_loop.rs

+                .block
+                .commit(&self.topology)
+                .unpack(|e| self.send_event(e))
+                .expect("INTERNAL BUG: Proxy tail failed to commit block");
+


Maybe instead of unwrapping we can try to commit the block in the if statement?
I think it would be more reliable in long term if we decide that to commit block it's not enough to just receive enough signatures.
And proxy tail can add it's signature on BlockCreated message.

this function is only callable by the ProxyTail and ValidBlock is guaranteed to have all signatures valid. Consequently, this cannot and must not fail unless the ProxyTail is itself faulty

Erigara · 2024-05-24T06:25:57Z

core/src/block.rs

+                if let Err(err) = Self::verify_proxy_tail_signature(self.as_ref(), topology) {
                    return WithEvents::new(Err((self, err.into())));
                }


We probably should check that whole set of sigantures is valid.

ValidBlock is guaranteed to have all signatures valid except ProxyTail

Signed-off-by: Marin Veršić <marin.versic101@gmail.com>

mversic requested review from s8sato, SamHSmith, outoftardis, Erigara, 0x009922, DCNick3, VAmuzing, Asem-Abdelhady and dima74 as code owners April 26, 2024 13:50

github-actions bot added iroha2-dev The re-implementation of a BFT hyperledger in RUST Refactor Improvement to overall code quality labels Apr 26, 2024

mversic force-pushed the remove_public_key branch 2 times, most recently from cee8760 to df420c1 Compare April 26, 2024 14:02

github-actions bot added the api-changes Changes in the API for client libraries label Apr 26, 2024

mversic force-pushed the remove_public_key branch from df420c1 to 120c0df Compare April 26, 2024 14:21

Erigara assigned mversic and Erigara Apr 26, 2024

mversic force-pushed the remove_public_key branch from 120c0df to f66a58f Compare April 27, 2024 08:00

mversic changed the title ~~[refactor] #4393: don't send public key with signature~~ (refactor): don't send public key with signature May 1, 2024

mversic changed the title ~~(refactor): don't send public key with signature~~ refactor!: don't send public key with signature May 1, 2024

mversic force-pushed the remove_public_key branch 2 times, most recently from dc4d7d7 to 3701fc5 Compare May 2, 2024 07:52

mversic removed the iroha2-dev The re-implementation of a BFT hyperledger in RUST label May 2, 2024

mversic force-pushed the remove_public_key branch from 3701fc5 to ecb3bce Compare May 2, 2024 08:33

Erigara requested changes May 2, 2024

View reviewed changes

data_model/src/block.rs Outdated Show resolved Hide resolved

data_model/src/block.rs Outdated Show resolved Hide resolved

core/src/sumeragi/network_topology.rs Outdated Show resolved Hide resolved

core/src/block.rs Show resolved Hide resolved

mversic force-pushed the remove_public_key branch 5 times, most recently from bdb903e to fb852e9 Compare May 8, 2024 10:42

mversic dismissed s8sato’s stale review via d1c63f2 May 22, 2024 06:48

mversic force-pushed the remove_public_key branch 2 times, most recently from d1c63f2 to 9edd012 Compare May 22, 2024 06:50

mversic commented May 22, 2024

View reviewed changes

core/src/sumeragi/view_change.rs Outdated Show resolved Hide resolved

Erigara requested changes May 23, 2024

View reviewed changes

Erigara mentioned this pull request May 23, 2024

How block hash should be derived? #4641

Open

mversic force-pushed the remove_public_key branch 6 times, most recently from 211e87a to ad1aa08 Compare May 23, 2024 18:46

mversic commented May 23, 2024

View reviewed changes

core/src/sumeragi/main_loop.rs Outdated Show resolved Hide resolved

mversic force-pushed the remove_public_key branch from ad1aa08 to 1479b74 Compare May 23, 2024 19:52

mversic mentioned this pull request May 24, 2024

Keep voting blocks #4643

Open

Erigara reviewed May 24, 2024

View reviewed changes

mversic force-pushed the remove_public_key branch 4 times, most recently from e366a7d to 434750f Compare May 27, 2024 07:28

mversic requested review from Erigara and s8sato May 27, 2024 08:08

mversic added 2 commits May 27, 2024 14:36

refactor!: don't send public key with signature

82ae1b0

Signed-off-by: Marin Veršić <marin.versic101@gmail.com>

fix: view change updates

8f6384e

Signed-off-by: Marin Veršić <marin.versic101@gmail.com>

mversic force-pushed the remove_public_key branch 4 times, most recently from ee215e4 to 40f7083 Compare May 27, 2024 12:05

refactor: remove public key from transaction and query

e013a1d

Signed-off-by: Marin Veršić <marin.versic101@gmail.com>

mversic force-pushed the remove_public_key branch from 40f7083 to e013a1d Compare May 27, 2024 12:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor!: don't send public key with signature #4518

refactor!: don't send public key with signature #4518

mversic commented Apr 26, 2024 •

edited

Erigara May 23, 2024

mversic May 23, 2024

s8sato May 24, 2024

mversic May 24, 2024

s8sato May 24, 2024

Erigara May 24, 2024 •

edited

mversic May 27, 2024

Erigara May 24, 2024

mversic May 27, 2024

Erigara May 24, 2024

mversic May 24, 2024 •

edited

refactor!: don't send public key with signature #4518

Are you sure you want to change the base?

refactor!: don't send public key with signature #4518

Conversation

mversic commented Apr 26, 2024 • edited

Description

Linked issue

Benefits

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Erigara May 24, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mversic May 24, 2024 • edited

Choose a reason for hiding this comment

mversic commented Apr 26, 2024 •

edited

Erigara May 24, 2024 •

edited

mversic May 24, 2024 •

edited