-
Notifications
You must be signed in to change notification settings - Fork 961
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
transports/onion: Add dial-only implementation of Transport
#2899
Conversation
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.
Documentation is missing, but I will add that, once the code itself is approved.
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.
Did an initial pass, thanks for tackling this!
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.
Thanks! I've left a few more comments with a mix of nits and other feedback :)
Very happy to see this happening! Thanks @umgefahren for the work and thanks @thomaseizinger for the reviews. Please ping me in case you need my input anywhere. |
Cross-referencing related effort on the nim-libp2p side: vacp2p/nim-libp2p#765 //CC @Menduist and @diegomrsantos |
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.
Thanks for the continued work here 😊
I left a few more comments. Looking forward to merging this!
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.
Thanks!
Let me know what you think of the commit I pushed. A few minor doc fixes, otherwise happy to merge this :)
Would you mind doing a smoke test with the example to check that we aren't shipping something outright broken?
It seems we have a CI failure from |
Interoperability tests are back to green with libp2p/test-plans#60. |
On what in particular? Removing the licence headers in each file? |
No. Regarding the two crates with licensing concerns. |
I'd be hesitant to merge this code into the main repository, for multiple reasons:
Merging a Tor transport into a PL-maintained libp2p implementation sends a signal that this transport is properly tested and vetted, no matter how many disclaimers we include. As a modular stack, it should be possible to have this code live outside of rust-libp2p (and outside the libp2p org). This would make it possible for people to play around and experiment with this code, without giving it the blessing it would receive from being a part of rust-libp2p. |
…to comply to the new doc_auto_cfg policy
Adding more context here. The ask for a Tor transport is as old as IPFS / libp2p itself. See for example this discussion from 2016 (!): libp2p/go-libp2p#79 |
I'll reply to your points from nim-libp2p point of view, since we'll soon merge our Tor transport. As always, you are free to do want you want, I'm just giving another perspective :)
EDIT: and just to clarify: the goal of the Tor transport is not to say "you just enable this flag and your application becomes magically private", it's here to say "if you are building an application with privacy in mind, you can use this flag as one of the tool used to keep it secure". But it's so easy to leak informations at every layer, the whole application has to be built with privacy in mind |
We are considering:
|
First, I want to thank for all the thoughts and comments on this PR. I see pretty much every point made here, but I have some additional things to say, and I want to offer a way to go forward. Regarding @marten-seemann comments:
Regarding @Menduist:I pretty much agree with all you said. I think we in this repository should look at methods on how we can steer behaviors towards using a given transport and also take a look at nim-libp2p here. But I think this is beyond the scope of this PR. I will open an issue about it, as soon as this is merged. Finally, I want to leave some comments of my own here. The reason why I wrote this transport was because I’m really interested in bringing transport level privacy to libp2p. The new iteration of the Dione project I wrote would benefit heavily from it and I could think of a lot of other applications where transport level privacy would be good. I recognize it will be quite hard to properly integrate that into libp2p, but we should do it anyway. I would also recommend looking into writing a generic (in customizable) but tested and capable onion router as libp2p behavior and transport. I would say it’s well in the scope of the project and very doable. In fact, there are Onion Routers written with libp2p (i.e. hoprnet). RecommendationsIf it’s fine by @mxinden and @thomaseizinger, I’m still in favor of merging here, although I would like to take some steps previously.
We should monitor how this transport develops and is being used. Throwing it out is of course always an option. However, I would do that, once we have better alternatives, transport level privacy seems to be a concern to some people. |
The requests for Tor support in libp2p / IPFS are as old as the project itself. Over the years, the IPFS and libp2p team has repeatedly decided to not merge any Tor transport into the main project. Where is the push to merge this into the main repository coming from? Why does this transport need the implicit blessing that comes with living in the main repo? One of the advantages of libp2p being a modular networking stack is that it is possible to include your own transports from repositories outside the main project. |
You are right, of course it could live outside of rust-libp2p. If this doesn't get merged, I will extract it into it's own repository. But I don't want to be the person making that call. |
From a maintainer PoV, I definitely see the appeal of certain transports living outside of the monorepo. This does however require us (@libp2p/rust-libp2p-maintainers) to make a commitment to API stability and reducing the number of breaking changes each version. Otherwise, consuming those out-of-tree transports is just a pain. What if we introduce a |
I opened a separate discussion for this here: #3072 |
That sounds like a great compromise to me. Should I close this PR and open a new one at the new repository? |
Let's wait for what the other maintainers have to say about this :) |
|
This pull request has merge conflicts. Could you please resolve them @umgefahren? 🙏 |
I think for the time being, it is better to leave this out of tree. Let's discuss more over at #3072 how we can stabilize the APIs to make maintaining transports and behaviours out of tree more pleasant. @umgefahren Feel free to tag me or any other maintainer any time you want feedback or if you are struggling with an upgrade. In the meantime, we can extend our README with a link to your repository to give it some visibility. Also, |
Thanks, I will publish the work I've done here on my own repository. |
If someone is interested: https://github.com/umgefahren/libp2p-tor |
Description
Adding a libp2p transport built on top of the newly released Arti. Currently only dialing is supported, since Onion Services are not supported by Arti yet and it will likely take some time until it's supported, since it's not funded yet.
The implementation should be considered as a first step towards a stable Tor based transport.
Links to any relevant issues
This implementation follows the discussion in #708 and should close this issue.
Open Questions
I added libp2p-onion to the default features. However this might be a bad idea, since arti has a lot of dependencies and therefore increases the build time of libp2p greatly.(There are no default features anymore)Since
there is no documentation yet andit's very untested (because of the transports nature automated testing is hard)libp2p-onion maybe shouldn't be integrated in libp2p at all now.Change checklist