Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RCORE-2070 Allow setting a security access group for the metadata realm keychain #7552

Merged
merged 2 commits into from
Apr 9, 2024

Conversation

tgoyne
Copy link
Member

@tgoyne tgoyne commented Apr 3, 2024

Access groups are shared storage for one or more apps on iOS (and other Apple platforms). Sharing a metadata Realm between apps requires placing the file in the access group storage and storing the encryption key in the access group's keychain.

Including the bundle ID in the service name breaks sharing the key between apps, as different apps will have different bundle IDs. For everything but un-sandboxed macOS there wasn't actually any reason to include the bundle ID in the first place, as each app has its own keychain anyway. As such, this switches back to not including it. On macOS this continues to include the bundle ID when not using an access group, as otherwise different applications could conflict with each other. This means that sharing users between macOS applications will currently only work if an encryption key is explicitly set or if the applications have sandboxing enabled.

Since this is slightly changing how keys are stored anyway, it also switches to using unique keys per server app ID rather than always using "metadata" as the account name.

The keychain code was mostly multiprocess-safe, but there was one race condition when two apps generated a new key at once which is fixed.

@tgoyne tgoyne self-assigned this Apr 3, 2024
@cla-bot cla-bot bot added the cla: yes label Apr 3, 2024
@tgoyne tgoyne force-pushed the tg/keychain-access-group branch 3 times, most recently from 14d9da7 to aac009b Compare April 3, 2024 23:55
@tgoyne tgoyne requested a review from ironage April 4, 2024 02:01
Copy link

coveralls-official bot commented Apr 4, 2024

Pull Request Test Coverage Report for Build thomas.goyne_273

Details

  • 181 of 197 (91.88%) changed or added relevant lines in 10 files are covered.
  • 2 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-4.0%) to 87.97%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/realm/object-store/impl/apple/keychain_helper.cpp 33 49 67.35%
Files with Coverage Reduction New Missed Lines %
src/realm/object-store/impl/apple/keychain_helper.cpp 1 70.09%
src/realm/object-store/impl/apple/keychain_helper.hpp 1 0.0%
Totals Coverage Status
Change from base Build 2205: -4.0%
Covered Lines: 65993
Relevant Lines: 75018

💛 - Coveralls

Copy link
Contributor

@ironage ironage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not an expert in this area, but it looks fine to me. I also take confidence from the legacy migration test. 👍

Access groups are shared storage for one or more apps on iOS (and other Apple
platforms). Sharing a metadata Realm between apps requires placing the file in
the access group storage and storing the encryption key in the access group's
keychain.

Including the bundle ID in the service name breaks sharing the key between
apps, as different apps will have different bundle IDs. For everything but
un-sandboxed macOS there wasn't actually any reason to include the bundle ID in
the first place, as each app has its own keychain anyway. As such, this
switches back to not including it. On macOS this continues to include the
bundle ID when not using an access group, as otherwise different applications
could conflict with each other. This means that sharing users between macOS
applications will currently only work if an encryption key is explicitly set or
if the applications have sandboxing enabled.

Since this is slightly changing how keys are stored anyway, it also switches to
using unique keys per server app ID rather than always using "metadata" as the
account name.

The keychain code was mostly multiprocess-safe, but there was one race
condition when two apps generated a new key at once which is fixed.
@tgoyne tgoyne merged commit f84ff38 into master Apr 9, 2024
39 checks passed
@tgoyne tgoyne deleted the tg/keychain-access-group branch April 9, 2024 00:00
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants