Implement message signing #97

vyzo · 2018-08-26T11:23:08Z

Implements message signing, optionally enabled with the WithMessageSigning option:

Adds two fields to the Message protobuf: signature and key.
When the option has been specified, all published messages are signed with the host key.
When a signature is present, it is always validated. If strict mode has been specified, then all unsigned messages are rejected without validation. Otherwise, it is up to the topic validator to reject unsigned messages.
RSA keys are attached on signed messages, otherwise the key is extracted from the peer ID.

TBD:

tests

vyzo · 2018-08-27T08:14:52Z

added some tests.

whyrusleeping · 2018-08-27T18:27:27Z

pubsub.go

@@ -500,22 +547,34 @@ loop:
 	}

 	if throttle {
-		log.Warningf("message validation throttled; dropping message from %s", src)


why drop the warning?

it gets logged on the return path, so this would be logging it twice.

Basically the function doesn't do the dropping any more, just validation; so the logging has been moved to the caller.

whyrusleeping · 2018-08-27T18:27:57Z

pubsub.go

 	}

 	for i := 0; i < rcount; i++ {
 		valid := <-rch
 		if !valid {
-			log.Warningf("message validation failed; dropping message from %s", src)


again, dropping the warning feels weird

whyrusleeping · 2018-08-27T18:32:24Z

sign.go

+		// no attached key, it must be extractable from the source ID
+		pubk, err = pid.ExtractPublicKey()
+		if err != nil {
+			return err


maybe log a more specific error here. "could not validate because public key was not available or extractable: $ERR"

sure, that's probably a good idea.

whyrusleeping · 2018-08-27T18:33:01Z

sign.go

+		return err
+	}
+
+	if m.Key == nil {


i'd pull this whole logic out into a 'get key' method either on the message, or that takes a message

whyrusleeping · 2018-08-27T18:35:05Z

sign.go

+		}
+	}
+
+	xm := pb.Message{


I wonder if we can do something where we enforce that the protobuf puts the signature at the end, and simply slice the raw bytes to the right point instead of remarshaling

that would be very nice indeed -- i don't like the remarshal either.

we could also have a pre-allocated buffer where we WriteMsg into instead of marshaling.

Let's not rely on slicing protobufs, if at all possible.

Please read: How-To Crypto ipfs/notes#249. We need to include something that says that the signature is for pubsub.

Regardless, we need to make sure we can add new fields and have them covered by the signature by default.

mmm, good point. We could prefix the data we're signing with something like "libp2p-pubsub"

Hrm, not convinced it's absolutely necessary as this is transient data, but sure we can add a prefix.

Added a libp2p-pubsub: prefix.

whyrusleeping · 2018-08-27T18:38:28Z

pb/rpc.proto

@@ -19,6 +19,8 @@ message Message {
 	optional bytes data = 2;
 	optional bytes seqno = 3;
 	repeated string topicIDs = 4;
+	optional bytes signature = 5;


in light of my other comment, how about we make key come before signature, and space them out a bit (instead of 5 and 6, use 50 and 51) so future additions to the protocol don't have to come after it.

cc @Stebalien @warpfork to tell me "no, you cant just slice protobufs like that"

the key is not included in the signature, so it can stay where it is.

I'm not sure I can say "you can't" but I probably "wouldn't" if I could help it. I'd be a lot more comfortable if an envelope type {content, sig} was added.

I feel like holistically, even across wildly different formats, people tend to rue the day they made in-band signatures. For (distant, but again, holistically comparable) example, android putting sigs on apks inside the filesystem has been no end of comedic problems over the years.

Yeah, I guess one way to do this would be to have in the top level protobuf both the existing regular unsigned messages, and a new field for signed messages.

that also kind of disrupts the chronology of the development of the protobuf. i think we'd probably be best off keeping it as is

whyrusleeping · 2018-08-27T18:43:36Z

Approach looks good to me, nice and simple. My main concern now is making sure we don't shoot ourselves in the foot on perf. This code as written looks pretty allocation heavy.

whyrusleeping · 2018-08-27T18:45:33Z

pubsub.go

@@ -186,6 +190,13 @@ func WithValidateThrottle(n int) Option {
 	}
 }

+func WithMessageSigning() Option {


Maybe we should consider using different keys than our nodes peer key?

Sure, we can do that too -- the from in published messages will have to change to match the signing key though.

I think we can have a WithMessageSigningKey option that explicitly specifies the signing key.

Yeah, from can be any valid key. However, I'd prefer it if it were a peer ID (by default at least).

Also, this can't be optional.

Implemented strict mode with a flag in the WithMessageSigning option.

I don't know about this, it has perfomance implications.

I'd rather use CIDs or hashes to identify messages but we need to do something.

Well, ideally we would use message signing with strict mode.
I would argue that it's sane to keep the default behaviour unchanged (for a while at least), and provide the easy upgrade path of WithMessageSigning(true) for applications that need to secure their pubsub.
Eventually we can make it the default.

I don't think we need to take reflexive action and dump our simple message id construction just yet -- note that using hashes also has some perf implications.

I don't think we need to take reflexive action

Either is fine by me. My preference is to layer things (handle authorship at a higher layer) but it is useful to have that baked into the bottom layer.

I would argue that it's sane to keep the default behaviour unchanged (for a while at least), and provide the easy upgrade path of WithMessageSigning(true) for applications that need to secure their pubsub.

I'm not worried about "security", I'm worried about griefing by bored children (e.g., what happens on IRC). However, as long as we turn it on by default in go-ipfs (with strict off), I can live with it.

Makes sense to enable it in go-ipfs, I'll add the relevant options when we integrate it.

mhchia · 2018-08-28T10:12:05Z

pubsub.go

@@ -452,7 +463,7 @@ func (p *PubSub) pushMsg(vals []*topicVal, src peer.ID, msg *Message) {
 	}
 	p.markSeen(id)

-	if len(vals) > 0 {
+	if len(vals) > 0 || msg.Signature != nil {


A dumb question: Is it correct that PubSub.processLoop only focuses on dispatching events to workers, and leave computations(like this signature validation) done by goroutines, to avoid increasing response time?

Yes, this is correct. Also, concurrency -- not doing this validation in parallel wastes idle cores.

mhchia · 2018-08-28T10:22:01Z

pubsub.go

@@ -89,6 +90,9 @@ type PubSub struct {
 	peers        map[peer.ID]chan *RPC
 	seenMessages *timecache.TimeCache

+	// key for signing messages; nil when signing is disabled (default for now)


Does it make sense to have a flag as a field of the struct PubSub indicating whether to verify the signatures or not and is set when the PubSub is initialized? This way the existing code can just check this flag, instead of checking msg.Signature != nil. Because in my imagination msg.Signature == nil should be an invalid behavior when authentication is required, while it is valid when authentication is not enabled.

The signatures are always verified when present.
The question is what happens when the signature is not present, in which case a validator can reject the message as unsigned when authentication is required.

We could have a flag that only accepts signed messages however.

Ahh you're right. If the authentication is required, it is sufficient to just add the check for whether the signature is available in that case. Thank you!

vyzo · 2018-08-28T19:12:50Z

rebased on master.

bigs

i really like this change and think it's cleanly implemented. could we maybe make this configurable behavior for now so we can do some testing in the IPFS cluster? i can't imagine it'd be a huge drain on perf relative to the usefulness it adds to the end user

Stebalien · 2018-09-04T22:59:31Z

sign.go

+		return err
+	}
+
+	xm := pb.Message{


This is fine if we're never planning on adding additional fields. Otherwise, we should probably copy the entire message, remove just the signature (and key), and remarshal that. That should keep the unknown fields.

It's probably more futureproof to do it that way, although ... message copying again.

It should be as efficient or more efficient than the current code. I'm suggesting:

xm := *m xm.Signature = nil xm.Key = nil

Sure, we can do that.

Stebalien · 2018-09-04T23:01:37Z

sign.go

+
+	m.Signature = sig
+	switch key.Type() {
+	case crypto.RSA:


Ah. Actually, the "correct" way to do this is to:

Generate a peer ID.

Try to extract the key from the peer ID (ExtractPublicKey).

We may (will) have other key types that don't "embed".

It's also a lot more slower. Are we planning on adding other non-embeddable keys in the future?

maybe that overhead is negligible in the grand scale of things though.

We could add some "Embeds(key) bool` helper somewhere.

Are we planning on adding other non-embeddable keys in the future?

I don't think sekp256 keys embed and there's a PR in progress to add more general ECDSA keys that definitely won't embed.

Ok, but let's define a helper function in go-libp2p-peer.

edit: or go-libp2p-crypto, but somewhere more general than here.

for now I added an extraction test.

vyzo · 2018-09-05T17:57:24Z

@whyrusleeping

My main concern now is making sure we don't shoot ourselves in the foot on perf.

Agreed! I am concerned that we start entering the validation pipeline with it.

This code as written looks pretty allocation heavy.

I think that spawning an (admittedly very short-lived) goroutine for each message to validate the signatures should be a bigger concern.
Maybe we should pre-spawn NUMCPU goroutines to handle the validation instead and feed them through a buffered channel; if the buffer fills, we should drop the message.

Stebalien · 2018-09-05T22:04:43Z

pubsub.go

+	m := &pb.Message{
+		Data:     data,
+		TopicIDs: []string{topic},
+		From:     []byte(p.host.ID()),


This needs to be the ID of the key we're using to sign.

indeed, good catch.

it's also not a bug in the current implementation, as we don't allow arbitrary keys yet.

I will fix it anyway, so that it's futureproof (in case we want to allow arbitrary keys in the future)

vyzo · 2018-09-06T18:09:22Z

Per discussion with @whyrusleeping on zoom, we decided that the goroutine issue should not block merging this, but it's definitely something we want to fix.
I'll make a follow up issue and pr.

vyzo · 2018-09-06T19:04:15Z

Follow up in #103.

so that we don't do the goroutine dance for just a single validator, which ought to be the most common case.

vyzo · 2018-10-13T23:23:33Z

rebased on master

ghost assigned vyzo Aug 26, 2018

ghost added the in progress label Aug 26, 2018

vyzo requested review from whyrusleeping and Stebalien August 26, 2018 11:23

vyzo mentioned this pull request Aug 27, 2018

Graduate pubsub from being experimental ipfs/kubo#5383

Closed

whyrusleeping reviewed Aug 27, 2018

View reviewed changes

vyzo force-pushed the feat/signing branch from ef2dec8 to 6bfec03 Compare August 28, 2018 09:01

mhchia reviewed Aug 28, 2018

View reviewed changes

vyzo force-pushed the feat/signing branch from 6bfec03 to 946109e Compare August 28, 2018 19:12

bigs approved these changes Sep 4, 2018

View reviewed changes

Stebalien reviewed Sep 4, 2018

View reviewed changes

Stebalien reviewed Sep 5, 2018

View reviewed changes

vyzo mentioned this pull request Sep 6, 2018

Optimize Validator Pipeline #103

Closed

bigs added the feature label Sep 11, 2018

raulk added the req:ethereum label Sep 20, 2018

vyzo mentioned this pull request Oct 5, 2018

[Question] Multiaddr of publisher available to subscriber #109

Closed

add signature and key fields to Message

d2f7a66

vyzo added 15 commits October 14, 2018 02:22

scaffolding for signature validation

64aab34

implement validateSignature

7c508b4

optimize fast path for single topic validator

7c1012c

so that we don't do the goroutine dance for just a single validator, which ought to be the most common case.

sign messages when a signing key is present

a6c349b

option to enable message signing

9d250ae

implement signMessage and verifyMessageSignature

60a06ce

test message signing

1ef82b2

use MatchesPublicKey, don't rewrite it.

cd32e17

test floodsub with message signing

4addc89

reify key extraction logic, with more context in error messages

777c68f

prefix signed data with libp2p-pubsub:

9fa8f64

strict mode for message signing

3788f50

more idiomatic message copying for signature purposes

b353ddc

use the ID corresponding to signKey for published messages

bd15183

try to extract the key in order to decide whether to attach the key

4dc7963

vyzo force-pushed the feat/signing branch from c4b3bd1 to 4dc7963 Compare October 13, 2018 23:23

Stebalien approved these changes Oct 13, 2018

View reviewed changes

vyzo merged commit 4eb6b7c into master Oct 13, 2018

ghost removed the in progress label Oct 13, 2018

vyzo deleted the feat/signing branch October 13, 2018 23:42

vasco-santos mentioned this pull request Nov 20, 2020

Rendezvous message signing libp2p/specs#303

Open

Implement message signing #97

Implement message signing #97

Conversation

vyzo commented Aug 26, 2018 • edited

vyzo commented Aug 27, 2018

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

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

whyrusleeping commented Aug 27, 2018

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

mhchia Aug 28, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyzo commented Aug 28, 2018

bigs left a comment

Choose a reason for hiding this comment

Stebalien Sep 4, 2018 • 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

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

vyzo Sep 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyzo commented Sep 5, 2018 • 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

vyzo commented Sep 6, 2018

vyzo commented Sep 6, 2018

vyzo commented Oct 13, 2018

vyzo commented Aug 26, 2018 •

edited

mhchia Aug 28, 2018 •

edited

Stebalien Sep 4, 2018 •

edited

vyzo Sep 5, 2018 •

edited

vyzo commented Sep 5, 2018 •

edited