Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Exploratory refactor-2 of libp2p + HTTP #103
Exploratory refactor-2 of libp2p + HTTP #103
Changes from all commits
bc6a41f
3843e8b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be is obvious, but I forgot to note this here: In production this MUST be an
https
endpoint. It's not here to make testing a bit easier.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can probably be a constant value here. It is simply some identifier for this protocol. Maybe
/ipni-sync/1
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the best go-libp2p api to expose optional server authentication to users?
My original idea was to determine if a user wanted to authenticate the server if
peerInfo.ID
was set. The problem is that this can be very annoying to users because:* A user might not know if the multiaddr they are passing is an HTTP transport or Stream transport.
* A user might be passing multiple multiaddrs (both an HTTP transport and a stream transport), and stream transports require a server ID (libp2p streams MUST be authenticated). In the case that a user doesn't need server peer id auth, they have no way of expressing that. They'll pay for HTTP server auth because they passed it in (since it is required for the stream transports).
Now I'm thinking we introduce a new option to the RoundTripper/http.Client functions. Something like
ServerMustAuthenticate
. Users would pass this option to tell the http host that it MUST authenticate the server (either via HTTP Peer ID Auth or stream auth). If this is not passed in, we can assume the client doesn't care if the server's peer id has been authenticated and we can ignore the passed inpeerInfo.ID
.Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think having an option to allow authentication to be bypassed works. This is also not an unfamailiar thing to do, as it is similar to
tls.Config.InsecureSkipVerify
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note, if we see an
httpath
component we can tell the http host about this mapping viahttpHost.AddPeerMetadata
. That would save us a round trip for.well-known/libp2p
if we have it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MarcoPolo This would work for the
/head
path since it does not change (i.e. is well-known), but will not work for all the advertisement and entries paths, since they all end with a CID that is almost always different. It would be awesome if there were a way to match well-known path prefixes (e.g./ipni/v1/ad/
) to a protocol ID. Then the cache could do a longest prefix match to lookup the metadata.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm misunderstanding something. In the current version of master we have the notion of a syncer's rootURL. This represents the prefix that the IPNI protocol is mounted at. If we want to GET the head we do
rootURL + "/head"
and if we want a cid we dorootURL + cid.String()
.I'm noting here that we could save a round trip to .well-known/libp2p telling us the prefix by using the rootURL. We would do something like
clientHost.AddPeerMetadata(peer.id, libp2phttp.WellKnownProtoMap{"/ipni/v1": rootURL}
.You probably need to do this for backwards compatibility as well as an optimization, since existing deployed publishers won't have set up a
/.well-known/libp2p
endpoint yet.I don't quite understand why you'd want to do a prefix match, what am I missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am probably not full understanding the correct use. The code immediately following this comment is my attempt to use this feature. Maybe I need to omit the "head" or any other thing following the root prefix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay. I think I see. You don't actually need to do any of the below. The path in
WellKnownProtocolMeta
is the path prefix for an application protocol. So for IPNI it would be something like/ipni/1
(or just/ipni
). It's the path prefix that defines where this application protocol is mounted on the server. Let me create a patch of what I mean.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://gist.github.com/MarcoPolo/30c5c138a126473a949ab1879c16c37a
We learn about this prefix out-of-band by using the
httpath
in the multiaddr if it's present.