Migrating Objects from Pre Catalina unexpected results

[gregcotten]

Hi there! I have a macOS 10.15 non-sanboxed, hardened-runtime app without the Keychain Sharing entitlement enabled.

I have a very simple valet setup

let valet = Valet.valet(with: Identifier(nonEmpty: Bundle.main.bundleIdentifier!)!, accessibility: .whenUnlocked)

I have recently upgraded to Valet 3.2.8. Please note this issue below did not occur before the kSecUseDataProtectionKeychain = true update.

On 10.14 and before, writing to this keychain puts a "application password" in the "login" keychain.

In Catalina, however, writing to this keychain results in a "Missing Entitlement" error. I know this isn't a Valet-specific issue. I go ahead and add the Keychain Sharing entitlement as I read somewhere that that could fix the issue - lo and behold I'm able to write to the keychain!

However, It's not writing to the "login" keychain but instead the "iCloud" keychain and seems to be using the default Keychain Access Group identifier ($teamID).($appIdentifier) as the access group instead of granting the app access directly. This is all a bit weird, and also migrateObjectsFromPreCatalina() does not perform an in-place adjustment but instead creates a duplicate keychain item because it now is an "iCloud" item and not a "login" item.

Any insight here is much appreciated!

[dfed]

Hi @gregcotten!

Thank you for the report! This is extremely useful information.

The fact that you needed to add a Keychain Sharing entitlement in order to read from that simple Valet setup is not the intended expectation of this library. However, I have reproduced the issue locally. My test application setup has the keychain entitlement (because that's how I test kSecAttrAccessGroup keychains) – it didn't occur to me that apps without a keychain sharing entitlement would get a kSecErrMissingEntitlement when trying to interact with a simple keychain.

The reason we added kSecUseDataProtectKeychain = true to all macOS Valets in #193 was that according to Apple's documentation and our tests, kSecUseDataProtectKeychain = true is required when using either kSecAttrAccessGroup or kSecAttrAccessible. In other words, kSecUseDataProtectKeychain = true is needed on macOS Catalina both for keychain sharing and for setting under what conditions a keychain item can be accessed. Without kSecUseDataProtectKeychain = true on macOS Catalina, the accessibility value you set in a Valet's initializer is entirely meaningless.

I think this means that we need to add to the README that macOS applications should enable the Keychain Sharing entitlement, whether or not they actually need keychain sharing. This is unfortunate, but I think the accessibility flag being a lie is worse. It is also unfortunate that Apple doesn't mention the Keychain Sharing entitlement requirement in its kSecUseDataProtectKeychain documentation.

It's not writing to the "login" keychain but instead the "iCloud" keychain

I think this is somewhat expected, bear with me as I explain. Apple has two different keychain implementations on macOS. The legacy macOS implementation, and the ported iOS implementation. Prior to macOS Catalina, there was some ambiguous overlap in how these keychains worked. In macOS Catalina, Apple required app developers to be explicit regarding which keychain was being used: if you don't use kSecUseDataProtectKeychain = true, you're using the legacy macOS keychain.

From what you've told me, I think the "iCloud" keychain displayed in Keychain Access.app shows items in the ported iOS implementation. Your Valet.valet(with: Identifier(nonEmpty: Bundle.main.bundleIdentifier!)!, accessibility: .whenUnlocked) will never set kSecAttrSynchronizable = true, which is required to sync items between machines using iCloud. In other words, The display of your items under the “iCloud” keychain doesn’t mean your keychain items in it are leaving your device.

Now that we're explicitly opting into the ported iOS implementation with kSecUseDataProtectKeychain = true, I think it makes sense (given the above assumptions) that your keychain item is showing up in the "iCloud" panel in Keychain Access.app.

[it] seems to be using the default Keychain Access Group identifier ($teamID).($appIdentifier) as the access group instead of granting the app access directly.

According to Apple's documentation on sharing keychain items between applications, it seems that Apple will automatically add a kSecAttrAccessGroup = ($teamID).($firstKeychainSharingGroupIdentifier) for you automatically. This is why you're seeing your ($teamID).($appIdentifier) as the access group when you inspect the item in Keychain Access.app. This should be additive, and doesn't affect Valet's sandboxing.

Hopefully that explanation helps! I'm going to keep this issue open until I land a PR adding documentation to our README that mentions the Keychain Sharing entitlement requirement. Again, thank you for reporting in detail what you were seeing. Happy to answer whatever questions you may have.

[gregcotten]

Thanks so much for the explanation! This all makes sense now.

I guess the only lingering weirdness is migrateObjectsFromPreCatalina() not being able to perform an in-place modification (instead duplicating the keychain item into the macOS “iCloud” keychain) if your macOS app previously didn’t have that entitlement enabled when writing to the keychain. I wonder if there is any way to delete the old one?

[dfed]

The duplication is unfortunate, but I can't remove the old key without potentially leading some applications to have data loss.

Imagine the following scenario:

1. An application X uses an iCloud Valet to sync login data between machines
2. A customer Y has a Mac running Catalina and the latest version of application X (which uses Valet 3.2.8)
3. That customer Y has another Mac running pre-Catalina and an older version of application X (which uses Valet 3.2.7 or prior)

If migrateObjectsFromPreCatalina() deleted the "login" keychain item, and the customer Y were to open the app on his newer Mac, I believe Y would be signed out of application X on their pre-Catalina machine.

Applications that use keychain access group sharing would have a similar problem if a customer updated one app in the keychain access group that uses Valet but didn't update the others.

Leaving the "login" item in the keychain doesn't have any user-facing negative affects, though I fully understand it is confusing for a developer trying to inspect their own keychain. If you're interested in creating a fork that deletes the old data, you can change the removesOnCompletion parameter on line 260 to true:

Valet/Sources/Valet/Valet.swift

Lines 251 to 261 in 72ec12a

	public func migrateObjectsFromPreCatalina() -> MigrationResult {
	var baseQuery = keychainQuery
	#if swift(>=5.1)
	baseQuery[kSecUseDataProtectionKeychain as String] = false
	#else
	baseQuery["nleg"] = false // kSecUseDataProtectionKeychain for Xcode 9 and Xcode 10 compatibility.
	#endif
	// We do not need to remove these items on completion, since we are updating the kSecUseDataProtectionKeychain attribute in-place.
	return migrateObjects(matching: baseQuery, removeOnCompletion: false)
	}

[dfed]

I've updated the release notes for 3.2.8 to include a note that the keychain sharing entitlement must be enabled to use Valet on macOS.

[gregcotten]

Looks good to me!