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.

Sign up for GitHub

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

Enabling listening on tcp port when proxy is used for peer connections #7726

Open
wants to merge 5 commits into
base: RC_2_0
Choose a base branch
from
+30 −12
Open

Enabling listening on tcp port when proxy is used for peer connections #7726

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3bacdcc
Enabling listening on tcp port when proxy is used for peer connections
Aug 8, 2024
4bf0c21
Correct natpmp
Masong19hippows Oct 2, 2024
2db609b
Adding listen proxy setting
Masong19hippows Oct 16, 2024
eb2a910
Fixed always on listening
Masong19hippows Oct 17, 2024
91081ce
Changing listen_on_proxy to proxy_accept_incoming
Masong19hippows Oct 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions bindings/python/src/session_settings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ void bind_session_settings()
.def_readwrite("type", &proxy_settings::type)
.def_readwrite("proxy_peer_connections", &proxy_settings::proxy_peer_connections)
.def_readwrite("proxy_hostnames", &proxy_settings::proxy_hostnames)
.def_readwrite("listen_on_proxy", &proxy_settings::listen_on_proxy)
;
#endif
}
Expand Down
4 changes: 4 additions & 0 deletions include/libtorrent/aux_/proxy_settings.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,10 @@ namespace aux {
// This is only supported by SOCKS5 and HTTP.
bool proxy_hostnames = true;

// defaults to false for integration compatibility. It means that the
// torrent port is listening with tcp and udp while there is a proxy configured.
bool listen_on_proxy = false;
Masong19hippows marked this conversation as resolved.
Show resolved Hide resolved

// determines whether or not to exempt peer and web seed connections
// from using the proxy. This defaults to true, i.e. peer connections are
// proxied by default.
Expand Down
3 changes: 3 additions & 0 deletions include/libtorrent/settings_pack.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -876,6 +876,9 @@ namespace aux {
// any). This is only supported by SOCKS5 and HTTP.
proxy_hostnames,

// if true, listening on the defined port will be enabled while using proxy
listen_on_proxy,
Masong19hippows marked this conversation as resolved.
Show resolved Hide resolved

// if true, peer connections are made (and accepted) over the
// configured proxy, if any. Web seeds as well as regular bittorrent
// peer connections are considered "peer connections". Anything
Expand Down
1 change: 1 addition & 0 deletions simulation/make_proxy_settings.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ inline lt::aux::proxy_settings make_proxy_settings(
aux::proxy_settings ps;
ps.type = proxy_type;
ps.proxy_hostnames = false;
ps.listen_on_proxy = false;
// this IP and ports are specific to test_http_connection.cpp
if (proxy_type != settings_pack::none)
{
Expand Down
1 change: 1 addition & 0 deletions simulation/utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ void set_proxy(lt::session& ses, int proxy_type, test_transfer_flags_t const fla
else
p.set_str(settings_pack::proxy_hostname, "50.50.50.50");
p.set_bool(settings_pack::proxy_hostnames, true);
p.set_bool(settings_pack::listen_on_proxy, false);
p.set_bool(settings_pack::proxy_peer_connections, bool(flags & tx::proxy_peers));
p.set_bool(settings_pack::proxy_tracker_connections, proxy_peers);
p.set_bool(settings_pack::socks5_udp_send_local_ep, true);
Expand Down
1 change: 1 addition & 0 deletions src/proxy_settings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ void init(proxy_settings& p, Settings const& sett)
p.type = settings_pack::proxy_type_t(sett.get_int(settings_pack::proxy_type));
p.port = std::uint16_t(sett.get_int(settings_pack::proxy_port));
p.proxy_hostnames = sett.get_bool(settings_pack::proxy_hostnames);
p.listen_on_proxy = sett.get_bool(settings_pack::listen_on_proxy);
p.proxy_peer_connections = sett.get_bool(
settings_pack::proxy_peer_connections);
p.proxy_tracker_connections = sett.get_bool(
Expand Down
1 change: 1 addition & 0 deletions src/session_handle.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1040,6 +1040,7 @@ namespace {
p.set_int(settings_pack::proxy_type, s.type);
p.set_int(settings_pack::proxy_port, s.port);
p.set_bool(settings_pack::proxy_hostnames,s.proxy_hostnames);
p.set_bool(settings_pack::listen_on_proxy,s.listen_on_proxy);
p.set_bool(settings_pack::proxy_peer_connections, s.proxy_peer_connections);

apply_settings(std::move(p));
Expand Down
29 changes: 17 additions & 12 deletions src/session_impl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -830,6 +830,8 @@ bool ssl_server_name_callback(ssl::stream_handle_type stream_handle, std::string
if (val) s.set_int(settings_pack::proxy_type, int(val.int_value()));
val = settings.dict_find_int("proxy_hostnames");
if (val) s.set_bool(settings_pack::proxy_hostnames, val.int_value() != 0);
val = settings.dict_find_int("listen_on_proxy");
if (val) s.set_bool(settings_pack::listen_on_proxy, val.int_value() != 0);
val = settings.dict_find_int("proxy_peer_connections");
if (val) s.set_bool(settings_pack::proxy_peer_connections, val.int_value() != 0);
val = settings.dict_find_string("hostname");
Expand Down Expand Up @@ -2056,7 +2058,8 @@ namespace {
// if we don't proxy peer connections, don't apply the special logic for
// proxies
if (m_settings.get_int(settings_pack::proxy_type) != settings_pack::none
&& m_settings.get_bool(settings_pack::proxy_peer_connections))
&& m_settings.get_bool(settings_pack::proxy_peer_connections)
&& !m_settings.get_bool(settings_pack::listen_on_proxy))
{
// we will be able to accept incoming connections over UDP. so use
// one of the ports the user specified to use a consistent port
Expand Down Expand Up @@ -2820,12 +2823,12 @@ namespace {
async_accept(listener, ssl);

// don't accept any connections from our local listen sockets if we're
// using a proxy. We should only accept peers via the proxy, never
// directly.
// using a proxy and the correct setting isn't set.
// This path is only for accepting incoming TCP sockets. The udp_socket
// class also restricts incoming packets based on proxy settings.
if (m_settings.get_int(settings_pack::proxy_type) != settings_pack::none
&& m_settings.get_bool(settings_pack::proxy_peer_connections))
&& m_settings.get_bool(settings_pack::proxy_peer_connections)
&& !m_settings.get_bool(settings_pack::listen_on_proxy))
return;

auto listen = std::find_if(m_listen_sockets.begin(), m_listen_sockets.end()
Expand Down Expand Up @@ -5522,11 +5525,11 @@ namespace {
if (m_listen_sockets.empty()) return 0;
if (sock)
{
// if we're using a proxy, we won't be able to accept any TCP
// connections. Not even uTP connections via the port we know about.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to update or extend this comment, rather than removing it

// The DHT may use the implied port to make it work, but the port we
// announce here has no relevance for that.
if (sock->flags & listen_socket_t::proxy)

if (sock->flags & listen_socket_t::proxy
&& !m_settings.get_bool(settings_pack::listen_on_proxy))
return 0;

if (!(sock->flags & listen_socket_t::accept_incoming))
Expand All @@ -5535,6 +5538,11 @@ namespace {
return std::uint16_t(sock->tcp_external_port());
}

if (m_settings.get_int(settings_pack::proxy_type) != settings_pack::none
&& m_settings.get_bool(settings_pack::proxy_peer_connections)
&& !m_settings.get_bool(settings_pack::listen_on_proxy))
return 0;

#ifdef TORRENT_SSL_PEERS
for (auto const& s : m_listen_sockets)
{
Expand Down Expand Up @@ -5566,9 +5574,6 @@ namespace {
return std::uint16_t(sock->tcp_external_port());
}

if (m_settings.get_int(settings_pack::proxy_type) != settings_pack::none
&& m_settings.get_bool(settings_pack::proxy_peer_connections))
return 0;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have expected this also depend on the listen_on_proxy setting, no?


for (auto const& s : m_listen_sockets)
{
Expand Down Expand Up @@ -5664,7 +5669,7 @@ namespace {

if (!s->natpmp_mapper
&& !(s->flags & listen_socket_t::local_network)
&& !(s->flags & listen_socket_t::proxy))
&& m_settings.get_bool(settings_pack::listen_on_proxy))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you don't configure a proxy and leave listen_on_proxy as false; it looks like this change would disable NAT-PMP in that case. I think what you meant to do was to add another condition to the flags & proxy check.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there might be a simpler way of doing this. Rather than checking the settings here, I think you should be able to check the listen_socket_t::accept_incoming flag. You'd have to make sure this flag is set correctly depending on the listen_on_proxy setting when the sockets are created though

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arvidn would it be acceptable to just remove the proxy being a factor in whether the user can enable natpmp? Or is this something that is best left in another PR/Issue/Discussion?

{
// the natpmp constructor may fail and call the callbacks
// into the session_impl.
Expand Down Expand Up @@ -6891,7 +6896,7 @@ namespace {
// connected to the internet. The whole point is to forward ports through
// the gateway
if ((s->flags & listen_socket_t::local_network)
|| (s->flags & listen_socket_t::proxy))
|| (s->flags & !m_settings.get_bool(settings_pack::listen_on_proxy)))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not right. the & is a bit-wise AND operation. you can mask the flags field against flags/bits set in this field. get_bool() returns a bool, not a flag value. I think you meant to preserve the existing check and add this additional one.

return;

if (!s->upnp_mapper)
Expand Down
1 change: 1 addition & 0 deletions src/settings_pack.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,7 @@ constexpr int DISK_WRITE_MODE = settings_pack::enable_os_cache;
SET(enable_dht, true, &session_impl::update_dht),
SET(prefer_rc4, false, nullptr),
SET(proxy_hostnames, true, nullptr),
SET(listen_on_proxy, false, nullptr),
SET(proxy_peer_connections, true, nullptr),
SET(auto_sequential, true, &session_impl::update_auto_sequential),
SET(proxy_tracker_connections, true, nullptr),
Expand Down