Skip to content
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.

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

Race on account creation causing upload of 100 OTKs instead of 50 #4222

Open
BillCarsonFr opened this issue Nov 6, 2024 · 2 comments
Open

Comments

@BillCarsonFr
Copy link
Member

BillCarsonFr commented Nov 6, 2024

Can be reproduced on EX.

After login, the sdk will schedule:

  • The upload the current device keys
  • The first encryption sync

We can see from the logs that the initial key/upload is handled first. This initial upload also contained 50 otks, so the one time key count is updated

debug!("Marking one-time keys as published");
// First mark the current keys as published, as updating the key counts might
// generate some new keys if we're still below the limit.
self.mark_keys_as_published();
self.update_key_counts(&response.one_time_key_counts, None);

11-06 11:38:16.085 Updated uploaded one-time key count 0 -> 50. | crates/matrix-sdk-crypto/src/olm/account.rs:535 | spans: keys_upload{request_id="353c874435e841c294845723da7d190a"}

Then in a quick race the first sync response is handled, the sync response says:

"device_one_time_keys_count": { "signed_curve25519": 0 },

So the one time key count is updated again to 0 this time.

account.update_key_counts(
sync_changes.one_time_keys_counts,
sync_changes.unused_fallback_keys,
)

11-06 11:38:16.091  Updated uploaded one-time key count 50 -> 0. | crates/matrix-sdk-crypto/src/olm/account.rs:535 | spans: next_sync_with_lock > sync_once > handle_response > preprocess_to_device_events > receive_sync_changes

As a consequence the sdk considers that the intial 50 keys have been used, so it will generate new ones.

Not sure if this behavior could cause problems.

If I remind correctly on legacy sdk we never considered the response of key/upload to update the published count of one time key.
Instead we only track the count that we got from the sync.
That means that after the fisrt sync the current count was considered unknown, thus an empty key/upload call was made to have the current count.

Maybe could avoid this scenario?, but we expect the sync response to be valid (see element-hq/synapse#17491)

@BillCarsonFr
Copy link
Member Author

Another strange thing is that the initial key upload is uploading 50 OTKS but no fallback key.

Maybe we want the initial device key upload to only upload the device keys and no otk?

@andybalaam
Copy link
Member

We think this is minor because it doesn't break anything.

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

No branches or pull requests

2 participants