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

Add dehydrated flag to device_keys of dehydrated devices #3164

Merged
merged 11 commits into from
Mar 19, 2024
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions bindings/matrix-sdk-crypto-ffi/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,7 @@ async fn migrate_data(
user_id: parse_user_id(&data.account.user_id)?,
device_id: device_id.clone(),
pickle,
dehydrated: false, // dehydrated devices are never involved in migration
shared: data.account.shared,
uploaded_signed_key_count: data.account.uploaded_signed_key_count as u64,
creation_local_time: MilliSecondsSinceUnixEpoch(UInt::default()),
Expand Down
3 changes: 3 additions & 0 deletions crates/matrix-sdk-crypto/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ Breaking changes:
- Move `OlmMachine::export_room_keys` to `matrix_sdk_crypto::store::Store`.
(Call it with `olm_machine.store().export_room_keys(...)`.)

- Add new `dehydrated` property to `olm::account::PickledAccount`.
([#3164](https://github.com/matrix-org/matrix-rust-sdk/pull/3164))

Additions:

- When Olm message decryption fails, report the error code(s) from the failure.
Expand Down
1 change: 1 addition & 0 deletions crates/matrix-sdk-crypto/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ hkdf = "0.12.3"
hmac = "0.12.1"
http = { workspace = true, optional = true } # feature = testing only
itertools = { workspace = true }
js_option = "0.1.1"
matrix-sdk-qrcode = { workspace = true, optional = true }
matrix-sdk-common = { workspace = true }
pbkdf2 = { version = "0.12.2", default-features = false }
Expand Down
12 changes: 10 additions & 2 deletions crates/matrix-sdk-crypto/src/dehydrated_devices.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ impl DehydratedDevices {
let user_id = self.inner.user_id();
let user_identity = self.inner.store().private_identity();

let account = Account::new(user_id);
let account = Account::new_dehydrated(user_id);
let store = Arc::new(CryptoStoreWrapper::new(user_id, MemoryStore::new()));

let verification_machine = VerificationMachine::new(
Expand Down Expand Up @@ -378,6 +378,7 @@ fn expand_pickle_key(key: &[u8; 32], device_id: &DeviceId) -> Box<[u8; 32]> {
mod tests {
use std::{collections::BTreeMap, iter};

use js_option::JsOption;
use matrix_sdk_test::async_test;
use ruma::{
api::client::keys::get_keys::v3::Response as KeysQueryResponse, assign,
Expand All @@ -390,7 +391,7 @@ mod tests {
create_session, get_prepared_machine_test_helper, to_device_requests_to_content,
},
olm::OutboundGroupSession,
types::events::ToDeviceEvent,
types::{DeviceKeys as DeviceKeysType, events::ToDeviceEvent},
utilities::json_convert,
EncryptionSettings, OlmMachine,
};
Expand Down Expand Up @@ -477,6 +478,13 @@ mod tests {
!request.fallback_keys.is_empty(),
"The dehydrated device creation request should contain some fallback keys"
);

let device_keys: DeviceKeysType = request.device_keys.deserialize_as().unwrap();
assert_eq!(
device_keys.dehydrated,
JsOption::Some(true),
"The device keys of the dehydrated device should be marked as dehydrated."
);
}

#[async_test]
Expand Down
5 changes: 5 additions & 0 deletions crates/matrix-sdk-crypto/src/identities/device.rs
Original file line number Diff line number Diff line change
Expand Up @@ -481,6 +481,11 @@ impl Device {

Ok(raw_encrypted)
}

/// Whether or not the device is a dehydrated device.
pub fn is_dehydrated(&self) -> bool {
self.inner.inner.dehydrated.unwrap_or(false)
}
}

/// A read only view over all devices belonging to a user.
Expand Down
28 changes: 26 additions & 2 deletions crates/matrix-sdk-crypto/src/olm/account.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ use std::{
sync::Arc,
};

use js_option::JsOption;
use ruma::{
api::client::{
dehydrated_device::{DehydratedDeviceData, DehydratedDeviceV1},
Expand Down Expand Up @@ -161,6 +162,8 @@ pub struct StaticAccountData {
pub device_id: OwnedDeviceId,
/// The associated identity keys.
pub identity_keys: Arc<IdentityKeys>,
/// Whether the account is for a dehydrated device.
pub dehydrated: bool,
// The creation time of the account in milliseconds since epoch.
creation_local_time: MilliSecondsSinceUnixEpoch,
}
Expand Down Expand Up @@ -281,13 +284,17 @@ impl StaticAccountData {
),
]);

DeviceKeys::new(
let mut ret = DeviceKeys::new(
(*self.user_id).to_owned(),
(*self.device_id).to_owned(),
Self::ALGORITHMS.iter().map(|a| (**a).clone()).collect(),
keys,
Default::default(),
)
);
if self.dehydrated {
ret.dehydrated = JsOption::Some(true);
}
ret
}

/// Get the user id of the owner of the account.
Expand Down Expand Up @@ -352,6 +359,9 @@ pub struct PickledAccount {
pub pickle: AccountPickle,
/// Was the account shared.
pub shared: bool,
/// Whether this is for a dehydrated device
#[serde(default)]
pub dehydrated: bool,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding this flag here will break things that create PickledAccount, such as https://github.com/matrix-org/matrix-rust-sdk-crypto-wasm/blob/main/src/libolm_migration.rs#L136 I'm not sure if there's a good way to deal with this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They'll need to update the bindings just like you did with the matrix-sdk-crypto-ffi bindings.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I just wasn't sure if there was a better way to do it. I assume this means that the version will need to be bumped since this is a breaking change. Is there a way to flag this so that when the next release gets made, the version will be bumped correctly?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're still in the 0.X.Y range of releases, so the correct version will get bumped by design. Adding this as a breaking change to the changelog might make sense to notify people about it.

/// The number of uploaded one-time keys we have on the server.
pub uploaded_signed_key_count: u64,
/// The local time creation of this account (milliseconds since epoch), used
Expand Down Expand Up @@ -399,6 +409,7 @@ impl Account {
user_id: user_id.into(),
device_id: device_id.into(),
identity_keys: Arc::new(identity_keys),
dehydrated: false,
creation_local_time: MilliSecondsSinceUnixEpoch::now(),
},
inner: Box::new(account),
Expand All @@ -424,6 +435,17 @@ impl Account {
Self::new_helper(account, user_id, &device_id)
}

/// Create a new random Olm Account for a dehydrated device
pub fn new_dehydrated(user_id: &UserId) -> Self {
let account = InnerAccount::new();
let device_id: OwnedDeviceId =
base64_encode(account.identity_keys().curve25519.as_bytes()).into();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant to ask earlier, but on the MSC, I suggested using URL-safe unpadded Base64 instead of plain Base64, since the device ID gets put in path parameters. If you don't have any objections to that, I may as well change this code to make this device ID URL-safe before we merge.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We were pondering this when we did the iOS implementation and decided against it. The reason being that it would be confusing to have two different representations for a Curve25510 key. One being in the path and one being uploaded as the device keys. Both encodings would end up in the logs as well and confuse potential bug hunters.


let mut ret = Self::new_helper(account, user_id, &device_id);
ret.static_data.dehydrated = true;
ret
}

/// Get the immutable data for this account.
pub fn static_data(&self) -> &StaticAccountData {
&self.static_data
Expand Down Expand Up @@ -593,6 +615,7 @@ impl Account {
device_id: self.device_id().to_owned(),
pickle,
shared: self.shared(),
dehydrated: self.static_data.dehydrated,
uploaded_signed_key_count: self.uploaded_key_count(),
creation_local_time: self.static_data.creation_local_time,
}
Expand Down Expand Up @@ -646,6 +669,7 @@ impl Account {
user_id: (*pickle.user_id).into(),
device_id: (*pickle.device_id).into(),
identity_keys: Arc::new(identity_keys),
dehydrated: pickle.dehydrated,
creation_local_time: pickle.creation_local_time,
},
inner: Box::new(account),
Expand Down
10 changes: 10 additions & 0 deletions crates/matrix-sdk-crypto/src/types/device_keys.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

use std::collections::BTreeMap;

use js_option::JsOption;
use ruma::{
serde::Raw, DeviceKeyAlgorithm, DeviceKeyId, OwnedDeviceId, OwnedDeviceKeyId, OwnedUserId,
};
Expand Down Expand Up @@ -52,6 +53,10 @@ pub struct DeviceKeys {
/// Signatures for the device key object.
pub signatures: Signatures,

/// Whether the device is a dehydrated device or not
#[serde(default, skip_serializing_if = "JsOption::is_undefined")]
pub dehydrated: JsOption<bool>,

/// Additional data added to the device key information by intermediate
/// servers, and not covered by the signatures.
#[serde(default, skip_serializing_if = "UnsignedDeviceInfo::is_empty")]
Expand All @@ -77,6 +82,7 @@ impl DeviceKeys {
algorithms,
keys,
signatures,
dehydrated: JsOption::Undefined,
unsigned: Default::default(),
other: BTreeMap::new(),
}
Expand Down Expand Up @@ -182,6 +188,8 @@ struct DeviceKeyHelper {
pub device_id: OwnedDeviceId,
pub algorithms: Vec<EventEncryptionAlgorithm>,
pub keys: BTreeMap<OwnedDeviceKeyId, String>,
#[serde(default, skip_serializing_if = "JsOption::is_undefined")]
pub dehydrated: JsOption<bool>,
pub signatures: Signatures,
#[serde(default, skip_serializing_if = "UnsignedDeviceInfo::is_empty")]
pub unsigned: UnsignedDeviceInfo,
Expand Down Expand Up @@ -216,6 +224,7 @@ impl TryFrom<DeviceKeyHelper> for DeviceKeys {
device_id: value.device_id,
algorithms: value.algorithms,
keys: keys?,
dehydrated: value.dehydrated,
signatures: value.signatures,
unsigned: value.unsigned,
other: value.other,
Expand All @@ -233,6 +242,7 @@ impl From<DeviceKeys> for DeviceKeyHelper {
device_id: value.device_id,
algorithms: value.algorithms,
keys,
dehydrated: value.dehydrated,
signatures: value.signatures,
unsigned: value.unsigned,
other: value.other,
Expand Down
Loading