evgenyneu / keychain-swift

Helper functions for saving text in Keychain securely for iOS, OS X, tvOS and watchOS.
MIT License
2.82k stars 345 forks source link

Added item class feature #95

Open Oleygen opened 5 years ago

Oleygen commented 5 years ago

Adding possibility to set a class for items to set/get/delete/clear. Backwards compatibility reached by using default parameter (genericPassword). Please check is this pull request is valuable for your library, and notify me if it needs any improvement or I've missed some caveats. Please note: There is missing objective-c compatibility changes, and mb some other files should be modified (you can help if specify what may I missed)

evgenyneu commented 5 years ago

Thanks for the update @Oleygen. What motivated you to add this feature?

Oleygen commented 5 years ago

I'm using your lib in my app. I need to store RSA private key on a device, and I've noticed that all items are stored as password, which is not correct for my case

evgenyneu commented 5 years ago

Did you need to store your key as kSecClassCertificate?

Oleygen commented 5 years ago

Yes, exactly

evgenyneu commented 5 years ago

Is saving text as kSecClassCertificate different than saving it as kSecClassGenericPassword?

Oleygen commented 5 years ago

1) It differes in a way it displays in a keychain 2) Apple documentation notes that items stores on disk in a different way, based on its class (see https://developer.apple.com/documentation/security/keychain_services/keychain_items/item_class_keys_and_values)

evgenyneu commented 5 years ago

It differes in a way it displays in a keychain

In what "way", sorry? And why does it matter?

Apple documentation notes that items stores on disk in a different way, based on its class (see https://developer.apple.com/documentation/security/keychain_services/keychain_items/item_class_keys_and_values)

The documentation says

The item's class dictates which attributes apply and enables the system to decide whether or not the data should be encrypted on disk. For example, passwords require encryption, but certificates don't because they are not secret.

So I guess, the text stored as kSecClassCertificate won't be encrypted in the keychain. Why is it important to store things without encryption?

Oleygen commented 5 years ago

here is keychain UI block: https://imgur.com/a/BNjyRNa as you can see, password, certs and keys are separated. To summarize our conversation: yes, item class is mostly about semantic

How do you think do you need this changes for your repo? Cause I'd like to be fully correct in my apps. Also I'd like to add enum swift wrapper on top of OSStatus error, will add pull request later

evgenyneu commented 5 years ago

How do you think do you need this changes for your repo?

No, sorry. While specifying keychain classes will be useful to some users of the library, I am not sure this is something that majority of users of this library care about. I don't think it is worth increasing the complexity of the library to introduce this feature. The only point of this library is to save text to keychain without caring about details. There are many alternative full-featured Keychain libraries that people can choose if they need extra features.

Also I'd like to add enum swift wrapper on top of OSStatus error, will add pull request later

Cool, could you explain what is this change about and your motivation before implementing it?