Skip to content
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

Added Access options to the get parameters with a default option set. #123

Closed
wants to merge 2 commits into from
Closed

Added Access options to the get parameters with a default option set. #123

wants to merge 2 commits into from

Conversation

robertbarclay
Copy link

Added Access options to the get parameters with a default option set. As posted in:

#78 (comment)

This provides a way for the developer to specify the same accessibility level that they saved the keychain item as to query out the keychain item. By default it is set to when unlocked, but it can be default to be afterFirst unlock to be able to access in the background

Also Added some basic tests to validate the passing of the access options for the get parameter.

Added tests to validate the passing of the access options.
@evgenyneu
Copy link
Owner

Thanks for the update, I thought we only need to set kSecAttrAccessible when creating a keychain item. Could you point me to Apple documentation that says that kSecAttrAccessible also needs to be set when reading an item from keychain?

All I could find here is this :

You control an app’s access to a keychain item relative to the state of a device by setting the item’s kSecAttrAccessible attribute when you create the item.

@robertbarclay
Copy link
Author

robertbarclay commented Nov 4, 2019

Sadly there is not much documentation here on it, and the behaviors seems to be changing somewhat, and getting more strict. I see conflicting posts, so it's hard to know with the lack of clear documentation on this matter. the example apps that apple provides have this stuff conspicuously absent, though i have found it time and again alluded to in the forums. I only recently ran into it in particular with iOS 13.1.1. They did a patch in the keychain, and behavior seemed to change significantly. When adding the accessibility level to all the queries, it solved those problems. i had been working with a new library for it, and it was absent, and ran into a bunch of issues. I liken the query to a SQL query to the DB, where the limits are a bunch of where statements. What was surprising to me about the observed behavior, which i did find documentation on, was that if its not specified, by default it is .whenUnlocked on the query. I can try to find that, again lot for reading on this. SO you are querying the keychain, if you do not have it specified with a where accessible = .whenUnlocked if you specify nothing. this does not matter if you are in the foreground with the application unlocked in most use cases. It only if you have an event that opens your application in the background while it's locked that you would ever run into this scenario. I get it constantly since i am handling incoming encrypted VOIP pushes for an incoming call and i must access the keychain single threaded and respond to the voip event to CallKit. In this case, the application has been launched while the system is locked. the work around was basically waiting until to do the query until you were unlocked, but in the case of NEEDING it outside of the being unlocked, you must query it from the keychain the correct way, which appears to require us to include the accessibility levels.

I've spent the last several months pouring over forums and reading everything on the keychain, to find that by adding it not just to the save and create query but to the search query, i am being more precise about my intentions on the query on the access levels, and it works flawlessly. It does not appear to be needed on the Delete, but mostly with the SecItemCopyMatching. While it may not technically not be required on all the queries, and can be defaulted, it seems in some locked scenarios, that having it more refined in the search query parameters ensures that it is always found. Maybe if nil, it should not be set at all, and only conditionally added if needing to be specified other than .whenUnlocked. That might be clearer.

The case is if you are setting a an entry to be a particular level, particularly a weaker level, then you should expect to query it at that expected level. It's already trying to query at a stricter level. Only the developer would know this, and allowing the developer to specify in the query, makes it more manageable. You will not run into it in most cases, since the application is unlocked, but in some edge cases you must. allowing the developer to specify only makes it easier and the library more usable. If you depend on the data in app extensions, or in the background while the device is locked, etc, you must it seems.

here a a few resources, but its not explicitly stated:
https://developer.apple.com/documentation/security/keychain_services/keychain_items/restricting_keychain_item_accessibility

and the source of the quotes, yes, it from a jailbreaking repo: NitinJami/keychaineditor#9

https://stackoverflow.com/questions/22403734/keychain-item-reported-as-errsecitemnotfound-but-receive-errsecduplicateitem-o

Probably the best though from Quinn "The Eskimo":
https://forums.developer.apple.com/thread/109353?q=keychain%20SecItemCopy*

… include the parameter in the Query, so it uses the built in defaults. When specified, it can be made more precise. Updated tests to accommodate for it.
@evgenyneu
Copy link
Owner

@robertbarclay, thanks for the information. I guess the only way to demonstrate if we need kSecAttrAccessible when reading from Keychain is to create a simple demo app:

  • First, we write to Keychain with accessibleAfterFirstUnlock.

  • Then we lock the device.

  • While in the background, the app reads from Keychain.

  • We Unlock the device and check if that read was successful.

What do you think?

@robertbarclay
Copy link
Author

that should do it, maybe. Normally your app would be suspended 30 seconds after, so you would need to be awoken or launched from the background to access it in a locked backgrounded state. This happens with push-kit, but it's hard to get it exactly into this exact scenario. (this does seem broken at the moment in iOS 13, but that is a different matter). you could maybe schedule a background task while and put the phone into the locked state before. This is how the Voip PushKit event is when running in the background, while the phone is locked. I have not found another easy way to do this scenario, but it is possible, but requires something interacting with your app in the background, not you directly.

So when you query the keychain, you are actually querying multiple data sets depending on the group, etc that your app has access too. by default you have access to the application keychain, and by specifying the application group when you create it, if you have the entitlements, then you are saving it into the keychain group, which is a different keychain than the application keychain. There is also the iCloud keychain as well, and MDM, depending on how the device is configured. While it is correct to not necessarily have it on the query parameters, technically you can have it stored in two places depending on how you stored it. I could write a key to the application keychain and to the group keychain. they could have the same label, but by my query parameters i set it to be in different groups. if i set them to be different accessible levels, i can get myself in a state where the key is there per the save, but i cannot query it with just the generic query, i need to be more explicit about my query, depending on the keychain and the lock state. (you would never want to do this, but you can). I want this key matching this access level and group, when i do the query. in most cases i do not need to include that in the query, but in complex cases, i must include it to get the right result. and according to some post i have seen, if you are not explicit in your query for the access level if it contains one you do not have access to and one that you do, you get neither. so its better in these cases to specify, since you know what access level is needed, since you the developer storing it defined it, you can also query it with that access level. Any field that is attached to that class of the keychain item you can include in your query to narrow the search, or make it more explicit. Also, if you have save the key with the invalid access level, by specifying in the access level, you would get null, so you can respond by logging the user out, etc and recreating the key with the proper access level. this is a way for graceful recovery or migration. if you accidentally did not specify your access level on the key in a release, and you have a version in the wild, even if you change how you save it, they key is still invalid for the access level until you have cleared it and resaved it. If you query with the filter for the access level, it comes back empty, and you then can respond by recreating correctly, without having to do a lot of migration code.if you have not set, the only time you would run into it would be when the device is not in that access level, like locked in the background, and you would inexplicably fail when all the code is correct and valid.

@evgenyneu
Copy link
Owner

Ok, here is a demo app:

https://github.com/evgenyneu/background-keychain

Tested it on my iPhone 6 with iOS 12.4.2. For me, it did read from keychain without specifying kSecAttrAccessible.

Let me know how it works for you. :)

@robertbarclay robertbarclay deleted the fix/background_lock_issue branch March 9, 2020 16:53
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.

None yet

2 participants