From 4236715ff9e4b9be4484fda43b2fab353615bdf2 Mon Sep 17 00:00:00 2001 From: Austin Zheng Date: Mon, 21 Aug 2017 11:46:56 -0700 Subject: [PATCH 1/8] Allow the keychain service to be customized when configuring sync metadata --- src/impl/apple/keychain_helper.cpp | 87 +++++++++++++++++++----------- src/impl/apple/keychain_helper.hpp | 6 ++- src/sync/impl/sync_metadata.cpp | 5 +- src/sync/impl/sync_metadata.hpp | 4 +- src/sync/sync_manager.cpp | 9 ++-- src/sync/sync_manager.hpp | 4 +- 6 files changed, 76 insertions(+), 39 deletions(-) diff --git a/src/impl/apple/keychain_helper.cpp b/src/impl/apple/keychain_helper.cpp index bdaafee63..1d7277d6c 100644 --- a/src/impl/apple/keychain_helper.cpp +++ b/src/impl/apple/keychain_helper.cpp @@ -36,9 +36,10 @@ namespace keychain { KeychainAccessException::KeychainAccessException(int32_t error_code) : std::runtime_error(util::format("Keychain returned unexpected status code: %1", error_code)) { } -CFPtr convert_string(const std::string& string); -CFPtr build_search_dictionary(const std::string& account, const std::string& service, - util::Optional group); +namespace { + +constexpr size_t key_size = 64; +const std::string account = "metadata"; CFPtr convert_string(const std::string& string) { @@ -55,9 +56,9 @@ CFPtr build_search_dictionary(const std::string& account { auto d = adoptCF(CFDictionaryCreateMutable(nullptr, 0, &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks)); - if (!d) { + if (!d) throw std::bad_alloc(); - } + CFDictionaryAddValue(d.get(), kSecClass, kSecClassGenericPassword); CFDictionaryAddValue(d.get(), kSecReturnData, kCFBooleanTrue); CFDictionaryAddValue(d.get(), kSecAttrAccessible, kSecAttrAccessibleAlways); @@ -65,51 +66,75 @@ CFPtr build_search_dictionary(const std::string& account CFDictionaryAddValue(d.get(), kSecAttrService, convert_string(service).get()); #if TARGET_IPHONE_SIMULATOR #else - if (group) { + if (group) CFDictionaryAddValue(d.get(), kSecAttrAccessGroup, convert_string(*group).get()); - } #endif return d; } -std::vector metadata_realm_encryption_key() +/// Get the encryption key for a given service. +util::Optional> try_getting_key(const std::string& service) { - const size_t key_size = 64; - const std::string service = "io.realm.sync.keychain"; - const std::string account = "metadata"; - auto search_dictionary = build_search_dictionary(account, service, none); CFDataRef retained_key_data; if (OSStatus status = SecItemCopyMatching(search_dictionary.get(), (CFTypeRef *)&retained_key_data)) { - if (status != errSecItemNotFound) { - throw KeychainAccessException(status); - } - - // Key was not found. Generate a new key, store it, and return it. - std::vector key(key_size); - arc4random_buf(key.data(), key_size); - auto key_data = adoptCF(CFDataCreate(nullptr, reinterpret_cast(key.data()), key_size)); - if (!key_data) { - throw std::bad_alloc(); - } - - CFDictionaryAddValue(search_dictionary.get(), kSecValueData, key_data.get()); - if (OSStatus status = SecItemAdd(search_dictionary.get(), nullptr)) { + if (status != errSecItemNotFound) throw KeychainAccessException(status); - } - return key; + // Key was not found. + return none; } - CFPtr key_data = adoptCF(retained_key_data); // Key was previously stored. Extract it. - if (key_size != CFDataGetLength(key_data.get())) { + CFPtr key_data = adoptCF(retained_key_data); + if (key_size != CFDataGetLength(key_data.get())) throw std::runtime_error("Password stored in keychain was not expected size."); - } auto key_bytes = reinterpret_cast(CFDataGetBytePtr(key_data.get())); return std::vector(key_bytes, key_bytes + key_size); } +void set_key(const std::vector& key, const std::string& service) +{ + auto search_dictionary = build_search_dictionary(account, service, none); + auto key_data = adoptCF(CFDataCreate(nullptr, reinterpret_cast(key.data()), key_size)); + if (!key_data) + throw std::bad_alloc(); + + CFDictionaryAddValue(search_dictionary.get(), kSecValueData, key_data.get()); + if (OSStatus status = SecItemAdd(search_dictionary.get(), nullptr)) + throw KeychainAccessException(status); } + +} // anonymous namespace + +std::vector metadata_realm_encryption_key(const util::Optional& service_name, + bool check_legacy_service) +{ + const std::string default_service = "io.realm.sync.keychain"; + const auto& service = service_name.value_or(default_service); + if (!service_name) + check_legacy_service = false; + + util::Optional> existing_key = try_getting_key(service); + if (!existing_key && check_legacy_service) { + existing_key = try_getting_key(default_service); + if (existing_key) { + // Write it to the keychain before returning it. + set_key(*existing_key, service); + return *existing_key; + } + } + if (!existing_key) { + // Make a completely new key. + std::vector key(key_size); + arc4random_buf(key.data(), key_size); + set_key(key, service); + return key; + } else { + return *existing_key; + } } + +} // keychain +} // realm diff --git a/src/impl/apple/keychain_helper.hpp b/src/impl/apple/keychain_helper.hpp index fd3c27ae2..07c19fff9 100644 --- a/src/impl/apple/keychain_helper.hpp +++ b/src/impl/apple/keychain_helper.hpp @@ -22,11 +22,15 @@ #include #include #include +#include + +#include namespace realm { namespace keychain { -std::vector metadata_realm_encryption_key(); +std::vector metadata_realm_encryption_key(const util::Optional& service_name, + bool check_legacy_service); class KeychainAccessException : public std::runtime_error { public: diff --git a/src/sync/impl/sync_metadata.cpp b/src/sync/impl/sync_metadata.cpp index cc11d5ef5..b6f1e0200 100644 --- a/src/sync/impl/sync_metadata.cpp +++ b/src/sync/impl/sync_metadata.cpp @@ -76,7 +76,8 @@ namespace realm { SyncMetadataManager::SyncMetadataManager(std::string path, bool should_encrypt, - util::Optional> encryption_key) + util::Optional> encryption_key, + const util::Optional& keychain_service) { constexpr uint64_t SCHEMA_VERSION = 2; @@ -87,7 +88,7 @@ SyncMetadataManager::SyncMetadataManager(std::string path, config.schema_mode = SchemaMode::Automatic; #if REALM_PLATFORM_APPLE if (should_encrypt && !encryption_key) { - encryption_key = keychain::metadata_realm_encryption_key(); + encryption_key = keychain::metadata_realm_encryption_key(keychain_service, File::exists(path)); } #endif if (should_encrypt) { diff --git a/src/sync/impl/sync_metadata.hpp b/src/sync/impl/sync_metadata.hpp index 3b68ae7c9..00dcf5324 100644 --- a/src/sync/impl/sync_metadata.hpp +++ b/src/sync/impl/sync_metadata.hpp @@ -201,9 +201,11 @@ friend class SyncFileActionMetadata; /// If the platform supports it, setting `should_encrypt` to `true` and not specifying an encryption key will make /// the object store handle generating and persisting an encryption key for the metadata database. Otherwise, an /// exception will be thrown. + /// `keychain_service` should only be set for Apple platforms; its value is ignored in all other cases. SyncMetadataManager(std::string path, bool should_encrypt, - util::Optional> encryption_key=none); + util::Optional> encryption_key=none, + const util::Optional& keychain_service=none); private: SyncUserMetadataResults get_users(bool marked) const; diff --git a/src/sync/sync_manager.cpp b/src/sync/sync_manager.cpp index e25d906fe..7c81180ee 100644 --- a/src/sync/sync_manager.cpp +++ b/src/sync/sync_manager.cpp @@ -40,7 +40,8 @@ SyncManager& SyncManager::shared() void SyncManager::configure_file_system(const std::string& base_file_path, MetadataMode metadata_mode, util::Optional> custom_encryption_key, - bool reset_metadata_on_error) + bool reset_metadata_on_error, + const util::Optional& keychain_service) { struct UserCreationData { std::string identity; @@ -73,12 +74,14 @@ void SyncManager::configure_file_system(const std::string& base_file_path, try { m_metadata_manager = std::make_unique(m_file_manager->metadata_path(), true, - std::move(custom_encryption_key)); + std::move(custom_encryption_key), + keychain_service); } catch (RealmFileException const& ex) { if (reset_metadata_on_error && m_file_manager->remove_metadata_realm()) { m_metadata_manager = std::make_unique(m_file_manager->metadata_path(), true, - std::move(custom_encryption_key)); + std::move(custom_encryption_key), + keychain_service); } else { throw; } diff --git a/src/sync/sync_manager.hpp b/src/sync/sync_manager.hpp index 2527e0fb3..f6c7ab481 100644 --- a/src/sync/sync_manager.hpp +++ b/src/sync/sync_manager.hpp @@ -67,10 +67,12 @@ friend class SyncSession; static SyncManager& shared(); // Configure the metadata and file management subsystems. This MUST be called upon startup. + // `keychain_service` is only relevant for Apple platforms; its value is ignored in all other cases. void configure_file_system(const std::string& base_file_path, MetadataMode metadata_mode=MetadataMode::Encryption, util::Optional> custom_encryption_key=none, - bool reset_metadata_on_error=false); + bool reset_metadata_on_error=false, + const util::Optional& keychain_service=none); // Immediately run file actions for a single Realm at a given original path. // Returns whether or not a file action was successfully executed for the specified Realm. From 9bafa2a63f3685879d78668366efc38fc0d7448c Mon Sep 17 00:00:00 2001 From: Austin Zheng Date: Mon, 21 Aug 2017 16:21:41 -0700 Subject: [PATCH 2/8] Make object store get bundle identifier --- src/impl/apple/keychain_helper.cpp | 57 +++++++++++++++--------------- src/impl/apple/keychain_helper.hpp | 6 +--- src/sync/impl/sync_metadata.cpp | 5 ++- src/sync/impl/sync_metadata.hpp | 3 +- src/sync/sync_manager.cpp | 9 ++--- src/sync/sync_manager.hpp | 4 +-- 6 files changed, 37 insertions(+), 47 deletions(-) diff --git a/src/impl/apple/keychain_helper.cpp b/src/impl/apple/keychain_helper.cpp index 1d7277d6c..6cda6fed5 100644 --- a/src/impl/apple/keychain_helper.cpp +++ b/src/impl/apple/keychain_helper.cpp @@ -38,10 +38,10 @@ KeychainAccessException::KeychainAccessException(int32_t error_code) namespace { +using CFStringPtr = CFPtr; constexpr size_t key_size = 64; -const std::string account = "metadata"; -CFPtr convert_string(const std::string& string) +CFStringPtr convert_string(const std::string& string) { auto result = adoptCF(CFStringCreateWithBytes(nullptr, reinterpret_cast(string.data()), string.size(), kCFStringEncodingASCII, false)); @@ -51,7 +51,8 @@ CFPtr convert_string(const std::string& string) return result; } -CFPtr build_search_dictionary(const std::string& account, const std::string& service, +CFPtr build_search_dictionary(const CFStringPtr& account, + const CFStringPtr& service, __unused util::Optional group) { auto d = adoptCF(CFDictionaryCreateMutable(nullptr, 0, &kCFTypeDictionaryKeyCallBacks, @@ -62,8 +63,8 @@ CFPtr build_search_dictionary(const std::string& account CFDictionaryAddValue(d.get(), kSecClass, kSecClassGenericPassword); CFDictionaryAddValue(d.get(), kSecReturnData, kCFBooleanTrue); CFDictionaryAddValue(d.get(), kSecAttrAccessible, kSecAttrAccessibleAlways); - CFDictionaryAddValue(d.get(), kSecAttrAccount, convert_string(account).get()); - CFDictionaryAddValue(d.get(), kSecAttrService, convert_string(service).get()); + CFDictionaryAddValue(d.get(), kSecAttrAccount, account.get()); + CFDictionaryAddValue(d.get(), kSecAttrService, service.get()); #if TARGET_IPHONE_SIMULATOR #else if (group) @@ -73,7 +74,7 @@ CFPtr build_search_dictionary(const std::string& account } /// Get the encryption key for a given service. -util::Optional> try_getting_key(const std::string& service) +util::Optional> try_getting_key(const CFStringPtr& account, const CFStringPtr& service) { auto search_dictionary = build_search_dictionary(account, service, none); CFDataRef retained_key_data; @@ -94,7 +95,7 @@ util::Optional> try_getting_key(const std::string& service) return std::vector(key_bytes, key_bytes + key_size); } -void set_key(const std::vector& key, const std::string& service) +void set_key(const std::vector& key, const CFStringPtr& account, const CFStringPtr& service) { auto search_dictionary = build_search_dictionary(account, service, none); auto key_data = adoptCF(CFDataCreate(nullptr, reinterpret_cast(key.data()), key_size)); @@ -108,32 +109,32 @@ void set_key(const std::vector& key, const std::string& service) } // anonymous namespace -std::vector metadata_realm_encryption_key(const util::Optional& service_name, - bool check_legacy_service) +std::vector metadata_realm_encryption_key(bool check_legacy_service) { - const std::string default_service = "io.realm.sync.keychain"; - const auto& service = service_name.value_or(default_service); - if (!service_name) + const CFStringPtr account = convert_string("metadata"); + CFStringPtr default_service = convert_string("io.realm.sync.keychain"); + auto service = adoptCF(CFBundleGetIdentifier(CFBundleGetMainBundle())); + if (!service) { + service = std::move(default_service); check_legacy_service = false; - - util::Optional> existing_key = try_getting_key(service); - if (!existing_key && check_legacy_service) { - existing_key = try_getting_key(default_service); - if (existing_key) { - // Write it to the keychain before returning it. - set_key(*existing_key, service); - return *existing_key; - } } - if (!existing_key) { - // Make a completely new key. - std::vector key(key_size); - arc4random_buf(key.data(), key_size); - set_key(key, service); - return key; - } else { + + // Try retrieving the key. + if (auto existing_key = try_getting_key(account, service)) { return *existing_key; + } else if (check_legacy_service) { + // See if there's a key in the old shared keychain. + if (auto existing_legacy_key = try_getting_key(account, default_service)) { + // If so, copy it to the per-app keychain before returning it. + set_key(*existing_legacy_key, account, service); + return *existing_legacy_key; + } } + // Make a completely new key. + std::vector key(key_size); + arc4random_buf(key.data(), key_size); + set_key(key, account, service); + return key; } } // keychain diff --git a/src/impl/apple/keychain_helper.hpp b/src/impl/apple/keychain_helper.hpp index 07c19fff9..8915e78ee 100644 --- a/src/impl/apple/keychain_helper.hpp +++ b/src/impl/apple/keychain_helper.hpp @@ -22,15 +22,11 @@ #include #include #include -#include - -#include namespace realm { namespace keychain { -std::vector metadata_realm_encryption_key(const util::Optional& service_name, - bool check_legacy_service); +std::vector metadata_realm_encryption_key(bool check_legacy_service); class KeychainAccessException : public std::runtime_error { public: diff --git a/src/sync/impl/sync_metadata.cpp b/src/sync/impl/sync_metadata.cpp index b6f1e0200..4c2c9beb3 100644 --- a/src/sync/impl/sync_metadata.cpp +++ b/src/sync/impl/sync_metadata.cpp @@ -76,8 +76,7 @@ namespace realm { SyncMetadataManager::SyncMetadataManager(std::string path, bool should_encrypt, - util::Optional> encryption_key, - const util::Optional& keychain_service) + util::Optional> encryption_key) { constexpr uint64_t SCHEMA_VERSION = 2; @@ -88,7 +87,7 @@ SyncMetadataManager::SyncMetadataManager(std::string path, config.schema_mode = SchemaMode::Automatic; #if REALM_PLATFORM_APPLE if (should_encrypt && !encryption_key) { - encryption_key = keychain::metadata_realm_encryption_key(keychain_service, File::exists(path)); + encryption_key = keychain::metadata_realm_encryption_key(File::exists(path)); } #endif if (should_encrypt) { diff --git a/src/sync/impl/sync_metadata.hpp b/src/sync/impl/sync_metadata.hpp index 00dcf5324..6ed30766e 100644 --- a/src/sync/impl/sync_metadata.hpp +++ b/src/sync/impl/sync_metadata.hpp @@ -204,8 +204,7 @@ friend class SyncFileActionMetadata; /// `keychain_service` should only be set for Apple platforms; its value is ignored in all other cases. SyncMetadataManager(std::string path, bool should_encrypt, - util::Optional> encryption_key=none, - const util::Optional& keychain_service=none); + util::Optional> encryption_key=none); private: SyncUserMetadataResults get_users(bool marked) const; diff --git a/src/sync/sync_manager.cpp b/src/sync/sync_manager.cpp index 7c81180ee..e25d906fe 100644 --- a/src/sync/sync_manager.cpp +++ b/src/sync/sync_manager.cpp @@ -40,8 +40,7 @@ SyncManager& SyncManager::shared() void SyncManager::configure_file_system(const std::string& base_file_path, MetadataMode metadata_mode, util::Optional> custom_encryption_key, - bool reset_metadata_on_error, - const util::Optional& keychain_service) + bool reset_metadata_on_error) { struct UserCreationData { std::string identity; @@ -74,14 +73,12 @@ void SyncManager::configure_file_system(const std::string& base_file_path, try { m_metadata_manager = std::make_unique(m_file_manager->metadata_path(), true, - std::move(custom_encryption_key), - keychain_service); + std::move(custom_encryption_key)); } catch (RealmFileException const& ex) { if (reset_metadata_on_error && m_file_manager->remove_metadata_realm()) { m_metadata_manager = std::make_unique(m_file_manager->metadata_path(), true, - std::move(custom_encryption_key), - keychain_service); + std::move(custom_encryption_key)); } else { throw; } diff --git a/src/sync/sync_manager.hpp b/src/sync/sync_manager.hpp index f6c7ab481..2527e0fb3 100644 --- a/src/sync/sync_manager.hpp +++ b/src/sync/sync_manager.hpp @@ -67,12 +67,10 @@ friend class SyncSession; static SyncManager& shared(); // Configure the metadata and file management subsystems. This MUST be called upon startup. - // `keychain_service` is only relevant for Apple platforms; its value is ignored in all other cases. void configure_file_system(const std::string& base_file_path, MetadataMode metadata_mode=MetadataMode::Encryption, util::Optional> custom_encryption_key=none, - bool reset_metadata_on_error=false, - const util::Optional& keychain_service=none); + bool reset_metadata_on_error=false); // Immediately run file actions for a single Realm at a given original path. // Returns whether or not a file action was successfully executed for the specified Realm. From 9828beef11c35ddc17b958ac7eb8d0436181977b Mon Sep 17 00:00:00 2001 From: Austin Zheng Date: Mon, 21 Aug 2017 16:27:45 -0700 Subject: [PATCH 3/8] Remove irrelevant message --- src/sync/impl/sync_metadata.hpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/sync/impl/sync_metadata.hpp b/src/sync/impl/sync_metadata.hpp index 6ed30766e..3b68ae7c9 100644 --- a/src/sync/impl/sync_metadata.hpp +++ b/src/sync/impl/sync_metadata.hpp @@ -201,7 +201,6 @@ friend class SyncFileActionMetadata; /// If the platform supports it, setting `should_encrypt` to `true` and not specifying an encryption key will make /// the object store handle generating and persisting an encryption key for the metadata database. Otherwise, an /// exception will be thrown. - /// `keychain_service` should only be set for Apple platforms; its value is ignored in all other cases. SyncMetadataManager(std::string path, bool should_encrypt, util::Optional> encryption_key=none); From da5d3edde80162959ca318be54aa2447d1cd10e8 Mon Sep 17 00:00:00 2001 From: Austin Zheng Date: Wed, 23 Aug 2017 17:53:06 -0700 Subject: [PATCH 4/8] Address feedback --- src/impl/apple/keychain_helper.cpp | 37 +++++++++++++++++------------- 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/src/impl/apple/keychain_helper.cpp b/src/impl/apple/keychain_helper.cpp index 6cda6fed5..2178ff67b 100644 --- a/src/impl/apple/keychain_helper.cpp +++ b/src/impl/apple/keychain_helper.cpp @@ -29,6 +29,7 @@ using realm::util::CFPtr; using realm::util::adoptCF; +using realm::util::retainCF; namespace realm { namespace keychain { @@ -38,10 +39,9 @@ KeychainAccessException::KeychainAccessException(int32_t error_code) namespace { -using CFStringPtr = CFPtr; constexpr size_t key_size = 64; -CFStringPtr convert_string(const std::string& string) +CFPtr convert_string(const std::string& string) { auto result = adoptCF(CFStringCreateWithBytes(nullptr, reinterpret_cast(string.data()), string.size(), kCFStringEncodingASCII, false)); @@ -51,8 +51,7 @@ CFStringPtr convert_string(const std::string& string) return result; } -CFPtr build_search_dictionary(const CFStringPtr& account, - const CFStringPtr& service, +CFPtr build_search_dictionary(CFStringRef account, CFStringRef service, __unused util::Optional group) { auto d = adoptCF(CFDictionaryCreateMutable(nullptr, 0, &kCFTypeDictionaryKeyCallBacks, @@ -63,8 +62,8 @@ CFPtr build_search_dictionary(const CFStringPtr& account CFDictionaryAddValue(d.get(), kSecClass, kSecClassGenericPassword); CFDictionaryAddValue(d.get(), kSecReturnData, kCFBooleanTrue); CFDictionaryAddValue(d.get(), kSecAttrAccessible, kSecAttrAccessibleAlways); - CFDictionaryAddValue(d.get(), kSecAttrAccount, account.get()); - CFDictionaryAddValue(d.get(), kSecAttrService, service.get()); + CFDictionaryAddValue(d.get(), kSecAttrAccount, account); + CFDictionaryAddValue(d.get(), kSecAttrService, service); #if TARGET_IPHONE_SIMULATOR #else if (group) @@ -73,8 +72,8 @@ CFPtr build_search_dictionary(const CFStringPtr& account return d; } -/// Get the encryption key for a given service. -util::Optional> try_getting_key(const CFStringPtr& account, const CFStringPtr& service) +/// Get the encryption key for a given service, returning it only if it exists. +util::Optional> get_key(CFStringRef account, CFStringRef service) { auto search_dictionary = build_search_dictionary(account, service, none); CFDataRef retained_key_data; @@ -95,7 +94,7 @@ util::Optional> try_getting_key(const CFStringPtr& account, co return std::vector(key_bytes, key_bytes + key_size); } -void set_key(const std::vector& key, const CFStringPtr& account, const CFStringPtr& service) +void set_key(const std::vector& key, CFStringRef account, CFStringRef service) { auto search_dictionary = build_search_dictionary(account, service, none); auto key_data = adoptCF(CFDataCreate(nullptr, reinterpret_cast(key.data()), key_size)); @@ -111,20 +110,26 @@ void set_key(const std::vector& key, const CFStringPtr& account, const CFS std::vector metadata_realm_encryption_key(bool check_legacy_service) { - const CFStringPtr account = convert_string("metadata"); - CFStringPtr default_service = convert_string("io.realm.sync.keychain"); - auto service = adoptCF(CFBundleGetIdentifier(CFBundleGetMainBundle())); - if (!service) { - service = std::move(default_service); + CFStringRef account = CFSTR("metadata"); + CFStringRef default_service = CFSTR("io.realm.sync.keychain"); + + CFPtr managed_service; + if (auto bundle_id = retainCF(CFBundleGetIdentifier(CFBundleGetMainBundle()))) { + managed_service = adoptCF(CFStringCreateWithFormat(NULL, NULL, + CFSTR("%@ - Realm Sync Metadata Key"), + bundle_id.get())); + } else { + managed_service = retainCF(default_service); check_legacy_service = false; } // Try retrieving the key. - if (auto existing_key = try_getting_key(account, service)) { + CFStringRef service = managed_service.get(); + if (auto existing_key = get_key(account, service)) { return *existing_key; } else if (check_legacy_service) { // See if there's a key in the old shared keychain. - if (auto existing_legacy_key = try_getting_key(account, default_service)) { + if (auto existing_legacy_key = get_key(account, default_service)) { // If so, copy it to the per-app keychain before returning it. set_key(*existing_legacy_key, account, service); return *existing_legacy_key; From 9a917a7ad7ece00a873cfc559aa8d7a961b9074c Mon Sep 17 00:00:00 2001 From: Austin Zheng Date: Wed, 23 Aug 2017 18:30:57 -0700 Subject: [PATCH 5/8] Address feedback --- src/impl/apple/keychain_helper.cpp | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/src/impl/apple/keychain_helper.cpp b/src/impl/apple/keychain_helper.cpp index 2178ff67b..de693c36a 100644 --- a/src/impl/apple/keychain_helper.cpp +++ b/src/impl/apple/keychain_helper.cpp @@ -111,34 +111,31 @@ void set_key(const std::vector& key, CFStringRef account, CFStringRef serv std::vector metadata_realm_encryption_key(bool check_legacy_service) { CFStringRef account = CFSTR("metadata"); - CFStringRef default_service = CFSTR("io.realm.sync.keychain"); + CFStringRef legacy_service = CFSTR("io.realm.sync.keychain"); - CFPtr managed_service; + CFPtr service; if (auto bundle_id = retainCF(CFBundleGetIdentifier(CFBundleGetMainBundle()))) { - managed_service = adoptCF(CFStringCreateWithFormat(NULL, NULL, - CFSTR("%@ - Realm Sync Metadata Key"), - bundle_id.get())); + service = adoptCF(CFStringCreateWithFormat(NULL, NULL, CFSTR("%@ - Realm Sync Metadata Key"), bundle_id.get())); } else { - managed_service = retainCF(default_service); + service = retainCF(legacy_service); check_legacy_service = false; } // Try retrieving the key. - CFStringRef service = managed_service.get(); - if (auto existing_key = get_key(account, service)) { + if (auto existing_key = get_key(account, service.get())) { return *existing_key; } else if (check_legacy_service) { - // See if there's a key in the old shared keychain. - if (auto existing_legacy_key = get_key(account, default_service)) { - // If so, copy it to the per-app keychain before returning it. - set_key(*existing_legacy_key, account, service); + // See if there's a key stored using the legacy shared keychain item. + if (auto existing_legacy_key = get_key(account, legacy_service)) { + // If so, copy it to the per-app keychain item before returning it. + set_key(*existing_legacy_key, account, service.get()); return *existing_legacy_key; } } // Make a completely new key. std::vector key(key_size); arc4random_buf(key.data(), key_size); - set_key(key, account, service); + set_key(key, account, service.get()); return key; } From 11a40be9a47964753a58a195313f276051b5c23d Mon Sep 17 00:00:00 2001 From: Austin Zheng Date: Fri, 25 Aug 2017 17:03:54 -0700 Subject: [PATCH 6/8] Fix issue with `retainCF` and null pointers --- src/impl/apple/keychain_helper.cpp | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/impl/apple/keychain_helper.cpp b/src/impl/apple/keychain_helper.cpp index de693c36a..3b14bbc82 100644 --- a/src/impl/apple/keychain_helper.cpp +++ b/src/impl/apple/keychain_helper.cpp @@ -114,11 +114,15 @@ std::vector metadata_realm_encryption_key(bool check_legacy_service) CFStringRef legacy_service = CFSTR("io.realm.sync.keychain"); CFPtr service; - if (auto bundle_id = retainCF(CFBundleGetIdentifier(CFBundleGetMainBundle()))) { - service = adoptCF(CFStringCreateWithFormat(NULL, NULL, CFSTR("%@ - Realm Sync Metadata Key"), bundle_id.get())); - } else { + CFStringRef bundle_id = CFBundleGetIdentifier(CFBundleGetMainBundle()); + if (!bundle_id) { service = retainCF(legacy_service); check_legacy_service = false; + } else { + // FIXME: Get rid of this manual retain once we fix CFPtr to accept null. + CFRetain(bundle_id); + service = adoptCF(CFStringCreateWithFormat(NULL, NULL, CFSTR("%@ - Realm Sync Metadata Key"), bundle_id)); + CFRelease(bundle_id); } // Try retrieving the key. From 16884985e08c45b4f5fc4b48c96558e9b4885ae3 Mon Sep 17 00:00:00 2001 From: Austin Zheng Date: Mon, 28 Aug 2017 12:36:53 -0700 Subject: [PATCH 7/8] Remove unnecessary retain/release --- src/impl/apple/keychain_helper.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/impl/apple/keychain_helper.cpp b/src/impl/apple/keychain_helper.cpp index 3b14bbc82..7198a3e81 100644 --- a/src/impl/apple/keychain_helper.cpp +++ b/src/impl/apple/keychain_helper.cpp @@ -119,10 +119,8 @@ std::vector metadata_realm_encryption_key(bool check_legacy_service) service = retainCF(legacy_service); check_legacy_service = false; } else { - // FIXME: Get rid of this manual retain once we fix CFPtr to accept null. - CFRetain(bundle_id); + // FIXME: Collapse the null check on bundle_id once CFPtr is fixed to accept null pointers. service = adoptCF(CFStringCreateWithFormat(NULL, NULL, CFSTR("%@ - Realm Sync Metadata Key"), bundle_id)); - CFRelease(bundle_id); } // Try retrieving the key. From d9c727a269b565c428db2139d0897584839c3a54 Mon Sep 17 00:00:00 2001 From: Austin Zheng Date: Mon, 28 Aug 2017 15:10:04 -0700 Subject: [PATCH 8/8] Address feedback --- src/impl/apple/keychain_helper.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/impl/apple/keychain_helper.cpp b/src/impl/apple/keychain_helper.cpp index 7198a3e81..f2cbe1ffd 100644 --- a/src/impl/apple/keychain_helper.cpp +++ b/src/impl/apple/keychain_helper.cpp @@ -115,12 +115,11 @@ std::vector metadata_realm_encryption_key(bool check_legacy_service) CFPtr service; CFStringRef bundle_id = CFBundleGetIdentifier(CFBundleGetMainBundle()); - if (!bundle_id) { + if (CFStringRef bundle_id = CFBundleGetIdentifier(CFBundleGetMainBundle())) + service = adoptCF(CFStringCreateWithFormat(NULL, NULL, CFSTR("%@ - Realm Sync Metadata Key"), bundle_id)); + else { service = retainCF(legacy_service); check_legacy_service = false; - } else { - // FIXME: Collapse the null check on bundle_id once CFPtr is fixed to accept null pointers. - service = adoptCF(CFStringCreateWithFormat(NULL, NULL, CFSTR("%@ - Realm Sync Metadata Key"), bundle_id)); } // Try retrieving the key.