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 udp shadowsocks proxy support to mullvad ios #7132

Merged
merged 1 commit into from
Nov 14, 2024

Conversation

buggmagnet
Copy link
Contributor

@buggmagnet buggmagnet commented Nov 5, 2024

This PR adds Shadowsocks obfuscation capabilities to the Packet Tunnel.
It cannot be used as of yet, but there is a test called testRunningShadowsocksObfuscatorProxy that makes use of the newly introduced obfuscation protocol.


This change is Reviewable

@buggmagnet buggmagnet added the iOS Issues related to iOS label Nov 5, 2024
@buggmagnet buggmagnet self-assigned this Nov 5, 2024
Copy link

linear bot commented Nov 5, 2024

Copy link
Contributor Author

@buggmagnet buggmagnet 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 12 files reviewed, 2 unresolved discussions


mullvad-ios/src/tunnel_obfuscator_proxy/ffi.rs line 10 at r1 (raw file):

static INIT_LOGGING: Once = Once::new();

#[allow(dead_code)]

note to self : this shouldn't be necessary, investigate


ios/MullvadRustRuntimeTests/UnsafeListener.swift line 11 at r1 (raw file):

import Network

class UnsafeListener<T: Connection> {

note to self: Re add warning comment to not use this in production

@buggmagnet buggmagnet requested a review from dlon November 5, 2024 16:10
Copy link
Member

@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.

Reviewed 4 of 12 files at r1.
Reviewable status: 4 of 12 files reviewed, 3 unresolved discussions (waiting on @buggmagnet)


mullvad-ios/src/tunnel_obfuscator_proxy/mod.rs line 23 at r1 (raw file):

        peer: SocketAddr,
        obfuscation_protocol: TunnelObfuscatorProtocol,
    ) -> io::Result<Self> {

Nit: Method could be infallible (just return Self { .. }).

Copy link
Contributor

@rablador rablador left a comment

Choose a reason for hiding this comment

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

Reviewed 12 of 12 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @buggmagnet)


ios/MullvadRustRuntimeTests/UDPConnection.swift line 75 at r1 (raw file):

        return try await withCheckedThrowingContinuation { continuation in
            nwConnection.receiveMessage { data, _, _, error in
                guard let data else {

Can there be data and error at the same time? Also, wouldn't checking for error first be more interesting since it could give us a hint of what's going on. As it is now we might never get to it if data is empty.


ios/MullvadRustRuntime/TunnelObfuscator.swift line 14 at r1 (raw file):

import Network

public enum TunnelObfuscationProtocol {

Not sure there's a good alternative for a name here, but it's somewhat confusing with both TunnelObfuscationProtocol and TunnelObfuscatorProtocol.


ios/MullvadRustRuntime/TunnelObfuscator.swift line 75 at r1 (raw file):

            let obfuscationProtocol = switch obfuscationProtocol {
            case .udpOverTcp: TunnelObfuscatorProtocol(0)

This int param means very little on the Swift side. Can we use an enum with raw value or something else instead?

Copy link
Contributor Author

@buggmagnet buggmagnet 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: all files reviewed, 6 unresolved discussions (waiting on @rablador)


ios/MullvadRustRuntimeTests/UDPConnection.swift line 75 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Can there be data and error at the same time? Also, wouldn't checking for error first be more interesting since it could give us a hint of what's going on. As it is now we might never get to it if data is empty.

Whilst it's technically valid to read a UDP datagram of size 0, in our case it's not a desired behaviour.
We can make sure to check for errors before data (or the lack thereof) though


ios/MullvadRustRuntime/TunnelObfuscator.swift line 14 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Not sure there's a good alternative for a name here, but it's somewhat confusing with both TunnelObfuscationProtocol and TunnelObfuscatorProtocol.

Yes it's not great I agree, I'll think of something better.


ios/MullvadRustRuntime/TunnelObfuscator.swift line 75 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

This int param means very little on the Swift side. Can we use an enum with raw value or something else instead?

Unfortunately, due to how C enums are imported in Swift it's not straightforward to do so.

The other complication comes from the fact that the header file for FFI functions is automatically generated.
We have ways around this but that would require further customisation of the automatically imported FFI header, which is a lot of work.

I would like to do that, but that would probably be a better candidate for a hack day.
For now, I think this is an acceptable tradeoff.

@buggmagnet buggmagnet force-pushed the add-udp-shadowsocks-proxy-support-to-mullvad-ios-ios-882 branch from 82d5634 to b051647 Compare November 6, 2024 15:34
Copy link
Contributor

@rablador rablador left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @buggmagnet)

@buggmagnet buggmagnet force-pushed the add-udp-shadowsocks-proxy-support-to-mullvad-ios-ios-882 branch from b051647 to 0ed4bfc Compare November 11, 2024 11:36
rablador
rablador previously approved these changes Nov 11, 2024
Copy link
Contributor

@rablador rablador left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@buggmagnet buggmagnet force-pushed the add-udp-shadowsocks-proxy-support-to-mullvad-ios-ios-882 branch from 0ed4bfc to 26b75ba Compare November 13, 2024 14:41
Copy link
Contributor

@rablador rablador left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@dlon dlon merged commit e8bead5 into main Nov 14, 2024
56 checks passed
@dlon dlon deleted the add-udp-shadowsocks-proxy-support-to-mullvad-ios-ios-882 branch November 14, 2024 08:25
Copy link
Collaborator

@mojganii mojganii left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iOS Issues related to iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants