Send client-generated session GUID for identification purposes #28892
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is the first half of a change that *may* fix ppy#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.
This was referenced Jul 17, 2024
peppy
reviewed
Jul 17, 2024
| @@ -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"; | |||
There was a problem hiding this comment.
Any reason for not making this X-Osu-Version-Hash? I think the X- prefix is pretty standard for custom headers.
There was a problem hiding this comment.
This header is already being sent and received, so mostly the reason is not wanting to faff about with
- sending this header under 2 header names in the interim
- updating the server side to read the new one
- waiting a bit so that the old header name can be deleted from client
If you're fine with said faffery (or a hard break) I can go do it.
There was a problem hiding this comment.
I'd just send under both for now and we can figure what to do about it in the future. Remove old at soem point.
There was a problem hiding this comment.
See also conversation at https://discord.com/channels/188630481301012481/188630652340404224/1263134581679198310 I guess.
peppy
approved these changes
Jul 17, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
jticlaim 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:A client instance connects to spectator server using JWT A.
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
jticlaim in said JWT.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
jticlaim 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
jticlaim 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.
Testing this in a scenario that has at least a sliver of resemblance to the real world can be performed using the instructions in #26338 (comment).