From d1db6fd62d7351e988aec3bc629a43526a7598a0 Mon Sep 17 00:00:00 2001 From: Bruce Riley Date: Wed, 14 Aug 2024 14:12:26 -0500 Subject: [PATCH 1/2] Node: Unknown guardian tweaks --- node/pkg/p2p/p2p.go | 60 ++++++++++++++++++------------- node/pkg/processor/observation.go | 58 +++++++++++++++--------------- 2 files changed, 65 insertions(+), 53 deletions(-) diff --git a/node/pkg/p2p/p2p.go b/node/pkg/p2p/p2p.go index 3ea025f7ab..6c90baf5a9 100644 --- a/node/pkg/p2p/p2p.go +++ b/node/pkg/p2p/p2p.go @@ -760,24 +760,30 @@ func Run(params *RunParams) func(ctx context.Context) error { gs := params.gst.Get() if gs == nil { // No valid guardian set yet - dropping heartbeat - logger.Log(params.components.SignedHeartbeatLogLevel, "skipping heartbeat - no guardian set", - zap.Any("value", s), - zap.String("from", envelope.GetFrom().String())) + if logger.Core().Enabled(params.components.SignedHeartbeatLogLevel) { + logger.Log(params.components.SignedHeartbeatLogLevel, "skipping heartbeat - no guardian set", + zap.Any("value", s), + zap.String("from", envelope.GetFrom().String())) + } break } if heartbeat, err := processSignedHeartbeat(envelope.GetFrom(), s, gs, params.gst, params.disableHeartbeatVerify); err != nil { p2pMessagesReceived.WithLabelValues("invalid_heartbeat").Inc() - logger.Log(params.components.SignedHeartbeatLogLevel, "invalid signed heartbeat received", - zap.Error(err), - zap.Any("payload", msg.Message), - zap.Any("value", s), - zap.Binary("raw", envelope.Data), - zap.String("from", envelope.GetFrom().String())) + if logger.Core().Enabled(params.components.SignedHeartbeatLogLevel) { + logger.Log(params.components.SignedHeartbeatLogLevel, "invalid signed heartbeat received", + zap.Error(err), + zap.Any("payload", msg.Message), + zap.Any("value", s), + zap.Binary("raw", envelope.Data), + zap.String("from", envelope.GetFrom().String())) + } } else { p2pMessagesReceived.WithLabelValues("valid_heartbeat").Inc() - logger.Log(params.components.SignedHeartbeatLogLevel, "valid signed heartbeat received", - zap.Any("value", heartbeat), - zap.String("from", envelope.GetFrom().String())) + if logger.Core().Enabled(params.components.SignedHeartbeatLogLevel) { + logger.Log(params.components.SignedHeartbeatLogLevel, "valid signed heartbeat received", + zap.Any("value", heartbeat), + zap.String("from", envelope.GetFrom().String())) + } func() { if len(heartbeat.P2PNodeId) != 0 { @@ -981,24 +987,30 @@ func Run(params *RunParams) func(ctx context.Context) error { gs := params.gst.Get() if gs == nil { // No valid guardian set yet - dropping heartbeat - logger.Log(params.components.SignedHeartbeatLogLevel, "skipping heartbeat - no guardian set", - zap.Any("value", s), - zap.String("from", envelope.GetFrom().String())) + if logger.Core().Enabled(params.components.SignedHeartbeatLogLevel) { + logger.Log(params.components.SignedHeartbeatLogLevel, "skipping heartbeat - no guardian set", + zap.Any("value", s), + zap.String("from", envelope.GetFrom().String())) + } break } if heartbeat, err := processSignedHeartbeat(envelope.GetFrom(), s, gs, params.gst, params.disableHeartbeatVerify); err != nil { p2pMessagesReceived.WithLabelValues("invalid_heartbeat").Inc() - logger.Log(params.components.SignedHeartbeatLogLevel, "invalid signed heartbeat received", - zap.Error(err), - zap.Any("payload", msg.Message), - zap.Any("value", s), - zap.Binary("raw", envelope.Data), - zap.String("from", envelope.GetFrom().String())) + if logger.Core().Enabled(params.components.SignedHeartbeatLogLevel) { + logger.Log(params.components.SignedHeartbeatLogLevel, "invalid signed heartbeat received", + zap.Error(err), + zap.Any("payload", msg.Message), + zap.Any("value", s), + zap.Binary("raw", envelope.Data), + zap.String("from", envelope.GetFrom().String())) + } } else { p2pMessagesReceived.WithLabelValues("valid_heartbeat").Inc() - logger.Log(params.components.SignedHeartbeatLogLevel, "valid signed heartbeat received", - zap.Any("value", heartbeat), - zap.String("from", envelope.GetFrom().String())) + if logger.Core().Enabled(params.components.SignedHeartbeatLogLevel) { + logger.Log(params.components.SignedHeartbeatLogLevel, "valid signed heartbeat received", + zap.Any("value", heartbeat), + zap.String("from", envelope.GetFrom().String())) + } func() { if len(heartbeat.P2PNodeId) != 0 { diff --git a/node/pkg/processor/observation.go b/node/pkg/processor/observation.go index b67574db9b..3fcea6318d 100644 --- a/node/pkg/processor/observation.go +++ b/node/pkg/processor/observation.go @@ -97,6 +97,7 @@ func (p *Processor) handleSingleObservation(addr []byte, m *gossipv1.Observation start := time.Now() observationsReceivedTotal.Inc() + their_addr := common.BytesToAddress(addr) hash := hex.EncodeToString(m.Hash) s := p.state.signatures[hash] if s != nil && s.submitted { @@ -116,35 +117,6 @@ func (p *Processor) handleSingleObservation(addr []byte, m *gossipv1.Observation ) } - // Verify the Guardian's signature. This verifies that m.Signature matches m.Hash and recovers - // the public key that was used to sign the payload. - pk, err := crypto.Ecrecover(m.Hash, m.Signature) - if err != nil { - p.logger.Warn("failed to verify signature on observation", - zap.String("messageId", m.MessageId), - zap.String("digest", hash), - zap.String("signature", hex.EncodeToString(m.Signature)), - zap.String("addr", hex.EncodeToString(addr)), - zap.Error(err)) - observationsFailedTotal.WithLabelValues("invalid_signature").Inc() - return - } - - // Verify that addr matches the public key that signed m.Hash. - their_addr := common.BytesToAddress(addr) - signer_pk := common.BytesToAddress(crypto.Keccak256(pk[1:])[12:]) - - if their_addr != signer_pk { - p.logger.Info("invalid observation - address does not match pubkey", - zap.String("messageId", m.MessageId), - zap.String("digest", hash), - zap.String("signature", hex.EncodeToString(m.Signature)), - zap.String("addr", hex.EncodeToString(addr)), - zap.String("pk", signer_pk.Hex())) - observationsFailedTotal.WithLabelValues("pubkey_mismatch").Inc() - return - } - // Determine which guardian set to use. The following cases are possible: // // - We have already seen the message and generated ourObservation. In this case, use the guardian set valid at the time, @@ -197,6 +169,34 @@ func (p *Processor) handleSingleObservation(addr []byte, m *gossipv1.Observation return } + // Verify the Guardian's signature. This verifies that m.Signature matches m.Hash and recovers + // the public key that was used to sign the payload. + pk, err := crypto.Ecrecover(m.Hash, m.Signature) + if err != nil { + p.logger.Warn("failed to verify signature on observation", + zap.String("messageId", m.MessageId), + zap.String("digest", hash), + zap.String("signature", hex.EncodeToString(m.Signature)), + zap.String("addr", hex.EncodeToString(addr)), + zap.Error(err)) + observationsFailedTotal.WithLabelValues("invalid_signature").Inc() + return + } + + // Verify that addr matches the public key that signed m.Hash. + signer_pk := common.BytesToAddress(crypto.Keccak256(pk[1:])[12:]) + + if their_addr != signer_pk { + p.logger.Info("invalid observation - address does not match pubkey", + zap.String("messageId", m.MessageId), + zap.String("digest", hash), + zap.String("signature", hex.EncodeToString(m.Signature)), + zap.String("addr", hex.EncodeToString(addr)), + zap.String("pk", signer_pk.Hex())) + observationsFailedTotal.WithLabelValues("pubkey_mismatch").Inc() + return + } + // Hooray! Now, we have verified all fields on SignedObservation and know that it includes // a valid signature by an active guardian. We still don't fully trust them, as they may be // byzantine, but now we know who we're dealing with. From bccc0eb58c8de23c5181503fb57d6b13f43ed4d6 Mon Sep 17 00:00:00 2001 From: Bruce Riley Date: Wed, 14 Aug 2024 14:48:28 -0500 Subject: [PATCH 2/2] Code review rework --- node/pkg/p2p/p2p.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/node/pkg/p2p/p2p.go b/node/pkg/p2p/p2p.go index 6c90baf5a9..bb1238daee 100644 --- a/node/pkg/p2p/p2p.go +++ b/node/pkg/p2p/p2p.go @@ -760,7 +760,7 @@ func Run(params *RunParams) func(ctx context.Context) error { gs := params.gst.Get() if gs == nil { // No valid guardian set yet - dropping heartbeat - if logger.Core().Enabled(params.components.SignedHeartbeatLogLevel) { + if logger.Level().Enabled(params.components.SignedHeartbeatLogLevel) { logger.Log(params.components.SignedHeartbeatLogLevel, "skipping heartbeat - no guardian set", zap.Any("value", s), zap.String("from", envelope.GetFrom().String())) @@ -769,7 +769,7 @@ func Run(params *RunParams) func(ctx context.Context) error { } if heartbeat, err := processSignedHeartbeat(envelope.GetFrom(), s, gs, params.gst, params.disableHeartbeatVerify); err != nil { p2pMessagesReceived.WithLabelValues("invalid_heartbeat").Inc() - if logger.Core().Enabled(params.components.SignedHeartbeatLogLevel) { + if logger.Level().Enabled(params.components.SignedHeartbeatLogLevel) { logger.Log(params.components.SignedHeartbeatLogLevel, "invalid signed heartbeat received", zap.Error(err), zap.Any("payload", msg.Message), @@ -779,7 +779,7 @@ func Run(params *RunParams) func(ctx context.Context) error { } } else { p2pMessagesReceived.WithLabelValues("valid_heartbeat").Inc() - if logger.Core().Enabled(params.components.SignedHeartbeatLogLevel) { + if logger.Level().Enabled(params.components.SignedHeartbeatLogLevel) { logger.Log(params.components.SignedHeartbeatLogLevel, "valid signed heartbeat received", zap.Any("value", heartbeat), zap.String("from", envelope.GetFrom().String())) @@ -987,7 +987,7 @@ func Run(params *RunParams) func(ctx context.Context) error { gs := params.gst.Get() if gs == nil { // No valid guardian set yet - dropping heartbeat - if logger.Core().Enabled(params.components.SignedHeartbeatLogLevel) { + if logger.Level().Enabled(params.components.SignedHeartbeatLogLevel) { logger.Log(params.components.SignedHeartbeatLogLevel, "skipping heartbeat - no guardian set", zap.Any("value", s), zap.String("from", envelope.GetFrom().String())) @@ -996,7 +996,7 @@ func Run(params *RunParams) func(ctx context.Context) error { } if heartbeat, err := processSignedHeartbeat(envelope.GetFrom(), s, gs, params.gst, params.disableHeartbeatVerify); err != nil { p2pMessagesReceived.WithLabelValues("invalid_heartbeat").Inc() - if logger.Core().Enabled(params.components.SignedHeartbeatLogLevel) { + if logger.Level().Enabled(params.components.SignedHeartbeatLogLevel) { logger.Log(params.components.SignedHeartbeatLogLevel, "invalid signed heartbeat received", zap.Error(err), zap.Any("payload", msg.Message), @@ -1006,7 +1006,7 @@ func Run(params *RunParams) func(ctx context.Context) error { } } else { p2pMessagesReceived.WithLabelValues("valid_heartbeat").Inc() - if logger.Core().Enabled(params.components.SignedHeartbeatLogLevel) { + if logger.Level().Enabled(params.components.SignedHeartbeatLogLevel) { logger.Log(params.components.SignedHeartbeatLogLevel, "valid signed heartbeat received", zap.Any("value", heartbeat), zap.String("from", envelope.GetFrom().String()))