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
feat: initial implementation #1
feat: initial implementation #1
Changes from 2 commits
83323ca
0ddd505
6566eb5
d1a0b6e
c7e5892
bc873f6
4b96c4a
5f36c1a
77c9330
999ba3c
fab0c5b
308b83a
00b6c7c
f4b98ac
66744f1
e03e370
26046df
f2942d8
c34b6bb
f494051
c6feb2a
1e262a8
c55d539
2d2f576
e91c9c0
0d9c6f8
9779c7c
4c9dc3e
65b05b8
02e34f0
db86cf2
2020f9c
6fff08e
6b008c3
39a5308
b06ddb0
c9b535f
5011943
d25be74
ae0139e
eef1770
7fd1cfb
a0ee13f
709fad7
f9d8f07
d060dac
4c9cc8a
587c7fa
dbeb701
2ecd19a
11dda24
4db6b55
65145f8
d6c1f74
97f25ad
f599f48
2595bf8
1360e56
78c3ae0
688ddf7
4468ad5
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.
Note on why JWT did not feel great because
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.
Is this meant to follow RFC 6750? The "Bearer" Auth scheme is registered to that RFC https://www.iana.org/assignments/http-authschemes/http-authschemes.xhtml.
I haven't looked closer here, because I'm assuming we'll opt to use the HTTP Peer ID authentication flow instead.
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.
Note: We could've avoided doing anything special with the auth if we'd relied on libp2p doing the auth for us.
About the protocol format:
About using libp2p for the transport:
/dnsaddr/registration.libp2p.direct
without a peerID suffix) which seems fine (even more so with DNSSEC), although a bit of extra wrapper code likely needed in each implementation since IIRC it's not supported out of the box in most implementationsIt'd be nice to evolve this into being able to do HTTP over libp2p and drop the auth header by doing one or both of:
The result means there'd be very little custom code here at all given the user is already leveraging a libp2p implementation.
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.
Both of these seem like desire-able things that we may want at some point in the future. Is it worth investigating now before we implement some other solution that quickly goes out of date?
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 doesn't seem particularly desirable to bother with.
WDYT @MarcoPolo, does it make more sense to land the spec then do this business with the signed envelopes?
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.
Yes. The spec is very close to being ready. And if we have a use case, then I'd be very motivated to get it landed.
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 see that we're essentially reusing the acme challenge as the authentication challenge. This is probably (?) okay for this use case, but I think it would better practice to not reuse challenges in this way. The HTTP Peer ID Auth flow would use a challenge just for the authentication (and that's where the extra round trip comes from).
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.
It seemed like grabbing this spec meant less overhead for implementers given that those using signed peer records (i.e. for gossipsub which is in most implementations) already do most of this. However, as can be seen from the Go code here go-libp2p hid most of the internals away such that actually getting at them was painful and/or required copy-pasting.
Alternatives welcome (including if it's just tossing all this and landing the HTTP PeerID Auth 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.
I wasn't planning on using a payload type and registering it in the codec table, however:
/
is valid as an escape for a user defined string without relying on the table that works for me/
is fine and that it's also ok to leave it empty https://github.com/libp2p/specs/blob/master/RFC/0002-signed-envelopes.md#payload-type-information the Go implementation disagrees and has tests to enforce this https://github.com/libp2p/go-libp2p/blob/a0349afb888abafcf7991c183197859759134dfc/core/record/envelope_test.go#L145There 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 @sukunrt might have thoughts on ^
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 we should just define the interface we actually need in this package and not require a full Datastore implementation.
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.
check err?