crypto: update signing_key on device upsert - #566
Closed
IIIIbntttt wants to merge 1 commit into
Closed
IIIIbntttt wants to merge 1 commit into
IIIIbntttt wants to merge 1 commit into
Conversation
A client that logs out and logs back in under the same device ID gets a fresh Olm account. Logging out only sets deleted=true, so the row survives, and the ON CONFLICT SET list did not carry signing_key: the row came back with the new identity_key next to the previous signing_key. FindDeviceByKey does not filter deleted, so the Olm path resolves that mixed row and validateDevice rejects every to-device event from the device with "received update for device with different signing key". m.room_key never lands and the megolm session cannot be decrypted.
Author
|
@tulir Why did you close my PR? |
This file contains hidden or 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
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.
Problem
A client that logs out and logs back in under the same device ID gets a fresh Olm
account. After that round trip
crypto_deviceholds a mixed row: the newidentity_keynext to the previoussigning_key. Every Olm message from thatdevice is then dropped with
so
m.room_keynever lands and the megolm session cannot be decrypted. Seen inproduction on a bridge: the user silently stopped being able to send anything.
How the row gets mixed
PutDevicesonly setsdeleted=true.validateDevicewould reject the new keys on a signing key mismatch, but theexisting-device lookup goes through
GetDevices, which filtersdeleted=false.The soft-deleted row is invisible there,
existing == nil, nothing to compareagainst, so the new keys pass.
PutDevicesrunsINSERT … ON CONFLICT (user_id, device_id) DO UPDATE. The SETlist covers
identity_key,deleted,trustandname— but notsigning_key, so the old one stays in the row.FindDeviceByKey, which — unlikeGetDevices— does not filterdeleted. It finds the mixed row,validateDevicecompares the ed25519 key from the envelope against the stalesigning_key, and the event is discarded.The change
signing_key=excluded.signing_keyadded to the ON CONFLICT SET list.Outside tests
PutDeviceis called fromcrypto/machine.go(own device on start)and from
verificationhelper(sas.go,reciprocate.go); in the latter three thedevice comes straight out of
GetOrFetchDevice, so the signing key onlyround-trips through the store and the change is a no-op there.
PutDevicesiscalled from
crypto/devicelist.gofor/keys/queryresults, which have alreadypassed
validateDevice.Test
crypto/signing_key_repro_test.goreproduces the round trip on the existinggetCryptoStoreshelper. Without the change it fails withexpected "signing-2", actual "signing-1"; with it./crypto/...is green(
-tags goolm;crypto/libolmneeds libolm headers and was not built).Not fixed here
The asymmetry underneath is left alone:
GetDevicesfiltersdeleted=false,FindDeviceByKeydoes not. The validation path cannot see a soft-deleted row andlets new keys through, while the Olm path sees that same row and rejects
everything against it. Whether to align the two is your call.