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 WireGuard over Shadowsocks #6326

Merged
merged 11 commits into from
Aug 16, 2024
Merged

Add WireGuard over Shadowsocks #6326

merged 11 commits into from
Aug 16, 2024

Conversation

dlon
Copy link
Member

@dlon dlon commented Jun 7, 2024


This change is Reviewable

@dlon dlon force-pushed the add-shadowsocks-obfuscation branch 9 times, most recently from cc6dcd6 to 1885ae3 Compare June 10, 2024 14:16
@dlon dlon marked this pull request as ready for review June 10, 2024 14:57
@dlon dlon changed the title WIP - Add WireGuard over Shadowsocks Add WireGuard over Shadowsocks Jun 10, 2024
Copy link
Member Author

@dlon dlon left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 33 files reviewed, 6 unresolved discussions (waiting on @Serock3)


docs/relay-selector.md line 58 at r2 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

Or something to clarify that shadowsocks is used while connecting and not for communication

Done.


mullvad-relay-selector/src/relay_selector/helpers.rs line 145 at r2 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

Is this correct? Should we really compare the extra addresses to the main address, not to the users IP setting?

Good question. I guess the IP setting determines the family used by wg_in_addr, so will it not be the same?


mullvad-relay-selector/src/relay_selector/matcher.rs line 43 at r2 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

For consistentcy with the above formatting

Done.

@dlon dlon force-pushed the add-shadowsocks-obfuscation branch from 913d064 to 2fec20b Compare July 26, 2024 13:35
Copy link
Member Author

@dlon dlon left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 34 files reviewed, 6 unresolved discussions (waiting on @Serock3)


mullvad-relay-selector/src/relay_selector/matcher.rs line 166 at r2 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

Nightly fmt

        // If Shadowsocks is specifically asked for, we must check if the specific relay supports
        // our port. If there are extra addresses, then all ports are available, so we do
        // not need to do this.

Done.


mullvad-relay-selector/src/relay_selector/matcher.rs line 176 at r2 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

This line does not compile for me for some reason, even though it should be fine (and rust-analyzer doesn't complain). Cargo doesn't seem to find the from impl.

Is this still the case now?

@dlon dlon force-pushed the add-shadowsocks-obfuscation branch 3 times, most recently from d439f0f to ae7faf4 Compare July 31, 2024 07:50
@dlon dlon force-pushed the add-shadowsocks-obfuscation branch from 6419bca to 5ce68d3 Compare August 8, 2024 13:34
gui/src/main/daemon-rpc.ts Show resolved Hide resolved
mullvad-relay-selector/src/relay_selector/mod.rs Outdated Show resolved Hide resolved
mullvad-relay-selector/src/relay_selector/helpers.rs Outdated Show resolved Hide resolved
mullvad-relay-selector/tests/relay_selector.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@Serock3 Serock3 left a comment

Choose a reason for hiding this comment

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

:lgtm:

@dlon dlon force-pushed the add-shadowsocks-obfuscation branch 4 times, most recently from 2f9dc4f to a3ba162 Compare August 15, 2024 16:40
Copy link
Contributor

@Serock3 Serock3 left a comment

Choose a reason for hiding this comment

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

Reviewed 10 of 32 files at r1, 1 of 10 files at r2, 7 of 18 files at r4, 2 of 4 files at r5, 3 of 7 files at r6, 6 of 9 files at r9, 4 of 9 files at r12, 1 of 1 files at r13, 1 of 2 files at r14, 2 of 3 files at r15, 1 of 1 files at r16, 1 of 1 files at r17, 7 of 9 files at r18, 7 of 7 files at r19, all commit messages.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @dlon)


mullvad-relay-selector/src/relay_selector/helpers.rs line 145 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

Good question. I guess the IP setting determines the family used by wg_in_addr, so will it not be the same?

After digging a bit, it seems that the address type of wg_in_addr will be determined by the user settings according to the resolve_ip_version fn. I guess we miss the opportunity to switch to IPv6, in case the user IP version is "any" and the extra address is IPv6, but that is probably so unlikely that we don't have to worry about it. I suggest adding a here comment clarifying this behavior though, in case we add many IPv6 extra addresses and want to make it smarter in the future.

Code snippet:

pub fn resolve_ip_version(ip_version: Constraint<IpVersion>) -> IpVersion {
    match ip_version {
        Constraint::Any | Constraint::Only(IpVersion::V4) => IpVersion::V4,
        Constraint::Only(IpVersion::V6) => IpVersion::V6,
    }
}

En gammal draft-kommentar från reviewable som följde med när jag godkände

@dlon dlon force-pushed the add-shadowsocks-obfuscation branch from a3ba162 to 854c9ba Compare August 16, 2024 07:14
@dlon dlon merged commit fe70598 into main Aug 16, 2024
57 of 58 checks passed
@dlon dlon deleted the add-shadowsocks-obfuscation branch August 16, 2024 07:22
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.

2 participants