Vote and Cert validation #3521

lukaszrzasik · 2024-07-31T15:17:22Z

This PR:

This PR tries to improve a vote and a certificate validation. The existing problem is that both a view and a certificate include a view number. These view numbers are not signed in any way and can be easily spoofed by a malicious actor, e.g. a valid certificate can be received by a malicious leader which can then re-send the received certificate with changed view numbers. These will be accepted as valid certificates by the other nodes and can wreck havoc on the consensus.

The approach taken by this PR is following:

Changes in the Vote trait:
The Vote trait is now Committable on its own (not only its data). Explained below for SimpleVote.
create_signed_vote is now a trait associated function.

Changes to SimpleVote:
vote_commitment calculates the commitment for the whole vote (data + view number).
There's a new private associated function called commit which calculates the commit based on the data and the view number. This method is used in create_signed_vote and in the commit method.

Changes to SimpleCertificate:
The vote_commitment field is now private and should not be used in any way. There is a new associated function which created a certificate.
vote_commitment method calculates and returns the vote's commitment instead of returning the stored the commitment. This means the commitment is calculated from the data and the view number.
is_valid_cert now correctly checks the received signature against the locally calculated commitment which includes the vote's data and the view number.

This PR does not:

Key places to review:

The crucial parts are in:
is_valid_cert method and vote_commitment method of the SimpleCertificate.

ss-es · 2024-07-31T15:32:28Z

crates/types/src/simple_certificate.rs

@@ -70,7 +70,7 @@ pub struct SimpleCertificate<TYPES: NodeType, VOTEABLE: Voteable, THRESHOLD: Thr
 /// The data this certificate is for. I.e the thing that was voted on to create this Certificate
 pub data: VOTEABLE,
 /// commitment of all the votes this cert should be signed over
- pub vote_commitment: Commitment<VOTEABLE>,
+ pub data_commitment: Commitment<VOTEABLE>,


We may not be able to change this, but I'm not completely sure. I would lean towards keeping the old name out of caution.

This doesn't affect the bincode serialization of the struct, but I believe it will change the JSON serialization. I'm not sure what repositories use JSON (if any), though I think the sequencer at the very least has a test that might catch this.

Yeah I think @jbearer mentioned a few weeks ago that changing the schema will cause downstream changes. Are we still in the "try not to change public interfaces" mode? Or are we past that.

jparr721 · 2024-08-01T20:12:21Z

crates/task-impls/src/consensus/mod.rs

@@ -375,11 +375,11 @@ impl<TYPES: NodeType, I: NodeImplementation<TYPES>> ConsensusTaskState<TYPES, I>

 debug!(
 "Attempting to publish proposal after forming a TC for view {}",
- *qc.view_number
+ *qc.view_number()


You're a hero for making these little fixes!!

bfish713

I've been trying to wrap my head around this and I think there is some room to clear things up. I keep getting confused on the various usages of vote.commit(), and vote_commentment() as well as where we use the "data commitment". I think maybe we can remove one or the other, and possibly the Committable bound from a few places and that would help clear things up.

bfish713 · 2024-08-15T14:26:41Z

crates/types/src/simple_certificate.rs

 /// commitment of all the votes this cert should be signed over
- pub vote_commitment: Commitment<VOTEABLE>,
+ /// Do not use this field, the commitment needs to be calculated instead
+ vote_commitment: Commitment<VOTE>,


Can we keep this type the same as it was before, since we don't need it anyway: Commitment<VOTE::Data>

bfish713 · 2024-08-15T14:30:42Z

crates/types/src/simple_vote.rs

+ fn vote_commitment(&self) -> Commitment<Self> {
+ self.commit()


Can we just remove if we can just call commit() anyway?

bfish713 · 2024-08-15T14:32:42Z

crates/types/src/vote.rs

@@ -90,33 +105,39 @@ type SignersMap<COMMITMENT, KEY> = HashMap<
 /// Accumulates votes until a certificate is formed. This implementation works for all simple vote and certificate pairs
 pub struct VoteAccumulator<
 TYPES: NodeType,
- VOTE: Vote<TYPES>,
- CERT: Certificate<TYPES, Voteable = VOTE::Commitment>,
+ VOTE: Vote<TYPES> + Committable,


Adding the Commitable bound shouldn't be needed

bfish713 · 2024-08-15T14:33:46Z

crates/types/src/vote.rs

 {
 /// Add a vote to the total accumulated votes. Returns the accumulator or the certificate if we
 /// have accumulated enough votes to exceed the threshold for creating a certificate.
 pub fn accumulate(&mut self, vote: &VOTE, membership: &TYPES::Membership) -> Either<(), CERT> {
 let key = vote.signing_key();

- let vote_commitment = vote.date_commitment();
+ let vote_commitment = vote.vote_commitment();


vote.commit()?

bfish713 · 2024-08-15T14:33:47Z

crates/types/src/vote.rs

- VoteAccumulator<TYPES, VOTE, CERT>
+impl<
+ TYPES: NodeType,
+ VOTE: Vote<TYPES> + Committable + Clone,


Same do we need Committable

bfish713 · 2024-08-15T14:38:35Z

crates/types/src/simple_vote.rs

- match TYPES::SignatureKey::sign(private_key, data.commit().as_ref()) {
+ match TYPES::SignatureKey::sign(
+ private_key,
+ <SimpleVote<TYPES, DATA> as Vote<TYPES>>::commit(&data, view).as_ref(),


Does Self::commit() not work here?

I'm getting a compiler error because of the name clash. I can change the Vote::commit name if it helps with the readability. Wdyt?

For now, I've simplified it to:

<Self as Vote<TYPES>>::commit(&data, view).as_ref()

The error:

error[E0034]: multiple applicable items in scope --> crates/types/src/simple_vote.rs:153:19 | 153 | Self::commit(&data, view).as_ref(), | ^^^^^^ multiple `commit` found | note: candidate #1 is defined in an impl of the trait `Vote` for the type `SimpleVote<TYPES, DATA>` --> crates/types/src/simple_vote.rs:164:5 | 164 | fn commit(data: &DATA, view: TYPES::Time) -> Commitment<Self> { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ note: candidate #2 is defined in an impl of the trait `Committable` for the type `SimpleVote<TYPES, DATA>` --> crates/types/src/simple_vote.rs:173:5 | 173 | fn commit(&self) -> Commitment<Self> { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use fully-qualified syntax to disambiguate | 153 | <SimpleVote<TYPES, DATA> as Vote>::commit(&data, view).as_ref(), | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 153 | Committable::commit(&data, view).as_ref(), | ~~~~~~~~~~~~~

bfish713 · 2024-08-15T14:42:22Z

crates/types/src/vote.rs

+ /// Gets the (Data + view number) commitment of the vote
+ fn vote_commitment(&self) -> Commitment<Self>;


Same as other comment if Vote must be committable then this is redundant right?

bfish713 · 2024-08-15T14:44:14Z

crates/types/src/vote.rs

@@ -156,7 +177,7 @@ impl<TYPES: NodeType, VOTE: Vote<TYPES>, CERT: Certificate<TYPES, Voteable = VOT

 // TODO: Get the stake from the stake table entry.
 *total_stake_casted += stake_table_entry.stake();
- total_vote_map.insert(key, (vote.signature(), vote.date_commitment()));
+ total_vote_map.insert(key, (vote.signature(), vote.vote_commitment()));


I think now that we are calculating a commitment on this call we should not be calling vote_commitment repeatedly and just reuse the value already calculated above

bfish713 · 2024-08-15T14:44:45Z

crates/types/src/vote.rs

@@ -173,8 +194,8 @@ impl<TYPES: NodeType, VOTE: Vote<TYPES>, CERT: Certificate<TYPES, Voteable = VOT
 );

 let cert = CERT::create_signed_certificate(
- vote.date_commitment(),
- vote.date().clone(),
+ vote.commit(),


Same here, we already calculated this value, lets not calculated it again

ss-es · 2024-08-15T17:41:46Z

crates/types/src/simple_certificate.rs

@@ -94,22 +116,21 @@ impl<TYPES: NodeType, VOTEABLE: Voteable + Committable, THRESHOLD: Threshold<TYP
 };
 committable::RawCommitmentBuilder::new("Certificate")
 .field("data", self.data.commit())
- .field("vote_commitment", self.vote_commitment)


Can we leave this in? It would be easier, because we wouldn't have to do any version switch on the certificate

bfish713 · 2024-08-22T19:59:31Z

Can we close this now?

lukaszrzasik added 2 commits July 31, 2024 17:16

Whole vote is now commitable (data + view)

9278fe4

Fmt and comments

b696d9f

ss-es reviewed Jul 31, 2024

View reviewed changes

lukaszrzasik added 2 commits July 31, 2024 22:02

A certificate includes a whole vote, including the view

14054e6

is_valid_cert correctly validates the certificate

3ee6957

jparr721 reviewed Aug 1, 2024

View reviewed changes

Merge branch 'main' into lr/vote-cert-validation

c574ee1

lukaszrzasik changed the title ~~Lr/vote cert validation~~ Vote and Cert validation Aug 6, 2024

lukaszrzasik mentioned this pull request Aug 14, 2024

Lr/vote cert validation v2 #3547

Closed

lukaszrzasik added 2 commits August 14, 2024 21:30

Calculate the signed commit for the certificate

ea5b1f6

Merge branch 'main' into lr/vote-cert-validation

3a36c02

lukaszrzasik marked this pull request as ready for review August 14, 2024 19:43

lukaszrzasik requested a review from bfish713 as a code owner August 14, 2024 19:43

Merge branch 'main' into lr/vote-cert-validation

ceb352d

bfish713 reviewed Aug 15, 2024

View reviewed changes

ss-es reviewed Aug 15, 2024

View reviewed changes

lukaszrzasik added 7 commits August 19, 2024 13:03

Revert vote_commitment type

190527e

Revert the change to SimpleCertificate::commit

261f3e5

Merge branch 'main' into lr/vote-cert-validation

bcb2ff3

Further simplify type casting

8227f22

Merge branch 'main' into lr/vote-cert-validation

073accb

Merge branch 'main' into lr/vote-cert-validation

2280ff2

Merge branch 'main' into lr/vote-cert-validation

d71516a

lukaszrzasik closed this Sep 3, 2024

Ancient123 deleted the lr/vote-cert-validation branch October 7, 2024 21:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vote and Cert validation #3521

Vote and Cert validation #3521

lukaszrzasik commented Jul 31, 2024 •

edited

Loading

ss-es Jul 31, 2024

jparr721 Jul 31, 2024

jparr721 Aug 1, 2024

bfish713 left a comment

bfish713 Aug 15, 2024

bfish713 Aug 15, 2024

bfish713 Aug 15, 2024

bfish713 Aug 15, 2024

bfish713 Aug 15, 2024

bfish713 Aug 15, 2024

lukaszrzasik Aug 19, 2024

bfish713 Aug 15, 2024

bfish713 Aug 15, 2024

bfish713 Aug 15, 2024

ss-es Aug 15, 2024

bfish713 commented Aug 22, 2024

		fn vote_commitment(&self) -> Commitment<Self> {
		self.commit()

		/// Gets the (Data + view number) commitment of the vote
		fn vote_commitment(&self) -> Commitment<Self>;

Vote and Cert validation #3521

Vote and Cert validation #3521

Conversation

lukaszrzasik commented Jul 31, 2024 • edited Loading

This PR:

This PR does not:

Key places to review:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bfish713 left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bfish713 commented Aug 22, 2024

lukaszrzasik commented Jul 31, 2024 •

edited

Loading