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
add HTTP spec #508
add HTTP spec #508
Changes from 23 commits
bc1aa59
1f075f6
12f86b8
146c09a
5398f5d
b6c1bc2
8a57943
d506145
946f516
3681472
dd5d07c
46d1857
ebe612c
7e5a077
db2b3b5
6319458
c7c9c43
454e25c
a25267b
3014b22
f96359b
1e87960
d0f0d93
8fbd64a
71415b0
4a03bb0
877899d
dc71f2c
d8850aa
78e8ca1
d30efda
8628b5a
3c0ac40
75bc635
f95e4db
e3eb9dc
95ffe6d
8f44d00
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.
h2c to show how it can be multiplexed and negociated in many ways (header compression, binary based protocol, ...) ?
Feel free to ignore this comment if you think it's making the graph too complex.
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.
So this is a bit confusing to me. Above you are saying the you generally refer to HTTP semantics and the next sentence says that a goal is to interoperate with existing HTTP servers and clients which refers to the transport, correct?
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.
Refers to both actually
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.
If I understand correctly, this only specifies the path but not the method (GET / POST) to use when accessing this protocol over HTTP and that's up to the specific protocol to define how to run it over HTTP?
If so, should we add this explainer in the spec?
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.
Hm.. the methods will be specific to each protocol at each mount point, so not part of this spec?
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.
That makes sense. As I see it there are two sections in this document:
So should we mention it in the specs that a libp2p protocol supporting http transport should specify the http method and headers to be used for the protocol. For the path they can expose it via the wellknown endpoint
/.well-known/libp2p/protocols
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'm not sure I understand. An application protocol would be built using HTTP semantics, and that protocol would then be able to run on libp2p streams or "standard" http transports like h2, h3.
What do you mean by:
This spec does not define how you would take an existing libp2p protocol and map it to HTTP semantics. That is best done by the specific protocol itself. But maybe I'm misunderstanding your point?
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.
We might run into the HTTP header size limit here (e.g. for HTTP/3: https://datatracker.ietf.org/doc/html/rfc9114#section-4.2.2), especially when using PQ keys.
Would it make sense to use a separate HTTP endpoint here, and do the handshake using the body of a request / response to that endpoint?
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.
How big are pq keys? It's pretty nice and ergonomic to do auth in the headers because then standard stuff like this works:
Or even middlewares:
A survey shows that many support at least 8KB in headers.
We could only support the libp2p-token in the auth header, and do the the noise handshake on a dedicated endpoint. That would mean we wouldn't be able to piggyback on a request to do auth. It would also mean that doing something with client auth is always a 2 step process. It also adds a bit of complexity I think since there are two flows for auth, instead of everything happening in the headers.
If we can stay below the 8KB limit, which may be possible I think sticking with the headers for everything would be nicer.
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.
Don't we have to define the code point of this token? I am not too familiar with the design intentions around our noise extension registry but from an implementer PoV, I wouldn't know at the moment, how to extract this token.
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.
Yep, I just pushed the relevant changes to the noise spec. It adds two new optional pb fields