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

Use encrypted dns proxy code in mullvad ios ios 818 #6874

Merged
merged 0 commits into from
Sep 30, 2024

Conversation

pinkisemils
Copy link
Collaborator

@pinkisemils pinkisemils commented Sep 26, 2024

With these changes, we are practically finishing off the encrypted DNS proxy work. These changes make use of the FFI exposed in mullvad-ios::encrypted_dns_proxy to implement the transport provider for our MullvadRest module in Swift. Most of the great work here has been done by @buggmagnet.


This change is Reviewable

Copy link

linear bot commented Sep 26, 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 3 of 6 files at r1, all commit messages.
Reviewable status: 0 of 6 files reviewed, 2 unresolved discussions (waiting on @buggmagnet and @pinkisemils)


ios/MullvadREST/Transport/TransportProvider.swift line 20 at r1 (raw file):

    private var currentTransportType: TransportStrategy.Transport
    private let parallelRequestsMutex = NSLock()
    private let encryptedDNSTransport: EncryptedDNSTransport!

Remove exclamation mark. A let constant that is assigned in an init function will always be non-optional.


ios/MullvadREST/Transport/EncryptedDNS/EncryptedDNSTransport.swift line 21 at r1 (raw file):

    private let encryptedDnsProxy: EncryptedDNSProxy
    private let dispatchQueue = DispatchQueue(label: "net.mullvad.EncryptedDNSTransport")
    private var dnsProxyTask: URLSessionTask!

Should be optional since it's nilled in stop().

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 3 of 6 files at r1.
Reviewable status: 0 of 6 files reviewed, 3 unresolved discussions (waiting on @buggmagnet, @pinkisemils, and @rablador)


ios/MullvadREST/Transport/EncryptedDNS/EncryptedDNSTransport.swift line 67 at r1 (raw file):

                    }
                    // Do not call the completion handler if the request was cancelled in flight
                    if let cancelledError = maybeError as? URLError, cancelledError.code == .cancelled {

Nit: Prefer a guard here, since we're returning early.

@buggmagnet buggmagnet added the iOS Issues related to iOS label Sep 26, 2024
Copy link
Contributor

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


ios/MullvadREST/Transport/TransportProvider.swift line 20 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Remove exclamation mark. A let constant that is assigned in an init function will always be non-optional.

Done


ios/MullvadREST/Transport/EncryptedDNS/EncryptedDNSTransport.swift line 21 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Should be optional since it's nilled in stop().

Done


ios/MullvadREST/Transport/EncryptedDNS/EncryptedDNSTransport.swift line 67 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Nit: Prefer a guard here, since we're returning early.

We discussed this offline and came to the conclusion that the current construction is fine as is.

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: 0 of 6 files reviewed, 4 unresolved discussions (waiting on @buggmagnet, @pinkisemils, and @rablador)


ios/MullvadREST/Transport/TransportProvider.swift line 58 at r2 (raw file):

        if strategy == transportStrategy {
            if strategy.connectionTransport() == .encryptedDNS {

nit : we are doing to stop proxy in different ways, somewhere in deinit and some like here we calling that directly. maybe we need to unify them in one place.

@pinkisemils
Copy link
Collaborator Author

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 r2, all commit messages.
Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @buggmagnet, @pinkisemils, and @rablador)

@pinkisemils pinkisemils force-pushed the use-encrypted-dns-proxy-code-in-mullvad-ios-ios-818 branch from 939548e to 980c837 Compare September 30, 2024 07:14
@pinkisemils pinkisemils merged commit 980c837 into main Sep 30, 2024
161 of 208 checks passed
@pinkisemils pinkisemils deleted the use-encrypted-dns-proxy-code-in-mullvad-ios-ios-818 branch September 30, 2024 07:14
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