Skip to content

Commit

Permalink
MXCrypto: Only create one olm session at a time per device
Browse files Browse the repository at this point in the history 
element-hq/element-ios/issues/2331
  • Loading branch information
@manuroe
manuroe committed May 28, 2020
1 parent 04b4d58 commit 59e087a
Show file tree
Hide file tree
Showing 3 changed files with 197 additions and 23 deletions.
1 change: 1 addition & 0 deletions CHANGES.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ Changes in Matrix iOS SDK in 0.16.6 (2020-05-xx)
================================================

Improvements:
* MXCrypto: Only create one olm session at a time per device (vector-im/riot-ios/issues/2331).

Bug fix:
* MXSecretShareManager: Fix crash in cancelRequestWithRequestId (vector-im/riot-ios/issues/3272).
Expand Down
151 changes: 128 additions & 23 deletions MatrixSDK/Crypto/MXCrypto.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@
#import "MXCrossSigningInfo_Private.h"
#import "MXCrossSigning_Private.h"

#import "NSArray+MatrixSDK.h"

/**
The store to use for crypto.
*/
Expand All @@ -57,6 +59,11 @@

NSString *const MXDeviceListDidUpdateUsersDevicesNotification = @"MXDeviceListDidUpdateUsersDevicesNotification";

static NSString *const kMXCryptoOneTimeKeyClaimCompleteNotification = @"kMXCryptoOneTimeKeyClaimCompleteNotification";
static NSString *const kMXCryptoOneTimeKeyClaimCompleteNotificationDevicesKey = @"kMXCryptoOneTimeKeyClaimCompleteNotificationDevicesKey";
static NSString *const kMXCryptoOneTimeKeyClaimCompleteNotificationErrorKey = @"kMXCryptoOneTimeKeyClaimCompleteNotificationErrorKey";


#ifdef MX_CRYPTO

// Frequency with which to check & upload one-time keys
Expand Down Expand Up @@ -111,6 +118,10 @@ @interface MXCrypto ()
// The queue to manage bulk import and export of keys.
// It only reads and writes keys from and to the crypto store.
dispatch_queue_t cargoQueue;

// The list of devices (by their identity key) we are establishing
// an olm session with.
NSMutableArray<NSString*> *ensureOlmSessionsInProgress;
}
@end

Expand Down Expand Up @@ -1729,7 +1740,8 @@ - (instancetype)initWithMatrixSession:(MXSession*)matrixSession cryptoQueue:(dis
decryptionQueue = [MXCrypto dispatchQueueForUser:_mxSession.matrixRestClient.credentials.userId];

cargoQueue = dispatch_queue_create([NSString stringWithFormat:@"MXCrypto-Cargo-%@", _mxSession.myDeviceId].UTF8String, DISPATCH_QUEUE_SERIAL);


ensureOlmSessionsInProgress = [NSMutableArray array];

_olmDevice = [[MXOlmDevice alloc] initWithStore:_store];

Expand Down Expand Up @@ -1964,27 +1976,118 @@ - (MXHTTPOperation*)ensureOlmSessionsForDevices:(NSDictionary<NSString* /* userI

if (devicesWithoutSession.count == 0)
{
NSLog(@"[MXCrypto] ensureOlmSessionsForDevices: Have already sessions for all");
if (success)
{
success(results);
}
return nil;
}


NSString *oneTimeKeyAlgorithm = kMXKeySignedCurve25519Type;

// Prepare the request for claiming one-time keys
// Devices for which we will make a /claim request
MXUsersDevicesMap<NSString*> *usersDevicesToClaim = [[MXUsersDevicesMap<NSString*> alloc] init];
// The same but devices are listed by their identity key
NSMutableArray<NSString*> *devicesToClaim = [NSMutableArray array];

// Devices (by their identity key) that are waiting for a response to /claim request
// That can be devices for which we are going to make a /claim request OR devices that
// already have a pending requests.
// Once we have emptied this array, we can call the success or the failure block. The
// operation is complete.
NSMutableArray<NSString*> *devicesInProgress = [NSMutableArray array];

// Prepare the request for claiming one-time keys
for (MXDeviceInfo *device in devicesWithoutSession)
{
[usersDevicesToClaim setObject:oneTimeKeyAlgorithm forUser:device.userId andDevice:device.deviceId];
NSString *deviceIdentityKey = device.identityKey;

// Claim only if a request is not yet pending
if (![ensureOlmSessionsInProgress containsObject:deviceIdentityKey])
{
[usersDevicesToClaim setObject:oneTimeKeyAlgorithm forUser:device.userId andDevice:device.deviceId];
[devicesToClaim addObject:deviceIdentityKey];

[ensureOlmSessionsInProgress addObject:deviceIdentityKey];
}

[devicesInProgress addObject:deviceIdentityKey];
}

// TODO: this has a race condition - if we try to send another message
// while we are claiming a key, we will end up claiming two and setting up
// two sessions.
//
// That should eventually resolve itself, but it's poor form.

NSLog(@"[MXCrypto] ensureOlmSessionsForDevices: %@ out of %@ sessions to claim one time keys", @(usersDevicesToClaim.count), @(devicesWithoutSession.count));


// Wait for the result of claim request(s)
// Listen to the dedicated notification
MXWeakify(self);
__block id observer;
observer = [[NSNotificationCenter defaultCenter] addObserverForName:kMXCryptoOneTimeKeyClaimCompleteNotification object:self queue:nil usingBlock:^(NSNotification * _Nonnull note) {
MXStrongifyAndReturnIfNil(self);

NSMutableArray<NSString*> *devices = note.userInfo[kMXCryptoOneTimeKeyClaimCompleteNotificationDevicesKey];
NSError *error = note.userInfo[kMXCryptoOneTimeKeyClaimCompleteNotificationErrorKey];

// Was it a /claim request for us?
if ([devicesInProgress mx_intersectArray:devices])
{
if (error)
{
NSLog(@"[MXCrypto] ensureOlmSessionsForDevices: Got a notification failure for %@ devices. Fail our current pool of %@ devices", @(devices.count), @(devicesInProgress.count));

// Consider the failure for all requests of the current pool
[self->ensureOlmSessionsInProgress removeObjectsInArray:devicesInProgress];
[devicesInProgress removeAllObjects];

// The game is over for this pool
[[NSNotificationCenter defaultCenter] removeObserver:observer];
if (failure)
{
failure(error);
}
}
else
{
for (NSString *deviceIdentityKey in devices)
{
if ([devicesInProgress containsObject:deviceIdentityKey])
{
MXDeviceInfo *device = [self.store deviceWithIdentityKey:deviceIdentityKey];
NSString *olmSessionId = [self.olmDevice sessionIdForDevice:deviceIdentityKey];

// Update the result
MXOlmSessionResult *olmSessionResult = [results objectForDevice:device.deviceId forUser:device.userId];
olmSessionResult.sessionId = olmSessionId;

// This device is no more in progress
[devicesInProgress removeObject:deviceIdentityKey];
[self->ensureOlmSessionsInProgress removeObject:deviceIdentityKey];
}
}

NSLog(@"[MXCrypto] ensureOlmSessionsForDevices: Got olm sessions for %@ devices. Still missing %@ sessions", @(devices.count), @(devicesInProgress.count));

// If the pool is empty, we are done
if (!devicesInProgress.count)
{
[[NSNotificationCenter defaultCenter] removeObserver:observer];
if (success)
{
success(results);
}
}
}
}
}];


if (usersDevicesToClaim.count == 0)
{
NSLog(@"[MXCrypto] ensureOlmSessionsForDevices: All missing sessions are already pending");
return nil;
}


NSLog(@"[MXCrypto] ensureOlmSessionsForDevices: claimOneTimeKeysForUsersDevices (users: %tu - devices: %tu)",
usersDevicesToClaim.map.count, usersDevicesToClaim.count);
Expand Down Expand Up @@ -2020,27 +2123,29 @@ - (MXHTTPOperation*)ensureOlmSessionsForDevices:(NSDictionary<NSString* /* userI
continue;
}

NSString *sid = [self verifyKeyAndStartSession:oneTimeKey userId:userId deviceInfo:deviceInfo];

// Update the result for this device in results
olmSessionResult.sessionId = sid;
[self verifyKeyAndStartSession:oneTimeKey userId:userId deviceInfo:deviceInfo];
}
}
}

if (success)
{
success(results);
}

// Broadcast the /claim request is done
[[NSNotificationCenter defaultCenter] postNotificationName:kMXCryptoOneTimeKeyClaimCompleteNotification
object:self
userInfo: @{
kMXCryptoOneTimeKeyClaimCompleteNotificationDevicesKey: devicesToClaim
}];

} failure:^(NSError *error) {

NSLog(@"[MXCrypto] ensureOlmSessionsForDevices: claimOneTimeKeysForUsersDevices request failed.");

if (failure)
{
failure(error);
}
// Broadcast the /claim request is done
[[NSNotificationCenter defaultCenter] postNotificationName:kMXCryptoOneTimeKeyClaimCompleteNotification
object:self
userInfo: @{
kMXCryptoOneTimeKeyClaimCompleteNotificationDevicesKey: devicesToClaim,
kMXCryptoOneTimeKeyClaimCompleteNotificationErrorKey: error
}];
}];
}

Expand Down
68 changes: 68 additions & 0 deletions MatrixSDKTests/MXCryptoTests.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1660,6 +1660,74 @@ - (void)testInvitedMemberInACryptedRoom2

#pragma mark - Edge cases

// Trying to set up several olm sessions in parallel should result in the creation of a single olm session
//
// - Have Alice and Bob
// - Make Alice know Bob's device
// - Move to the crypto thread (this is an internal technical test)
// - Create a first olm session
// -> It must succeed
// - Create a second olm session in parallel
// -> It must not create another HTTP request
// -> It must succeed using the same olm session

- (void)testEnsureSingleOlmSession
{
// - Have Alice and Bob
[matrixSDKTestsE2EData doE2ETestWithAliceAndBobInARoom:self cryptedBob:YES warnOnUnknowDevices:NO readyToTest:^(MXSession *aliceSession, MXSession *bobSession, NSString *roomId, XCTestExpectation *expectation) {

// - Make Alice know Bob's device
[aliceSession.crypto downloadKeys:@[bobSession.myUserId] forceDownload:NO success:^(MXUsersDevicesMap<MXDeviceInfo *> *usersDevicesInfoMap, NSDictionary<NSString *,MXCrossSigningInfo *> *crossSigningKeysMap) {

// - Move to the crypto thread (this is an internal technical test)
dispatch_async(aliceSession.crypto.cryptoQueue, ^{

MXHTTPOperation *operation;
__block NSString *olmSessionId;


// - Create a first olm session
operation = [aliceSession.crypto ensureOlmSessionsForUsers:@[bobSession.myUserId] success:^(MXUsersDevicesMap<MXOlmSessionResult *> *results) {

// -> It must succeed
olmSessionId = [results objectForDevice:bobSession.myDeviceId forUser:bobSession.myUserId].sessionId;
XCTAssertNotNil(olmSessionId);

} failure:^(NSError *error) {
XCTFail(@"The operation should not fail - NSError: %@", error);
[expectation fulfill];
}];

XCTAssertNotNil(operation);


// - Create a second olm session in parallel
operation = [aliceSession.crypto ensureOlmSessionsForUsers:@[bobSession.myUserId] success:^(MXUsersDevicesMap<MXOlmSessionResult *> *results) {

// -> It must succeed using the same olm session
NSString *olmSessionId2 = [results objectForDevice:bobSession.myDeviceId forUser:bobSession.myUserId].sessionId;
XCTAssertNotNil(olmSessionId2);
XCTAssertEqualObjects(olmSessionId, olmSessionId2);

[expectation fulfill];

} failure:^(NSError *error) {
XCTFail(@"The operation should not fail - NSError: %@", error);
[expectation fulfill];
}];

// -> It must not create another HTTP request
XCTAssertNil(operation);

});

} failure:^(NSError *error) {
XCTFail(@"Cannot set up intial test conditions - error: %@", error);
[expectation fulfill];
}];
}];
}

- (void)testReplayAttack
{
[matrixSDKTestsE2EData doE2ETestWithAliceAndBobInARoom:self cryptedBob:YES warnOnUnknowDevices:NO readyToTest:^(MXSession *aliceSession, MXSession *bobSession, NSString *roomId, XCTestExpectation *expectation) {
Expand Down

0 comments on commit 59e087a

Please sign in to comment.