Respond to CR.

This commit is contained in:
Matthew Chen 2018-10-05 10:32:32 -04:00
parent f9e90215b3
commit a697072271
11 changed files with 104 additions and 112 deletions

View File

@ -260,10 +260,10 @@ CHECKOUT OPTIONS:
:commit: 8b8326cd50bc488663a3d3743f1a92b90f4d85b4
:git: https://github.com/signalapp/HKDFKit.git
SignalCoreKit:
:commit: 358ec16833d9b8b6e1410d83fa47c819c533fe91
:commit: e5b6aa3c078d7c2fbc154e5dd806b8e55211697d
:git: https://github.com/signalapp/SignalCoreKit.git
SignalMetadataKit:
:commit: 954cbfa767e130626d2e87cc029769a1977c8edd
:commit: b0e664410dd3d709355bfdb9d464ae02644aeb74
:git: https://github.com/signalapp/SignalMetadataKit
SocketRocket:
:commit: 9f9563a83cd8960503074aa8de72206f83fb7a69

View File

@ -169,8 +169,10 @@ public class ProfileFetcherJob: NSObject {
profileNameEncrypted: signalServiceProfile.profileNameEncrypted,
avatarUrlPath: signalServiceProfile.avatarUrlPath)
let isUDRecipientId = signalServiceProfile.unidentifiedAccessKey != nil
udManager.setIsUDRecipientId(signalServiceProfile.recipientId, isUDRecipientId: isUDRecipientId)
// TODO: We may want to only call setSupportsUnidentifiedDelivery if
// supportsUnidentifiedDelivery is true.
let supportsUnidentifiedDelivery = signalServiceProfile.unidentifiedAccessKey != nil
udManager.setSupportsUnidentifiedDelivery(supportsUnidentifiedDelivery, recipientId: signalServiceProfile.recipientId)
udManager.setShouldAllowUnrestrictedAccess(recipientId: signalServiceProfile.recipientId, shouldAllowUnrestrictedAccess: signalServiceProfile.hasUnrestrictedUnidentifiedAccess)
}
@ -227,6 +229,6 @@ public class SignalServiceProfile: NSObject {
// The docs don't agree with the response from staging.
self.unidentifiedAccessKey = try params.optionalBase64EncodedData(key: "unidentifiedAccess")
self.hasUnrestrictedUnidentifiedAccess = try params.optionalBool(key: "unrestrictedUnidentifiedAccess", defaultValue: false)
self.hasUnrestrictedUnidentifiedAccess = try params.optional(key: "unrestrictedUnidentifiedAccess") ?? false
}
}

View File

@ -102,7 +102,7 @@ NSError *EnsureDecryptError(NSError *_Nullable error, NSString *fallbackErrorDes
return self;
}
#pragma mark - Singletons
#pragma mark - Dependencies
- (OWSBlockingManager *)blockingManager
{
@ -118,6 +118,13 @@ NSError *EnsureDecryptError(NSError *_Nullable error, NSString *fallbackErrorDes
return SSKEnvironment.shared.identityManager;
}
- (id<OWSUDManager>)udManager
{
OWSAssertDebug(SSKEnvironment.shared.udManager);
return SSKEnvironment.shared.udManager;
}
#pragma mark - Blocking
- (BOOL)isEnvelopeSenderBlocked:(SSKProtoEnvelope *)envelope
@ -405,7 +412,7 @@ NSError *EnsureDecryptError(NSError *_Nullable error, NSString *fallbackErrorDes
UInt64 serverTimestamp = envelope.serverTimestamp;
id<SMKCertificateValidator> certificateValidator =
[[SMKCertificateDefaultValidator alloc] initWithTrustRoot:self.trustRoot];
[[SMKCertificateDefaultValidator alloc] initWithTrustRoot:self.udManager.trustRoot];
[self.dbConnection asyncReadWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) {
@try {
@ -479,20 +486,6 @@ NSError *EnsureDecryptError(NSError *_Nullable error, NSString *fallbackErrorDes
}];
}
- (ECPublicKey *)trustRoot
{
NSData *_Nullable trustRootData = [NSData dataFromBase64String:kUDTrustRoot];
OWSAssert(trustRootData);
OWSAssert(trustRootData.length == ECCKeyLength + 1);
NSError *error;
ECPublicKey *_Nullable trustRoot = [[ECPublicKey alloc] initWithSerializedKeyData:trustRootData error:&error];
if (error || !trustRoot) {
// This exits.
OWSFail(@"Invalid UD trust root.");
}
return trustRoot;
}
- (void)processException:(NSException *)exception envelope:(SSKProtoEnvelope *)envelope
{
OWSLogError(

View File

@ -29,12 +29,11 @@ public class OWSMessageSend: NSObject {
// We "fail over" to REST sends after _any_ error sending
// via the web socket.
@objc
public var useWebsocketIfAvailable = true
public var hasWebsocketSendFailed = false
// We "fail over" to non-UD sends after certain errors sending
// via UD.
// We "fail over" to non-UD sends after auth errors sending via UD.
@objc
public var canUseUD = true
public var hasUDAuthFailed = false
@objc
public let udAccessKey: SMKUDAccessKey?

View File

@ -39,7 +39,6 @@
#import "TSOutgoingMessage.h"
#import "TSPreKeyManager.h"
#import "TSQuotedMessage.h"
#import "TSRequest+UD.h"
#import "TSRequest.h"
#import "TSSocketManager.h"
#import "TSThread.h"
@ -232,7 +231,7 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException";
return self;
}
#pragma mark -Dependencies
#pragma mark - Dependencies
- (id<ContactsManagerProtocol>)contactsManager
{
@ -467,6 +466,10 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException";
success:(void (^)(void))success
failure:(RetryableFailureHandler)failure
{
if (message.thread && message.thread.isGroupThread) {
[self saveInfoMessageForGroupMessage:message inThread:message.thread];
}
[self.udManager
ensureSenderCertificateObjCWithSuccess:^(SMKSenderCertificate *senderCertificate) {
[self sendMessageToService:message senderCertificate:senderCertificate success:success failure:failure];
@ -881,10 +884,11 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException";
// Consume an attempt.
messageSend.remainingAttempts = messageSend.remainingAttempts - 1;
BOOL isUDSend = (messageSend.canUseUD && messageSend.udAccessKey != nil && messageSend.senderCertificate != nil);
OWSLogVerbose(@"isUDSend: %d, canUseUD: %d, udAccessKey: %d, senderCertificate: %d",
BOOL isUDSend
= (!messageSend.hasUDAuthFailed && messageSend.udAccessKey != nil && messageSend.senderCertificate != nil);
OWSLogVerbose(@"isUDSend: %d, hasUDAuthFailed: %d, udAccessKey: %d, senderCertificate: %d",
isUDSend,
messageSend.canUseUD,
messageSend.hasUDAuthFailed,
messageSend.udAccessKey != nil,
messageSend.senderCertificate != nil);
@ -973,7 +977,7 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException";
}
// TODO: UD sends over websocket.
if (messageSend.useWebsocketIfAvailable && TSSocketManager.canMakeRequests && !isUDSend) {
if (!messageSend.hasWebsocketSendFailed && TSSocketManager.canMakeRequests && !isUDSend) {
[TSSocketManager.sharedManager makeRequest:request
success:^(id _Nullable responseObject) {
[self messageSendDidSucceed:messageSend deviceMessages:deviceMessages success:successHandler];
@ -985,7 +989,7 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException";
// Websockets can fail in different ways, so we don't decrement remainingAttempts for websocket
// failure. Instead we fall back to REST, which will decrement retries. e.g. after linking a new
// device, sync messages will fail until the websocket re-opens.
messageSend.useWebsocketIfAvailable = NO;
messageSend.hasWebsocketSendFailed = YES;
[self sendMessageToRecipient:messageSend success:successHandler failure:failureHandler];
});
}];
@ -999,14 +1003,14 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException";
NSInteger statusCode = response.statusCode;
NSData *_Nullable responseData = error.userInfo[AFNetworkingOperationFailingURLResponseDataErrorKey];
if (isUDSend && statusCode > 0) {
if (isUDSend && (statusCode == 401 || statusCode == 403)) {
// If a UD send fails due to service response (as opposed to network
// failure), mark recipient as _not_ in UD mode, then retry.
//
// TODO: Do we want to discriminate based on exact error?
OWSLogDebug(@"UD send failed; failing over to non-UD send.");
[self.udManager setIsUDRecipientId:recipient.uniqueId isUDRecipientId:NO];
messageSend.canUseUD = NO;
[self.udManager setSupportsUnidentifiedDelivery:NO recipientId:recipient.uniqueId];
messageSend.hasUDAuthFailed = YES;
dispatch_async([OWSDispatch sendingQueue], ^{
[self sendMessageToRecipient:messageSend success:successHandler failure:failureHandler];
});
@ -1479,6 +1483,7 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException";
error:&error];
messageType = TSUnidentifiedSenderMessageType;
} else {
// This may throw an exception.
id<CipherMessage> encryptedMessage =
[cipher encryptMessage:[plainText paddedMessageBody] protocolContext:transaction];
serializedMessage = encryptedMessage.serialized;
@ -1517,24 +1522,20 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException";
}
}
// TODO: Huh?
//- (void)saveGroupMessage:(TSOutgoingMessage *)message inThread:(TSThread *)thread
//{
// if (message.groupMetaMessage == TSGroupMetaMessageDeliver) {
// // TODO: Why is this necessary?
// [message save];
// } else if (message.groupMetaMessage == TSGroupMetaMessageQuit) {
// [[[TSInfoMessage alloc] initWithTimestamp:message.timestamp
// inThread:thread
// messageType:TSInfoMessageTypeGroupQuit
// customMessage:message.customMessage] save];
// } else {
// [[[TSInfoMessage alloc] initWithTimestamp:message.timestamp
// inThread:thread
// messageType:TSInfoMessageTypeGroupUpdate
// customMessage:message.customMessage] save];
// }
//}
- (void)saveInfoMessageForGroupMessage:(TSOutgoingMessage *)message inThread:(TSThread *)thread
{
if (message.groupMetaMessage == TSGroupMetaMessageQuit) {
[[[TSInfoMessage alloc] initWithTimestamp:message.timestamp
inThread:thread
messageType:TSInfoMessageTypeGroupQuit
customMessage:message.customMessage] save];
} else {
[[[TSInfoMessage alloc] initWithTimestamp:message.timestamp
inThread:thread
messageType:TSInfoMessageTypeGroupUpdate
customMessage:message.customMessage] save];
}
}
// Called when the server indicates that the devices no longer exist - e.g. when the remote recipient has reinstalled.
- (void)handleStaleDevicesWithResponseJson:(NSDictionary *)responseJson

View File

@ -16,11 +16,13 @@ public enum OWSUDError: Error {
@objc func setup()
@objc func trustRoot() -> ECPublicKey
// MARK: - Recipient state
@objc func isUDRecipientId(_ recipientId: String) -> Bool
@objc func supportsUnidentifiedDelivery(recipientId: String) -> Bool
@objc func setIsUDRecipientId(_ recipientId: String, isUDRecipientId: Bool)
@objc func setSupportsUnidentifiedDelivery(_ value: Bool, recipientId: String)
// Returns the UD access key for a given recipient if they are
// a UD recipient and we have a valid profile key for them.
@ -95,13 +97,13 @@ public class OWSUDManagerImpl: NSObject, OWSUDManager {
// MARK: - Recipient state
@objc
public func isUDRecipientId(_ recipientId: String) -> Bool {
public func supportsUnidentifiedDelivery(recipientId: String) -> Bool {
return dbConnection.bool(forKey: recipientId, inCollection: kUDRecipientModeCollection, defaultValue: false)
}
@objc
public func setIsUDRecipientId(_ recipientId: String, isUDRecipientId: Bool) {
if isUDRecipientId {
public func setSupportsUnidentifiedDelivery(_ value: Bool, recipientId: String) {
if value {
dbConnection.setBool(true, forKey: recipientId, inCollection: kUDRecipientModeCollection)
} else {
dbConnection.removeObject(forKey: recipientId, inCollection: kUDRecipientModeCollection)
@ -112,12 +114,11 @@ public class OWSUDManagerImpl: NSObject, OWSUDManager {
// a UD recipient and we have a valid profile key for them.
@objc
public func udAccessKeyForRecipient(_ recipientId: String) -> SMKUDAccessKey? {
guard isUDRecipientId(recipientId) else {
guard supportsUnidentifiedDelivery(recipientId: recipientId) else {
return nil
}
guard let profileKey = profileManager.profileKeyData(forRecipientId: recipientId) else {
// Mark as "not a UD recipient".
setIsUDRecipientId(recipientId, isUDRecipientId: false)
return nil
}
do {
@ -125,7 +126,6 @@ public class OWSUDManagerImpl: NSObject, OWSUDManager {
return udAccessKey
} catch {
Logger.error("Could not determine udAccessKey: \(error)")
setIsUDRecipientId(recipientId, isUDRecipientId: false)
return nil
}
}
@ -147,14 +147,14 @@ public class OWSUDManagerImpl: NSObject, OWSUDManager {
do {
let certificate = try SMKSenderCertificate.parse(data: certificateData)
guard isValidCertificate(certificate: certificate) else {
guard isValidCertificate(certificate) else {
Logger.warn("Current sender certificate is not valid.")
return nil
}
return certificate
} catch {
OWSLogger.error("Certificate could not be parsed: \(error)")
owsFailDebug("Certificate could not be parsed: \(error)")
return nil
}
}
@ -194,7 +194,7 @@ public class OWSUDManagerImpl: NSObject, OWSUDManager {
return SignalServiceRestClient().requestUDSenderCertificate().then { (certificateData) -> Promise<(Data, SMKSenderCertificate)> in
let certificate = try SMKSenderCertificate.parse(data: certificateData)
guard self.isValidCertificate(certificate: certificate) else {
guard self.isValidCertificate(certificate) else {
throw OWSUDError.invalidData(description: "Invalid sender certificate returned by server")
}
@ -202,14 +202,34 @@ public class OWSUDManagerImpl: NSObject, OWSUDManager {
}
}
private func isValidCertificate(certificate: SMKSenderCertificate) -> Bool {
let expirationMs = certificate.expirationTimestamp
let nowMs = NSDate.ows_millisecondTimeStamp()
private func isValidCertificate(_ certificate: SMKSenderCertificate) -> Bool {
let certificateValidator = SMKCertificateDefaultValidator(trustRoot: trustRoot())
// Ensure that the certificate will not expire in the next hour.
// We want a threshold long enough to ensure that any outgoing message
// sends will complete before the expiration.
let isValid = nowMs + kHourInMs < expirationMs
return isValid
let nowMs = NSDate.ows_millisecondTimeStamp()
let anHourFromNowMs = nowMs + kHourInMs
do {
try certificateValidator.validate(senderCertificate: certificate, validationTime: anHourFromNowMs)
return true
} catch {
OWSLogger.error("Invalid certificate")
return false
}
}
@objc
public func trustRoot() -> ECPublicKey {
let trustRootData: Data = NSData(fromBase64String: kUDTrustRoot) as Data
do {
return try ECPublicKey(serializedKeyData: trustRootData)
} catch {
// This exits.
owsFail("Invalid trust root.")
}
}
// MARK: - Unrestricted Access

View File

@ -1,13 +0,0 @@
//
// Copyright (c) 2018 Open Whisper Systems. All rights reserved.
//
#import "TSRequest.h"
@class SMKUDAccessKey;
@interface TSRequest (UD)
- (void)useUDAuth:(SMKUDAccessKey *)udAccessKey;
@end

View File

@ -1,21 +0,0 @@
//
// Copyright (c) 2018 Open Whisper Systems. All rights reserved.
//
#import "TSRequest+UD.h"
#import <SignalCoreKit/NSData+OWS.h>
#import <SignalMetadataKit/SignalMetadataKit-Swift.h>
@implementation TSRequest (UD)
- (void)useUDAuth:(SMKUDAccessKey *)udAccessKey
{
OWSAssertDebug(udAccessKey);
// Suppress normal auth headers.
self.shouldHaveAuthorizationHeaders = NO;
// Add UD auth header.
[self setValue:[udAccessKey.keyData base64EncodedString] forHTTPHeaderField:@"Unidentified-Access-Key"];
}
@end

View File

@ -2,6 +2,8 @@
// Copyright (c) 2018 Open Whisper Systems. All rights reserved.
//
@class SMKUDAccessKey;
@interface TSRequest : NSMutableURLRequest
@property (nonatomic) BOOL shouldHaveAuthorizationHeaders;
@ -26,4 +28,8 @@
method:(NSString *)method
parameters:(nullable NSDictionary<NSString *, id> *)parameters;
#pragma mark - UD
- (void)useUDAuth:(SMKUDAccessKey *)udAccessKey;
@end

View File

@ -5,6 +5,8 @@
#import "TSRequest.h"
#import "TSAccountManager.h"
#import "TSConstants.h"
#import <SignalCoreKit/NSData+OWS.h>
#import <SignalMetadataKit/SignalMetadataKit-Swift.h>
@implementation TSRequest
@ -110,4 +112,16 @@
}
}
#pragma mark - UD
- (void)useUDAuth:(SMKUDAccessKey *)udAccessKey
{
OWSAssertDebug(udAccessKey);
// Suppress normal auth headers.
self.shouldHaveAuthorizationHeaders = NO;
// Add UD auth header.
[self setValue:[udAccessKey.keyData base64EncodedString] forHTTPHeaderField:@"Unidentified-Access-Key"];
}
@end

View File

@ -139,13 +139,4 @@ public class ParamParser {
return data
}
// MARK: Bool
public func optionalBool(key: Key, defaultValue: Bool) throws -> Bool {
guard let optionalValue: Bool = try optional(key: key) else {
return defaultValue
}
return optionalValue
}
}