diff --git a/services/identity/src/client_service.rs b/services/identity/src/client_service.rs --- a/services/identity/src/client_service.rs +++ b/services/identity/src/client_service.rs @@ -1336,6 +1336,11 @@ ) })?; + let maybe_keyserver_device_id = self + .client + .get_keyserver_device_id_for_user(&user_id) + .await?; + // Verify device list payload let signed_list: SignedDeviceList = device_list_payload.parse()?; let device_list_payload = @@ -1344,6 +1349,7 @@ &device_list_payload, &new_primary_device_id, Some(&previous_primary_device_id), + maybe_keyserver_device_id.as_ref(), )?; debug!(user_id, "Attempting to revoke user's old access tokens"); diff --git a/services/identity/src/device_list.rs b/services/identity/src/device_list.rs --- a/services/identity/src/device_list.rs +++ b/services/identity/src/device_list.rs @@ -223,16 +223,24 @@ Ok(()) } +/// Verifies device list containing only a primary device, and optionally +/// a keyserver for keyserver owners. Such device lists are created during +/// user registration, primary device restore and primary device logout. +/// +/// The device list must be a singleton consisting of primary device ID, for +/// keyservr owners it's a tuple of primary device ID and keyserver device ID. pub fn verify_singleton_device_list( device_list: &DeviceListUpdate, expected_primary_device_id: &str, // expected primary device ID for "lastPrimarySignature". // Use `None` if the device list isn't expected to contain last signature. expected_previous_primary_device_id: Option<&String>, + expected_keyserver_device_id: Option<&String>, ) -> Result<(), tonic::Status> { use tonic::Status; use tonic_status_messages::INVALID_DEVICE_LIST_UPDATE as INVALID_DEVICE_LIST; + // Verify lastPrimarySignature match ( &device_list.last_primary_signature, expected_previous_primary_device_id, @@ -255,6 +263,7 @@ } }; + // verify curPrimarySignature let Some(signature) = &device_list.current_primary_signature else { debug!("Missing curPrimarySignature for singleton device list"); return Err(Status::invalid_argument(INVALID_DEVICE_LIST)); @@ -266,11 +275,7 @@ signature, )?; - if device_list.devices.len() != 1 { - debug!("Invalid device list length"); - return Err(Status::invalid_argument(INVALID_DEVICE_LIST)); - } - + // verify primary device ID if device_list .devices .first() @@ -281,6 +286,49 @@ return Err(Status::invalid_argument(INVALID_DEVICE_LIST)); } + // NOTE: for better dev experience, we can be less strict for now, + // allowing both singleton/tuple device list when keyserver is present + let strict_device_list_checks_enabled = + std::env::var("COMM_SERVICES_STRICT_DEVICE_LIST_CHECKS") + .ok() + .is_some(); + + if !strict_device_list_checks_enabled { + let max_expected_device_list_length = + if expected_keyserver_device_id.is_some() { + 2 + } else { + 1 + }; + + if device_list.devices.is_empty() + || device_list.devices.len() > max_expected_device_list_length + { + debug!("Invalid device list length"); + return Err(Status::invalid_argument(INVALID_DEVICE_LIST)); + } + } else { + // verify keyserver device ID and device list length + if let Some(keyserver_device_id) = expected_keyserver_device_id { + if device_list.devices.len() != 2 { + debug!("Invalid device list length"); + return Err(Status::invalid_argument(INVALID_DEVICE_LIST)); + } + + if device_list + .devices + .last() + .filter(|it| *it == keyserver_device_id) + .is_none() + { + debug!("Invalid keyserver device ID for tuple device list"); + return Err(Status::invalid_argument(INVALID_DEVICE_LIST)); + } + } else if device_list.devices.len() != 1 { + debug!("Invalid device list length"); + return Err(Status::invalid_argument(INVALID_DEVICE_LIST)); + } + } Ok(()) } diff --git a/services/identity/src/grpc_services/authenticated.rs b/services/identity/src/grpc_services/authenticated.rs --- a/services/identity/src/grpc_services/authenticated.rs +++ b/services/identity/src/grpc_services/authenticated.rs @@ -558,11 +558,17 @@ let parsed_device_list: SignedDeviceList = message.signed_device_list.parse()?; + let maybe_keyserver_device_id = self + .db_client + .get_keyserver_device_id_for_user(&user_id) + .await?; + let update_payload = DeviceListUpdate::try_from(parsed_device_list)?; crate::device_list::verify_singleton_device_list( &update_payload, &device_id, None, + maybe_keyserver_device_id.as_ref(), )?; self diff --git a/services/identity/src/grpc_utils.rs b/services/identity/src/grpc_utils.rs --- a/services/identity/src/grpc_utils.rs +++ b/services/identity/src/grpc_utils.rs @@ -328,6 +328,8 @@ &update_payload, &primary_device_id, None, + // when registering user, one has no keyserver yet + None, )?; Ok(Some(signed_list))