-
Notifications
You must be signed in to change notification settings - Fork 101
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
Implemented byte decoder for onion addresses. Corrected onion bit size. #21
Conversation
@@ -166,6 +166,7 @@ func TestBytesToString(t *testing.T) { | |||
testString("/ip4/127.0.0.1/udp/1234", "047f0000011104d2") | |||
testString("/ip4/127.0.0.1/tcp/4321", "047f0000010610e1") | |||
testString("/ip4/127.0.0.1/udp/1234/ip4/127.0.0.1/tcp/4321", "047f0000011104d2047f0000010610e1") | |||
testString("/onion/aaimaq4ygg2iegci:80", "bc030010c0439831b48218480050") |
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 port? is it always strictly tcp? or is it an onion port? (meaning the transport is abstracted out)
If the client has to know the protocol, then I would imagine this being better: /onion/<hash>/tcp/80
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.
cc @david415 as he knows more about this than me. we were discussing elsewhere
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 would say separating out the port is better too, but I followed what the existing partial implementation was already doing. My understanding is that it is tcp.
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 it ALWAYS tcp? where can we get confirmation of this?
- even if it IS always is TCP today, does it make sense to list it so that we can plug other transports in over tor? (sctp, quic, etc)
- this is external-facing, right? the other side gets the port too?
- if so, does this count as leaking identifying information? port numbers could be correlated. most onion sites are
:80
(afaik), and port numbers are hard, but still. it is some info. not sure how much worse than response timing ...
could really use others' viewpoints on this. cc @david415 @leif
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.
17:41 <•jbenet> ianopolous: if you need a decision now, i think the port should be layered.
17:42 <•jbenet> so "/tcp/<port>" not ":<tcp-port>"
17:42 <•jbenet> which -- now that i think about it -- may already be what's there now?
@jbenet @ianopolous strictly speaking the port number is NOT a TCP port as Tor is a stream transport... and currently it does in fact support UNIX domain sockets for onion service applications... meaning that if you wanted to sandbox a tor onion service application you could remove it's access to the tcp/ip stack and only allow to use a particular UNIX domain socket to receive it's incoming onion service connections. |
likewise when clients talk to the tor onion service they do not have to utilize a TCP/IP stack at all... and can instead utilize a UNIX domain socket to make the SOCKS connection to tor. |
therefore the dialing client needs a tor control port endpoint (either tcp/ip or unix domain socket file) and 96 bits of addressing information (onion address 80 + port number 16). the listening server needs the tor control port endpoint AND a 1024 bit RSA private key. the current go-libp2p API suggests that we should put all this information in the onion multiaddr. |
it is correct that the onion address and port be located in the same layer of multiaddr specification. what does NOT make sense is to have a TCP port on a onion service! where's the TCP? Tor is a stream transport not a TCP transport. please either merge this code or the other PR ;-p |
Hey @ianopolous and @david415 thanks for hashing this out. this looks good to me. I'm going to merge this one because it has the error thing, fixes the protocols.csv too, and is more exact on the slices. @ianopolous can you rebase? it's no longer mergeable cleanly, sorry for the delay... |
@ianopolous also change the multiaddr csv in https://github.com/multiformats/multiaddr |
Sure, no problem. I'm away until Monday though, so will be after then. |
ok... looks like you need to update your branch before we can merge by clicking the github merge button. ;-p |
@jbenet rebase done |
can we please get this merged soon? |
Merged, thank you! |
Thank you for all the hard work on this-- and for your patience! ❤️ 🎉 |
The onion size is 96: the address 80 and the port number 16 combined. This is what it says in [multiformats/multiaddr's list](https://github.com/multiformats/multiaddr/blob/master/protocols.csv#L13), and [here](multiformats/go-multiaddr#21 (comment)) See multiformats/go-multiaddr@e32e9cd
The onion size is 96: the address 80 and the port number 16 combined. This is what it says in [multiformats/multiaddr's list](https://github.com/multiformats/multiaddr/blob/master/protocols.csv#L13), and [here](multiformats/go-multiaddr#21 (comment)) See multiformats/go-multiaddr@e32e9cd
The onion size is 96: the address 80 and the port number 16 combined. This is what it says in [multiformats/multiaddr's list](https://github.com/multiformats/multiaddr/blob/master/protocols.csv#L13), and [here](multiformats/go-multiaddr#21 (comment)) See multiformats/go-multiaddr@e32e9cd
dep: import go-libp2p-mplex into the libp2p org
No description provided.