Failed to deserialise key from key backup: missing field sender_claimed_keys

[kegsay]

During analysis of a rageshake log after an unable-to-decrypt error, I see an attempt to download a copy of the session key from backup:

2024-03-12T18:09:44.012078Z DEBUG matrix_sdk::http_client: Got response | crates/matrix-sdk/src/http_client/mod.rs:182 | spans: send{server_versions=[V1_0, V1_1, V1_2, V1_3, V1_4, V1_5] config=RequestConfig { timeout: 30s } request_id="REQ-9" method=GET uri="https://matrix-client.matrix.org/_matrix/client/v3/room_keys/keys/!GqhFoQpBgGGiTmwObj:matrix.org/6CXk1z0cXcDImm+eoL32PV4HVT1YS0C5GrefUSDRvhQ" status=200 response_size="794 B"}
2024-03-12T18:09:44.012846Z  WARN matrix_sdk::encryption::backups: Couldn't decrypt a room key we downloaded from backups, session ID: 6CXk1z0cXcDImm+eoL32PV4HVT1YS0C5GrefUSDRvhQ, error: Json(Error("missing field `sender_claimed_keys`", line: 1, column: 452)) | crates/matrix-sdk/src/encryption/backups/mod.rs:469
2024-03-12T18:09:44.013940Z  INFO matrix_sdk_crypto::store: Successfully imported room keys total_count=0 imported_count=0 room_keys={} | crates/matrix-sdk-crypto/src/store/mod.rs:1555

The specification claims sender_claimed_keys is required but @poljar mentions:

The sender_claimed_keys is probably a deserialization issue with Ruma, that field is useless anyway.

Given the failure mode is pretty bad (cannot decrypt a message), we should make that field optional.

[richvdh]

This is related to element-hq/element-web#26774, in which we "solved" this problem by ignoring such backups.

It has to be said that such backups are non-spec-compliant, and are presumably from old (or at least, non-spec-compliant) clients. It's not obvious to me that we should allow the problem to propagate by continuing to accept such non-compliant backups.

(In other words: if we want to make sender_claimed_keys optional, let's spec it rather than doing our own thing.)

[kegsay]

I'm not sure I agree here. The failure mode is bad, and end users will not care that the reason they are unable to see year's old messages is because the key omitted a required field. If the field is useless, then we should absolutely make it optional in the spec. However, I don't think we should be waiting on a spec change to fix this, hence we would be "doing our own thing" until that lands, in the interests of letting people read their old messages.

[richvdh]

However, I don't think we should be waiting on a spec change to fix this, hence we would be "doing our own thing" until that lands, in the interests of letting people read their old messages.

Well, we should do both. I'd be happy to see a fix for this merged provided there was an MSC proposing a change to the spec.

If we just work aroundit in the rust-sdk (or ruma), then it means that the next person to implement a matrix client also has to violate the spec in the same way. And pretty soon you start wondering WTF the point of a spec is if everyone just has to ignore it.

[poljar]

There's little point in arguing here, Ruma won't accept a PR without an MSC.

[BillCarsonFr]

This could be caused by famedly/matrix-dart-sdk#1739

E.g Fluffy chat will upload keys wihtout sender_claimed_keys but with a wrong sender_clencaimed_keys

{
    "algorithm": "m.megolm.v1.aes-sha2",
    "forwarding_curve25519_key_chain": [],
    "sender_key": "L6YjvXHUd2sXE4PXsjle5wcOzRWJ2i5Te8BJuprUEQo",
    "sender_clencaimed_keys": {
        "ed25519": "RF+Ea/dqHc+o95O4C4opqpcMB7zFI0NEJPh8d8WVd4s"
    },
    "session_key": "AQAAAACF93J0moubZruUeSm/R1b+UzPURaOvsJMUl42JMtyGd/VezrM8afgb+Ej4gqGomISmlwWjcVsBx7QF4+GsV9lQGh9tlpEFVIHbqSXC224LZdZXpZU3uHEKtAeahFPvW9tV7p0vheorkB18r2xbQjWSHgGvrsGJqASeTY3+bQ/C4eVPO0pQopAfjCzmNvIY3BdrjjJoHKL1/+dCGlrPhXcW",
    "session_id": "5U87SlCikB+MLOY28hjcF2uOMmgcovX/50IaWs+FdxY"
}

[kegsay]

Great find :D

[BillCarsonFr]

Fixed in famedly famedly/matrix-dart-sdk#1740

[bnjbvr]

We can close the bug then (please do that yourself in that kind of situation), and reopen if anything is missing.

[richvdh]

The point here is less that famedly were doing the wrong thing, and more that we unnecessarily reject backups that are missing a property we don't even use.

[poljar]

Sorry for the confusion here. The sender_claimed_keys field is important. I mixed this up with the forwarding_curve25519_key_chain field.

The sender_claimed_key field is used to determine the owner of a room key, it's put into the signing_keys property of an InboundGroupSession:

matrix-rust-sdk/crates/matrix-sdk-crypto/src/identities/device.rs

Line 196 in 4325812

session.signing_keys().get(&DeviceKeyAlgorithm::Ed25519).and_then(|k| k.ed25519())

As such, I think this can be closed since we can't accept such backups, and the property can't be ignored during deseralization.