Introduce basic network Identity package by rargulati · Pull Request #82 · threshold-network/keep-core

rargulati · 2018-04-24T18:57:56Z

"My Name Is" is a song by American rapper Eminem from his major-label debut album The Slim Shady LP (1999). The song samples Labi Siffre's 1975 track "I Got The...". The song was ranked at #26 on "VH1's 100 Greatest Songs of the '90s".[2] "My Name Is" was also ranked #6 on Q Magazine's "1001 Best Songs Ever". [3] The song was placed at number 39 by Rolling Stone on their list of "100 Greatest Hip-Hop songs of all time" in April 2016.[4] The recording garnered Eminem his first Grammy Award for Best Rap Solo Performance at the 42nd Grammy Awards in 2000.

More at https://en.wikipedia.org/wiki/My_Name_Is

The goal behind net.Identity is to expose a minimal subset of network
level identifiers for external consumption (ie, in registering the
association between a network and protocol identifiers).

We identifiy peers within the libp2p network by their peer.ID and
ed25519 public keys. These public keys are generated from the
corresponding private keys.

This PR attempts to focus the work started in #69

TODO:

Tests
Address any review

rargulati

Should the implementation be in a file in net rather than identity? I'm leaning towards yes.

rargulati · 2018-04-24T20:39:57Z

+}
+
+func pubKeyToID(pub ci.PubKey) peer.ID {
+	// From go-libp2p-peer: PKI-based identities for libp2p


rargulati · 2018-04-24T20:40:14Z

+
+func (pi PeerIdentity) addIdentityToStore() (pstore.Peerstore, error) {
+	ps := pstore.NewPeerstore()
+	// HACK: see github.com/rargulati/go-libp2p-crypto for fix


no longer true, remove

Shadowfiend · 2018-04-25T15:23:54Z

Hmm… Not sure I follow putting it into a net.go file?I think the base interfaces/structures/functions live in a packagename/packagename.go file (we actually need to move net/interface.go to net/net.go), and if there's additional functionality that makes sense to break out we put it in named files under packagename/. For an example of my thinking, see https://github.com/keep-network/keep-core/tree/6c1f05c49bccbd33c1f5118c4e126bd9f2bbe0c6/pkg/beacon/relay/dkg, with dkg.go implementing the main DKG functionality, and sibling files like message.go and marshaling.go dealing with supporting functionality.

rargulati · 2018-04-25T16:13:29Z

So you're saying rather than make this an identity package just do this as net/identity.go?

Shadowfiend · 2018-04-25T21:30:49Z

I just realized I actually misread your comment earlier. Like, really really badly misread. Don't know what I was thinking.

I was saying if we've got an identity package, the file should probably be identity/identity.go---which is where it is hehe.

I don't think the implementation should be in net, but rather in net/libp2p.

Shadowfiend · 2018-04-25T22:15:53Z

 }
+
+// An ID corresponds to the identification of a member in a peer-to-peer network.
+type ID peer.ID


Rule of thumb: if we're importing libp2p into the net package, we're leaking our transport layer.

I think this needs to be some variation of what my branch calls ClientIdentifier (which is a bad name). Let me try and get that branch up so we can talk more concretely.

This could have been a string or interface{}, but I really, really hated the idea of adding more conversion code. I get that I have to do that though ha.

PR is up @ #83.

Shadowfiend · 2018-04-25T22:16:24Z

+// member will generate or provide a keypair, which will correspond to a network
+// ID. Consumers of the net package require an ID to register with protocol level
+// ID's, as well as a public key for authentication.
+type Identity interface {


I do think this will need to be out here. The form may be slightly different.

Not convinced we need this as an exported thing now that we have net.TransportIdentifier. The question might end up being about consumers though: who calls ID and PubKeyFromID and what do they do with it? I think it might be best to move this into libp2p.go until we see a need for pulling it out.

Shadowfiend · 2018-04-25T22:17:11Z

-}
-
-type PeerIdentity struct {
+type peerIdentity struct {


👏 to making this unexported.

rargulati · 2018-04-28T00:32:19Z

Should I move all of the code to net/libp2p.go or just the implementation of id.ProviderName() in net/libp2p.go and leave the rest in identity/. Basically figuring out if I need to nuke the identity package and move the stuff in there to net and net/libp2p.go.

Shadowfiend · 2018-04-30T16:17:31Z

Assuming we still mean net/libp2p/*, I think if you're wondering it's safest to start by sticking it all the libp2p package. If we see that the identity stuff is big enough to be its own thing, it can graduate to a separate package, probably under libp2p/. Right now I don't really see the need to expose identity outside of libp2p-land, because I'm not seeing any other consumers quite yet (this may change).

Shadowfiend

Other than the question about Identity, I feel like this is ready to be merged and iterated on. Some things will become clearer as we start actually writing the code that connects to libp2p and opens/joins a broadcast channel.

Shadowfiend · 2018-04-30T19:25:58Z

+// member will generate or provide a keypair, which will correspond to a network
+// ID. Consumers of the net package require an ID to register with protocol level
+// ID's, as well as a public key for authentication.
+type Identity interface {


Not convinced we need this as an exported thing now that we have net.TransportIdentifier. The question might end up being about consumers though: who calls ID and PubKeyFromID and what do they do with it? I think it might be best to move this into libp2p.go until we see a need for pulling it out.

Shadowfiend · 2018-04-30T19:26:29Z

+
+// LoadOrGenerateIdentity allows a client to provide or generate an Identity that
+// will be used to reference the client in the peer-to-peer network.
+func LoadOrGenerateIdentity(randseed int64, filePath string) (net.Identity, error) {


I would see if this and AddIdentityToStore can be unexported (might not be the case).

The goal behind `net.Identity` is to expose a minimal subset of network level identifiers for external consumption (ie, in registering the association between a network and protocol identifiers). We identifiy peers within the libp2p network by their peer.ID and ed25519 public keys. These public keys are generated from the corresponding private keys.

The interface now resides in net/interfaces.go (along with other net interfaces).

The identity package is the local implementation of net.Identity. This is expressed as `peerIdentity`. This allows consumers to generate a `identity.peerIdentity` which implements `net.Identity` and gives the client access to adding an identity to a store as well as all the functionality of `net.Identity`.

Simple tests for the local implementation of the interface.

Currently we use go-libp2p-crypto as our underlying PKI lib. This may change in the future as the application becomes more fleshed out.

Prove that we can store and retrieve an identity from an addressbook/store (in this case, go-libp2p-peerstore), and that we can generate a valid public/private key pair that can sign and verify a signature over a message.

Rather than having a separate identity package for the nework, collapse this abstraction directly into the network (as part of the p2p package). Our libp2p implementation will be contained directly within the p2p abstraction (which is a network level abstraction). Our libp2p identity can now be hidden behind this abstraction.

In an effort to ensure our interfaces don't leak information about the underlying transport, we consolidate the implementation under the p2p package with a simpler interface implementation. Furthermore, we remove unnecessary abstractions (intermediate PubKey and ID types) and opt to use strings/[]byte where appropriate as most interfaces will accept, return, or trivial convert between native types.

The Identity interface is no longer needed as it's unnecessary to expose to external consumers. Rather we rely on TransportIdentifier. We modify this interface to have an ID() method which exposes a string.

Refactor the implementation to implement the TransportIdentifier interface as well as modifying methods to use said interface (as well as unexporting methods). Remove the need for net.TransportIdentifier.ID().

rargulati · 2018-05-01T00:37:46Z

Removed the Identity interface.

The casting of the internal type is meh, but the alternative (ie. adding an ID() string method to net.TransportIdentifier) doesn't necessarily make anything better.

Shadowfiend

Haven't taken a deep look at the tests yet, but left some notes and questions.

Shadowfiend · 2018-05-01T01:35:30Z

+)
+
+// Implementation of the TransportIdentifier interface
+type networkID peer.ID


Not sure we need to alias this. The main point of net.TransportIdentifier is that internally we can refer to our type exactly as it is, and when it crosses an interface boundary it can be referenced as a net.TransportIdentifier.

You can't define new methods on a non-local types. Redefining external interfaces in terms of a new interface in a different package is considered illegal:

func (ni peer.ID) ProviderName() string { return "libp2p" }

does not compile

Ah yes, of course… < slaps forehead >

Shadowfiend · 2018-05-01T01:36:16Z

@@ -0,0 +1,102 @@
+package net


This should be in package libp2p, under net/.

Hm, I wish you told me this when it was net/p2p/ to just rename to libp2p ;-P

Shadowfiend · 2018-05-01T01:37:55Z

+// Implementation of the TransportIdentifier interface
+type networkID peer.ID
+
+func (ni networkID) ProviderName() string {


Thoughts on making this *peer.ID (or *networkID if we choose to keep the alias)?

Can make this *networkID, but can't do peer.ID as that would be redefining an interface and type out of its own package.

Shadowfiend · 2018-05-01T01:38:25Z

+// member will generate or provide a keypair, which will correspond to a network
+// ID. Consumers of the net package require an ID to register with protocol level
+// ID's, as well as a public key for authentication.
+type peerIdentity struct {


Confused… What's the difference between this and peer.ID?

This is our internal means of referencing this stuff. I guess I haven't been clear in how this stuff will be used.

My 2¢: if we've got a bundle of things that can produce a peer.ID, we might as well use that as the TransportIdentifier implementor. The code that interacts with TransportIdentifier won't care, as it won't interact with the internals.

Shadowfiend · 2018-05-01T01:39:23Z

+	return pid
+}
+
+func (pi *peerIdentity) PubKeyFromID(id TransportIdentifier) (ci.PubKey, error) {


Is this something we expect someone to invoke outside of the libp2p package?

Shadowfiend · 2018-05-01T01:43:30Z

+
+// loadOrGenerateIdentity allows a client to provide or generate an Identity that
+// will be used to reference the client in the peer-to-peer network.
+func loadOrGenerateIdentity(randseed int64, filePath string) (*peerIdentity, error) {


I think I asked this earlier, but it feels like we should make this decision as far removed from here as possible. Probably in the Connect function or whatever will be bootstrapping the network connection. Having a loadOrGenerateIdentity function that in turn actually can do one of three things (load, generate deterministically, and generate cryptographically secure) is not ideal.

Since there's no code other than tests consuming this, I think we can just lose this function and implement the tests by calling the generate* functions directly.

Shadowfiend · 2018-05-01T01:45:11Z

+}
+
+// generateIdentity generates a public/private-key pair
+// (using the libp2p/crypto wrapper for golang/crypto) provided a reader.


Not really “provided a reader” anymore, right?

Shadowfiend · 2018-05-01T01:45:18Z

+
+// generateIdentity generates a public/private-key pair
+// (using the libp2p/crypto wrapper for golang/crypto) provided a reader.
+// Use randseed for deterministic IDs, otherwise we'll use cryptographically secure psuedorandomness.


pseudo*, but I think this whole line is no longer valid.

rargulati · 2018-05-01T02:00:32Z

I think we're kind of on the same page, but not really. There's been quite a bit of churn on this concept over two PRs. The goal here is to make small slices, and open PRs that consume these concepts, but I see that a lot of context is lost for the reviewer (as I have the context in my head). I'll need to take a different approach.

It will be best if I close this PR. I'll wait for your PRs to get merged in, and then introduce a larger PR that shows all of these concepts as a single unit.

Shadowfiend · 2018-05-01T02:09:25Z

Let's focus on merging #83 at the start of tomorrow, as that's the last PR that touches the network interface in a significant way (there's another branch that adds net.Provider, but that's not crucial right now I think, and you can always steal those commits for your PR if you'd like).

rargulati force-pushed the my-name-is branch from 0c276d6 to 5df3d06 Compare April 24, 2018 20:38

rargulati commented Apr 24, 2018

View reviewed changes

Shadowfiend reviewed Apr 25, 2018

View reviewed changes

rargulati force-pushed the my-name-is branch from d8842b5 to e99eb44 Compare April 27, 2018 01:04

rargulati changed the base branch from master to protocally-identical April 27, 2018 01:04

Shadowfiend suggested changes Apr 30, 2018

View reviewed changes

Raghav Gulati and others added 9 commits April 30, 2018 13:44

Move Identity interface, add documentation

579317c

The interface now resides in net/interfaces.go (along with other net interfaces).

Initial crack at net.Identity tests

954ff6b

Simple tests for the local implementation of the interface.

Add comment for net.PubKey type alias.

5abde2f

Currently we use go-libp2p-crypto as our underlying PKI lib. This may change in the future as the application becomes more fleshed out.

Add basic tests for net.Identity

929a703

Prove that we can store and retrieve an identity from an addressbook/store (in this case, go-libp2p-peerstore), and that we can generate a valid public/private key pair that can sign and verify a signature over a message.

Remove Identity, modify TransportIdentifier

52fd4a5

The Identity interface is no longer needed as it's unnecessary to expose to external consumers. Rather we rely on TransportIdentifier. We modify this interface to have an ID() method which exposes a string.

rargulati force-pushed the my-name-is branch from 19230e2 to 3c3d787 Compare April 30, 2018 22:44

Move p2p to net, update implementation

7962510

Refactor the implementation to implement the TransportIdentifier interface as well as modifying methods to use said interface (as well as unexporting methods). Remove the need for net.TransportIdentifier.ID().

rargulati force-pushed the my-name-is branch from 3c3d787 to 7962510 Compare May 1, 2018 00:35

Shadowfiend suggested changes May 1, 2018

View reviewed changes

rargulati closed this May 1, 2018

rargulati deleted the my-name-is branch May 25, 2018 00:32

Uh oh!

Conversation

rargulati commented Apr 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rargulati left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shadowfiend commented Apr 25, 2018

Uh oh!

rargulati commented Apr 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Shadowfiend commented Apr 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rargulati Apr 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rargulati commented Apr 28, 2018

Uh oh!

Shadowfiend commented Apr 30, 2018

Uh oh!

Shadowfiend left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rargulati commented May 1, 2018

Uh oh!

Shadowfiend left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

rargulati commented Apr 24, 2018 •

edited

Loading

rargulati commented Apr 25, 2018 •

edited

Loading

rargulati Apr 25, 2018 •

edited

Loading