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

p2p: Bare minimum to support UNIX sockets #9979

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@laanwj
Member

laanwj commented Mar 12, 2017

Two commit on top of #9919 (first adds the implementation, second adds a test).
Make it possible to listen and connect to UNIX sockets for P2P. There are two main use cases for this:

  • Testing without possibility of port collisions
  • TOR hidden service listening with less risk of privacy leaks by not opening any internet ports

Usage: specify :unix:<path> for bind, whitebind, connect, addnode.

@laanwj laanwj added the P2P label Mar 12, 2017

laanwj added some commits Mar 4, 2017

rpc: UNIX sockets support
Add functionality for RPC over UNIX sockets, on platforms that support
UNIX sockets. This is specified with `:unix:/path/to/socket` to
`-rpcbind` and `-rpcconnect`. By default the socket path is relative to the
data directory.
rpc: More robust localhost-UNIX detection
evhttp has no direct support for UNIX sockets thus
is not able to recognize UNIX peers. Add custom code in
HTTPRequest::GetPeer and evunix to recognize these connections
as coming from localhost.

The previous solution did not work on some libevent versions.
p2p: Bare minimum to support UNIX sockets
Make it possible to listen and connect to UNIX sockets for P2P.
There are two main use cases for this:

- Testing without possibility of port collisions
- TOR hidden service listening with less risk of privacy leaks by not
  opening any internet ports

Usage: specify `:unix:<path>` for `bind`, `whitebind`, `connect`,
`addnode`.

@laanwj laanwj referenced this pull request Mar 18, 2017

Open

Unix Domain Sockets #5029

@JeremyRubin

Not sure I'm familiar enough with libevent to give more than a concept ack. I read through it & it looks great. Concept Ack.

@@ -331,6 +337,8 @@ static bool HTTPBindAddresses(struct evhttp* http)
} else if (mapMultiArgs.count("-rpcbind")) { // Specific bind address
const std::vector<std::string>& vbind = mapMultiArgs.at("-rpcbind");
for (std::vector<std::string>::const_iterator i = vbind.begin(); i != vbind.end(); ++i) {
if (boost::starts_with(*i, RPC_ADDR_PREFIX_UNIX)) // Skip UNIX sockets here

This comment has been minimized.

@JeremyRubin

JeremyRubin Apr 5, 2017

Contributor

an easy way to replicate this without a new boost dep is:

if (!i->rfind(RPC_ADD_PREFIX_UNIX, 0))
@JeremyRubin

JeremyRubin Apr 5, 2017

Contributor

an easy way to replicate this without a new boost dep is:

if (!i->rfind(RPC_ADD_PREFIX_UNIX, 0))

This comment has been minimized.

@laanwj

laanwj Apr 5, 2017

Member

I like boost::starts_with. It's python idom (and many other languages) so easy to read. If we want to do without that, I think we should simply implement our own in terms of what you write.

@laanwj

laanwj Apr 5, 2017

Member

I like boost::starts_with. It's python idom (and many other languages) so easy to read. If we want to do without that, I think we should simply implement our own in terms of what you write.

raii_evhttp_connection evcon = obtain_evhttp_connection_base(base.get(), host, port);
raii_evhttp_connection evcon;
if (boost::starts_with(host, RPC_ADDR_PREFIX_UNIX)) {
#if defined(HAVE_SOCKADDR_UN) && defined(LIBEVENT_EXPERIMENTAL)

This comment has been minimized.

@JeremyRubin

JeremyRubin Apr 5, 2017

Contributor

Might be better to have the ifdef define two different functions HandleUnixSocket.

I'm always nervous with compiler directives inside functions, unless the code switched is ~1 line. Large ifdefs are in a couple places in this PR.

@JeremyRubin

JeremyRubin Apr 5, 2017

Contributor

Might be better to have the ifdef define two different functions HandleUnixSocket.

I'm always nervous with compiler directives inside functions, unless the code switched is ~1 line. Large ifdefs are in a couple places in this PR.

This comment has been minimized.

@laanwj

laanwj Apr 5, 2017

Member

Indeed, factoring this out to a function makes sense

@laanwj

laanwj Apr 5, 2017

Member

Indeed, factoring this out to a function makes sense

@@ -505,6 +547,12 @@ void StopHTTPServer()
event_base_free(eventBase);
eventBase = 0;
}
#ifdef HAVE_SOCKADDR_UN
//! Clean up UNIX sockets on shutdown
for (const auto& path: cleanupUNIXSockets) {

This comment has been minimized.

@JeremyRubin

JeremyRubin Apr 5, 2017

Contributor

slight preference for something with ownership guarantees of

std::vector<std::unique_ptr<void>> cleanupUNIXSockets;
// must be non null value
cleanupUNIXSockets.emplace_back(1, [=](void*) { evunix_remove_socket(path); });
cleanupUNIXSockets.clear();

so that sockets aren't double removed.

Maybe wrap the evunix stuff in a class and just keep a vector of those around?

@JeremyRubin

JeremyRubin Apr 5, 2017

Contributor

slight preference for something with ownership guarantees of

std::vector<std::unique_ptr<void>> cleanupUNIXSockets;
// must be non null value
cleanupUNIXSockets.emplace_back(1, [=](void*) { evunix_remove_socket(path); });
cleanupUNIXSockets.clear();

so that sockets aren't double removed.

Maybe wrap the evunix stuff in a class and just keep a vector of those around?

This comment has been minimized.

@laanwj

laanwj Apr 5, 2017

Member

Ok, makes sense

@laanwj

laanwj Apr 5, 2017

Member

Ok, makes sense

Show outdated Hide outdated src/httpserver.cpp Outdated
@ryanofsky

utACK 9abf3c9, though many files are out of date.

I'm wondering why this is labeled "bare minimum to support UNIX sockets" and when support seems pretty complete. Are there other p2p features over unix sockets that you'd want to support?

// Listen for incoming connections
if (listen(hListenSocket, SOMAXCONN) == SOCKET_ERROR)
{
strError = strprintf(_("Error: Listening for incoming connections failed (listen returned error %s)"), NetworkErrorString(WSAGetLastError()));

This comment has been minimized.

@ryanofsky

ryanofsky May 8, 2017

Contributor

In commit "p2p: Bare minimum to support UNIX sockets"

Any reason this error is translated while others are not?

@ryanofsky

ryanofsky May 8, 2017

Contributor

In commit "p2p: Bare minimum to support UNIX sockets"

Any reason this error is translated while others are not?

return false;
}
SOCKET hListenSocket = (SOCKET)fd;
LogPrintf("P2P bound to %s\n", path.string());

This comment has been minimized.

@ryanofsky

ryanofsky May 8, 2017

Contributor

In commit "p2p: Bare minimum to support UNIX sockets"

Maybe mention path is unix socket path

@ryanofsky

ryanofsky May 8, 2017

Contributor

In commit "p2p: Bare minimum to support UNIX sockets"

Maybe mention path is unix socket path

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 1, 2017

Member

Thanks for reviewing @JeremyRubin @jnewbery

though many files are out of date.

I somewhat dread having to rebase this, but hope to do so in the near future.

I'm wondering why this is labeled "bare minimum to support UNIX sockets" and when support seems pretty complete. Are there other p2p features over unix sockets that you'd want to support?

There's no support in RPC calls yet, for example. Unix sockets can only be passed on the command line.

Member

laanwj commented Jun 1, 2017

Thanks for reviewing @JeremyRubin @jnewbery

though many files are out of date.

I somewhat dread having to rebase this, but hope to do so in the near future.

I'm wondering why this is labeled "bare minimum to support UNIX sockets" and when support seems pretty complete. Are there other p2p features over unix sockets that you'd want to support?

There's no support in RPC calls yet, for example. Unix sockets can only be passed on the command line.

@jnewbery

This comment has been minimized.

Show comment
Hide comment
@jnewbery

jnewbery Jun 1, 2017

Member

I somewhat dread having to rebase this, but hope to do so in the near future.

Yes - rebase looks like it's going to be a big job :(

Let me know if I can help in any way - I'm definitely interested in getting Unix socket support for RPC.

Member

jnewbery commented Jun 1, 2017

I somewhat dread having to rebase this, but hope to do so in the near future.

Yes - rebase looks like it's going to be a big job :(

Let me know if I can help in any way - I'm definitely interested in getting Unix socket support for RPC.

@ryanofsky

This comment has been minimized.

Show comment
Hide comment
@ryanofsky

ryanofsky Jun 1, 2017

Contributor

I somewhat dread having to rebase this, but hope to do so in the near future.

I didn't try rebasing, but the merge with master doesn't look bad at all. The only C++ code conflicts are mapMultiArgs -> gArgs replacements. There is a huge scary looking conflict in the test framework util.py, but it just comes from the initialize_chain function moving to another file, and the changes there are minor.

Contributor

ryanofsky commented Jun 1, 2017

I somewhat dread having to rebase this, but hope to do so in the near future.

I didn't try rebasing, but the merge with master doesn't look bad at all. The only C++ code conflicts are mapMultiArgs -> gArgs replacements. There is a huge scary looking conflict in the test framework util.py, but it just comes from the initialize_chain function moving to another file, and the changes there are minor.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 24, 2017

Member

Closing this for now. I'll probably pick it up again for 0.16.

Member

laanwj commented Jun 24, 2017

Closing this for now. I'll probably pick it up again for 0.16.

@laanwj laanwj closed this Jun 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment