google / GTMAppAuth

Apple platforms SDK for using AppAuth with Google libraries.
Apache License 2.0
379 stars 198 forks source link

KeychainAttribute does not work as expected #236

Closed olvrlrnz closed 9 months ago

olvrlrnz commented 10 months ago

Describe the bug

Specifying .useDataProtectionKeychain and/or .keychainAccessGroup("<group>") when creating a KeychainStore has no effect on the keychain item.

To Reproduce

  1. Allocate a store:
    let store = KeychainStore(
        itemName: "foo",
        keychainAttributes: [.useDataProtectionKeychain, .keychainAccessGroup(name: "TEAMID.com.example.foobar")]
    )
  2. Save an AuthSession
    try store.save(authSession: auth)
  3. Check the 'Access Control' tab in Keychain Access. There's only an entry for the calling application under 'Always allow access by these applications'.

Expected behavior

Environment

Additional context

I believe there's a bug in KeychainAttribute.keyName. It should not return the quoted constants but rather the constants itself. So kSecUseDataProtectionKeychain instead of "kSecUseDataProtectionKeychain" because KeychainWrapper uses the return value as-is to assemble the query dictionary for SecItem(Add|Delete|CopyMatching) here

olvrlrnz commented 10 months ago

This quick and dirty fix made it work for me on MacOS: GTMAppAuth.patch I guess the isMaxMacOSVersionGreaterThanTenOneFive macro does not do what was intended here. I never saw the key in my queries, that got me thinking.

Also, according to Apple [1], passing accessibility keys along to KeychainHelper (like here) is only supported if kSecUseDataProtectionKeychain is set to true

[1] Apple:

You can use this attribute for macOS keychain items only if you also set a value of true for the kSecUseDataProtectionKeychain key, the kSecAttrSynchronizable key, or both. For any item marked as synchronizable, the value for the kSecAttrAccessible key may only be one whose name does not end with ThisDeviceOnly, as those cannot sync to another device.

mdmathias commented 10 months ago

Hi and thanks for the issue! Since you have a patch, would you mind sending up a pull request for us to take a look at?

mdmathias commented 10 months ago

I have drafted https://github.com/google/GTMAppAuth/pull/237 to address this. Still need to do some testing in the example apps and hope to write some unit tests around this issue as well.

mdmathias commented 10 months ago

Some visual proof that this draft PR begins to address the issue.

Screenshot 2024-01-19 at 6 00 28 PM