SignedSSVMessage #345

MatheusFranco99 · 2024-01-10T14:22:25Z

Overview

Introduce SignedSSVMessage, which encapsulates SSVMessage along with a signature and a sender identifier fields.

Changes

New SignedSSVMesage structure in types
DecodePubsubMsg now returns a SignedSSVMesage
MsgValidation adapted to receive a SignedSSVMesage instead of a SSVMessage.

Note: though SSVMessage is no longer the transmitted message in the network, the Runner still processes the type SSVMessage in the ProcessMessage function.

Tests

Encoding
Valid SignedSSVMessage
Invalid SignedSSVMessage with no data
Invalid SignedSSVMessage with empty signature
Invalid SignedSSVMessage with zero signer
Invalid SignedSSVMessage with wrong data

GalRogozinski · 2024-01-10T20:24:54Z

p2p/validation/msg_validation.go

@@ -15,11 +16,19 @@ type MsgValidatorFunc = func(ctx context.Context, p peer.ID, msg *pubsub.Message

 func MsgValidation(runner ssv.Runner) MsgValidatorFunc {
 	return func(ctx context.Context, p peer.ID, msg *pubsub.Message) pubsub.ValidationResult {
-		ssvMsg, err := DecodePubsubMsg(msg)
+		signedSSVMsg, err := DecodePubsubMsg(msg)


maybe this is a complaint to myself.. but we should have deleted MsgValidation since it is definitely not what the code does at the moment

Sure, let's remove it in another PR

GalRogozinski

Looks great!

GalRogozinski · 2024-01-10T20:27:38Z

types/generate.go

@@ -7,7 +7,7 @@ package types
 //go:generate go run github.com/ferranbt/fastssz/sszgen --path share.go --include ./operator.go,./messages.go,./signer.go,./domain_type.go

 //go:generate rm -f ./messages_encoding.go
-//go:generate go run github.com/ferranbt/fastssz/sszgen --path messages.go --exclude-objs ValidatorPK,MessageID,MsgType
+//go:generate go run github.com/ferranbt/fastssz/sszgen --path messages.go --include ./operator.go --exclude-objs ValidatorPK,MessageID,MsgType


not sure why this changed?

Because of OperatorID addition

GalRogozinski · 2024-01-10T20:34:20Z

types/messages.go

+// SSVMessage is the main message passed within the SSV network. It encapsulates the SSVMessage structure and a signature
+type SignedSSVMessage struct {
+	OperatorID OperatorID
+	Signature  []byte `ssz-max:"512"`     // Current signature max size allow keys up to 512*8 = 4096 bits


maybe it is just nice to comment that this is created by the operator's private key

GalRogozinski

I think maybe we need to add at least one test where we validate a real signature
to show that use RSA with correct padding

MatheusFranco99 · 2024-02-19T10:59:17Z

@GalRogozinski
The current tests only check msg.Validate() which doesn't check the signature. This is because we don't specify here how this signature should be created or validated.
I didn't add such a restriction on the type of signature because the message validation module (in p2p/validation) will be removed from spec in a future PR (right?) and the validator or runner don't have access to this signature.
What do you think?

GalRogozinski · 2024-02-19T12:53:48Z

@MatheusFranco99

The spec should achieve 2 objectives in my eyes:

Make sure that the node implementation doesn't accidentally change the protocol w.o detection
Allow anyone to implement a node, ideally w.o looking at any other source.

Currently the spec doesn't exactly reach those objectives and in the spec rewrite I am hoping to change this.
In the meanwhile I wish to do things correctly for new additions :-)

Imagine someone changes the RSA parameters (for better security) and then creates an incompatibility.
This should be caught by the spec

MatheusFranco99 · 2024-02-19T13:01:22Z

@GalRogozinski
Sure, I'll be adding a check in message validation to verify the RSA signature and tests for it.

GalRogozinski

LGTM

y0sher · 2024-03-18T09:33:54Z

types/spectest/tests/signedssvmsg/test.go

+		if err == nil {
+			var pk *rsa.PublicKey
+			pk, err = types.PemToPublicKey(test.RSAPublicKey)
+			if err != nil {
+				panic(err.Error())
+			}
+
+			messageHash := sha256.Sum256(msg.Data)
+			err = rsa.VerifyPKCS1v15(pk, crypto.SHA256, messageHash[:], msg.Signature)


for a future PR, something we are working in ssv-node is encapsulating all operator pubkey/private key operation to make them less dependent on specific functions. so if for example we'll want to change our RSA implementation it will be easy.

That's great. I will try to encapsulate it here too.

y0sher

FYI this is probably a fork. we are creating signed messages in the node a bit differently right now.
https://github.com/bloxapp/ssv/blob/main/network/commons/common.go#L43

it might be good idea to consider using the same approach in the node unless this fork has some significant advantages.

MatheusFranco99 · 2024-03-18T09:51:02Z

@y0sher
Yes, I'm aware of that. The point of this PR is not only to define a type for this buffer approach that we're using on the node, but it also is a pre-requisite for the next "reduce signatures" PR. This signed message type will now be used in the runner module.

GalRogozinski

@y0sher @MatheusFranco99

This is a fork because the implementation puts signature first and the PR puts operator first, right?

If so we should change this to not be a fork because I don't think there is a real advantage here...

y0sher · 2024-03-19T11:13:09Z

@y0sher @MatheusFranco99

This is a fork because the implementation puts signature first and the PR puts operator first, right?

If so we should change this to not be a fork because I don't think there is a real advantage here...

Its a fork because the implementation uses its own way of encoding the sig and operators. even if you create the struct in the PR in the same order it doesn't mean the final encoded data will be the same unless you enforce the struct to be encoded in the same way.
In your PR the encoding is SSZ, where in the impl it is just raw data bytes on a buffer.

GalRogozinski · 2024-03-20T13:36:00Z

@moshe-blox @y0sher

There are 2 things we can do here:

Delete the ssz encoding and use the same encoding as impl
Make this part of the upcoming fork

The only reason I would use SSZ is just for the sake of consistency, since we use SSZ everywhere. Besides this I don't see an advantage. But this is good enough for me to opt for 2.
If you guys don't want to change to SSZ then we can go for 1.

GalRogozinski

Currently we fork so changed order is fine

MatheusFranco99 added 7 commits January 10, 2024 11:11

Add SignedSSVMessage

3ea4b69

Add SSZ encoding

3aebee8

Test structures for SignedSSVMessage

0fc89ba

Test utils and encoding test

f919fda

Tests

5d151fb

Generate tests

7965ba8

Adapt message validation

1c3ace2

MatheusFranco99 self-assigned this Jan 10, 2024

Fix expected error; Bring back commented tests

e67c4b5

MatheusFranco99 requested a review from GalRogozinski January 10, 2024 14:33

GalRogozinski reviewed Jan 10, 2024

View reviewed changes

GalRogozinski self-requested a review January 10, 2024 20:29

GalRogozinski reviewed Jan 10, 2024

View reviewed changes

Add comment on signature

e2e279b

MatheusFranco99 requested a review from GalRogozinski January 15, 2024 09:41

GalRogozinski requested changes Feb 18, 2024

View reviewed changes

MatheusFranco99 added 4 commits February 19, 2024 13:46

Merge branch 'main' into signed-ssv-msg

43da24c

Adjust ssz-max in SignedSSVMessage

9b1750d

Include RSA check in test

c74a5c1

Generate JSONs

b9469eb

MatheusFranco99 requested a review from GalRogozinski February 20, 2024 08:29

GalRogozinski approved these changes Feb 20, 2024

View reviewed changes

MatheusFranco99 requested review from MatusKysel and y0sher February 21, 2024 06:49

MatusKysel approved these changes Feb 24, 2024

View reviewed changes

MatheusFranco99 changed the title ~~SignedMsgType~~ SignedSSVMessage Mar 15, 2024

y0sher reviewed Mar 18, 2024

View reviewed changes

y0sher approved these changes Mar 18, 2024

View reviewed changes

GalRogozinski requested changes Mar 18, 2024

View reviewed changes

GalRogozinski approved these changes Mar 21, 2024

View reviewed changes

GalRogozinski changed the base branch from main to Atlanta March 21, 2024 11:42

GalRogozinski merged commit df58d97 into Atlanta Mar 21, 2024
2 checks passed

GalRogozinski mentioned this pull request Mar 25, 2024

Partial Signature Verification Aggregation #369

Merged

4 tasks

moshe-blox mentioned this pull request Mar 27, 2024

Replace redundant BLS in QBFT with RSA #372

Merged

5 tasks

GalRogozinski mentioned this pull request Mar 28, 2024

SignedSSVMessage #374

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SignedSSVMessage #345

SignedSSVMessage #345

MatheusFranco99 commented Jan 10, 2024 •

edited

Loading

GalRogozinski Jan 10, 2024

MatheusFranco99 Jan 11, 2024

GalRogozinski left a comment

GalRogozinski Jan 10, 2024

GalRogozinski Jan 11, 2024

GalRogozinski Jan 10, 2024

GalRogozinski left a comment

MatheusFranco99 commented Feb 19, 2024

GalRogozinski commented Feb 19, 2024

MatheusFranco99 commented Feb 19, 2024

GalRogozinski left a comment

y0sher Mar 18, 2024

MatheusFranco99 Mar 18, 2024

y0sher left a comment

MatheusFranco99 commented Mar 18, 2024

GalRogozinski left a comment

y0sher commented Mar 19, 2024 •

edited

Loading

GalRogozinski commented Mar 20, 2024

GalRogozinski left a comment

SignedSSVMessage #345

SignedSSVMessage #345

Conversation

MatheusFranco99 commented Jan 10, 2024 • edited Loading

Overview

Changes

Tests

GalRogozinski Jan 10, 2024

Choose a reason for hiding this comment

MatheusFranco99 Jan 11, 2024

Choose a reason for hiding this comment

GalRogozinski left a comment

Choose a reason for hiding this comment

GalRogozinski Jan 10, 2024

Choose a reason for hiding this comment

GalRogozinski Jan 11, 2024

Choose a reason for hiding this comment

GalRogozinski Jan 10, 2024

Choose a reason for hiding this comment

GalRogozinski left a comment

Choose a reason for hiding this comment

MatheusFranco99 commented Feb 19, 2024

GalRogozinski commented Feb 19, 2024

MatheusFranco99 commented Feb 19, 2024

GalRogozinski left a comment

Choose a reason for hiding this comment

y0sher Mar 18, 2024

Choose a reason for hiding this comment

MatheusFranco99 Mar 18, 2024

Choose a reason for hiding this comment

y0sher left a comment

Choose a reason for hiding this comment

MatheusFranco99 commented Mar 18, 2024

GalRogozinski left a comment

Choose a reason for hiding this comment

y0sher commented Mar 19, 2024 • edited Loading

GalRogozinski commented Mar 20, 2024

GalRogozinski left a comment

Choose a reason for hiding this comment

MatheusFranco99 commented Jan 10, 2024 •

edited

Loading

y0sher commented Mar 19, 2024 •

edited

Loading