Bug 24972 - Unable to query identity from iOS KeyChain
Summary: Unable to query identity from iOS KeyChain
Status: RESOLVED FIXED
Alias: None
Product: iOS
Classification: Xamarin
Component: Xamarin.iOS.dll ()
Version: master
Hardware: Macintosh Mac OS
: Normal normal
Target Milestone: 8.10
Assignee: Sebastien Pouliot
URL:
Depends on:
Blocks:
 
Reported: 2014-12-01 21:28 UTC by Artur Shamsutdinov
Modified: 2015-02-18 11:52 UTC (History)
3 users (show)

Tags:
Is this bug a regression?: ---
Last known good build:

Notice (2018-05-24): bugzilla.xamarin.com is now in read-only mode.

Please join us on Visual Studio Developer Community and in the Xamarin and Mono organizations on GitHub to continue tracking issues. Bugzilla will remain available for reference in read-only mode. We will continue to work on open Bugzilla bugs, copy them to the new locations as needed for follow-up, and add the new items under Related Links.

Our sincere thanks to everyone who has contributed on this bug tracker over the years. Thanks also for your understanding as we make these adjustments and improvements for the future.


Please create a new report on Developer Community or GitHub with your current version information, steps to reproduce, and relevant error messages or log files if you are hitting an issue that looks similar to this resolved bug and you do not yet see a matching new report.

Related Links:
Status:
RESOLVED FIXED

Description Artur Shamsutdinov 2014-12-01 21:28:18 UTC
Hi!

Our task is to save/load public&private key pair received from the server to iOS keychanin.
We use key class identity:
kSecClassIdentity
Identity item.
An identity is a certificate together with its associated private key. Because an identity is the combination of a private key and a certificate, this class shares attributes of both kSecClassKeyand kSecClassCertificate.

Saving works fine:

        var options = NSDictionary.FromObjectAndKey(NSObject.FromObject(Password), SecImportExport.Passphrase);
        NSDictionary[] result;
        SecStatusCode statusCode = SecImportExport.ImportPkcs12(cert, options, out result);
        var identity = result[0][SecImportExport.Identity];
        var record = new SecRecord(SecKind.Identity);
        record.SetValueRef(identity);
        SecKeyChain.Add(record);

But when we try to query keychain for identity:

var rec = new SecRecord(SecKind.Identity);
SecStatusCode rc;
var result = (SecIdentity)SecKeyChain.QueryAsConcreteType(rec, out rc);

Status code is always SecStatusCode.Param.

I believe that the problem is here:

    public SecRecord(SecKind secKind)
    {
      IntPtr num = SecClass.FromSecKind(secKind);
      if (num == SecClass.Identity) 
        this._queryDict = new NSMutableDictionary();
      else
        this.queryDict = NSMutableDictionary.LowlevelFromObjectAndKey(num, SecClass.SecClassKey);
    }

If SecKind is Identity you don’t put it to the dictionary. That works fine when I add identity to keychain but fails querying.

So I tried to put SecClassKey using reflection:

        var rec = new SecRecord(SecKind.Identity);
        var assembly =
            Assembly.Load("monotouch, Version=0.0.0.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065");
        var secClassType = assembly.GetType("MonoTouch.Security.SecClass");
        var identityProp = secClassType.GetProperty("Identity", BindingFlags.Static | BindingFlags.Public | BindingFlags.GetProperty);
        var identityKeyClass = identityProp.GetValue(null);
        var secKeyProp = secClassType.GetProperty("SecClassKey", BindingFlags.Static | BindingFlags.Public | BindingFlags.GetProperty);
        var keyClassKey = secKeyProp.GetValue(null);

        var secRecordType = rec.GetType();
        var setValueMethod = secRecordType.GetMethod("SetValue", BindingFlags.NonPublic | BindingFlags.Instance, null, CallingConventions.Any, new []{typeof(IntPtr), typeof(IntPtr)}, null);

        setValueMethod.Invoke(rec, new[] {identityKeyClass, keyClassKey});

        SecStatusCode rc;
        var result = (SecIdentity) SecKeyChain.QueryAsConcreteType(rec, out rc);
        if (rc != SecStatusCode.Success)
        {
            throw new SecurityException(rc);
        }
        return new NSUrlCredential(result, null, NSUrlCredentialPersistence.None);


And that works! I’m able to get my identity from the key chain now.
As for me this looks like a bug. But maybe I’m wrong. In this case please explain how that should be done and what is wrong in my code,
Thank you!

Artur Shamsutdinov.
Comment 2 Sebastien Pouliot 2014-12-02 10:44:28 UTC
@Artur, from memory (it's been a while) the `SecRecord` constructor was changed to be this way to fix some other test cases.

I'll verify which of our unit tests fails without this code.
Comment 3 Artur Shamsutdinov 2014-12-02 15:12:45 UTC
@Sebastian thank you!
I'm not an experienced iOS developed but I see that in the existing source that when identity is added to the keychain SecClassKey is not required but when it's requested from the keychain SecClasKey should be in the dictionary and it the moment it's not possible to do that with Xamarin without dirty hacks.
Comment 4 Artur Shamsutdinov 2014-12-02 15:14:24 UTC
Sebastien sorry for the wrong spelling of your name.
Comment 5 Sebastien Pouliot 2014-12-02 16:08:13 UTC
@Artur nm the spelling, it's a common mistake: french vs spanish spelling :-)

The special case for `SecClassKey` was added by me, more than 3 years ago when we added code to support `ValueRef`. 

> commit c7c28368aba8a7fa684413b0263177bc5f32c392
> Author: Sebastien Pouliot <sebastien@xamarin.com>
> Date:   Mon Nov 21 19:29:21 2011 -0500

It was not possible to use a `SecClassKey` before this.

> when identity is added to the keychain SecClassKey is not required

At that time it was not simply "not required" but was refused (by iOS) when supplied. AFAIR query'ing  worked fine at the time but I'm still digging thru the tests to find out what (and when) things changed (and how we can fix this without regressing existing code).
Comment 6 Artur Shamsutdinov 2014-12-02 16:50:40 UTC
@Sebastien looks like something has changed

Take a look on this documentation:
https://developer.apple.com/library/mac/documentation/Security/Conceptual/CertKeyTrustProgGuide/iPhone_Tasks/iPhone_Tasks.html
listings '2-3' and '2-4' show that they don't put 'kSecClass' when adding an identity to a keychain but they do put it for 'SecItemCopyMatching'.
Comment 7 Sebastien Pouliot 2014-12-02 20:33:42 UTC
Yes, it's interesting. There are sometimes a few minor differences between the Mac (your link) and the iOS pages, i.e.

https://developer.apple.com/library/ios/documentation/Security/Conceptual/CertKeyTrustProgGuide/iPhone_Tasks/iPhone_Tasks.html

However in this case both samples (of both pages) are identical.

The following code works for me, in the sense that I get `code ==Success`. 

			using (SecRecord rec = new SecRecord (SecKind.Identity))
			using (var id = IdentityTest.GetIdentity ()) {
				rec.SetValueRef (id);
				SecStatusCode code = SecKeyChain.Add (rec);
				Assert.True (code == SecStatusCode.DuplicateItem || code == SecStatusCode.Success);

				var match = SecKeyChain.QueryAsConcreteType (rec, out code);
				Assert.That (code, Is.EqualTo (SecStatusCode.Success), "QueryAsRecord");
			}

However `match` is `null`, which kind of defeat the purpose. It's not the only test but they likely share the pattern (assume `Success` to be a success).
Comment 8 Sebastien Pouliot 2014-12-02 21:12:20 UTC
Unlike other classes the API is not symmetric for SecIdentity. Furthermore in this case the return value (SecStatusCode) is not trustworthy. 

The current "best" (not using too many IntPtr or reflection) way to retrieve a SecIdentity is to do the following:

	using (SecRecord rec = new SecRecord (SecKind.Identity)) {
		(rec.ToDictionary () as NSMutableDictionary) ["class"] = new NSString ("idnt");

		SecStatusCode code;
		var match = SecKeyChain.QueryAsConcreteType (rec, out code);
		Assert.That (code, Is.EqualTo (SecStatusCode.Success), "QueryAsRecord-2");
		Assert.NotNull (match, "match-2");
	}

That will set back the `class` before doing the query and return the identify.

A future version of XI will ensure that a simpler

	using (SecRecord rec = new SecRecord (SecKind.Identity)) {
		...
	}

can be used for both adding and querying SecIdentity from the key chain.
Comment 9 Artur Shamsutdinov 2014-12-02 22:00:35 UTC
Many thanks Sebastien!

> The following code works for me, in the sense that I get `code ==Success`. 
>
>           using (SecRecord rec = new SecRecord (SecKind.Identity))
>           using (var id = IdentityTest.GetIdentity ()) {
>               rec.SetValueRef (id);
>               SecStatusCode code = SecKeyChain.Add (rec);
>                Assert.True (code == SecStatusCode.DuplicateItem || code ==
SecStatusCode.Success);
>
>               var match = SecKeyChain.QueryAsConcreteType (rec, out code);
>               Assert.That (code, Is.EqualTo (SecStatusCode.Success),
"QueryAsRecord");
>            }
>
>However `match` is `null`, which kind of defeat the purpose. It's not the only
>test but they likely share the pattern (assume `Success` to be a success).

Yes I tryed that and had the same result. But this is not my case - I have no identity ref for quering.

>Unlike other classes the API is not symmetric for SecIdentity. Furthermore in
>this case the return value (SecStatusCode) is not trustworthy. 

So I should check only the match itself but no the result code?

>        (rec.ToDictionary () as NSMutableDictionary) ["class"] = new NSString
> ("idnt");
I tried this approach but in my case statuscode is Success but result is null though when I use reflection everythig is fine.
Comment 10 Sebastien Pouliot 2014-12-02 22:08:50 UTC
> But this is not my case - I have no identity ref for quering.

Yes, my test case was different and it tried to get back what it added (without success). I still had the same error with a query that did not include the secidentity.

> So I should check only the match itself but no the result code?

The status code is not trustworthy (i.e. success does not mean it worked) but it's not useless or random (if you do not get Success then something went bad). IOW check both (code and match).

> I tried this approach but in my case statuscode is Success but result is null
> though when I use reflection everythig is fine.

Weird, that should be identical when it reach Apple code/API.

I'll need a self-contained test case, can you attach one to the bug report ?
Comment 11 Artur Shamsutdinov 2014-12-02 22:31:18 UTC
>I'll need a self-contained test case, can you attach one to the bug report ?
I'll try to make it today.
Comment 12 Artur Shamsutdinov 2014-12-03 04:40:45 UTC
@Sebastien checked again. Your approach works on sample. Will test on a real application again tomorrow.
Thank you for your help!
Comment 13 Sebastien Pouliot 2014-12-03 20:15:58 UTC
Update: everything seemed to work without the workaround until I tried on a iOS 5.1.1 device. There I can still duplicate the original issue (kind of make sense considering the date of the original fix). 

I'll make sure to handle this transparently. Please let me know if you find any other non-working case :-)
Comment 14 Artur Shamsutdinov 2014-12-03 21:37:56 UTC
Tested again on our app - your approach works fine.
Comment 15 Sebastien Pouliot 2015-02-18 11:52:22 UTC
Fixed in d76f77a256815e84c60034ce61bb40e74ee0fe7f

QA: unit tests added in the same revision