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.

Sign up for GitHub

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

Jump to bottom

Improve VPN rekeying reliability #664

Merged

samsymons merged 12 commits into main from sam/improve-rekeying-reliability

Feb 18, 2024

Contributor

samsymons commented Feb 15, 2024 •

edited

Loading

Please review the release process for BrowserServicesKit here.

Required:

Task/Issue URL: https://app.asana.com/0/414235014887631/1206607513978260/f
iOS PR: duckduckgo/iOS#2478
macOS PR: duckduckgo/macos-browser#2207
What kind of version bump will this require?: Major

Description:

This PR fixes three issues:

The rekeying implementation was deleting the old key, and then registering a new one - however, if the registration call failed, then it got left in a confused state with a connection that didn't work
The connection tester is what is responsible for triggering rekeying and the DAU pixel, but it was accidentally setting isRunning to false even when it was running just fine
The DAU event was sent only when the connection was idle, which isn't necessary

Steps to test this PR:

Make sure client PRs run and connect to NetP, try switching servers etc., generally just smoke test NetP
Check out this branch in one of the clients and change the validityInterval in NetworkProtectionKeyStore to something shorter, like 30 seconds
Edit the updateKeyPairExpirationDate function to also set a date of only 30 seconds in the future, so that you can test more frequent rekeying
Run NetP, then watch the Console logs and check that rekeying is succeeding frequently (make sure you stop other network traffic so that the network is idle and rekeying can complete)

OS Testing:

Internal references:

Software Engineering Expectations
Technical Design Template

samsymons added 3 commits

February 14, 2024 15:02


          Make sure that the connection tester is accurate about when it is run…

bba5786

…ning.


          Move the DAU pixel event above the idle check.

b19383c


          Only replace the old key after rekeying completes.

8ba5599

samsymons requested a review from diegoreymendez

February 15, 2024 04:54

samsymons added 2 commits

February 14, 2024 21:03


          Fix SwiftLint and compilation.

f16c4a6


          Undo expiration date change.

02a6717

samsymons commented

View reviewed changes

Sources/NetworkProtection/Diagnostics/NetworkProtectionConnectionTester.swift Outdated Show resolved Hide resolved

This was referenced Feb 15, 2024

Improve VPN rekeying reliability duckduckgo/iOS#2478

Merged

Improve VPN rekeying reliability duckduckgo/macos-browser#2207

Merged

samsymons commented

View reviewed changes

Sources/NetworkProtection/NetworkProtectionDeviceManager.swift Show resolved Hide resolved

samsymons commented

View reviewed changes

Sources/NetworkProtection/PacketTunnelProvider.swift Show resolved Hide resolved

samsymons commented

View reviewed changes

Sources/NetworkProtection/PacketTunnelProvider.swift Show resolved Hide resolved

samsymons commented

View reviewed changes

Sources/NetworkProtection/KeyManagement/NetworkProtectionKeyStore.swift Outdated Show resolved Hide resolved

samsymons commented

View reviewed changes

Sources/NetworkProtection/KeyManagement/NetworkProtectionKeyStore.swift Outdated Show resolved Hide resolved

samsymons and others added 2 commits

February 14, 2024 21:42


          Make sure that newKeyPair doesn’t overwrite an existing key.

4717e42


          Some suggestions for Sam's rekeying fixes (#665)

d95ed0d

* Adds some suggestions for Sam's PR

* Removes an unnecessary break

diegoreymendez mentioned this pull request

Suggestions for rekeying fixes. #666

Merged

Contributor

diegoreymendez commented Feb 16, 2024

@samsymons - Added some suggestions here: #666

diegoreymendez and others added 2 commits

February 16, 2024 15:46


          Suggestions for rekeying fixes. (#666)

b85fdcc

* Adds some suggestions for Sam's PR

* Removes an unnecessary break

* Makes some changes to fix rekeying

* Rolls back an unintentional change


          Merge branch 'main' into sam/improve-rekeying-reliability

1b8c247

* main:
  Improved tunnel session access (#655)

diegoreymendez approved these changes

View reviewed changes

Sources/NetworkProtection/Diagnostics/NetworkProtectionConnectionTester.swift Outdated Show resolved Hide resolved

samsymons added 3 commits

February 18, 2024 10:31


          Add additional unit tests.

8e47370


          Add a test to make sure that failed registration doesn’t change the key.

63ebcfd


          Re-enable file_length check.

da1ee4b

samsymons merged commit 39a0ed6 into main

7 checks passed

samsymons deleted the sam/improve-rekeying-reliability branch

February 18, 2024 19:13

samsymons added a commit to duckduckgo/iOS that referenced this pull request


          Improve VPN rekeying reliability (#2478)

6971f75

Task/Issue URL: https://app.asana.com/0/414235014887631/1206607513978260/f
Tech Design URL:
CC:

Description:

Client PR for duckduckgo/BrowserServicesKit#664

samsymons added a commit to duckduckgo/macos-browser that referenced this pull request


          Improve VPN rekeying reliability (#2207)

ded95f1

Task/Issue URL: https://app.asana.com/0/414235014887631/1206607513978260/f
Tech Design URL:
CC:

Description:

Client PR for duckduckgo/BrowserServicesKit#664

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet