granoff / Lockbox

Objective-C utility class for storing data securely in the key chain.
MIT License
847 stars 87 forks source link

SecItemCopyMatching failed for key #27

Closed magin closed 10 years ago

magin commented 10 years ago

Somewhy I get this in my console when using Lockbox:

+[Lockbox objectForKey:] [Line 110] SecItemCopyMatching failed for key com.magin.MaginSpeedMode.udidKey2: -25308 +[Lockbox setObject:forKey:accessibility:] [Line 94] SecItemAdd failed for key com.magin.MaginSpeedMode.udidKey2: -25308

christophberlin commented 10 years ago

Do you run into this issue with iOS 7 or iOS 8 BETA. I experience the same issue when running iOS 8 BETA 3 but iOS 7.1 works like a charm.

TheoJL commented 10 years ago

I also getting this occasionally with iOS7.1. This leads to a infinite loop for me because it always returns nil, even if I set the key right after. I think it only happens in debug environment but I'm not sure.

pabloivan57 commented 10 years ago

I'm also getting +[Lockbox setObject:forKey:accessibility:] [Line 94] SecItemAdd failed for key is this an issue with iOS 8?

magin commented 10 years ago

@granoff Any news on this issue? Still happening on 7.1

christophberlin commented 10 years ago

I revise my earlier comment that iOS8 could be the issue. Beta5 is working fine and others are noting that it happens on 7.1 as well...so it appears that the issue is related to something else

granoff commented 10 years ago

So, interesting error as it turns out. Error -25308 maps to errSecInteractionNotAllowed the description for which is:

Interaction with the user is required in order to grant access or process a request; however, user interaction with the Security Server has been disabled by the program.

So... what are you doing in your code that might cause this scenario? :-)

Perhaps some context of how you're using this when you see the error would be useful in tracking down what's really happening (i.e some code snippets or other details). The unit tests pass under iOS 7.0, 7.1, and iOS 8.0 beta, fwiw.

christophberlin commented 10 years ago

Hey, I don't think I am doing anything special. At startup (initial view controller) I run a method called startupCheck() that simply checks for an existing license key and done. On several occasions the app crashed on me with the listed error above BUT only when running the simulator and < iOS8 BETA 5. At this point I have a hard time replicating the issue and providing debug logs. Therefore I may not be as much as of a help but wanted to provide some feedback anyway.

-(void)startUpCheck
{
BOOL existingLicenseKey = NO;
NSString *apiKey;

[self.activityIndicator startAnimating];

if (!self.timeout)
{
    if ([Lockbox stringForKey:LOCKBOX_KEY])
    {
        apiKey = [Lockbox stringForKey:LOCKBOX_KEY];
        existingLicenseKey = YES;
    }
    else
    {
granoff commented 10 years ago

Thanks @cberlin2015 . The only thing I can think of (and this is really grasping at straws) is that the 2 rapid-fire lookups (if there is a key present) could be upsetting the keychain subsystem. Who knows, right? Might I make the following suggestion for an improvement:

if (!self.timeout)
{
    apiKey = [Lockbox stringForKey:LOCKBOX_KEY];
    if (apiKey) {
        existingLicenseKey = YES;
    } else {
    ...
    }
}

I'd be curious if that reduces the occurrences at all.

magin commented 10 years ago

Thanks for the reply @granoff!

First of all thanks for creating this library - it is super simple and very convenient for working with the Keychain.

I was able to escape from the -25308 error by setting

#define DEFAULT_ACCESSIBILITY kSecAttrAccessibleAlwaysThisDeviceOnly

This was best the best accessibility for my workflow. For more info on Keychain item accessibility see what Apple says: Keychain Item Accessibility Constants

Currently on the master repo this constant is kSecAttrAccessibleWhenUnlocked which will not work if the app does something in the background with Keychain while the device is locked (as was my case with background fetching in iOS 7).

However now, with this new accessibility setting, I occasionaly get error -34018 which is not even documented in Keychain Result Codes

Here is my workflow and how I'm using Lockbox to give you some context:

I am using Lockbox for persisting a deviceId property between app installs. (using it as a a replacement for uniqueIdentifier which was deprecated in iOS 5 and removed completely in iOS 7) I am also using it for persisting the device APN token.

I am sending the deviceId I get from Lockbox over the network to my backend service each time a request is made. The app I am building is an app that heavily relies on the network and sometimes can make 5-10 requests in just a few seconds. So it is accessing Keychain quite often indeed.

Here is how I interact with Lockbox:

-(void)setApnToken:(NSString *)apnToken
{
    if (apnToken.length && ![_apnToken isEqualToString:apnToken])
    {
        [Lockbox setString:apnToken forKey:@"apnToken"];
      }
}

- (NSString *)apnToken
{
    return ObjectOrNull([Lockbox stringForKey:@"apnToken"]);
}

-(NSString *)UUID
{
    NSString *UUID = [Lockbox stringForKey:@"currentDeviceUDID"];

    if (!UUID)
    {
        UUID = [[NSUUID UUID] UUIDString];
        [Lockbox setString:UUID forKey:@"currentDeviceUDID"];
    }

    return UUID;
}

I hope everything is clear and if not, please let me know so I can elaborate.

What would you recommend doing to try to debug/overcome this issue? Thanks!

granoff commented 10 years ago

Ah yes! Device accessibility. Another contributor added that; a nice addition indeed, and now that you mention it, that makes sense and explains the error you saw.

The error you now sometimes see is mentioned in this repo's README, but I found this question (with an answer similar to what is in the README) here: http://stackoverflow.com/questions/20344255/secitemadd-and-secitemcopymatching-returns-error-code-34018-errsecmissingentit . The error code apparently maps to errSecMissingEntitlement. Of all things the unit tests are the culprit and need to be code signed.

Your code looks reasonable enough. Have you considered caching apnToken once you've read it from the keychain rather than read it over and over again?

magin commented 10 years ago

@granoff Thanks for the response - yes, definitely I will cache in memory what I get from Lockbox on app launch since it's not volatile. I have implemented the solution and will update how it goes.

magin commented 10 years ago

Hello everyone. I did the fix suggested by @granoff to cache values in-memory once I read them from Keychain via Lockbox and so far for the past month it has worked fine. Both on iOS 7 and 8. Feel free to reach out if you have any questions.

rivera-ernesto commented 10 years ago

Isn't it something that could turn into a patch for the library?

SirWellington commented 10 years ago

I agree. It is also an issue I am facing. The API calls could be parameterized, with a default value of kSecAttrAccessibleWhenUnlocked.

magin commented 9 years ago

Just to update: I am still using on daily basis kSecAttrAccessibleAlwaysThisDeviceOnly plus caching what Lockbox returns and had not had any problems since August.

colasbd commented 9 years ago

The problem occurs here, with iOS9

-[Lockbox objectForKey:] [Line 133] SecItemCopyMatching failed for key com.company.app.AuthToken: -34018