From 9777ecffaca5f90e4ffedab83f00ad53fd019b88 Mon Sep 17 00:00:00 2001 From: protolambda Date: Tue, 21 Jul 2020 19:47:43 +0200 Subject: [PATCH 1/6] Signature policy, option to reject signatures if they are set --- pubsub.go | 79 ++++++++++++++++++++++++++++++++++++--------------- sign.go | 35 +++++++++++++++++++++++ tracer.go | 1 + validation.go | 2 ++ 4 files changed, 94 insertions(+), 23 deletions(-) diff --git a/pubsub.go b/pubsub.go index 8a97d391..8751d5da 100644 --- a/pubsub.go +++ b/pubsub.go @@ -145,7 +145,7 @@ type PubSub struct { // source ID for signed messages; corresponds to signKey signID peer.ID // strict mode rejects all unsigned messages prior to validation - signStrict bool + signPolicy MessageSignaturePolicy ctx context.Context } @@ -212,8 +212,8 @@ func NewPubSub(ctx context.Context, h host.Host, rt PubSubRouter, opts ...Option maxMessageSize: DefaultMaxMessageSize, peerOutboundQueueSize: 32, signID: h.ID(), - signKey: h.Peerstore().PrivKey(h.ID()), - signStrict: true, + signKey: nil, + signPolicy: StrictSign, incoming: make(chan *RPC, 32), publish: make(chan *Message), newPeers: make(chan peer.ID), @@ -251,8 +251,14 @@ func NewPubSub(ctx context.Context, h host.Host, rt PubSubRouter, opts ...Option } } - if ps.signStrict && ps.signKey == nil { - return nil, fmt.Errorf("strict signature verification enabled but message signing is disabled") + if ps.signPolicy.mustSign() { + if ps.signID == "" { + return nil, fmt.Errorf("strict signature usage enabled but message author was disabled") + } + ps.signKey = ps.host.Peerstore().PrivKey(ps.signID) + if ps.signKey == nil { + return nil, fmt.Errorf("can't sign for peer %s: no private key", ps.signID) + } } if err := ps.disc.Start(ps); err != nil { @@ -303,17 +309,23 @@ func WithPeerOutboundQueueSize(size int) Option { } } +// WithMessageSignaturePolicy sets the mode of operation for producing and verifying message signatures. +func WithMessageSignaturePolicy(policy MessageSignaturePolicy) Option { + return func(p *PubSub) error { + p.signPolicy = policy + return nil + } +} + // WithMessageSigning enables or disables message signing (enabled by default). +// Deprecated: signature verification without message signing, +// or message signing without verification, are not recommended. func WithMessageSigning(enabled bool) Option { return func(p *PubSub) error { if enabled { - p.signKey = p.host.Peerstore().PrivKey(p.signID) - if p.signKey == nil { - return fmt.Errorf("can't sign for peer %s: no private key", p.signID) - } + p.signPolicy |= msgSigning } else { - p.signKey = nil - p.signStrict = false + p.signPolicy &^= msgSigning } return nil } @@ -328,23 +340,31 @@ func WithMessageAuthor(author peer.ID) Option { if author == "" { author = p.host.ID() } - if p.signKey != nil { - newSignKey := p.host.Peerstore().PrivKey(author) - if newSignKey == nil { - return fmt.Errorf("can't sign for peer %s: no private key", author) - } - p.signKey = newSignKey - } p.signID = author return nil } } +// WithNoAuthor omits the author data of pubsub messages, and disables the use of signatures. +func WithNoAuthor() Option { + return func(p *PubSub) error { + p.signID = "" + p.signPolicy &^= msgSigning + return nil + } +} + // WithStrictSignatureVerification is an option to enable or disable strict message signing. // When enabled (which is the default), unsigned messages will be discarded. +// Deprecated: signature verification without message signing, +// or message signing without verification, are not recommended. func WithStrictSignatureVerification(required bool) Option { return func(p *PubSub) error { - p.signStrict = required + if required { + p.signPolicy |= msgVerification + } else { + p.signPolicy &^= msgVerification + } return nil } } @@ -942,10 +962,23 @@ func (p *PubSub) pushMsg(msg *Message) { } // reject unsigned messages when strict before we even process the id - if p.signStrict && msg.Signature == nil { - log.Debugf("dropping unsigned message from %s", src) - p.tracer.RejectMessage(msg, rejectMissingSignature) - return + if p.signPolicy.mustVerify() { + if p.signPolicy.mustSign() { + if msg.Signature == nil { + log.Debugf("dropping unsigned message from %s", src) + p.tracer.RejectMessage(msg, rejectMissingSignature) + return + } + // Actual signature verification happens in the validation pipeline, + // after checking if the message was already seen or not, + // to avoid unnecessary signature verification processing-cost. + } else { + if msg.Signature != nil { + log.Debugf("dropping message with unexpected signature from %s", src) + p.tracer.RejectMessage(msg, rejectUnexpectedSignature) + return + } + } } // reject messages claiming to be from ourselves but not locally published diff --git a/sign.go b/sign.go index ea76214e..8c6da715 100644 --- a/sign.go +++ b/sign.go @@ -9,6 +9,41 @@ import ( "github.com/libp2p/go-libp2p-core/peer" ) +// MessageSignaturePolicy describes if signatures are produced, expected, and/or verified. +type MessageSignaturePolicy uint8 + +// LaxSign and LaxNoSign are deprecated. In the future msgSigning and msgVerification can be unified. +const ( + // msgSigning is set when the locally produced messages must be signed + msgSigning MessageSignaturePolicy = 1 << iota + // msgVerification is set when external messages must be verfied + msgVerification +) + +const ( + // StrictSign produces signatures and expects and verifies incoming signatures + StrictSign = msgSigning | msgVerification + // StrictNoSign does not produce signatures and drops and penalises incoming messages that carry one + StrictNoSign = msgVerification + // LaxSign produces signatures and validates incoming signatures iff one is present + // Deprecated: it is recommend to either strictly enable, or strictly disable, signatures. + LaxSign = msgSigning + // LaxNoSign does not produce signatures and validates incoming signatures iff one is present + // Deprecated: it is recommend to either strictly enable, or strictly disable, signatures. + LaxNoSign = 0 +) + +// mustVerify is true when a message signature must be verified. +// If signatures are not expected, verification checks if the signature is absent. +func (policy MessageSignaturePolicy) mustVerify() bool { + return policy&msgVerification != 0 +} + +// mustSign is true when messages should be signed, and incoming messages are expected to have a signature. +func (policy MessageSignaturePolicy) mustSign() bool { + return policy&msgSigning != 0 +} + const SignPrefix = "libp2p-pubsub:" func verifyMessageSignature(m *pb.Message) error { diff --git a/tracer.go b/tracer.go index fbf524ad..48e163a4 100644 --- a/tracer.go +++ b/tracer.go @@ -29,6 +29,7 @@ const ( rejectBlacklstedPeer = "blacklisted peer" rejectBlacklistedSource = "blacklisted source" rejectMissingSignature = "missing signature" + rejectUnexpectedSignature = "unexpected signature" rejectInvalidSignature = "invalid signature" rejectValidationQueueFull = "validation queue full" rejectValidationThrottled = "validation throttled" diff --git a/validation.go b/validation.go index 3e235c81..977403ae 100644 --- a/validation.go +++ b/validation.go @@ -241,6 +241,8 @@ func (v *validation) validateWorker() { // signature validation is performed synchronously, while user validators are invoked // asynchronously, throttled by the global validation throttle. func (v *validation) validate(vals []*topicVal, src peer.ID, msg *Message) { + // If signature verification is enabled, but signing is disabled, + // the Signature is required to be nil upon receiving the message in PubSub.pushMsg. if msg.Signature != nil { if !v.validateSignature(msg) { log.Warnf("message signature validation failed; dropping message from %s", src) From de309c3c309ca4fa34732e420b549bc0692667e4 Mon Sep 17 00:00:00 2001 From: protolambda Date: Tue, 21 Jul 2020 20:01:57 +0200 Subject: [PATCH 2/6] update misleading old field comments --- pubsub.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pubsub.go b/pubsub.go index 8751d5da..005a7259 100644 --- a/pubsub.go +++ b/pubsub.go @@ -140,9 +140,9 @@ type PubSub struct { // function used to compute the ID for a message msgID MsgIdFunction - // key for signing messages; nil when signing is disabled (default for now) + // key for signing messages; nil when signing is disabled signKey crypto.PrivKey - // source ID for signed messages; corresponds to signKey + // source ID for signed messages; corresponds to signKey, empty when signing is disabled signID peer.ID // strict mode rejects all unsigned messages prior to validation signPolicy MessageSignaturePolicy From a04676155153b011e33159a81a13359f23091271 Mon Sep 17 00:00:00 2001 From: protolambda Date: Tue, 21 Jul 2020 20:36:57 +0200 Subject: [PATCH 3/6] no-author forces no-seqno --- pubsub.go | 6 ++++-- topic.go | 12 +++++++----- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/pubsub.go b/pubsub.go index 005a7259..ab451a5b 100644 --- a/pubsub.go +++ b/pubsub.go @@ -142,7 +142,8 @@ type PubSub struct { // key for signing messages; nil when signing is disabled signKey crypto.PrivKey - // source ID for signed messages; corresponds to signKey, empty when signing is disabled + // source ID for signed messages; corresponds to signKey, empty when signing is disabled. + // If empty, the author and seq-nr are completely omitted from the messages. signID peer.ID // strict mode rejects all unsigned messages prior to validation signPolicy MessageSignaturePolicy @@ -345,7 +346,8 @@ func WithMessageAuthor(author peer.ID) Option { } } -// WithNoAuthor omits the author data of pubsub messages, and disables the use of signatures. +// WithNoAuthor omits the author and seq-number data of messages, and disables the use of signatures. +// Not recommended to use with the default message ID function, see WithMessageIdFn. func WithNoAuthor() Option { return func(p *PubSub) error { p.signID = "" diff --git a/topic.go b/topic.go index dcfb0624..9cdac5f6 100644 --- a/topic.go +++ b/topic.go @@ -164,13 +164,15 @@ func (t *Topic) Publish(ctx context.Context, data []byte, opts ...PubOpt) error return ErrTopicClosed } - seqno := t.p.nextSeqno() - id := t.p.host.ID() m := &pb.Message{ Data: data, TopicIDs: []string{t.topic}, - From: []byte(id), - Seqno: seqno, + From: nil, + Seqno: nil, + } + if t.p.signID != "" { + m.From = []byte(t.p.signID) + m.Seqno = t.p.nextSeqno() } if t.p.signKey != nil { m.From = []byte(t.p.signID) @@ -193,7 +195,7 @@ func (t *Topic) Publish(ctx context.Context, data []byte, opts ...PubOpt) error } select { - case t.p.publish <- &Message{m, id, nil}: + case t.p.publish <- &Message{m, t.p.host.ID(), nil}: case <-t.p.ctx.Done(): return t.p.ctx.Err() } From e1add25bbd310fad332c175cd0d8d0029fe9093b Mon Sep 17 00:00:00 2001 From: protolambda Date: Tue, 21 Jul 2020 22:34:19 +0200 Subject: [PATCH 4/6] update test: verify no signature is present --- floodsub_test.go | 52 +++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 47 insertions(+), 5 deletions(-) diff --git a/floodsub_test.go b/floodsub_test.go index 6245c129..374e96a7 100644 --- a/floodsub_test.go +++ b/floodsub_test.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "crypto/sha256" + "encoding/base64" "fmt" "io" "math/rand" @@ -717,13 +718,48 @@ func assertPeerList(t *testing.T, peers []peer.ID, expected ...peer.ID) { } } -func TestNonsensicalSigningOptions(t *testing.T) { +func TestWithNoSigning(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() - hosts := getNetHosts(t, ctx, 1) - _, err := NewFloodSub(ctx, hosts[0], WithMessageSigning(false), WithStrictSignatureVerification(true)) - if err == nil { - t.Error("expected constructor to fail on nonsensical options") + + hosts := getNetHosts(t, ctx, 2) + psubs := getPubsubs(ctx, hosts, WithNoAuthor(), WithMessageIdFn(func(pmsg *pb.Message) string { + // silly content-based test message-ID: just use the data as whole + return base64.URLEncoding.EncodeToString(pmsg.Data) + })) + + connect(t, hosts[0], hosts[1]) + + topic := "foobar" + data := []byte("this is a message") + + sub, err := psubs[1].Subscribe(topic) + if err != nil { + t.Fatal(err) + } + + time.Sleep(time.Millisecond * 10) + + err = psubs[0].Publish(topic, data) + if err != nil { + t.Fatal(err) + } + + msg, err := sub.Next(ctx) + if err != nil { + t.Fatal(err) + } + if msg.Signature != nil { + t.Fatal("signature in message") + } + if msg.From != nil { + t.Fatal("from in message") + } + if msg.Seqno != nil { + t.Fatal("seqno in message") + } + if string(msg.Data) != string(data) { + t.Fatalf("unexpected data: %s", string(msg.Data)) } } @@ -758,6 +794,12 @@ func TestWithSigning(t *testing.T) { if msg.Signature == nil { t.Fatal("no signature in message") } + if msg.From == nil { + t.Fatal("from not in message") + } + if msg.Seqno == nil { + t.Fatal("seqno not in message") + } if string(msg.Data) != string(data) { t.Fatalf("unexpected data: %s", string(msg.Data)) } From 441d7dff4471c9feb97920da86677a1dbddd9656 Mon Sep 17 00:00:00 2001 From: protolambda Date: Wed, 22 Jul 2020 15:08:34 +0200 Subject: [PATCH 5/6] update rejected message scoring --- score.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/score.go b/score.go index 777530e7..8ced22e9 100644 --- a/score.go +++ b/score.go @@ -570,6 +570,8 @@ func (ps *peerScore) RejectMessage(msg *Message, reason string) { fallthrough case rejectInvalidSignature: fallthrough + case rejectUnexpectedSignature: + fallthrough case rejectSelfOrigin: ps.markInvalidMessageDelivery(msg.ReceivedFrom, msg) return From 1a71b37a813127390164df97c20cf09a0e68992c Mon Sep 17 00:00:00 2001 From: protolambda Date: Wed, 22 Jul 2020 15:18:40 +0200 Subject: [PATCH 6/6] reject unexpected auth info --- pubsub.go | 11 +++++++++++ score.go | 2 ++ tracer.go | 1 + 3 files changed, 14 insertions(+) diff --git a/pubsub.go b/pubsub.go index ab451a5b..5cb01ac5 100644 --- a/pubsub.go +++ b/pubsub.go @@ -980,6 +980,17 @@ func (p *PubSub) pushMsg(msg *Message) { p.tracer.RejectMessage(msg, rejectUnexpectedSignature) return } + // If we are expecting signed messages, and not authoring messages, + // then do no accept seq numbers, from data, or key data. + // The default msgID function still relies on Seqno and From, + // but is not used if we are not authoring messages ourselves. + if p.signID == "" { + if msg.Seqno != nil || msg.From != nil || msg.Key != nil { + log.Debugf("dropping message with unexpected auth info from %s", src) + p.tracer.RejectMessage(msg, rejectUnexpectedAuthInfo) + return + } + } } } diff --git a/score.go b/score.go index 8ced22e9..6c5738b6 100644 --- a/score.go +++ b/score.go @@ -572,6 +572,8 @@ func (ps *peerScore) RejectMessage(msg *Message, reason string) { fallthrough case rejectUnexpectedSignature: fallthrough + case rejectUnexpectedAuthInfo: + fallthrough case rejectSelfOrigin: ps.markInvalidMessageDelivery(msg.ReceivedFrom, msg) return diff --git a/tracer.go b/tracer.go index 48e163a4..1d809ef3 100644 --- a/tracer.go +++ b/tracer.go @@ -30,6 +30,7 @@ const ( rejectBlacklistedSource = "blacklisted source" rejectMissingSignature = "missing signature" rejectUnexpectedSignature = "unexpected signature" + rejectUnexpectedAuthInfo = "unexpected auth info" rejectInvalidSignature = "invalid signature" rejectValidationQueueFull = "validation queue full" rejectValidationThrottled = "validation throttled"