Skip to content

fix(snownet): compare preshared_key on connection upsert - #9999

Merged
thomaseizinger merged 4 commits into
mainfrom
fix/also-compare-preshared-key
Jul 25, 2025
Merged

fix(snownet): compare preshared_key on connection upsert#9999
thomaseizinger merged 4 commits into
mainfrom
fix/also-compare-preshared-key

Conversation

@thomaseizinger

Copy link
Copy Markdown
Member

By chance, I've discovered in a CI failure that we won't be able to handshake a new session if the preshared_key changes. This makes a lot of sense. The preshared_key needs to be the same on both ends as it is a shared secret that gets mixed into the Noise handshake.

In following sequence of events, we would thus previously run into a "failed to decrypt handshake packet" scenario:

  1. Client requests a connection.
  2. Gateway authorizes the connection.
  3. Portal restarts / gets deployed. To my knowledge, this will rotate the preshared_key to a new secret. Restarting the portal also cuts all WebSockets and therefore, the Gateways response never arrives.
  4. Client reconnects to the WebSocket, requests a new connection.
  5. Gateway reuses the local connection but this connection still uses the old preshared_key!
  6. Client needs to wait for the Gateway's ICE timeout before it can establish a new connection.

How exactly (3) happens doesn't matter. There are probably other conditions as to where the WebSocket connections get cut and we cannot complete our connection handshake.

@thomaseizinger
thomaseizinger requested a review from jamilbk July 25, 2025 06:19
@vercel

vercel Bot commented Jul 25, 2025

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
firezone ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 25, 2025 9:03pm

@thomaseizinger
thomaseizinger marked this pull request as draft July 25, 2025 06:21
@thomaseizinger

Copy link
Copy Markdown
Member Author

3. To my knowledge, this will rotate the preshared_key to a new secret.

Ha! I just checked the Elixir source code and we actually do generate a new one for each flow. The code comments say that this must be ignored for existing connections. However, if the connection setup protocol does not succeed, we are screwed.

This in fact means that even without a portal deploy, anytime the connection setup gets interrupted through a WebSocket disconnect, this can happen.

@thomaseizinger
thomaseizinger marked this pull request as ready for review July 25, 2025 13:34
github-merge-queue Bot pushed a commit that referenced this pull request Jul 25, 2025
Whenever a client requests a connection to gateway, we need to generate
a preshared key that will be used for the underlying WireGuard tunnel.

When the connection setup broke or otherwise was lost, _after_ the
gateway the received the authorize_flow call, but _before_ the client
could receive the response (and initiate a tunnel), we would have to
wait until an ICE timeout occurred in order to reset state on the
gateway.

This is because the psk was not used to determine if this was a _new_
flow authorization. So the old authorization would be matched, and the
client would never be able to connect, since its tunnel was using the
new psk, and the gateway the old.

To fix this, we generate a secure random 32-byte `psk_base` on each
client and gateway. When a client wishes to connect to a gateway, we
compute the WireGuard preshared key as an HMAC over these two inputs.

This fixes the issue by ensuring that subsequent flow authorization
requests from a particular client to a particular gateway will yield the
same psk.

Related: #9999 
Related: firezone/infra#99
@thomaseizinger

thomaseizinger commented Jul 25, 2025

Copy link
Copy Markdown
Member Author

Ready for prime time! 🚀 🚀 🚀

@thomaseizinger
thomaseizinger added this pull request to the merge queue Jul 25, 2025
Merged via the queue into main with commit ce5650b Jul 25, 2025
106 checks passed
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