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

Check if the device is valid on every third connection attempt #5567

Merged
merged 6 commits into from
Dec 12, 2023

Conversation

dlon
Copy link
Member

@dlon dlon commented Dec 7, 2023

This PR changes the default constraints somewhat. WireGuard is used three times instead of two: once on a random port, once on port 53, and once using udp2tcp (if obfuscation is auto).

In general, when the tunnel protocol is WireGuard and no port is set, the relay selector simply alternates between a random port and port 53.

The device validity check occurs on every third attempt instead of on every other attempt.

Fix DES-486.


This change is Reviewable

Copy link

linear bot commented Dec 7, 2023

@dlon dlon force-pushed the fix-device-validity-check branch from 90904ad to 655668b Compare December 8, 2023 08:48
@dlon dlon marked this pull request as ready for review December 8, 2023 08:50
Copy link
Contributor

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

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


mullvad-relay-selector/src/lib.rs line 1054 at r2 (raw file):

    const fn get_auto_obfuscator_retry_attempt(retry_attempt: u32) -> Option<u32> {
        match retry_attempt % 4 {
            0 | 1 => None,

What are these numbers, and why were they chosen? 😊

Code quote:

        match retry_attempt % 4 {
            0 | 1 => None,

mullvad-relay-selector/src/lib.rs line 1056 at r2 (raw file):

            0 | 1 => None,
            // when the retry attempt is 2-3, 6-7, 10-11 ... obfuscation will be used
            filtered_retry => Some(retry_attempt / 4 + filtered_retry - 2),

It's nice that there's an explanation of when obfuscation will be used, but what is it that is being calculated in case Some is returned, and why is it defined this way?:)

Code quote:

            // when the retry attempt is 2-3, 6-7, 10-11 ... obfuscation will be used
            filtered_retry => Some(retry_attempt / 4 + filtered_retry - 2),

@dlon dlon force-pushed the fix-device-validity-check branch from fca61af to fd75cfd Compare December 11, 2023 09:25
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: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @MarkusPettersson98)


mullvad-relay-selector/src/lib.rs line 1054 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

What are these numbers, and why were they chosen? 😊

I've added some documentation.


mullvad-relay-selector/src/lib.rs line 1056 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

It's nice that there's an explanation of when obfuscation will be used, but what is it that is being calculated in case Some is returned, and why is it defined this way?:)

I've explained what this does in more detail now.

Turns out the expression was actually wrong, so thanks for pointing my attention to this. :)

Copy link
Contributor

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

:lgtm:

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dlon)


mullvad-relay-selector/src/lib.rs line 1056 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

I've explained what this does in more detail now.

Turns out the expression was actually wrong, so thanks for pointing my attention to this. :)

Nice, thanks! Glad I could help :D

@dlon dlon force-pushed the add-device-check-test branch from 2207aca to 1e30f44 Compare December 12, 2023 15:45
Base automatically changed from add-device-check-test to main December 12, 2023 15:50
@dlon dlon force-pushed the fix-device-validity-check branch from fd75cfd to 30c40fe Compare December 12, 2023 16:04
@dlon dlon force-pushed the fix-device-validity-check branch from 30c40fe to 02d81d7 Compare December 12, 2023 16:09
@dlon dlon merged commit df07bc4 into main Dec 12, 2023
25 of 26 checks passed
@dlon dlon deleted the fix-device-validity-check branch December 12, 2023 16:10
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