Skip to content
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

Allow configuring DCC listening IP and port #155

Merged
merged 6 commits into from Mar 29, 2019
Merged

Allow configuring DCC listening IP and port #155

merged 6 commits into from Mar 29, 2019

Conversation

moshekaplan
Copy link
Contributor

The library currently doesn't allow controlling which IP address or port are used for DCC.

  • The IP Address needs to be configurable, because sending a file via DCC over the Internet requires sending a public IP address, not the internal private IP.
  • The port needs to be configurable, for IRC clients behind a firewall or NAT.

It appears that the library user cannot control which IP address or port are used for DCC. 
* The IP Address needs to be configurable, because sending a file via DCC over the Internet requires sending a public IP address, not the internal private IP.
* The port needs to be configurable, for IRC clients behind a firewall or NAT.
@jaraco
Copy link
Owner

jaraco commented Jan 4, 2019

I'm not sure I understand the motivation for this change.

sending a file via DCC over the Internet requires sending a public IP address, not the internal private IP

Can you describe the scenario where the address returned by gethostbyname(gethostname()) is not the desired interface?

Would it be better instead to bind to the first address returned by getaddrinfo(None), such as with this code?

The port needs to be configurable

Are you suggesting that a user would route a particular port or series of ports through the firewall and then would use one of those pre-allocated ports in the DCC connection? ...rather than what they could do today which is bind to the port and then authorize the ephemeral port to be routed through the firewall?

I'm generally in favor of this change except for two things.

  1. Instead of taking address, port as separate parameters, it would be better to accept a single 'address' parameter. This parameter should default to (None, 0). The code should probably pass this parameter to socket.getaddrinfo to determine the address family to which to bind.
  2. I dislike that two function signatures need to be adjusted to make this change... Making it only one parameter will help... and I don't see any better approach than to change both signatures.

So if you can address (1), then I'll accept the change.

@moshekaplan
Copy link
Contributor Author

Can you describe the scenario where the address returned by gethostbyname(gethostname()) is not the desired interface?

An example is a machine with multiple network interfaces. They may want to expose the IRC server to one network and not another.

Would it be better instead to bind to the first address returned by getaddrinfo(None), such as with this code?

I don't think so. getaddrinfo(None, 0) appears to default to localhost (at least on my Windows system), which wouldn't be very useful for most users.

Are you suggesting that a user would route a particular port or series of ports through the firewall and then would use one of those pre-allocated ports in the DCC connection? ...rather than what they could do today which is bind to the port and then authorize the ephemeral port to be routed through the firewall?

Yes. I was under the impression that's how most IRC clients (mIRC, Hexchat, etc.) implement DCC sending. There might be security concerns with having a massive range of ports open all the time. I'm personally not aware of how an application would 'inform' a firewall to dynamically allow an incoming TCP connection. If you have any reading material on the subject, I'd like to learn more.

To make sure we're on the same page, here's my understanding of how DCC sends operate (at least on mIRC, which is one of the more popular windows IRC clients)

  1. The server admin specifies a pool of ports available for incoming connections.
  2. The server admin enters (or dynamically retrieves) the server's public IP.
  3. The server admin starts the server.
  4. A client sends a command to instruct the server to send a file.
  5. The server sends a DCC SEND to the client with the server's public IP address and an available port in the pool.
  6. The client connects to that port.
  7. The server sends the data to the client
  8. When the transfer is complete, the DCC connection is closed and the port is re-added to the pool to be available for future DCC sends.

Ideally, this package would also support long-running instances, so it would require timing-out DCC sends if the client doesn't connect and re-adding the port to the pool when the transfer is complete.

@jaraco
Copy link
Owner

jaraco commented Mar 29, 2019

I'm personally not aware of how an application would 'inform' a firewall to dynamically allow an incoming TCP connection. If you have any reading material on the subject, I'd like to learn more.

There are basically two protocols, Nat-PMP and UPnP. Most routers support one or the other, some out of the box and others after enabling the feature. So you're right, there's not a simple, reliable way to dynamically allocate ports to a given host... although certainly some systems like Xbox have engineered a solution that works in most environments. I'm not aware of anything in the Python ecosystem that abstracts this need. I have taken over maintenance of the nat-pmp library for Python, so can offer that as a possible solution.

But I agree - that's outside the scope of this effort, which is simply to give IRC the ability to listen on a pre-determined port.

getaddrinfo(None, 0) appears to default to localhost

I see the same on macOS. I continue to struggle with what I typically want - listen on all interfaces, IPv4 and IPv6.

Okay, so just to re-iterate, the only change I'm seeking still is to combine the host/port into one addr parameter. I'll see if I can push that over the line.

@jaraco jaraco merged commit c4924cf into jaraco:master Mar 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants