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 test for audit ticket MUL-02-002 #5979

Merged
merged 3 commits into from
Apr 9, 2024

Conversation

MarkusPettersson98
Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 commented Mar 19, 2024

This PR adds an integration test to check for regressions to audit ticket MUL-02-002 - Firewall allows deanonymization by eavesdropper. The fix for this issue has been included in the app since version 2020.5.


This change is Reviewable

Copy link

linear bot commented Mar 19, 2024

@MarkusPettersson98 MarkusPettersson98 force-pushed the add-test-for-2020-app-audit-des-419 branch 3 times, most recently from e7fbf6c to ccf30ab Compare March 27, 2024 14:05
@MarkusPettersson98 MarkusPettersson98 force-pushed the add-test-for-2020-app-audit-des-419 branch 6 times, most recently from fc78056 to 3a2ca13 Compare March 28, 2024 16:35
@MarkusPettersson98 MarkusPettersson98 marked this pull request as ready for review March 28, 2024 16:36
Copy link
Contributor

@hulthe hulthe left a comment

Choose a reason for hiding this comment

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

Very readable test. Nice.

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


test/test-manager/src/tests/tunnel.rs line 828 at r1 (raw file):

) -> Result<(), anyhow::Error> {
    // Step 0 - Disconnect from any active tunnel connection
    helpers::disconnect_and_wait(&mut mullvad_client).await?;

IIRC the runtime makes sure we're disconnected when we start a test


test/test-manager/src/tests/tunnel.rs line 853 at r1 (raw file):

    // identifiable payload
    // FIXME: This needs to be kept in sync with the magic payload string defined in `connection_cheker::net`.
    // An easy fix would be to make the payload for `ConnCheck` configurable.

dewit-do-it.gif


test/test-manager/src/tests/tunnel.rs line 869 at r1 (raw file):

    // Before proceeding, assert that the method of detecting identifiable packets work.
    conn_artist.check_connection().await?;
    let monitor_result = rogue_packet_monitor.into_result().await.unwrap();

unwrap?

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 all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @hulthe and @MarkusPettersson98)


test/test-manager/src/tests/tunnel.rs line 828 at r1 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

IIRC the runtime makes sure we're disconnected when we start a test

+1


test/test-manager/src/tests/tunnel.rs line 848 at r1 (raw file):

    };
    helpers::disconnect_and_wait(&mut mullvad_client).await?;
    let gateway = endpoint.endpoint.address;

gateway seems wrong. Also, it's not guaranteed to select the same endpoint when reconnecting (step 4).


test/test-manager/src/tests/tunnel.rs line 853 at r1 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

dewit-do-it.gif

+1. Or just define it as a constant somewhere.

@MarkusPettersson98 MarkusPettersson98 force-pushed the add-test-for-2020-app-audit-des-419 branch 2 times, most recently from 293901d to f7e9a3f Compare April 5, 2024 10:59
Copy link
Contributor Author

@MarkusPettersson98 MarkusPettersson98 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: 2 of 9 files reviewed, 4 unresolved discussions (waiting on @dlon and @hulthe)


test/test-manager/src/tests/tunnel.rs line 828 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

+1

Done.


test/test-manager/src/tests/tunnel.rs line 848 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

gateway seems wrong. Also, it's not guaranteed to select the same endpoint when reconnecting (step 4).

Done


test/test-manager/src/tests/tunnel.rs line 853 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

+1. Or just define it as a constant somewhere.

Done


test/test-manager/src/tests/tunnel.rs line 869 at r1 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

unwrap?

🙀
Fixed! 🧽

@MarkusPettersson98 MarkusPettersson98 force-pushed the add-test-for-2020-app-audit-des-419 branch 2 times, most recently from 28f8778 to 0256eee Compare April 8, 2024 10:47
Copy link
Contributor

@hulthe hulthe 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 7 of 7 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @dlon and @MarkusPettersson98)


test/connection-checker/src/cli.rs line 39 at r2 (raw file):

    /// Junk data for each UDP and TCP packet
    #[clap(long, requires = "leak")]
    pub payload: Option<String>,

Nit: I would use #[clap(default_value = "Hello there!")] instead of making this an option

@MarkusPettersson98 MarkusPettersson98 force-pushed the add-test-for-2020-app-audit-des-419 branch from 0256eee to 5ab094d Compare April 8, 2024 13:16
Copy link
Contributor Author

@MarkusPettersson98 MarkusPettersson98 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: 7 of 9 files reviewed, 4 unresolved discussions (waiting on @dlon and @hulthe)


test/connection-checker/src/cli.rs line 39 at r2 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

Nit: I would use #[clap(default_value = "Hello there!")] instead of making this an option

Ah, that's better!

@MarkusPettersson98 MarkusPettersson98 force-pushed the add-test-for-2020-app-audit-des-419 branch from 5ab094d to 0581d26 Compare April 8, 2024 13:19
Copy link
Contributor

@hulthe hulthe 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 r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dlon)

@MarkusPettersson98 MarkusPettersson98 force-pushed the add-test-for-2020-app-audit-des-419 branch 3 times, most recently from 0a09044 to aae7f1c Compare April 9, 2024 12:26
Copy link
Contributor

@hulthe hulthe 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 4 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dlon)

@MarkusPettersson98 MarkusPettersson98 force-pushed the add-test-for-2020-app-audit-des-419 branch from aae7f1c to 561273f Compare April 9, 2024 12:35
Copy link
Contributor

@hulthe hulthe 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 r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dlon)

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.

:lgtm:

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

@MarkusPettersson98 MarkusPettersson98 force-pushed the add-test-for-2020-app-audit-des-419 branch from 561273f to 55e911c Compare April 9, 2024 12:59
@MarkusPettersson98 MarkusPettersson98 merged commit 7fae8dc into main Apr 9, 2024
30 of 31 checks passed
@MarkusPettersson98 MarkusPettersson98 deleted the add-test-for-2020-app-audit-des-419 branch April 9, 2024 12:59
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.

3 participants