Skip to content

Commit

Permalink
Send client-generated session GUID for identification purposes
Browse files Browse the repository at this point in the history
This is the first half of a change that *may* fix
#26338 (it definitely fixes *one case*
where the issue happens, but I'm not sure if it will cover all of them).

As described in the issue thread, using the `jti` claim from the JWT
used for authorisation seemed like a decent idea. However, upon closer
inspection the scheme falls over badly in a specific scenario where:

1. A client instance connects to spectator server using JWT A.

2. At some point, JWT A expires, and is silently rotated by the game in
   exchange for JWT B.

   The spectator server knows nothing of this, and continues to only
   track JWT A, including the old `jti` claim in said JWT.

3. At some later point, the client's connection to one of the spectator
   server hubs drops out. A reconnection is automatically attempted,
   *but* it is attempted using JWT B.

   The spectator server was not aware of JWT B until now, and said JWT
   has a different `jti` claim than the old one, so to the spectator
   server, it looks like a completely different client connecting, which
   boots the user out of their account.

This PR adds a per-session GUID which is sent in a HTTP header on every
connection attempt to spectator server. This GUID will be used instead
of the `jti` claim in JWTs as a persistent identifier of a single user's
single lazer session, which bypasses the failure scenario described
above.

I don't think any stronger primitive than this is required. As far as I
can tell this is as strong a protection as the JWT was (which is to say,
not *very* strong), and doing this removes a lot of weird complexity
that would be otherwise incurred by attempting to have client ferry all
of its newly issued JWTs to the server so that it can be aware of them.
  • Loading branch information
bdach committed Jul 17, 2024
1 parent 5633297 commit 3006bae
Show file tree
Hide file tree
Showing 4 changed files with 16 additions and 2 deletions.
2 changes: 2 additions & 0 deletions osu.Game/Online/API/APIAccess.cs
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,8 @@ private WebSocketNotificationsClientConnector setUpNotificationsClient()

public string AccessToken => authentication.RequestAccessToken();

public Guid SessionIdentifier { get; } = Guid.NewGuid();

/// <summary>
/// Number of consecutive requests which failed due to network issues.
/// </summary>
Expand Down
2 changes: 2 additions & 0 deletions osu.Game/Online/API/DummyAPIAccess.cs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ public partial class DummyAPIAccess : Component, IAPIProvider

public string AccessToken => "token";

public Guid SessionIdentifier { get; } = Guid.NewGuid();

/// <seealso cref="APIAccess.IsLoggedIn"/>
public bool IsLoggedIn => State.Value > APIState.Offline;

Expand Down
6 changes: 6 additions & 0 deletions osu.Game/Online/API/IAPIProvider.cs
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,12 @@ public interface IAPIProvider
/// </summary>
string AccessToken { get; }

/// <summary>
/// Used as an identifier of a single local lazer session.
/// Sent across the wire for the purposes of concurrency control to spectator server.
/// </summary>
Guid SessionIdentifier { get; }

/// <summary>
/// Returns whether the local user is logged in.
/// </summary>
Expand Down
8 changes: 6 additions & 2 deletions osu.Game/Online/HubClientConnector.cs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ public class HubClientConnector : PersistentEndpointClientConnector, IHubClientC
{
public const string SERVER_SHUTDOWN_MESSAGE = "Server is shutting down.";

public const string VERSION_HASH_HEADER = @"OsuVersionHash";
public const string CLIENT_SESSION_ID_HEADER = @"X-Client-Session-ID";

/// <summary>
/// Invoked whenever a new hub connection is built, to configure it before it's started.
/// </summary>
Expand Down Expand Up @@ -68,8 +71,9 @@ protected override Task<PersistentEndpointClient> BuildConnectionAsync(Cancellat
options.Proxy.Credentials = CredentialCache.DefaultCredentials;
}
options.Headers.Add("Authorization", $"Bearer {API.AccessToken}");
options.Headers.Add("OsuVersionHash", versionHash);
options.Headers.Add(@"Authorization", @$"Bearer {API.AccessToken}");
options.Headers.Add(VERSION_HASH_HEADER, versionHash);
options.Headers.Add(CLIENT_SESSION_ID_HEADER, API.SessionIdentifier.ToString());
});

if (RuntimeFeature.IsDynamicCodeCompiled && preferMessagePack)
Expand Down

0 comments on commit 3006bae

Please sign in to comment.