Use String for SecAttrAccount key, not data

[petrdvorak]

Currently, the KeychainWrapper.swift uses Data type as a "key" to store records. We got our head scratching for quite some time and we could not find any actual benefit of the current code.

// Uniquely identify the account who will be accessing the keychain
let encodedIdentifier: Data? = key.data(using: String.Encoding.utf8)

// ...

keychainQueryDictionary[SecAttrAccount] = encodedIdentifier

link to code

Why don't you simply use String, like Apple does?

You use String everywhere up until that point, then you convert it to Data just before storing the record...

[jrendel]

So I don't really have a good answer for you as to the why. Years ago I pulled this code from a Ray Wenderlich tutorial in Objective C (https://www.raywenderlich.com/6475/basic-security-in-ios-5-tutorial-part-1). At the time I didn't have full understanding of the keychain api myself and used the class through several apps, making small changes to it over time, always thinking it would make a good open source library to put out there for other to use. When Swift came out, it was one of the first things I wrote in Swift during the beta as a way to work on learning the language.

So it has evolved over time from that original tutorial, where those values had been encoded and its just not something I've ever considered fully since then.

Do you see a downside to encoding it? The issue with changing it now, is the major migration issues everyone using the wrapper would face for existing keys.

[petrdvorak]

@jrendel Thank you for the explanation. We will have to figure this one out on our end. Fix of this issue will not help us just for the reason you mentioned, anyway. We already have ~100k users with records with Data keys (Touch ID protected... bummer...) that we need to somehow migrate.

Couple notes:

• The current status is a bug - SecAttrAccount corresponding value should be CFStringRef, CFDataRef works by an accident, see: https://developer.apple.com/reference/security/ksecattraccount?language=objc
• The solution of the problem could be "versioning" of some kind.
  • It could be done on "data level" - projects with old data would read the Data, otherwise String could be used. This however requires a separate internal Keychain service, just for the sake of the version information storage.
  • Or - better (?) and easier to implement - versioning could be a configuration option in SDK (for example, a useLegacyStorage = true flag).
• To give you some context... We discovered the issue during the Keychain data migration. We currently have some keychain items that are protected with Touch ID that were created using the KeychainWrapper class. The new library we use needs to work with those keys (we tried to map the Keychain service and accounts), but the new library internally uses String for the account name - the migration is not easily possible for us. We will have to do it incrementally, as users unlock the Touch ID protected records... The new library we use is a high-level SDK for banking authentication called PowerAuth and it has own implementation of Keychain.

[jrendel]

Thanks for the background info on this. I agree, it should be changed to use a String. I'll have to think through the best way to do so, but I do like your idea of a legacy story flag. That would be a simple way to address it. I could also do that in conjunction with a fall back, if it attempts to read a key just using a string and can't find it, to try with the encoded key (for users of the wrapper who don't notice the new configuration flag).

[sacriaf]

I've ran into a bug due to this issue recently when using this library in a MacOS app (I understand that's not officially supported yet). While the current implementations happens to work on iOS apps (I've verified that the Account value for general password items are set correctly in the keychain sql db), it doesn't work on a MacOS app. On a MacOS app, the general password keychain item gets created without an account value (verifiable via keychain access). Simply changing the value type from data to Strings fixes the issue. I realize it's been a while since this thread was created. Has there been any new insights on a patch? Regarding migration, my limited testing on iOS seems to indicate compatibility with keychains created before the change.