Reverse tunnel listen interface about eternalterminal

[MisterTea]

Interesting, thanks for sharing! This is a bug and we should fix it. Can you submit a PR to fix? It should be pretty simple to search for the places we call bind() and figure out which one is for tunneling.

[lostystyg]

SocketEndpoint::name is being used to determine the ip to bind the socket on (here)
I think the proper place to set the name is here, so e.x. the solution would be:

      } else {
        PortForwardSourceRequest pfsr;
+++     pfsr.mutable_source()->set_name("localhost");
        pfsr.mutable_source()->set_port(stoi(sourceDestination[0]));
        pfsr.mutable_destination()->set_port(stoi(sourceDestination[1]));
        pfsrs.push_back(pfsr);
      }

at this point the ip is being controlled by the client and etserver just binds the socket to a requested ip (localhost in this case), also it may be later converted to an option or smth for the client.

On the other hand it can be hardcoded on the etserver side, ex. here:

  ForwardSourceHandler::ForwardSourceHandler(
      shared_ptr<SocketHandler> _socketHandler, const SocketEndpoint& _source,
      const SocketEndpoint& _destination)
      : socketHandler(_socketHandler),
        source(_source),
        destination(_destination) {
+++ source->set_name("localhost");
    socketHandler->listen(source);
  }

What solution is preferable?

[MisterTea]

Yep, agreed that the first way makes the most sense.

[lostystyg]

Ofc not that easy) Fails because of this. Btw it doesn't make sense for me to perform this check and fail while the source.name is being used further in the networkSocketHandler

[MisterTea]

That check was to prevent a different issue. If the name is "localhost" we can skip that check (i.e. let's change the check to allow localhost through)

[lostystyg]

At this point it will be identical to just hardcoding localhost on the server. I think that theoretically server can listen for any host/ip that is specified by the client.
Furthermore, it seems for me that there only 2 types of tunneling: via pipe and via tcp. The check is performed for both pipe and tcp but it seems that it is required only to check that pipe filename is not specified on the client and forced to be generated on the server. Also, if the type of tunnel is pipe and there is a source without name, the code will skip temporarily filename creation without error and try to create a pipe socket without a filename. This is definitely not what should happen, so i suppose that the logic of this function should be a bit rewritten, so it could allow any source.name for tcp tunnels (the goal of this issue), and do not allow any source at all for pipe tunnels.
Basically:

if (!pfsr.has_source()) {
  // pipe stuff
} else if (pfsr.source().has_port()) {
  // tcp stuff
} else {
  // error
}

[lostystyg]

Hmm, i misunderstand this a bit, if pfsr.has_source() it is always a tcp tunnel even now. Why not then allow to specify a name? What was the purpose to do this check and fail? If client specify a rubbish name (means wrong ip or wrong hostname or literally rubbish) then it will fail inside networkSocketHandler with Error getting address info for ... or Error binding ... that i assume is the correct and obvious behavior, isn't it?

[lostystyg]

At the end of the day i see this as a right solution lostystyg#1
Using it for a week and it feels good for me
If it's ok for you i will rebase the PR to the upstream

[MisterTea]

That makes sense to me. Please submit a PR, thanks!