crypto: fix BackedUpRoomKey de/serialization

[BillCarsonFr]

Fixes #3247

A source of #3197

When downloading keys if the forwarding_curve25519_key_chain contains items the deserialisation would fail with error: Json(Error("invalid type: string \"EQCtWSJclhxEU9mGlF4erDHxIacpUQz70yjYVRPODTg\", expected an array of length 32", line: 1, column: 255))

As a consequence the key import fails silently with:
Successfully imported room keys total_count=0 imported_count=0 room_keys={}
That would cause unable to decrypt errors for history.

A serde attribute deserialize_curve_key_vec was missing.

• Public API changes documented in changelogs (optional)

Signed-off-by:

[poljar]

Looks good, left some small suggestions.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.62%. Comparing base (1e35188) to head (4b201a9).
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3246      +/-   ##
==========================================
- Coverage   83.62%   83.62%   -0.01%     
==========================================
  Files         236      236              
  Lines       24451    24451              
==========================================
- Hits        20447    20446       -1     
- Misses       4004     4005       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[richvdh]

AFAICT this is a fix to deserialization, not serialization. I have updated the PR title accordingly.

[poljar]

AFAICT this is a fix to _de_serialization, not serialization. I have updated the PR title accordingly.

It's for both:

[richvdh]

Hrm ok. Serialisation doesn't appear to be tested; and the bug it clams to fix (#3247), and the description of the PR, only mention deserialisation. Hence my confusion.

Any idea what the effect of this issue would have been on serialization? Presumably the forwarding_curve25519_key_chain property would have been serialised as an array of individual bytes, or something? But maybe that didn't really matter in practice because we so rarely use room keys with forwarding_curve25519_key_chain?

[poljar]

Any idea what the effect of this issue would have been on serialization? Presumably the forwarding_curve25519_key_chain property would have been serialised as an array of individual bytes, or something? But maybe that didn't really matter in practice because we so rarely use room keys with forwarding_curve25519_key_chain?

Yes, all of that is correct. It would not base64 encode the keys, it would instead create a 32 byte long array. I'm not completely sure, but I think it didn't matter as the SDK doesn't really use the forwarding chain, I would need to look at the codepaths where we receive room keys.

[richvdh]

As a consequence the key import fails silently with:

For links: the fact that deserialization errors were swallowed was a separate problem, seemingly fixed in #3149.