Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

namesys: verify signature in ipns validator #4628

Merged
merged 7 commits into from
Feb 7, 2018

Conversation

dirkmc
Copy link
Contributor

@dirkmc dirkmc commented Jan 31, 2018

No description provided.

@dirkmc dirkmc mentioned this pull request Jan 31, 2018
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than a couple of nits, awesome!

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait, has this changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@vyzo vyzo Jan 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@vyzo vyzo Jan 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And thanks @vyzo for pointing it out :)


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

@vyzo vyzo Jan 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

@dirkmc mind rebasing this one for us?

@dirkmc
Copy link
Contributor Author

dirkmc commented Feb 4, 2018

Sure, rebase is at #4648

@vyzo
Copy link
Contributor

vyzo commented Feb 4, 2018

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

@dirkmc
Copy link
Contributor Author

dirkmc commented Feb 4, 2018

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

@dirkmc
Copy link
Contributor Author

dirkmc commented Feb 4, 2018

Looks like that worked, thanks @vyzo

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

@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).

@whyrusleeping whyrusleeping merged commit 9014c64 into ipfs:master Feb 7, 2018
@dirkmc dirkmc deleted the fix/namesys-verify-pubk branch February 8, 2018 00:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants