Skip to content

[Keychain] Prevent keychain access from prompting user for permissions on macOS #75

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

Merged
merged 6 commits into from
Aug 26, 2022

Conversation

ncooke3
Copy link
Member

@ncooke3 ncooke3 commented Aug 25, 2022

Context

This PR adds the kSecUseDataProtectionKeychain search attribute to the keychain wrapper APIs in GULKeychainStorage.h and GULKeychainUtils.h. The search key will make the macOS keychain behave like the iOS-style keychain. This is needed to avoid the keychain access pop-ups that occur when a macOS-style keychain is accessed.

Permissions pop-up from `Keychain Access.app`

GoogleUtilities exposes two public headers that facilitate interaction with the Keychain: GULKeychainStorage.h and GULKeychainUtils.h. GULKeychainStorage.h implementation uses GULKeychainUtils.h to actually make call the Keychain's native APIs. Because of this, adding the kSecUseDataProtectionKeychain was, at minimum, required in GULKeychainUtils.h. But, to avoid future implementation changes where GULKeychainStorage.h no longer uses GULKeychainUtils.h as an implementation detail, I have ensured the key is inject at the GULKeychainStorage.h level. This will ensure that using both headers will always result in a querying the keychain with the kSecUseDataProtectionKeychain flag set to true, now and in the future.

Additionally, explicitly setting kSecUseDataProtectionKeychain to true only really affects macOS because the key is set to true by default. For macOS, setting kSecUseDataProtectionKeychain to true requires that targets that include the GoogleUtilities framework are signed with a provisioning profile that includes the Keychain Sharing capability.

More details at go/firebase-macos-keychain-popups

Outstanding work (in order of planned completion)
  • Open this PR with fix
  • Experiment with running macCatalyst tests (Done in 0449ae6. Can't remove it because macCatalyst has same constraints as macOS.)
  • Unblock CI with [Infra] Remove module imports check #76
  • Update 7.8.0's change-log entry with note w.r.t. provisioning profile when using GULKeychain* and targeting macOS.
  • Add top level documentation to GULKeychainStorage.h and GULKeychainUtils.h regarding macOS-specific constraints.
  • Manual testing on SDKs that depend on GoogleUtilities
  • Get this PR approved
  • Get this PR merged
  • Send a following PR to bump the podspec to 7.8.0
  • Stage for 7.8.0 release after above PR is merged
  • Add a corresponding bug number in PR description after PR is ready for release (no outstanding work, green CI, etc).

Fixes

@ncooke3
Copy link
Member Author

ncooke3 commented Aug 25, 2022

Will add a corresponding bug number in PR description after getting PR ready for release.

@google-oss-bot
Copy link

google-oss-bot commented Aug 25, 2022

Coverage Report 1

Affected Products

  • GoogleUtilities-ios-unit-GoogleUtilities.framework

    Overall coverage changed from 0.12% (eba0a92) to 0.12% (88d1516) by -0.00%.

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/Xoyxf8JkhX.html

@ncooke3 ncooke3 changed the title [Keychain] Prevent keychain access from prompting user for permissions on macOS] [Keychain] Prevent keychain access from prompting user for permissions on macOS Aug 25, 2022
@ncooke3 ncooke3 force-pushed the nc/macos-keychain-fix branch from 7487b41 to 7213821 Compare August 25, 2022 03:13
@ncooke3 ncooke3 marked this pull request as ready for review August 25, 2022 16:04
Copy link
Member

@paulb777 paulb777 left a comment

Choose a reason for hiding this comment

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

🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants