From fc5b786fb72e4f774b8f0e2beee57973bbbd72a3 Mon Sep 17 00:00:00 2001 From: moiseev-signal <122060238+moiseev-signal@users.noreply.github.com> Date: Thu, 5 Dec 2024 15:51:00 -0800 Subject: [PATCH] keytrans: Remove ChatSearchContext type --- .../libsignal/net/KeyTransparencyClient.java | 37 ++------ .../org/signal/libsignal/internal/Native.java | 5 +- rust/bridge/shared/src/net/keytrans.rs | 44 +++++----- rust/bridge/shared/types/src/keytrans.rs | 3 +- rust/net/src/keytrans.rs | 85 ++++++++----------- 5 files changed, 65 insertions(+), 109 deletions(-) diff --git a/java/client/src/main/java/org/signal/libsignal/net/KeyTransparencyClient.java b/java/client/src/main/java/org/signal/libsignal/net/KeyTransparencyClient.java index 4e06fd69e..b23b79584 100644 --- a/java/client/src/main/java/org/signal/libsignal/net/KeyTransparencyClient.java +++ b/java/client/src/main/java/org/signal/libsignal/net/KeyTransparencyClient.java @@ -7,7 +7,6 @@ package org.signal.libsignal.net; import java.util.Optional; import org.signal.libsignal.internal.CompletableFuture; -import org.signal.libsignal.internal.FilterExceptions; import org.signal.libsignal.internal.Native; import org.signal.libsignal.internal.NativeHandleGuard; import org.signal.libsignal.keytrans.SearchResult; @@ -85,14 +84,12 @@ public class KeyTransparencyClient { this.search( aci, aciIdentityKey, e164, unidentifiedAccessKey, usernameHash, store)); } - // Decoding of the last distinguished tree head happens "eagerly" while constructing the - // SearchContext. It may result in an IllegalArgumentError. - SearchContext searchContext = - new SearchContext(store.getAccountData(aci).orElse(null), lastDistinguishedTreeHead.get()); + // Decoding of the last distinguished tree head happens "eagerly" before making any network + // requests. + // It may result in an IllegalArgumentException. try (NativeHandleGuard tokioContextGuard = this.tokioAsyncContext.guard(); NativeHandleGuard chatGuard = chat.guard(); - NativeHandleGuard identityKeyGuard = aciIdentityKey.getPublicKey().guard(); - NativeHandleGuard searchContextGuard = searchContext.guard()) { + NativeHandleGuard identityKeyGuard = aciIdentityKey.getPublicKey().guard()) { return Native.KeyTransparency_Search( tokioContextGuard.nativeHandle(), chat.environment.value, @@ -102,7 +99,8 @@ public class KeyTransparencyClient { e164, unidentifiedAccessKey, usernameHash, - searchContextGuard.nativeHandle()) + store.getAccountData(aci).orElse(null), + lastDistinguishedTreeHead.get()) .thenApply( (handle) -> { SearchResult result = new SearchResult(handle); @@ -154,27 +152,4 @@ public class KeyTransparencyClient { }); } } - - /** - * Extra data accompanying the key transparency search request. - * - *

This is an implementation detail of the {@link - * org.signal.libsignal.net.KeyTransparencyClient} and is not supposed to be used directly. The - * context will be populated from the {@link Store}. - */ - static final class SearchContext extends NativeHandleGuard.SimpleOwner { - SearchContext(byte[] accountData, byte[] lastDistinguishedTreeHead) { - super( - FilterExceptions.filterExceptions( - () -> - // The only exception it can throw is an IllegalArgumentException, even though it - // is bridged as throwing Exception. - Native.KeyTransparency_NewSearchContext(accountData, lastDistinguishedTreeHead))); - } - - @Override - protected void release(long nativeHandle) { - Native.ChatSearchContext_Destroy(nativeHandle); - } - } } diff --git a/java/shared/java/org/signal/libsignal/internal/Native.java b/java/shared/java/org/signal/libsignal/internal/Native.java index 75531dfeb..8f1697f58 100644 --- a/java/shared/java/org/signal/libsignal/internal/Native.java +++ b/java/shared/java/org/signal/libsignal/internal/Native.java @@ -208,8 +208,6 @@ public final class Native { public static native CompletableFuture CdsiLookup_new(long asyncRuntime, long connectionManager, String username, String password, long request); public static native byte[] CdsiLookup_token(long lookup); - public static native void ChatSearchContext_Destroy(long handle); - public static native void ChatService_SetListenerAuth(long runtime, long chat, BridgeChatListener listener); public static native void ChatService_SetListenerUnauth(long runtime, long chat, BridgeChatListener listener); public static native CompletableFuture ChatService_auth_send(long asyncRuntime, long chat, long httpRequest, int timeoutMillis); @@ -373,8 +371,7 @@ public final class Native { public static native byte[] KeyTransparency_AciSearchKey(byte[] aci); public static native CompletableFuture KeyTransparency_Distinguished(long asyncRuntime, int environment, long chat, byte[] lastDistinguishedTreeHead); public static native byte[] KeyTransparency_E164SearchKey(String e164); - public static native long KeyTransparency_NewSearchContext(byte[] accountData, byte[] lastDistinguishedTreeHead) throws Exception; - public static native CompletableFuture KeyTransparency_Search(long asyncRuntime, int environment, long chat, byte[] aci, long aciIdentityKey, String e164, byte[] unidentifiedAccessKey, byte[] usernameHash, long context); + public static native CompletableFuture KeyTransparency_Search(long asyncRuntime, int environment, long chat, byte[] aci, long aciIdentityKey, String e164, byte[] unidentifiedAccessKey, byte[] usernameHash, byte[] accountData, byte[] lastDistinguishedTreeHead); public static native byte[] KeyTransparency_UsernameHashSearchKey(byte[] hash); public static native void KyberKeyPair_Destroy(long handle); diff --git a/rust/bridge/shared/src/net/keytrans.rs b/rust/bridge/shared/src/net/keytrans.rs index 700278a5c..4310dc5c5 100644 --- a/rust/bridge/shared/src/net/keytrans.rs +++ b/rust/bridge/shared/src/net/keytrans.rs @@ -10,10 +10,10 @@ use libsignal_bridge_types::net::chat::UnauthChat; pub use libsignal_bridge_types::net::{Environment, TokioAsyncContext}; use libsignal_bridge_types::support::AsType; use libsignal_core::{Aci, E164}; -use libsignal_keytrans::{KeyTransparency, LocalStateUpdate, StoredTreeHead}; -use libsignal_net::keytrans::{ - ChatSearchContext, Error, Kt, SearchKey, SearchResult, UsernameHash, +use libsignal_keytrans::{ + AccountData, KeyTransparency, LocalStateUpdate, StoredAccountData, StoredTreeHead, }; +use libsignal_net::keytrans::{Error, Kt, SearchKey, SearchResult, UsernameHash}; use libsignal_protocol::PublicKey; use prost::{DecodeError, Message}; @@ -35,7 +35,6 @@ fn KeyTransparency_UsernameHashSearchKey(hash: &[u8]) -> Vec { UsernameHash::from_slice(hash).as_search_key() } -bridge_handle_fns!(ChatSearchContext, clone = false, ffi = false, node = false); bridge_handle_fns!(SearchResult, clone = false, ffi = false, node = false); #[bridge_fn(node = false, ffi = false)] @@ -77,24 +76,8 @@ where T::decode(bytes.as_ref()) } -#[bridge_fn(node = false, ffi = false)] -fn KeyTransparency_NewSearchContext( - account_data: Option<&[u8]>, - last_distinguished_tree_head: &[u8], -) -> Result { - let account_data = account_data.map(try_decode).transpose()?; - let last_distinguished_tree_head = - try_decode(last_distinguished_tree_head).map(|stored: StoredTreeHead| stored.tree_head)?; - let distinguished_tree_head_size = last_distinguished_tree_head - .map(|head| head.tree_size) - .ok_or(Error::InvalidRequest("distinguished tree head is missing"))?; - Ok(ChatSearchContext { - account_data, - distinguished_tree_head_size, - }) -} - #[bridge_io(TokioAsyncContext, node = false, ffi = false)] +#[allow(clippy::too_many_arguments)] async fn KeyTransparency_Search( // TODO: it is currently possible to pass an env that does not match chat environment: AsType, @@ -104,7 +87,8 @@ async fn KeyTransparency_Search( e164: Option, unidentified_access_key: Option>, username_hash: Option>, - context: &ChatSearchContext, + account_data: Option>, + last_distinguished_tree_head: Box<[u8]>, ) -> Result { let username_hash = username_hash.map(UsernameHash::from); let config = environment @@ -135,13 +119,27 @@ async fn KeyTransparency_Search( } }; + let account_data = account_data + .map(|bytes| { + let stored: StoredAccountData = try_decode(bytes)?; + AccountData::try_from(stored).map_err(Error::from) + }) + .transpose()?; + + let last_distinguished_tree_head = + try_decode(last_distinguished_tree_head).map(|stored: StoredTreeHead| stored.tree_head)?; + let distinguished_tree_head_size = last_distinguished_tree_head + .map(|head| head.tree_size) + .ok_or(Error::InvalidRequest("distinguished tree head is missing"))?; + let result = kt .search( &aci, aci_identity_key, e164_pair, username_hash, - context.clone(), + account_data, + distinguished_tree_head_size, ) .await?; Ok(result) diff --git a/rust/bridge/shared/types/src/keytrans.rs b/rust/bridge/shared/types/src/keytrans.rs index c96c57868..16c85a2a6 100644 --- a/rust/bridge/shared/types/src/keytrans.rs +++ b/rust/bridge/shared/types/src/keytrans.rs @@ -2,9 +2,8 @@ // Copyright 2024 Signal Messenger, LLC. // SPDX-License-Identifier: AGPL-3.0-only // -use libsignal_net::keytrans::{ChatSearchContext, SearchResult}; +use libsignal_net::keytrans::SearchResult; use crate::*; -bridge_as_handle!(ChatSearchContext, ffi = false, node = false); bridge_as_handle!(SearchResult, ffi = false, node = false); diff --git a/rust/net/src/keytrans.rs b/rust/net/src/keytrans.rs index 712c46b4b..ebb64b427 100644 --- a/rust/net/src/keytrans.rs +++ b/rust/net/src/keytrans.rs @@ -82,6 +82,27 @@ struct RawChatSearchRequest { distinguished_tree_head_size: u64, } +impl RawChatSearchRequest { + fn new( + aci: &Aci, + aci_identity_key: &PublicKey, + e164: Option<&(E164, Vec)>, + username_hash: Option<&UsernameHash>, + account_data: Option<&AccountData>, + distinguished_tree_head_size: u64, + ) -> Self { + Self { + aci: aci.as_chat_value(), + aci_identity_key: BASE64_STANDARD.encode(aci_identity_key.serialize()), + e164: e164.map(|x| x.0.as_chat_value()), + username_hash: username_hash.map(|x| x.as_chat_value()), + unidentified_access_key: e164.map(|x| BASE64_STANDARD.encode(&x.1)), + last_tree_head_size: account_data.map(|acc| acc.last_tree_head.0.tree_size), + distinguished_tree_head_size, + } + } +} + impl From for chat::Request { fn from(request: RawChatSearchRequest) -> Self { Self { @@ -387,37 +408,6 @@ pub struct Kt<'a> { pub config: Config, } -#[derive(Debug, Clone, Default)] -pub struct ChatSearchContext { - pub account_data: Option, - pub distinguished_tree_head_size: u64, -} - -impl ChatSearchContext { - fn make_raw_request( - &self, - aci: &Aci, - aci_identity_key: &PublicKey, - e164: Option<&(E164, Vec)>, - username_hash: Option<&UsernameHash>, - ) -> RawChatSearchRequest { - fn get_tree_size(account_data: &StoredAccountData) -> Option { - let stored = account_data.last_tree_head.as_ref()?; - let head = stored.tree_head.as_ref()?; - Some(head.tree_size) - } - RawChatSearchRequest { - aci: aci.as_chat_value(), - aci_identity_key: BASE64_STANDARD.encode(aci_identity_key.serialize()), - e164: e164.map(|x| x.0.as_chat_value()), - username_hash: username_hash.map(|x| x.as_chat_value()), - unidentified_access_key: e164.map(|x| BASE64_STANDARD.encode(&x.1)), - last_tree_head_size: self.account_data.as_ref().and_then(get_tree_size), - distinguished_tree_head_size: self.distinguished_tree_head_size, - } - } -} - #[derive(Debug, Clone)] pub struct SearchResult { pub aci_identity_key: IdentityKey, @@ -451,10 +441,17 @@ impl<'a> Kt<'a> { aci_identity_key: &PublicKey, e164: Option<(E164, Vec)>, username_hash: Option>, - context: ChatSearchContext, + stored_account_data: Option, + distinguished_tree_head_size: u64, ) -> Result { - let raw_request = - context.make_raw_request(aci, aci_identity_key, e164.as_ref(), username_hash.as_ref()); + let raw_request = RawChatSearchRequest::new( + aci, + aci_identity_key, + e164.as_ref(), + username_hash.as_ref(), + stored_account_data.as_ref(), + distinguished_tree_head_size, + ); let response = self.send(raw_request.into()).await?; let chat_search_response = RawChatSerializedResponse::try_from(response) @@ -463,12 +460,6 @@ impl<'a> Kt<'a> { TypedSearchResponse::from_untyped(e164.is_some(), username_hash.is_some(), r) })?; - let ChatSearchContext { - account_data: stored_account_data, - distinguished_tree_head_size: _, - } = context; - - let account_data = stored_account_data.map(AccountData::try_from).transpose()?; let now = SystemTime::now(); // TODO: this could be a `validate` on the TypedSearchResponse, but then what to accept as the "expected" arguments? @@ -486,7 +477,7 @@ impl<'a> Kt<'a> { e164_monitoring_data, username_hash_monitoring_data, stored_last_tree_head, - ) = match account_data { + ) = match stored_account_data { None => (None, None, None, None), Some(acc) => { let AccountData { @@ -993,10 +984,8 @@ mod test { &aci_identity_key, use_e164.then_some(e164), use_username_hash.then_some(username_hash), - ChatSearchContext { - account_data: None, - distinguished_tree_head_size, - }, + None, + distinguished_tree_head_size, ) .await; @@ -1058,10 +1047,8 @@ mod test { &aci_identity_key, use_e164.then_some(e164), use_username_hash.then_some(username_hash.clone()), - ChatSearchContext { - account_data: None, - distinguished_tree_head_size, - }, + None, + distinguished_tree_head_size, ) .await .expect("can search");