Skip to content

Conversation

@artfwo
Copy link
Collaborator

@artfwo artfwo commented Nov 5, 2025

Description

We have couple of issues with regular wireguard key rotation: the server response is not very fast, and the client can attempt to connect while the key is being regenerated. This PR addresses the latter problem.

We have three places where key is regenerated currently:

  1. On application startup, using maybeRegenerateDeviceKey, which regenerates keys if key version is < 2.5 and no custom DNS is used.
  2. When connecting, using the same maybeRegenerateDeviceKey function.
  3. From KeyRegenerator with a timer, which regenerates keys weekly if the feature flag is enabled.

Both KeyRegenerator and maybeRegenerateDeviceKey schedule a TaskAddDevice to replace current device keys, and the server reply can take a few seconds to arrive (i've got as much as 7-8 seconds in my tests sometimes). VPN activation clears the task queue, so we may end up in a situation when the keys are updated on the server and not updated in the client.

This PR is one way to solve it by removing cases 1 and 3 above and moving the timer logic from KeyRegenerator under 2, which becomes a single point where key is regenerated immediately before connecting if required (see update maybeRegenerateDeviceKey() below). Failure to regenerate the key will abort connection.

There's a new controller state - RegeneratingKey - that updates the main screen (otherwise it's possible to trigger multiple key updates by repeatedly pushing the toggle).

One alternative solution I can think of would be locking guardian access (at least for adddevice and connection tasks), but that would require implementing some sort of locking mechanism.

Raising this mainly for discussion of the approach now, not for detailed review. More testing is probably needed around state changes (and some functional tests as well). One problem that I've discovered while working on this is that keys aren't updated when CLI command mozillavpn activate is used, that also seems to affect current stable version (no maybeRegenerateDeviceKey check?). Baku has also hinted that experimenting with cancelable flag for the affected tasks could result in a simpler fix.

Reference

VPN-2693

Checklist

  • My code follows the style guidelines for this project
  • I have not added any packages that contain high risk or unknown licenses (GPL, LGPL, MPL, etc. consult with DevOps if in question)
  • I have performed a self review of my own code
  • I have commented my code PARTICULARLY in hard to understand areas
  • I have added thorough tests where needed

@artfwo artfwo requested a review from flodolo as a code owner November 5, 2025 14:28
@artfwo artfwo marked this pull request as draft November 5, 2025 14:29
controller:
regeneratingKey:
value: Updating keys
comment: Connection status label for the main screen
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment is not very helpful for localizers. Which keys are being updated?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@flodolo thanks, i've improved it in 07c75bc

this string could be removed or changed depending on how this PR goes further, i'll ping you for re-reviewal of these parts then!

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