search

p2 / OAuth2

OAuth2 framework for macOS and iOS, written in Swift.
Other
1.15k stars 277 forks source link

Change default keychain access mode #321

Closed MrAsterisco closed 4 years ago

MrAsterisco commented 5 years ago

I have been using your library for a while in a medium-sized app which takes advantage of background app refresh. Recently, we started to do some reliability testing to see how long the app would be able to run flawlessly and I started to see a weird behavior, where the app would behave as if the user was not logged-in randomly. In other words, it looked like there were situations where my app wasn't able to restore the access and refresh tokens from the keychain.

After a lot of debugging and hours spent reading the issues in this repo, I found out the solution written among the lines in #116 and other StackOverflow questions related to the Keychain APIs.

The library uses kSecAttrAccessibleWhenUnlocked by default, which means that it won't be able to retrieve the tokens when launched in the background while the phone is locked. Changing it to kSecAttrAccessibleAfterFirstUnlock solves the issue.

I would like to suggest to either change the default value to the latter or, at least, to add some kind of documentation to clarify the issue. To someone that includes this library in their project, it is not clear at all that there is an incompatibility between background execution and the way the library stores tokens. Moreover, debugging these kind of things is a real pain.

I am happy to provide a PR for either solution, just let me know which one. I would be more inclined to just change the default value and keep everything simple, but also a warning somewhere in the docs might be enough.

ossus-lib commented 5 years ago

As you found out, you can change the default with the keychain_access_mode settings key, see https://github.com/p2/OAuth2/blob/master/Sources/Base/OAuth2Base.swift#L165 . This should remain the default since tokens are highly sensitive. But yes agree the documentation should be better.

MrAsterisco commented 5 years ago

I don't see having the token accessible after the first unlock as a security issue, but I understand your point. So how can we proceed?

ossus-lib commented 5 years ago

The security issue is that your tokens wouldn't be hardware encrypted once you've started and unlocked your phone until the phone is restarted.

We should call out in the README on how to change this.

MrAsterisco commented 5 years ago

Yes, that's what I was asking: do you want me to make a PR to update the Readme for this information or not?

ossus-lib commented 4 years ago

This is now in the README