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

Add bind-address-4 to upnp #845

Merged
merged 2 commits into from Jan 21, 2022
Merged

Conversation

LaserEyess
Copy link
Contributor

@LaserEyess LaserEyess commented Feb 23, 2019

Make sure the bind-address-4 address is being passed to upnp. This does not affect ipv6 (which shouldn't need either UPnP or NAT-PMP).

Fixes #804, but does not solve the issue for NAT-PMP, which requires knowledge of the gateway. If there is a way to do that properly, and cross platform, I think that should be another PR.

Based on https://trac.transmissionbt.com/ticket/5990

@LaserEyess LaserEyess changed the title WIP: Add bind-address-* to upnp WIP: Add bind-address-4 to upnp Feb 23, 2019
@LaserEyess LaserEyess force-pushed the upnp_bind_address branch 6 times, most recently from e28bf5d to 3e8e8e1 Compare February 24, 2019 18:14
@LaserEyess LaserEyess force-pushed the upnp_bind_address branch 3 times, most recently from eb7e7c6 to c16701b Compare March 17, 2019 14:19
@LaserEyess LaserEyess changed the title WIP: Add bind-address-4 to upnp Add bind-address-4 to upnp Jul 14, 2019
@LaserEyess
Copy link
Contributor Author

@ckerr @mikedld Could I get a review? Despite the lack of feedback from the person who made #804 this PR seems to correct the specific iffue of UPnP packets leaking on ipv4.

@LaserEyess
Copy link
Contributor Author

@ckerr rebased this patch for the C++ change. I need to test that it's still working, but otherwise I think it's ready for review.

ckerr
ckerr previously requested changes Jan 20, 2022
Copy link
Member

@ckerr ckerr left a comment

Choose a reason for hiding this comment

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

It's failing to build in CI:

port-forwarding.cc:98:106: error: no member named 'public_ipv4' in 'tr_session' [clang-diagnostic-error]
tr_upnpPulse(s->upnp, private_peer_port, is_enabled, do_check, tr_address_to_string(&s->session->public_ipv4->addr));
~~~~~~~~~~ ^

@ckerr ckerr added scope:core type:feat A new feature labels Jan 20, 2022
Pass the bind-address-ipv4 to upnpDiscover to allow for upnp to only
use the interface specified. Prevents upnp packet leaks.
@LaserEyess
Copy link
Contributor Author

Well that's on me for not testing even that it compiles, I apologize.

As for actually testing the feature, as far as I can tell it seems to be working. When I set the bind addr to 10.1.1.1 (a wireguard interface) I get that it fails with Required key not available, which is expected since the wg interface can't route it, but I do not see packets going out my eth0 interface. When set explicitly to eth0, it works as expected. Additionally, while there was talk in #804 about a crash on an invalid IP address, that doesn't actually happen.I can set the address to "not.an.ip.address" and it still works.

@LaserEyess
Copy link
Contributor Author

Also, I was unable to commit without adding TR_SKIP_HOOKS=1, I kept getting this error

checking code format
./libtransmission/quark.cc:415:19: error: code should be clang-formatted [-Wclang-format-violations]
    []() constexpr
                  ^
C/C++ code needs formatting

Yet I never touched that file, I assume it's unrelated.

@ckerr
Copy link
Member

ckerr commented Jan 21, 2022

checking code format
./libtransmission/quark.cc:415:19: error: code should be clang-formatted [-Wclang-format-violations]
    []() constexpr
                  ^
C/C++ code needs formatting

Yep, I've seen that one too. CI is running clang-tidy 12, but clang-tidy 13 sees quark.cc:415 differently. Unrelated to this PR.

@ckerr ckerr dismissed their stale review January 21, 2022 00:08

FTBFS has been resolved

@ckerr ckerr self-assigned this Jan 21, 2022
@ckerr ckerr removed their assignment Jan 21, 2022
@ckerr ckerr self-requested a review January 21, 2022 00:08
@ckerr ckerr merged commit 9e5e27a into transmission:main Jan 21, 2022
ile6695 pushed a commit to sweng-group-3/transmission-1 that referenced this pull request Jan 23, 2022
Pass the bind-address-ipv4 to upnpDiscover to allow for upnp to only
use the interface specified. Prevents upnp packet leaks.

Authored-by: LaserEyess <lasereyess@lasereyess.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

UPNP not obeying "bind-address-ipv4" setting
2 participants