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

Update ipns validator #4597

Closed
wants to merge 4 commits into from
Closed

Update ipns validator #4597

wants to merge 4 commits into from

Conversation

dirkmc
Copy link
Contributor

@dirkmc dirkmc commented Jan 20, 2018

Update go-ipfs to use new validation function format in go-libp2p-record
#4566 was getting messy so I closed and opened this new PR

License: MIT
Signed-off-by: Dirk McCormick <[email protected]>
@dirkmc dirkmc mentioned this pull request Jan 20, 2018
// need to do that here

// Author in key must match author in record
parts := strings.Split(r.Key, "/")
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need this split?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right the relative path component of the IPNS path is not stored in the DHT (only in DNS)

@vyzo vyzo requested a review from Stebalien January 21, 2018 07:10
License: MIT
Signed-off-by: Dirk McCormick <[email protected]>
@vyzo
Copy link
Contributor

vyzo commented Jan 21, 2018

You need to add the new dependencies to package.json using gx update.

@vyzo
Copy link
Contributor

vyzo commented Jan 21, 2018

you also need to go fmt -- codeclimate rightfully complains.

License: MIT
Signed-off-by: Dirk McCormick <[email protected]>
@dirkmc
Copy link
Contributor Author

dirkmc commented Jan 21, 2018

I wanted to ask you guys about the dependencies. I used gx update to update the go-libp2p-record dependency across the project, and it warned me that I would also need to update go-libp2p-kad-dht, but when I did that it broke a lot of things because of mismatches between the library versions that the new go-libp2p-kad-dht lib imports vs the library versions this project imports.

Is there some easy way of doing all this package management stuff so it will figure out what needs to be updated and do it one step?

@vyzo
Copy link
Contributor

vyzo commented Jan 22, 2018

Sounds like some dependency will need to propagage -- @Stebalien usually handles those.

License: MIT
Signed-off-by: Dirk McCormick <[email protected]>
@whyrusleeping
Copy link
Member

We need to at some point write a guide for "propogating gx dependencies"

@dirkmc
Copy link
Contributor Author

dirkmc commented Jan 23, 2018

The gold standard is whether a recovering java dev can understand it

@Stebalien
Copy link
Member

@dirkmc Actually, gx is very Java-esque in it's paranoia. But yes, this is blocked on the cmds PR #4568.

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.

LGTM. We'll have to hold for the cmds merge.

if err != nil {
return ErrInvalidAuthor
}
if string(pid) != string(r.Author) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we actually need to call string here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, we should be able to get rid of that cast once the go-libp2p-peer lib is updated

Copy link
Member

Choose a reason for hiding this comment

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

Got it.

@dirkmc
Copy link
Contributor Author

dirkmc commented Jan 24, 2018

Once the cmds merge is complete I will update the deps and test manually

@Stebalien Stebalien mentioned this pull request Jan 25, 2018
1 task
@Stebalien
Copy link
Member

So, unfortunately, this actually breaks things (the republisher test cases) because we allow publishing IPNS records using one key and then signing the DHT record with another. For example, when publishing an IPNS record with an alternative key, we sign it with the alternative key and then sign the DHT record with the peer key.

The correct solution here is to:

  1. Get rid of the DHT signature/author field. We can actually do this without breaking anything.
  2. Validate the IPNS (not DHT) signature in the validator.

Unfortunately, validating the IPNS signature in the validator is a bit tricky... so I'm not going to fix that in my existing pr (#4610).

@Stebalien Stebalien mentioned this pull request Jan 26, 2018
@dirkmc
Copy link
Contributor Author

dirkmc commented Jan 26, 2018

That makes sense, if there's a peer ID in the key there's no need to replicate it in the DHT record.
So are you imagining that the DHT would create a map[peer.ID]PubKey from the peer store and pass that to the validator function?

@Stebalien
Copy link
Member

So are you imagining that the DHT would create a map[peer.ID]PubKey from the peer store and pass that to the validator function?

No, we'll probably construct the IPNS validator as a closure and have it capture the peerstore (so it can extract the keys from that).

Ideally, the validator would take some additional "validation" information but we'll probably need to wait for a general-purpose IPRS for that.

@dirkmc
Copy link
Contributor Author

dirkmc commented Jan 27, 2018

Makes sense. Should we close this PR now that it's superseded by #4613?

@Stebalien
Copy link
Member

Yes.

@Stebalien Stebalien closed this Jan 28, 2018
@dirkmc dirkmc deleted the fix/namesys-vldn branch January 31, 2018 03:34
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