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

Update the UI to indicate that the tunnel is post-quantum #6214

Conversation

acb-mv
Copy link
Contributor

@acb-mv acb-mv commented May 3, 2024

This adds information on post-quantum status to TunnelState and displays it in the UI, in the format as in the desktop app. Additionally, unit tests have been created for TunnelState.


This change is Reviewable

pinkisemils and others added 30 commits May 3, 2024 09:35
@acb-mv acb-mv added the iOS Issues related to iOS label May 3, 2024
@acb-mv acb-mv requested review from rablador and buggmagnet May 3, 2024 14:22
@acb-mv acb-mv self-assigned this May 3, 2024
Copy link

linear bot commented May 3, 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.

It's almost there but there are a couple of issues still.

  1. The header bar should be green when we are in the connecting state
  2. The label "creating quantum secure connection" is truncated, we should make it fit (see screenshot number 3, from the desktop client)
  3. When we are in the connecting state, we should be seeing which relay we are connecting to, at the moment we are not showing anything. This information should be available from the connection state.
  4. Running on the simulator should reflect whether the connection is supposedly post quantum or not.

The current post quantum connection flow
IMG_8163.PNG

The non post quantum connection flow
IMG_8164.PNG

The desktop client connection flow (Please ignore the part that says "blocking internet" for now)
Screenshot 2024-05-06 at 09.32.35.png

Reviewed 12 of 13 files at r1, all commit messages.
Reviewable status: 12 of 13 files reviewed, 3 unresolved discussions (waiting on @acb-mv)


ios/MullvadSettings/QuantumResistanceSettings.swift line 17 at r1 (raw file):

}

public extension TunnelQuantumResistance {

nit
I think we can put it directly in the enum without the extension


ios/MullvadVPN/SimulatorTunnelProvider/SimulatorTunnelProviderHost.swift line 187 at r1 (raw file):

                    transportLayer: .udp,
                    remotePort: selectedRelay.endpoint.ipv4Relay.port,
                    isPostQuantum: false

I know the simulator doesn't run the network extension, but since we use it to generate screenshots for the AppStore submission,
we still want it to reflect the would-be connection state.

i.e. We should try to make it reflect here that a post quantum secure connection is established if possible.


ios/MullvadVPNTests/MullvadVPN/TunnelManager/TunnelStateTests.swift line 13 at r1 (raw file):

import XCTest

final class TunnelStateTests: XCTestCase {

Very nice !

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 13 of 13 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @acb-mv)


ios/MullvadVPN/TunnelManager/TunnelState+UI.swift line 52 at r1 (raw file):

            }

        // TODO: Is this the correct message here ?

I think it's alright. I don't think we need to specifically specify that we're negotiating keys as part of the connection flow.


ios/MullvadVPN/TunnelManager/TunnelState+UI.swift line 145 at r1 (raw file):

            )

        // TODO: Is this correct ?

Should be same as the .connecting, .reconnecting etc states I'd say.


ios/MullvadVPN/TunnelManager/TunnelState+UI.swift line 156 at r1 (raw file):

    }

    var localizedAccessibilityLabel: String {

Shouldn't these indicate quantum resistance as well?

Copy link
Contributor Author

@acb-mv acb-mv left a comment

Choose a reason for hiding this comment

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

Done

Reviewable status: 10 of 13 files reviewed, 6 unresolved discussions (waiting on @buggmagnet and @rablador)


ios/MullvadSettings/QuantumResistanceSettings.swift line 17 at r1 (raw file):

Previously, buggmagnet wrote…

nit
I think we can put it directly in the enum without the extension

I prefer to put derived values in an extension, to separate them from the core of the type, but that's just me. I can change it if it's a matter of policy.


ios/MullvadVPN/SimulatorTunnelProvider/SimulatorTunnelProviderHost.swift line 187 at r1 (raw file):

Previously, buggmagnet wrote…

I know the simulator doesn't run the network extension, but since we use it to generate screenshots for the AppStore submission,
we still want it to reflect the would-be connection state.

i.e. We should try to make it reflect here that a post quantum secure connection is established if possible.

Done.


ios/MullvadVPN/TunnelManager/TunnelState+UI.swift line 52 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

I think it's alright. I don't think we need to specifically specify that we're negotiating keys as part of the connection flow.

Done.


ios/MullvadVPN/TunnelManager/TunnelState+UI.swift line 145 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Should be same as the .connecting, .reconnecting etc states I'd say.

Done.


ios/MullvadVPN/TunnelManager/TunnelState+UI.swift line 156 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Shouldn't these indicate quantum resistance as well?

Done. (I didn't change the reconnecting one, as it doesn't mention security and rephrasing it is probably beyond the scope of this PR.)


ios/MullvadVPNTests/MullvadVPN/TunnelManager/TunnelStateTests.swift line 13 at r1 (raw file):

Previously, buggmagnet wrote…

Very nice !

Thanks!

@acb-mv acb-mv force-pushed the ios-662-update-ui-to-indicate-pq-connection-status branch from dfb3e5e to d0d8163 Compare May 6, 2024 14:21
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.

:lgtm:

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


ios/MullvadVPN/TunnelManager/TunnelState+UI.swift line 52 at r1 (raw file):

Previously, acb-mv wrote…

Done.

I left those TODOs to make sure we wouldn't forget them, that's on me !
But thanks for taking care of those 👍


ios/MullvadVPN/TunnelManager/TunnelState+UI.swift line 156 at r1 (raw file):

Previously, acb-mv wrote…

Done. (I didn't change the reconnecting one, as it doesn't mention security and rephrasing it is probably beyond the scope of this PR.)

Let's match what the deskop shows in terms of reconnecting IMHO
But also we can have that in another PR if needs be.

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 3 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@acb-mv acb-mv force-pushed the ios-662-update-ui-to-indicate-pq-connection-status branch from d0d8163 to c90da37 Compare May 7, 2024 09:10
@buggmagnet buggmagnet force-pushed the features/ios-post-quantum branch 6 times, most recently from 0c0e6f3 to 591aca8 Compare May 8, 2024 07:45
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 r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@buggmagnet buggmagnet force-pushed the features/ios-post-quantum branch from 591aca8 to 1de5f57 Compare May 8, 2024 09:12
@acb-mv acb-mv closed this May 10, 2024
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