Store sync metadata keychain info on per-app basis #521

austinzheng · 2017-08-21T19:04:08Z

It's not clear how to test this change in isolation, since it touches Apple's Keychain stuff. The binding-level integration tests continue to work unhindered.

austinzheng · 2017-08-21T22:43:24Z

This is ready for review. Apologies for the spam earlier.

bdash · 2017-08-23T21:30:20Z

src/impl/apple/keychain_helper.cpp

@@ -50,66 +51,91 @@ CFPtr<CFStringRef> convert_string(const std::string& string)
    return result;
 }

-CFPtr<CFMutableDictionaryRef> build_search_dictionary(const std::string& account, const std::string& service,
+CFPtr<CFMutableDictionaryRef> build_search_dictionary(const CFStringPtr& account,


Taking smart pointers by const reference is a little weird. Typically you'd take a raw pointer to indicate that you're merely borrowing a reference, rather than doing anything with the ownership of the object. For that reason I'd suggest just using CFStringRef for the string arguments.

I totally forgot about this pattern.

bdash · 2017-08-23T21:31:59Z

src/impl/apple/keychain_helper.cpp

 #endif
    return d;
 }

-std::vector<char> metadata_realm_encryption_key()
+/// Get the encryption key for a given service.
+util::Optional<std::vector<char>> try_getting_key(const CFStringPtr& account, const CFStringPtr& service)


Can this just be get_key?

bdash · 2017-08-23T21:33:55Z

src/impl/apple/keychain_helper.cpp

+{
+    const CFStringPtr account = convert_string("metadata");
+    CFStringPtr default_service = convert_string("io.realm.sync.keychain");
+    auto service = adoptCF(CFBundleGetIdentifier(CFBundleGetMainBundle()));


I think we want to use something beyond just the bare bundle identifier as the keychain item name to avoid conflicting with anything created by the app itself. Maybe something like "com.example.app - Realm Sync Metadata Key"?

adoptCF isn't appropriate here. CFBundleGetIdentifier does not return an owned object, only a borrowed object. Adopting the returned reference will result in a use after free. You want retainCF instead.

Good call. I need to review the CF ownership conventions before writing any more CF code.

The CF documentation has a section on ownership that's relatively readable. Many CF APIs describe their return values as using one of the two rules described within (create vs get).

bdash · 2017-08-23T21:41:00Z

src/impl/apple/keychain_helper.cpp

+
+std::vector<char> metadata_realm_encryption_key(bool check_legacy_service)
+{
+    const CFStringPtr account = convert_string("metadata");


These constants should use the CFSTR macro instead, so they end up in the __cfstring section of the binary rather than being allocated on the heap at runtime. There'd be no reason to use CFPtr with the resulting strings, either, as they'd have an eternal lifetime.

…adata

bdash · 2017-08-24T01:20:25Z

src/impl/apple/keychain_helper.cpp

+std::vector<char> metadata_realm_encryption_key(bool check_legacy_service)
+{
+    CFStringRef account = CFSTR("metadata");
+    CFStringRef default_service = CFSTR("io.realm.sync.keychain");


I think legacy_service would better reflect how the relationship between this and the check_legacy_service argument.

bdash · 2017-08-24T01:22:04Z

src/impl/apple/keychain_helper.cpp

+    }
+
+    // Try retrieving the key.
+    CFStringRef service = managed_service.get();


I'd be inclined to ditch this local and just add the .get() calls in the handful of places it is necessary. It's effectively free to call, and would avoid the need for the slightly odd managed_ prefix on the variable.

bdash · 2017-08-24T01:22:52Z

src/impl/apple/keychain_helper.cpp

+    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.


"See if there's a key stored using the legacy shared keychain item"?

bdash · 2017-08-24T01:23:12Z

src/impl/apple/keychain_helper.cpp

+    } 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.


The keychain isn't per application, the item name is.

austinzheng · 2017-08-24T01:50:59Z

Thank you for the review! I will hold off on merging this until the Cocoa PR has been updated to compile and the integration tests have been run successfully.

austinzheng · 2017-08-26T00:05:17Z

There was an issue that caused the Cocoa ROS integration tests to fail. I created realm/realm-core#2821 to track a fix and worked around it in my latest change.

bdash · 2017-08-28T18:27:23Z

src/impl/apple/keychain_helper.cpp

+        // 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);


There's no reason to retain / release bundle_id here.

bdash · 2017-08-28T20:25:44Z

src/impl/apple/keychain_helper.cpp

@@ -119,10 +119,8 @@ std::vector<char> 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.


I'm not 100% sure what you mean by "collapse the null check" here. We could write this like so, though:

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; }

We don't actually need to use retainCF for the temporary bundle_id, so your suggestion addresses the problem and there isn't anything we need to do in the future.

bdash · 2017-08-28T22:41:54Z

👍

austinzheng added the S:Review label Aug 21, 2017

austinzheng self-assigned this Aug 21, 2017

austinzheng requested a review from bdash August 21, 2017 19:04

austinzheng mentioned this pull request Aug 21, 2017

Use the application bundle ID for keychain work realm/realm-swift#5246

Merged

nirinchev mentioned this pull request Aug 21, 2017

Allow the keychain service to be customized realm/realm-dotnet#1519

Closed

austinzheng force-pushed the az/set-custom-keychain-service branch 3 times, most recently from 9e5ac24 to 5a52b82 Compare August 21, 2017 22:40

bdash suggested changes Aug 23, 2017

View reviewed changes

austinzheng added 4 commits August 23, 2017 17:33

Allow the keychain service to be customized when configuring sync met…

4236715

…adata

Make object store get bundle identifier

9bafa2a

Remove irrelevant message

9828bee

Address feedback

da5d3ed

austinzheng force-pushed the az/set-custom-keychain-service branch from 91a6f28 to da5d3ed Compare August 24, 2017 00:57

bdash reviewed Aug 24, 2017

View reviewed changes

Address feedback

9a917a7

bdash approved these changes Aug 24, 2017

View reviewed changes

Fix issue with retainCF and null pointers

11a40be

austinzheng changed the title ~~Allow the keychain service to be customized when configuring sync met…~~ Store sync metadata keychain info on per-app basis Aug 26, 2017

bdash reviewed Aug 28, 2017

View reviewed changes

Remove unnecessary retain/release

1688498

bdash reviewed Aug 28, 2017

View reviewed changes

Address feedback

d9c727a

austinzheng merged commit 54a98b2 into master Aug 28, 2017

austinzheng deleted the az/set-custom-keychain-service branch August 28, 2017 22:43

austinzheng removed the S:Review label Aug 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store sync metadata keychain info on per-app basis #521

Store sync metadata keychain info on per-app basis #521

austinzheng commented Aug 21, 2017 •

edited

Loading

austinzheng commented Aug 21, 2017

bdash Aug 23, 2017

austinzheng Aug 24, 2017

bdash Aug 23, 2017

bdash Aug 23, 2017

austinzheng Aug 24, 2017

bdash Aug 24, 2017

bdash Aug 23, 2017

bdash Aug 24, 2017

bdash Aug 24, 2017

bdash Aug 24, 2017

bdash Aug 24, 2017

austinzheng commented Aug 24, 2017

austinzheng commented Aug 26, 2017 •

edited

Loading

bdash Aug 28, 2017

bdash Aug 28, 2017

austinzheng Aug 28, 2017

bdash commented Aug 28, 2017

Store sync metadata keychain info on per-app basis #521

Store sync metadata keychain info on per-app basis #521

Conversation

austinzheng commented Aug 21, 2017 • edited Loading

austinzheng commented Aug 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austinzheng commented Aug 24, 2017

austinzheng commented Aug 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdash commented Aug 28, 2017

austinzheng commented Aug 21, 2017 •

edited

Loading

austinzheng commented Aug 26, 2017 •

edited

Loading