diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 0247558c8..c0cb276a0 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -1,2 +1,4 @@ v0.92.2 + +- keytrans: Periodically update distinguished tree head inside libsignal diff --git a/java/client/src/main/java/org/signal/libsignal/net/KeyTransparencyClient.kt b/java/client/src/main/java/org/signal/libsignal/net/KeyTransparencyClient.kt index 3c5237204..2b0d8499e 100644 --- a/java/client/src/main/java/org/signal/libsignal/net/KeyTransparencyClient.kt +++ b/java/client/src/main/java/org/signal/libsignal/net/KeyTransparencyClient.kt @@ -49,65 +49,6 @@ public class KeyTransparencyClient internal constructor( private val tokioAsyncContext: TokioAsyncContext, private val environment: Network.Environment, ) { - /** - * Request the latest distinguished tree head from the server and update it in the local store. - * - * Possible non-success results include: - * - [RequestResult.RetryableNetworkError] for errors related to communication with the server, - * including [RetryLaterException] when the client is being throttled, - * [ServerSideErrorException], [NetworkException], [NetworkProtocolException], and - * [TimeoutException]. - * - [RequestResult.NonSuccess] with [KeyTransparencyException] for errors related to key - * transparency logic. Retrying without changing any of the arguments (including the state of - * the store) is unlikely to yield a different result. - * - [RequestResult.NonSuccess] with [VerificationFailedException] (a subclass of - * [KeyTransparencyException]) indicating a failure to verify the data in key transparency - * server response, such as an incorrect proof or a wrong signature. - * - [RequestResult.ApplicationError] for invalid arguments or other caller errors that could have - * been avoided. - * - * @param store local persistent storage for key transparency related data, such as the latest - * tree heads and account monitoring data. It will be queried for the latest distinguished tree - * head before performing the server request and updated with data from the server response if - * it succeeds. Distinguished tree does not have to be present in the store prior to the call. - * @return an instance of [CompletableFuture] that completes with a [RequestResult] indicating - * success or containing the error details. - */ - public fun updateDistinguished(store: Store): CompletableFuture> { - val lastDistinguished = - try { - store.lastDistinguishedTreeHead.orElse(null) - } catch (t: Throwable) { - return CompletableFuture.completedFuture(RequestResult.ApplicationError(t)) - } - - return try { - NativeHandleGuard(tokioAsyncContext).use { tokioContextGuard -> - NativeHandleGuard(chatConnection).use { chatConnectionGuard -> - Native - .KeyTransparency_Distinguished( - tokioContextGuard.nativeHandle(), - environment.value, - chatConnectionGuard.nativeHandle(), - lastDistinguished, - ).mapWithCancellation( - onSuccess = { distinguished -> - try { - store.setLastDistinguishedTreeHead(distinguished) - RequestResult.Success(Unit) - } catch (t: Throwable) { - RequestResult.ApplicationError(t) - } - }, - onError = { err -> err.toRequestResult() }, - ) - } - } - } catch (t: Throwable) { - CompletableFuture.completedFuture(RequestResult.ApplicationError(t)) - } - } - /** * A unified key transparency operation that performs a search, a monitor, or both. * @@ -124,9 +65,6 @@ public class KeyTransparencyClient internal constructor( * - [CheckMode.Contact] - Another search request will be performed automatically and, if it succeeds, * the updated account data will be stored. * - * If the latest distinguished tree head is not present in the store, it will be requested from - * the server prior to performing the monitor via [updateDistinguished]. - * * Possible non-success results include: * - [RequestResult.RetryableNetworkError] for errors related to communication with the server, * including [RetryLaterException] when the client is being throttled, @@ -172,16 +110,6 @@ public class KeyTransparencyClient internal constructor( return CompletableFuture.completedFuture(RequestResult.ApplicationError(t)) } - if (lastDistinguishedTreeHead.isEmpty) { - return updateDistinguished(store).thenCompose { result -> - when (result) { - is RequestResult.Success -> - check(mode, aci, aciIdentityKey, e164, unidentifiedAccessKey, usernameHash, store) - else -> CompletableFuture.completedFuture(result) - } - } - } - return try { NativeHandleGuard(tokioAsyncContext).use { tokioContextGuard -> NativeHandleGuard(aciIdentityKey.publicKey).use { identityKeyGuard -> @@ -199,13 +127,16 @@ public class KeyTransparencyClient internal constructor( // Technically this is a required parameter, but passing null // to generate the error on the Rust side. store.getAccountData(aci).orElse(null), - lastDistinguishedTreeHead.get(), + lastDistinguishedTreeHead.orElse(null), mode.isSelf(), mode.isE164Discoverable() ?: true, ).mapWithCancellation( - onSuccess = { updatedAccountData -> + onSuccess = { (updatedAccountData, distinguished) -> try { store.setAccountData(aci, updatedAccountData) + if (distinguished.isNotEmpty()) { + store.setLastDistinguishedTreeHead(distinguished) + } RequestResult.Success(Unit) } catch (t: Throwable) { RequestResult.ApplicationError(t) diff --git a/java/client/src/main/java/org/signal/libsignal/net/UnauthenticatedChatConnection.java b/java/client/src/main/java/org/signal/libsignal/net/UnauthenticatedChatConnection.java index 3ad443d28..e5bdf28f9 100644 --- a/java/client/src/main/java/org/signal/libsignal/net/UnauthenticatedChatConnection.java +++ b/java/client/src/main/java/org/signal/libsignal/net/UnauthenticatedChatConnection.java @@ -33,7 +33,7 @@ public class UnauthenticatedChatConnection extends ChatConnection { this.keyTransparencyClient = new KeyTransparencyClient(this, tokioAsyncContext, ktEnvironment); } - private KeyTransparencyClient keyTransparencyClient; + private final KeyTransparencyClient keyTransparencyClient; static CompletableFuture connect( final TokioAsyncContext tokioAsyncContext, diff --git a/java/client/src/test/java/org/signal/libsignal/keytrans/TestStore.java b/java/client/src/test/java/org/signal/libsignal/keytrans/TestStore.java index 11cfc3e12..bfe8cd43b 100644 --- a/java/client/src/test/java/org/signal/libsignal/keytrans/TestStore.java +++ b/java/client/src/test/java/org/signal/libsignal/keytrans/TestStore.java @@ -11,16 +11,16 @@ import org.signal.libsignal.protocol.ServiceId; public class TestStore implements Store { public HashMap> storage = new HashMap<>(); - public byte[] lastDistinguishedTreeHead; + public Deque distinguishedTreeHeads = new ArrayDeque<>(); @Override public Optional getLastDistinguishedTreeHead() { - return Optional.ofNullable(lastDistinguishedTreeHead); + return Optional.ofNullable(this.distinguishedTreeHeads.peekLast()); } @Override public void setLastDistinguishedTreeHead(byte[] lastDistinguishedTreeHead) { - this.lastDistinguishedTreeHead = lastDistinguishedTreeHead; + this.distinguishedTreeHeads.push(lastDistinguishedTreeHead); } @Override diff --git a/java/client/src/test/java/org/signal/libsignal/net/KeyTransparencyClientTest.kt b/java/client/src/test/java/org/signal/libsignal/net/KeyTransparencyClientTest.kt index 128caf952..6fff7aff5 100644 --- a/java/client/src/test/java/org/signal/libsignal/net/KeyTransparencyClientTest.kt +++ b/java/client/src/test/java/org/signal/libsignal/net/KeyTransparencyClientTest.kt @@ -70,26 +70,10 @@ class KeyTransparencyClientTest { assertIs>(it) } - Assert.assertTrue(store.getLastDistinguishedTreeHead().isPresent) + Assert.assertTrue(store.lastDistinguishedTreeHead.isPresent) Assert.assertTrue(store.getAccountData(KeyTransparencyTest.TEST_ACI).isPresent) } - @Test - @Throws(Exception::class) - fun updateDistinguishedStagingIntegration() { - Assume.assumeTrue(INTEGRATION_TESTS_ENABLED) - - val net = Network(Network.Environment.STAGING, USER_AGENT, mapOf(), Network.BuildVariant.BETA) - val ktClient = connectAndGetClient(net).get() - - val store = TestStore() - ktClient.updateDistinguished(store).get().also { - assertIs>(it) - } - - Assert.assertTrue(store.getLastDistinguishedTreeHead().isPresent) - } - @Test @Throws(Exception::class) fun monitorInStagingIntegration() { @@ -118,6 +102,8 @@ class KeyTransparencyClientTest { // Following search there should be a single entry in the account history Assert.assertEquals(1, accountDataHistory.size.toLong()) + // Should have requested and stored the latest distinguished tree head + Assert.assertEquals(1, store.distinguishedTreeHeads.size) ktClient .check( @@ -134,6 +120,8 @@ class KeyTransparencyClientTest { } // Another entry in the account history after a successful monitor request Assert.assertEquals(2, accountDataHistory.size.toLong()) + // Should not have updated the distinguished tree head, as the last one was reused + Assert.assertEquals(1, store.distinguishedTreeHeads.size) } inline fun retryableNetworkExceptionsTestImpl( @@ -150,9 +138,20 @@ class KeyTransparencyClientTest { ) val store = TestStore() - val responseFuture = chat.keyTransparencyClient().updateDistinguished(store) + val responseFuture = + chat + .keyTransparencyClient() + .check( + CheckMode.Contact, + KeyTransparencyTest.TEST_ACI, + KeyTransparencyTest.TEST_ACI_IDENTITY_KEY, + null, + null, + null, + store, + ) - val (_, requestId) = remote.getNextIncomingRequest().get() + val (_, requestId) = remote.nextIncomingRequest.get() remote.sendResponse(requestId, statusCode, message, headers, byteArrayOf()) val result = responseFuture.get() @@ -174,9 +173,20 @@ class KeyTransparencyClientTest { ) val store = TestStore() - val responseFuture = chat.keyTransparencyClient().updateDistinguished(store) + val responseFuture = + chat + .keyTransparencyClient() + .check( + CheckMode.Contact, + KeyTransparencyTest.TEST_ACI, + KeyTransparencyTest.TEST_ACI_IDENTITY_KEY, + null, + null, + null, + store, + ) - val (_, requestId) = remote.getNextIncomingRequest().get() + val (_, requestId) = remote.nextIncomingRequest.get() remote.sendResponse(requestId, statusCode, message, headers, byteArrayOf()) val result = responseFuture.get() @@ -194,9 +204,20 @@ class KeyTransparencyClientTest { ) val store = TestStore() - val responseFuture = chat.keyTransparencyClient().updateDistinguished(store) + val responseFuture = + chat + .keyTransparencyClient() + .check( + CheckMode.Contact, + KeyTransparencyTest.TEST_ACI, + KeyTransparencyTest.TEST_ACI_IDENTITY_KEY, + null, + null, + null, + store, + ) - remote.getNextIncomingRequest().get() + remote.nextIncomingRequest.get() remote.guardedRun(NativeTesting::TESTING_FakeChatRemoteEnd_InjectConnectionInterrupted) val result = responseFuture.get() diff --git a/java/client/src/test/java/org/signal/libsignal/net/KeyTransparencyTest.java b/java/client/src/test/java/org/signal/libsignal/net/KeyTransparencyTest.java index e9bb75a8b..d184b6ddc 100644 --- a/java/client/src/test/java/org/signal/libsignal/net/KeyTransparencyTest.java +++ b/java/client/src/test/java/org/signal/libsignal/net/KeyTransparencyTest.java @@ -18,14 +18,14 @@ import org.signal.libsignal.protocol.ServiceId; import org.signal.libsignal.protocol.util.Hex; public class KeyTransparencyTest { - static final ServiceId.Aci TEST_ACI = + public static final ServiceId.Aci TEST_ACI = new ServiceId.Aci(UUID.fromString("90c979fd-eab4-4a08-b6da-69dedeab9b29")); - static final IdentityKey TEST_ACI_IDENTITY_KEY; - static final String TEST_E164 = "+18005550100"; - static final byte[] TEST_USERNAME_HASH = + public static final IdentityKey TEST_ACI_IDENTITY_KEY; + public static final String TEST_E164 = "+18005550100"; + public static final byte[] TEST_USERNAME_HASH = Hex.fromStringCondensedAssert( "dc711808c2cf66d5e6a33ce41f27d69d942d2e1ff4db22d39b42d2eff8d09746"); - static final byte[] TEST_UNIDENTIFIED_ACCESS_KEY = + public static final byte[] TEST_UNIDENTIFIED_ACCESS_KEY = Hex.fromStringCondensedAssert("108d84b71be307bdf101e380a1d7f2a2"); static { diff --git a/java/shared/java/org/signal/libsignal/internal/Native.kt b/java/shared/java/org/signal/libsignal/internal/Native.kt index c81b486bb..492915c55 100644 --- a/java/shared/java/org/signal/libsignal/internal/Native.kt +++ b/java/shared/java/org/signal/libsignal/internal/Native.kt @@ -619,9 +619,7 @@ internal object Native { @JvmStatic public external fun KeyTransparency_AciSearchKey(aci: ByteArray): ByteArray @JvmStatic - public external fun KeyTransparency_Check(asyncRuntime: ObjectHandle, environment: Int, chatConnection: ObjectHandle, aci: ByteArray, aciIdentityKey: ObjectHandle, e164: String?, unidentifiedAccessKey: ByteArray?, usernameHash: ByteArray?, accountData: ByteArray?, lastDistinguishedTreeHead: ByteArray, isSelfCheck: Boolean, isE164Discoverable: Boolean): CompletableFuture - @JvmStatic - public external fun KeyTransparency_Distinguished(asyncRuntime: ObjectHandle, environment: Int, chatConnection: ObjectHandle, lastDistinguishedTreeHead: ByteArray?): CompletableFuture + public external fun KeyTransparency_Check(asyncRuntime: ObjectHandle, environment: Int, chatConnection: ObjectHandle, aci: ByteArray, aciIdentityKey: ObjectHandle, e164: String?, unidentifiedAccessKey: ByteArray?, usernameHash: ByteArray?, accountData: ByteArray?, lastDistinguishedTreeHead: ByteArray?, isSelfCheck: Boolean, isE164Discoverable: Boolean): CompletableFuture> @JvmStatic public external fun KeyTransparency_E164SearchKey(e164: String): ByteArray @JvmStatic diff --git a/node/ts/Native.ts b/node/ts/Native.ts index 270b402e9..fecd37f08 100644 --- a/node/ts/Native.ts +++ b/node/ts/Native.ts @@ -511,8 +511,7 @@ type NativeFunctions = { KeyTransparency_AciSearchKey: (aci: Uint8Array) => Uint8Array; KeyTransparency_E164SearchKey: (e164: string) => Uint8Array; KeyTransparency_UsernameHashSearchKey: (hash: Uint8Array) => Uint8Array; - KeyTransparency_Check: (asyncRuntime: Wrapper, environment: number, chatConnection: Wrapper, aci: Uint8Array, aciIdentityKey: Wrapper, e164: string | null, unidentifiedAccessKey: Uint8Array | null, usernameHash: Uint8Array | null, accountData: Uint8Array | null, lastDistinguishedTreeHead: Uint8Array, isSelfCheck: boolean, isE164Discoverable: boolean) => CancellablePromise>; - KeyTransparency_Distinguished: (asyncRuntime: Wrapper, environment: number, chatConnection: Wrapper, lastDistinguishedTreeHead: Uint8Array | null) => CancellablePromise>; + KeyTransparency_Check: (asyncRuntime: Wrapper, environment: number, chatConnection: Wrapper, aci: Uint8Array, aciIdentityKey: Wrapper, e164: string | null, unidentifiedAccessKey: Uint8Array | null, usernameHash: Uint8Array | null, accountData: Uint8Array | null, lastDistinguishedTreeHead: Uint8Array | null, isSelfCheck: boolean, isE164Discoverable: boolean) => CancellablePromise<[Uint8Array, Uint8Array]>; RegistrationService_CreateSession: (asyncRuntime: Wrapper, createSession: RegistrationCreateSessionRequest, connectChat: ConnectChatBridge) => CancellablePromise; RegistrationService_ResumeSession: (asyncRuntime: Wrapper, sessionId: string, number: string, connectChat: ConnectChatBridge) => CancellablePromise; RegistrationService_RequestVerificationCode: (asyncRuntime: Wrapper, service: Wrapper, transport: string, client: string, languages: string[]) => CancellablePromise; @@ -1073,7 +1072,6 @@ const { registerErrors, KeyTransparency_E164SearchKey, KeyTransparency_UsernameHashSearchKey, KeyTransparency_Check, - KeyTransparency_Distinguished, RegistrationService_CreateSession, RegistrationService_ResumeSession, RegistrationService_RequestVerificationCode, @@ -1636,7 +1634,6 @@ export { registerErrors, KeyTransparency_E164SearchKey, KeyTransparency_UsernameHashSearchKey, KeyTransparency_Check, - KeyTransparency_Distinguished, RegistrationService_CreateSession, RegistrationService_ResumeSession, RegistrationService_RequestVerificationCode, diff --git a/node/ts/net/KeyTransparency.ts b/node/ts/net/KeyTransparency.ts index ce5989a32..d8aa9e54a 100644 --- a/node/ts/net/KeyTransparency.ts +++ b/node/ts/net/KeyTransparency.ts @@ -140,16 +140,15 @@ export interface Client { * * @param request - Key transparency client {@link Request}. * @param store - Local key transparency storage. It will be queried for both - * the account data and the latest distinguished tree head before sending the - * server request and, if the request succeeds, will be updated with the - * search operation results. + * the account data before sending the server request and, if the request + * succeeds, will be updated with the operation results. * @param options - options for the asynchronous operation. Optional. * * @returns A promise that resolves if the check succeeds and the local state has been updated * to reflect the latest changes. * * @throws {KeyTransparencyError} for errors related to key transparency logic, which - * includes missing required fields in the serialized data. Retrying the search without + * includes missing required fields in the serialized data. Retrying the check without * changing any of the arguments (including the state of the store) is unlikely to yield a * different result. * @throws {KeyTransparencyVerificationFailed} when it fails to @@ -181,10 +180,6 @@ export class ClientImpl implements Client { store: Store, options?: Readonly ): Promise { - const distinguished = await this._getLatestDistinguished( - store, - options ?? {} - ); const { abortSignal } = options ?? {}; const { aciInfo: { aci, identityKey: aciIdentityKey }, @@ -196,50 +191,27 @@ export class ClientImpl implements Client { e164: null, unidentifiedAccessKey: null, }; - const accountData = await this.asyncContext.makeCancellable( - abortSignal, - Native.KeyTransparency_Check( - this.asyncContext, - this.env, - this.chatService, - aci.getServiceIdFixedWidthBinary(), - aciIdentityKey, - e164, - unidentifiedAccessKey, - usernameHash ?? null, - await store.getAccountData(aci), - distinguished, - mode === 'self', - mode === 'self' ? request.isE164Discoverable : true - ) - ); + const [accountData, newDistinguished] = + await this.asyncContext.makeCancellable( + abortSignal, + Native.KeyTransparency_Check( + this.asyncContext, + this.env, + this.chatService, + aci.getServiceIdFixedWidthBinary(), + aciIdentityKey, + e164, + unidentifiedAccessKey, + usernameHash ?? null, + await store.getAccountData(aci), + await store.getLastDistinguishedTreeHead(), + mode === 'self', + mode === 'self' ? request.isE164Discoverable : true + ) + ); await store.setAccountData(aci, accountData); - } - - private async updateDistinguished( - store: Store, - { abortSignal }: Readonly - ): Promise> { - const bytes = await this.asyncContext.makeCancellable( - abortSignal, - Native.KeyTransparency_Distinguished( - this.asyncContext, - this.env, - this.chatService, - await store.getLastDistinguishedTreeHead() - ) - ); - await store.setLastDistinguishedTreeHead(bytes); - return bytes; - } - - async _getLatestDistinguished( - store: Store, - options: Readonly - ): Promise> { - return ( - (await store.getLastDistinguishedTreeHead()) ?? - (await this.updateDistinguished(store, options)) - ); + if (newDistinguished.length > 0) { + await store.setLastDistinguishedTreeHead(newDistinguished); + } } } diff --git a/node/ts/test/KeyTransparencyTest.ts b/node/ts/test/KeyTransparencyTest.ts index 3549aa6b2..e5e8092ec 100644 --- a/node/ts/test/KeyTransparencyTest.ts +++ b/node/ts/test/KeyTransparencyTest.ts @@ -108,7 +108,7 @@ describe('KeyTransparency network errors', () => { unauth._chatService, Environment.Staging ); - const promise = client._getLatestDistinguished(new InMemoryKtStore(), {}); + const promise = client.check(testRequest, new InMemoryKtStore(), {}); const request = await remote.assertReceiveIncomingRequest(); @@ -182,31 +182,36 @@ describe('KeyTransparency Integration', function (this: Mocha.Suite) { } expect(accountDataHistory.length).to.equal(1); + expect(store.distinguished.length).to.equal(1); await kt.check(testRequest, store, {}); expect(accountDataHistory.length).to.equal(2); + // Distinguished tree should not have been updated + expect(store.distinguished.length).to.equal(1); }); }); class InMemoryKtStore implements KT.Store { storage: Map, Array>>>; - distinguished: Readonly> | null; + distinguished: Array>>; constructor() { this.storage = new Map>>>(); - this.distinguished = null; + this.distinguished = new Array>(); } // eslint-disable-next-line @typescript-eslint/require-await async getLastDistinguishedTreeHead(): Promise | null> { - return this.distinguished; + return this.distinguished.at(-1) ?? null; } // eslint-disable-next-line @typescript-eslint/require-await async setLastDistinguishedTreeHead( bytes: Readonly> | null ) { - this.distinguished = bytes; + if (bytes !== null) { + this.distinguished.push(bytes); + } } // eslint-disable-next-line @typescript-eslint/require-await diff --git a/rust/bridge/ffi/cbindgen-testing.toml b/rust/bridge/ffi/cbindgen-testing.toml index df0672ba9..495f884e6 100644 --- a/rust/bridge/ffi/cbindgen-testing.toml +++ b/rust/bridge/ffi/cbindgen-testing.toml @@ -43,7 +43,6 @@ exclude = [ "CPromisebool", "CPromiseFfiCdsiLookupResponse", "CPromiseMutPointerRegistrationService", - "CPromiseOwnedBufferOfc_uchar", "FfiCdsiLookupResponse", "FfiCdsiLookupResponseEntry", "FfiChatListenerStruct", diff --git a/rust/bridge/shared/src/net/keytrans.rs b/rust/bridge/shared/src/net/keytrans.rs index 02d8decde..82908a781 100644 --- a/rust/bridge/shared/src/net/keytrans.rs +++ b/rust/bridge/shared/src/net/keytrans.rs @@ -11,13 +11,11 @@ use libsignal_bridge_types::net::chat::UnauthenticatedChatConnection; pub use libsignal_bridge_types::net::{Environment, TokioAsyncContext}; use libsignal_bridge_types::support::AsType; use libsignal_core::{Aci, E164}; -use libsignal_keytrans::{ - AccountData, LastTreeHead, LocalStateUpdate, StoredAccountData, StoredTreeHead, -}; +use libsignal_keytrans::{AccountData, StoredAccountData}; use libsignal_net_chat::api::RequestError; use libsignal_net_chat::api::keytrans::{ CheckMode, Error, KeyTransparencyClient, MaybePartial, SearchKey, TreeHeadWithTimestamp, - UnauthenticatedChatApi as _, UsernameHash, check, + UsernameHash, check, }; use libsignal_protocol::PublicKey; use prost::{DecodeError, Message}; @@ -52,10 +50,14 @@ async fn KeyTransparency_Check( unidentified_access_key: Option>, username_hash: Option>, account_data: Option>, - last_distinguished_tree_head: Box<[u8]>, + last_distinguished_tree_head: Option>, is_self_check: bool, is_e164_discoverable: bool, -) -> Result, RequestError> { + // Return a pair of (serialized account data, serialized distinguished) + // If the last_distinguished_tree_head was reused, serialized distinguished will be empty. + // Could have been an Option>, but that would be another layer of <> to handle in + // the bridging macro to the same effect. +) -> Result<(Vec, Vec), RequestError> { let config = environment.into_inner().env().keytrans_config; let username_hash = username_hash.map(UsernameHash::from); @@ -73,9 +75,12 @@ async fn KeyTransparency_Check( let e164_pair = make_e164_pair(e164, unidentified_access_key)?; - let last_distinguished_tree_head = try_decode_distinguished(last_distinguished_tree_head); + let last_distinguished_tree_head = + last_distinguished_tree_head.and_then(try_decode_distinguished); - let (maybe_partial_result, _updated_distinguished) = chat_connection + let previously_stored_distinguished = last_distinguished_tree_head.clone(); + + let (maybe_partial_result, updated_distinguished) = chat_connection .as_typed(|chat| { Box::pin(async move { let kt = KeyTransparencyClient::new(*chat, config); @@ -102,41 +107,15 @@ async fn KeyTransparency_Check( maybe_hash_search_key, now, )?; - // let serialized_distinguished = updated_distinguished.into_stored(now).encode_to_vec(); - Ok(serialized_account_data) -} - -#[bridge_io(TokioAsyncContext)] -async fn KeyTransparency_Distinguished( - // TODO: it is currently possible to pass an env that does not match chat - environment: AsType, - chat_connection: &UnauthenticatedChatConnection, - last_distinguished_tree_head: Option>, -) -> Result, RequestError> { - let config = environment.into_inner().env().keytrans_config; - - let known_distinguished = last_distinguished_tree_head - .map(try_decode) - .transpose() - .map_err(|_| invalid_request("could not decode account data"))? - .and_then(|stored: StoredTreeHead| stored.into_last_tree_head()); - - let LocalStateUpdate { - tree_head, - tree_root, - monitoring_data: _, - } = chat_connection - .as_typed(|chat| { - Box::pin(async move { - let kt = KeyTransparencyClient::new(*chat, config); - kt.distinguished(known_distinguished).await - }) - }) - .await?; - - let updated_distinguished = LastTreeHead(tree_head, tree_root).into_stored(SystemTime::now()); - let serialized = updated_distinguished.encode_to_vec(); - Ok(serialized) + let distinguished_to_be_stored = if let Some(known) = previously_stored_distinguished + && known.tree_head == updated_distinguished + { + // Distinguished has not been updated, we must keep its stored_at + vec![] + } else { + updated_distinguished.into_stored(now).encode_to_vec() + }; + Ok((serialized_account_data, distinguished_to_be_stored)) } fn invalid_request(msg: &'static str) -> RequestError { diff --git a/rust/bridge/shared/types/src/ffi/convert.rs b/rust/bridge/shared/types/src/ffi/convert.rs index f76d0c2f0..e3aa5afe9 100644 --- a/rust/bridge/shared/types/src/ffi/convert.rs +++ b/rust/bridge/shared/types/src/ffi/convert.rs @@ -1511,6 +1511,10 @@ macro_rules! ffi_result_type { // Like Result, we can't use `:ty` here because we need the resulting tokens to be matched // recursively. We can at least match several tokens in the second component though. (($a:tt, $($b:tt)+)) => (ffi::PairOf); + (($a:tt<$($aargs:tt),+>, $b:tt<$($bargs:tt),+>)) => (ffi::PairOf< + ffi_result_type!($a<$($aargs),+>), + ffi_result_type!($b<$($bargs),+>) + >); (Option<($a:tt, $($b:tt)+)>) => (ffi::OptionalPairOf); (u8) => (u8); diff --git a/rust/bridge/shared/types/src/jni/convert.rs b/rust/bridge/shared/types/src/jni/convert.rs index 6382f8445..993f66db5 100644 --- a/rust/bridge/shared/types/src/jni/convert.rs +++ b/rust/bridge/shared/types/src/jni/convert.rs @@ -2865,6 +2865,12 @@ macro_rules! jni_result_type { (Result<$typ:tt<$($args:tt),+> $(, $_:ty)?>) => { $crate::jni::Throwing)> }; + (Result<($a:tt, $b:tt>)) => { + $crate::jni::Throwing + }; + (Result<($a:tt<$($aargs:tt),+>, $b:tt<$($bargs:tt),+>) $(, $_:ty)?>) => { + $crate::jni::Throwing, $b<$($bargs),+>))> + }; (Option) => { ::jni::sys::jint }; @@ -2886,6 +2892,9 @@ macro_rules! jni_result_type { (($a:tt, $b:tt)) => { $crate::jni::JavaPair<'local, $crate::jni_result_type!($a), $crate::jni_result_type!($b)> }; + (($a:tt<$($aargs:tt),+>, $b:tt<$($bargs:tt),+>)) => { + $crate::jni::JavaPair<'local, $crate::jni_result_type!($a<$($aargs),+>), $crate::jni_result_type!($b<$($bargs),+>)> + }; (bool) => { ::jni::sys::jboolean }; diff --git a/rust/net/chat/src/api/keytrans/monitor_and_search.rs b/rust/net/chat/src/api/keytrans/monitor_and_search.rs index a5007e8da..df3397bd6 100644 --- a/rust/net/chat/src/api/keytrans/monitor_and_search.rs +++ b/rust/net/chat/src/api/keytrans/monitor_and_search.rs @@ -87,7 +87,8 @@ async fn update_distinguished_if_needed( Ok(LastTreeHead(tree_head, tree_root)) } -#[cfg_attr(test, derive(Clone, PartialEq))] +#[cfg_attr(test, derive(PartialEq))] +#[derive(Clone)] pub struct TreeHeadWithTimestamp { pub tree_head: LastTreeHead, pub stored_at_ms: u64, diff --git a/rust/net/chat/src/ws/keytrans.rs b/rust/net/chat/src/ws/keytrans.rs index b6caad3bb..cec2e3955 100644 --- a/rust/net/chat/src/ws/keytrans.rs +++ b/rust/net/chat/src/ws/keytrans.rs @@ -691,7 +691,7 @@ mod test { } #[tokio::test] - async fn search_with_wrong_identity_key() { + async fn search_with_wrong_identity_key_integration() { if !kt_integration_enabled() { println!("SKIPPED: running integration tests is not enabled"); return; diff --git a/swift/Sources/LibSignalClient/AsyncUtils.swift b/swift/Sources/LibSignalClient/AsyncUtils.swift index 956b85019..9fc5a75ef 100644 --- a/swift/Sources/LibSignalClient/AsyncUtils.swift +++ b/swift/Sources/LibSignalClient/AsyncUtils.swift @@ -100,6 +100,10 @@ extension SignalCPromiseOptionalPairOfc_charu832: PromiseStruct { typealias Result = SignalOptionalPairOfc_charu832 } +extension SignalCPromisePairOfOwnedBufferOfc_ucharOwnedBufferOfc_uchar: PromiseStruct { + typealias Result = SignalPairOfOwnedBufferOfc_ucharOwnedBufferOfc_uchar +} + extension SignalCPromiseFfiPreKeysResponse: PromiseStruct { typealias Result = SignalFfiPreKeysResponse } diff --git a/swift/Sources/LibSignalClient/KeyTransparency.swift b/swift/Sources/LibSignalClient/KeyTransparency.swift index eb460d592..1b7115f93 100644 --- a/swift/Sources/LibSignalClient/KeyTransparency.swift +++ b/swift/Sources/LibSignalClient/KeyTransparency.swift @@ -132,9 +132,8 @@ public enum KeyTransparency { /// - e164Info: E.164 identifying information. Optional. /// - usernameHash: Hash of the username. Optional. /// - store: Local key transparency storage. It will be queried for both - /// the account data and the latest distinguished tree head before sending the - /// server request and, if the request succeeds, will be updated with the - /// search operation results. + /// the account data before sending the server request and, if the + /// request succeeds, will be updated with the check results. /// - Throws: /// - ``SignalError/keyTransparencyError`` for errors related to key transparency logic, which /// includes missing required fields in the serialized data. Retrying the search without @@ -168,9 +167,9 @@ public enum KeyTransparency { let uak = e164Info?.unidentifiedAccessKey let accountData = await store.getAccountData(for: aciInfo.aci) - let distinguished = try await self.updateDistinguished(store) + let knownDistinguished = await store.getLastDistinguishedTreeHead() - let bytes = try await self.asyncContext.invokeAsyncFunction { promise, tokioContext in + let rawResponse = try await self.asyncContext.invokeAsyncFunction { promise, tokioContext in try! withAllBorrowed( self.chatConnection, aciInfo.aci, @@ -178,7 +177,7 @@ public enum KeyTransparency { uak, usernameHash, accountData, - distinguished + knownDistinguished ) { chatHandle, aciBytes, identityKeyHandle, uakBytes, hashBytes, accDataBytes, distinguishedBytes in signal_key_transparency_check( promise, @@ -197,31 +196,13 @@ public enum KeyTransparency { ) } } - await store.setAccountData(Data(consuming: bytes), for: aciInfo.aci) - } + let updatedAccountData = Data(consuming: rawResponse.first) + let updatedDistinguished = Data(consuming: rawResponse.second) - private func updateDistinguished(_ store: some Store) async throws -> Data { - let knownDistinguished = await store.getLastDistinguishedTreeHead() - let latestDistinguished = try await getDistinguished(knownDistinguished) - await store.setLastDistinguishedTreeHead(to: latestDistinguished) - return latestDistinguished - } - - internal func getDistinguished( - _ distinguished: Data? = nil - ) async throws -> Data { - let bytes = try await self.asyncContext.invokeAsyncFunction { promise, tokioContext in - try! withAllBorrowed(self.chatConnection, distinguished) { chatHandle, distinguishedBytes in - signal_key_transparency_distinguished( - promise, - tokioContext.const(), - self.environment.rawValue, - chatHandle.const(), - distinguishedBytes - ) - } + await store.setAccountData(updatedAccountData, for: aciInfo.aci) + if !updatedDistinguished.isEmpty { + await store.setLastDistinguishedTreeHead(to: updatedDistinguished) } - return Data(consuming: bytes) } } } diff --git a/swift/Sources/SignalFfi/signal_ffi.h b/swift/Sources/SignalFfi/signal_ffi.h index 44243ce1f..676f7bbe1 100644 --- a/swift/Sources/SignalFfi/signal_ffi.h +++ b/swift/Sources/SignalFfi/signal_ffi.h @@ -1125,6 +1125,11 @@ typedef struct { SignalFfiLoggerDestroy destroy; } SignalFfiLoggerStruct; +typedef struct { + SignalOwnedBuffer first; + SignalOwnedBuffer second; +} SignalPairOfOwnedBufferOfc_ucharOwnedBufferOfc_uchar; + /** * A C callback used to report the results of Rust futures. * @@ -1135,10 +1140,10 @@ typedef struct { * completed once. */ typedef struct { - void (*complete)(SignalFfiError *error, const SignalOwnedBuffer *result, const void *context); + void (*complete)(SignalFfiError *error, const SignalPairOfOwnedBufferOfc_ucharOwnedBufferOfc_uchar *result, const void *context); const void *context; SignalCancellationId cancellation_id; -} SignalCPromiseOwnedBufferOfc_uchar; +} SignalCPromisePairOfOwnedBufferOfc_ucharOwnedBufferOfc_uchar; typedef struct { const SignalUnauthenticatedChatConnection *raw; @@ -2126,9 +2131,7 @@ bool signal_init_logger(SignalLogLevel max_level, SignalFfiLoggerStruct logger); SignalFfiError *signal_key_transparency_aci_search_key(SignalOwnedBuffer *out, const SignalServiceIdFixedWidthBinaryBytes *aci); -SignalFfiError *signal_key_transparency_check(SignalCPromiseOwnedBufferOfc_uchar *promise, SignalConstPointerTokioAsyncContext async_runtime, uint8_t environment, SignalConstPointerUnauthenticatedChatConnection chat_connection, const SignalServiceIdFixedWidthBinaryBytes *aci, SignalConstPointerPublicKey aci_identity_key, const char *e164, SignalOptionalBorrowedSliceOfc_uchar unidentified_access_key, SignalOptionalBorrowedSliceOfc_uchar username_hash, SignalOptionalBorrowedSliceOfc_uchar account_data, SignalBorrowedBuffer last_distinguished_tree_head, bool is_self_check, bool is_e164_discoverable); - -SignalFfiError *signal_key_transparency_distinguished(SignalCPromiseOwnedBufferOfc_uchar *promise, SignalConstPointerTokioAsyncContext async_runtime, uint8_t environment, SignalConstPointerUnauthenticatedChatConnection chat_connection, SignalOptionalBorrowedSliceOfc_uchar last_distinguished_tree_head); +SignalFfiError *signal_key_transparency_check(SignalCPromisePairOfOwnedBufferOfc_ucharOwnedBufferOfc_uchar *promise, SignalConstPointerTokioAsyncContext async_runtime, uint8_t environment, SignalConstPointerUnauthenticatedChatConnection chat_connection, const SignalServiceIdFixedWidthBinaryBytes *aci, SignalConstPointerPublicKey aci_identity_key, const char *e164, SignalOptionalBorrowedSliceOfc_uchar unidentified_access_key, SignalOptionalBorrowedSliceOfc_uchar username_hash, SignalOptionalBorrowedSliceOfc_uchar account_data, SignalOptionalBorrowedSliceOfc_uchar last_distinguished_tree_head, bool is_self_check, bool is_e164_discoverable); SignalFfiError *signal_key_transparency_e164_search_key(SignalOwnedBuffer *out, const char *e164); diff --git a/swift/Sources/SignalFfi/signal_ffi_testing.h b/swift/Sources/SignalFfi/signal_ffi_testing.h index 7730a1e29..6a5e89ba9 100644 --- a/swift/Sources/SignalFfi/signal_ffi_testing.h +++ b/swift/Sources/SignalFfi/signal_ffi_testing.h @@ -236,6 +236,21 @@ typedef struct { SignalTestingSemaphore *raw; } SignalMutPointerTestingSemaphore; +/** + * A C callback used to report the results of Rust futures. + * + * cbindgen will produce independent C types like `SignalCPromisei32` and + * `SignalCPromiseProtocolAddress`. + * + * This derives Copy because it behaves like a C type; nevertheless, a promise should still only be + * completed once. + */ +typedef struct { + void (*complete)(SignalFfiError *error, const SignalOwnedBuffer *result, const void *context); + const void *context; + SignalRawCancellationId cancellation_id; +} SignalCPromiseOwnedBufferOfc_uchar; + typedef struct { SignalTestingValueHolder *raw; } SignalMutPointerTestingValueHolder; diff --git a/swift/Tests/LibSignalClientTests/KeyTransparencyTests.swift b/swift/Tests/LibSignalClientTests/KeyTransparencyTests.swift index 16e45e461..5e1b7579c 100644 --- a/swift/Tests/LibSignalClientTests/KeyTransparencyTests.swift +++ b/swift/Tests/LibSignalClientTests/KeyTransparencyTests.swift @@ -81,16 +81,6 @@ final class KeyTransparencyTests: TestCaseBase { func connectionWasInterrupted(_: UnauthenticatedChatConnection, error: Error?) {} } - func testUnknownDistinguished() async throws { - try self.nonHermeticTest() - - let net = Net(env: .staging, userAgent: userAgent, buildVariant: .production) - let chat = try await net.connectUnauthenticatedChat() - chat.start(listener: NoOpListener()) - - XCTAssertNoThrow { try await chat.keyTransparencyClient.getDistinguished() } - } - func testCheck() async throws { try self.nonHermeticTest() @@ -108,6 +98,8 @@ final class KeyTransparencyTests: TestCaseBase { store: store ) XCTAssertEqual(1, store.accountData[self.testAccount.aci]!.count) + XCTAssertEqual(1, store.distinguishedTreeHeads.count) + try await chat.keyTransparencyClient.check( for: .contact, account: self.testAccount.aciInfo, @@ -115,8 +107,10 @@ final class KeyTransparencyTests: TestCaseBase { store: store ) // Second check will send a monitor request, and should update account - // data in store + // data in store, but the distinguished tree should have been reused + // and not updated XCTAssertEqual(2, store.accountData[self.testAccount.aci]!.count) + XCTAssertEqual(1, store.distinguishedTreeHeads.count) } // These testing endpoints aren't generated in device builds, to save on code size. @@ -159,7 +153,13 @@ final class KeyTransparencyTests: TestCaseBase { ) defer { withExtendedLifetime(chat) {} } - async let future = chat.keyTransparencyClient.getDistinguished() + let store = TestStore() + let aciInfo = self.testAccount.aciInfo + async let future: () = chat.keyTransparencyClient.check( + for: .contact, + account: aciInfo, + store: store + ) let (_, id) = try await remote.getNextIncomingRequest()