namesys: verify signature in ipns validator #4628

dirkmc · 2018-01-31T03:36:23Z

No description provided.

Stebalien

Other than a couple of nits, awesome!

Stebalien · 2018-01-31T04:13:07Z

namesys/routing.go

-		return "", fmt.Errorf("ipns entry for %s has invalid signature", h)
+	// use the routing system to get the name.
+	_, ipnsKey := IpnsKeysForID(pid)
+	val, err := r.routing.GetValue(ctx, ipnsKey)


Probably a good idea to put a comment here stating that the DHT record validator validates the signature.

Stebalien · 2018-01-31T04:19:56Z

namesys/validator.go

+		// Check the ipns record signature with the public key
+		if ok, err := pubk.Verify(ipnsEntryDataForSig(entry), entry.GetSignature()); err != nil || !ok {
+			log.Debugf("failed to verify signature for ipns record %s", r.Key)
+			return ErrSignature


I'd return a different error for each of these cases. As-is, this may make it difficult to debug issues.

Are you saying a different error for the ok and err return values of pubk.Verify()

Or are you saying a different error for the return values from
peer.IDFromString(r.Key)
kbook.PubKey(pid)
pubk.Verify()

Just different error messages (instead of always using ErrSignature). That way, we can distinguish between, e.g., not having the key and failing to validate the signature.

Makes sense

Stebalien · 2018-01-31T04:23:22Z

namesys/selector.go

+		} else if r.GetSequence() == bestSeq {
+			rt, err := u.ParseRFC3339(string(r.GetValidity()))
+			if err != nil {
+				continue


I'd at least log an ERROR in these invalid cases. Ideally, we'd panic but let's leave it as a noisy log for a while before we do something like that.

vyzo · 2018-01-31T09:25:52Z

namesys/publisher.go

@@ -208,7 +194,7 @@ func PublishEntry(ctx context.Context, r routing.ValueStore, ipnskey string, rec
 	}

 	log.Debugf("Storing ipns entry at: %s", ipnskey)
-	// Store ipns entry at "/ipns/"+b58(h(pubkey))
+	// Store ipns entry at "/ipns/"+h(pubkey)


wait, has this changed?

I didn't change the relevant code, I just noticed that the comment was inaccurate.
It seems like the DHT key is "/ipns/" + string(<multihash>)
Note that in the IPNS path exposed to the user the hash is B58 encoded. The resolver decodes it to get the DHT key.

ok, cool. yeah, let's fix the comment if it's inaccurate.

vyzo · 2018-01-31T09:31:08Z

namesys/selector.go

+		if besti == -1 || r.GetSequence() > bestSeq {
+			bestSeq = r.GetSequence()
+			besti = i
+		} else if r.GetSequence() == bestSeq {


Is the else a bug here? you still want to parse when you see a higher sequence number.

I didn't write this code, I just moved it from publisher.go into its own file, so I'm not 100% sure what the original intent was here. But I believe it's just trying to find the index of the best record, not to parse the record. Am I understanding your question correctly?

Well it's not clear -- it seems to intent to parse the record with the best sequence.
The problem with the else is that if a new higher sequence is found, then it will be set as best signature but will not be parsed. and that's inconsistent.
I believe that's a bug for the intent of the code to parse the record with the best sequence number, it will only parse the first one!
If we remove the else and move the if on its own line below it would be consistent.

As I understand it the validator parses the record and checks that the EOL is in the correct format and has not expired. The selector just needs to pick the "best" record. In this code it chooses the best record as the record with

the highest sequence number

if sequence numbers are equal then

the highest expiration

if expiration is equal then

the highest by byte compare (so that the order is always consistent)

Having said that I think you're right that if the EOL cannot be parsed from the record validity then that record should not be chosen as the best record, so it might be worth adding that check in there.

@whyrusleeping what do you think?

So, we could avoid that by parsing the validity in the first condition (when we haven't selected a record yet). However, this case should never happen. That's why I suggested printing an error for now and eventually upgrading it to a panic.

@dirkmc I'm fine with this as-is but if you'd like to add that that extra validity check, feel free and tell me when you're comfortable with having this merged.

I added it in, seems like the right thing to do, thanks @Stebalien

And thanks @vyzo for pointing it out :)

vyzo · 2018-01-31T09:32:19Z

namesys/validator.go

+
+// NewIpnsRecordValidator returns a ValidChecker for IPNS records
+// The validator function will get a public key from the KeyBook
+// to verify the record's signature


can we add a comment that the public key has already been fetched?

Stebalien · 2018-01-31T19:37:56Z

namesys/selector.go

+		if besti == -1 || r.GetSequence() > bestSeq {
+			bestSeq = r.GetSequence()
+			besti = i
+		} else if r.GetSequence() == bestSeq {


So, we could avoid that by parsing the validity in the first condition (when we haven't selected a record yet). However, this case should never happen. That's why I suggested printing an error for now and eventually upgrading it to a panic.

whyrusleeping · 2018-02-03T23:32:52Z

@dirkmc mind rebasing this one for us?

dirkmc · 2018-02-04T03:27:49Z

Sure, rebase is at #4648

vyzo · 2018-02-04T08:30:45Z

@dirkmc there is no reason to open a new pr, you can force push the rebase on this one!

dirkmc · 2018-02-04T16:44:18Z

Ah ok I wasn't sure if that would mess up the history, let me give it a shot

dirkmc · 2018-02-04T17:21:04Z

Looks like that worked, thanks @vyzo

License: MIT Signed-off-by: Dirk McCormick <dirkmdev@gmail.com>

Stebalien · 2018-02-07T22:46:27Z

@whyrusleeping I believe this is ready to be merged. We don't have to bubble libp2p/go-libp2p-record#15 first. This doesn't yet remove signatures from DHT records but fixes the validation of IPNS records (stops signing them at the DHT record level and starts validating the IPNS signature).

dirkmc mentioned this pull request Jan 31, 2018

Remove DHT signatures #4613

Closed

Stebalien requested changes Jan 31, 2018

View reviewed changes

vyzo reviewed Jan 31, 2018

View reviewed changes

Stebalien approved these changes Jan 31, 2018

View reviewed changes

dirkmc mentioned this pull request Feb 4, 2018

namesys: verify signature in ipns validator (rebase) #4648

Closed

dirkmc force-pushed the fix/namesys-verify-pubk branch from a584eb0 to 95d5705 Compare February 4, 2018 16:48

dirkmc force-pushed the fix/namesys-verify-pubk branch from 95d5705 to b3636de Compare February 7, 2018 20:48

dirkmc added 6 commits February 7, 2018 16:18

namesys: verify signature in ipns validator

5e8c948

License: MIT Signed-off-by: Dirk McCormick <dirkmdev@gmail.com>

Code cleanup

dafa140

License: MIT Signed-off-by: Dirk McCormick <dirkmdev@gmail.com>

Comment fixes

e980e68

License: MIT Signed-off-by: Dirk McCormick <dirkmdev@gmail.com>

namesys: differentiate between validation errors

dbedee5

License: MIT Signed-off-by: Dirk McCormick <dirkmdev@gmail.com>

namesys: more comments

84fe582

License: MIT Signed-off-by: Dirk McCormick <dirkmdev@gmail.com>

namesys: discard records with invalid EOL in record selection

7b99538

License: MIT Signed-off-by: Dirk McCormick <dirkmdev@gmail.com>

dirkmc force-pushed the fix/namesys-verify-pubk branch from b3636de to 7b99538 Compare February 7, 2018 21:19

go fmt

f829093

License: MIT Signed-off-by: Dirk McCormick <dirkmdev@gmail.com>

dirkmc force-pushed the fix/namesys-verify-pubk branch from 6cf4699 to f829093 Compare February 7, 2018 21:28

Stebalien approved these changes Feb 7, 2018

View reviewed changes

whyrusleeping merged commit 9014c64 into ipfs:master Feb 7, 2018

dirkmc deleted the fix/namesys-verify-pubk branch February 8, 2018 00:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

namesys: verify signature in ipns validator #4628

namesys: verify signature in ipns validator #4628

dirkmc commented Jan 31, 2018

Stebalien left a comment

Stebalien Jan 31, 2018

Stebalien Jan 31, 2018

dirkmc Jan 31, 2018

Stebalien Jan 31, 2018

dirkmc Jan 31, 2018

Stebalien Jan 31, 2018

vyzo Jan 31, 2018

dirkmc Jan 31, 2018

vyzo Jan 31, 2018

vyzo Jan 31, 2018 •

edited

Loading

dirkmc Jan 31, 2018

vyzo Jan 31, 2018 •

edited

Loading

dirkmc Jan 31, 2018

Stebalien Jan 31, 2018

Stebalien Feb 7, 2018

dirkmc Feb 7, 2018

dirkmc Feb 7, 2018

vyzo Jan 31, 2018 •

edited

Loading

Stebalien Jan 31, 2018

whyrusleeping commented Feb 3, 2018

dirkmc commented Feb 4, 2018

vyzo commented Feb 4, 2018

dirkmc commented Feb 4, 2018

dirkmc commented Feb 4, 2018

Stebalien commented Feb 7, 2018

namesys: verify signature in ipns validator #4628

namesys: verify signature in ipns validator #4628

Conversation

dirkmc commented Jan 31, 2018

Stebalien 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

vyzo Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyzo Jan 31, 2018 • edited Loading

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 Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whyrusleeping commented Feb 3, 2018

dirkmc commented Feb 4, 2018

vyzo commented Feb 4, 2018

dirkmc commented Feb 4, 2018

dirkmc commented Feb 4, 2018

Stebalien commented Feb 7, 2018

vyzo Jan 31, 2018 •

edited

Loading

vyzo Jan 31, 2018 •

edited

Loading

vyzo Jan 31, 2018 •

edited

Loading