[Mono.Android] Add convenience KeyChain APIs

[simonrozsival]

We have added the ability to work with certificates installed on device available through Android.Security.KeyChain for client authentication in SslStream and SocketsHttpHandler (dotnet/runtime#103337) by creating an instance of X509Certificate2 as a wrapper around KeyStore.PrivateKeyEntry. This API is not straightforward to use though so I am proposing convenience APIs for the KeyChain class that would make it much easier for .NET developers to access certificates with non-exportable private keys through KeyChain.

Note: The runtime PR has been only merged very recently so this code currently won't work. I'll keep this PR open as a draft for now to get some initial feedback before the changes from the runtime flow into the android SDK.

[dotMorten]

These makes a lot of sense to me. It's boilerplate code that was hard for me to figure out, and this short-circuits that and makes it more accessible in a standard .net way

[dellis1972]

Looks good, It might be nice to have some documentation on how to use these new methods? Maybe in the features area ? https://github.com/dotnet/android/tree/main/Documentation/docs-mobile/features

[jonpryor]

I'm conflicted with this PR.

While we make most bindings partial types, historically all that we've added are "obvious overloads" that cannot conflict with future Java overloads, e.g. adding an Action method overload for Java methods accepting an IRunnable parameter.

The one quasi-exception to this is generateAsyncWrapper, e.g.:

android/src/Mono.Android/metadata

Line 864 in b74d0fe

<attr api-since="5" path="/api/package[@name='android.accounts']/interface[@name='AccountManagerFuture']/method[@name='getResult']" name="generateAsyncWrapper">true</attr>

generateAsyncWrapper causes generator to emit a *Async "overload". If the declaring method is on an interface, extension methods are emitted:

//   <attr api-since="5" path="/api/package[@name='android.accounts']/interface[@name='AccountManagerFuture']/method[@name='getResult']" name="generateAsyncWrapper">true</attr>

namespace Android.Accounts {

	public partial interface IAccountManagerFuture : IJavaObject, IJavaPeerable {
		Java.Lang.Object? Result {
			get; 
		}
	}

	// Emitted because of `generateAsyncWrapper`
	public static partial class IAccountManagerFutureExtensions {
		public static global::System.Threading.Tasks.Task<Java.Lang.Object?> GetResultAsync (this Android.Accounts.IAccountManagerFuture self, long timeout, Java.Util.Concurrent.TimeUnit? unit)
		{
			return global::System.Threading.Tasks.Task.Run (() => self.GetResult (timeout, unit));
		}
	}
}

For classes, it's a normal method:

//   <attr api-since="8" path="/api/package[@name='javax.xml.validation']/class[@name='Validator']/method[@name='validate']" name="generateAsyncWrapper">true</attr>

namespace Javax.Xml.Validation {

	public abstract partial class Validator : Java.Lang.Object {
		public abstract void Validate (Javax.Xml.Transform.ISource? source, Javax.Xml.Transform.IResult? result);

		public global::System.Threading.Tasks.Task ValidateAsync (Javax.Xml.Transform.ISource? source, Javax.Xml.Transform.IResult? result)
		{
			return global::System.Threading.Tasks.Task.Run (() => Validate (source, result));
		}
	}
}

In retrospect, I suspect we should have always emitted extension methods (and/or used CancellationToken in the "overload"), because if Google ever adds e.g. a Validator.validateAsync(Source, Result) method, it'll collide, and we'll have problems. If we always emitted extension methods, then if Google added a *Async method, there'd be "breakage", but it would be an API break on upgrade, not an ABI break. (Meaning existing binaries would continue working as expected, while new code would use the new Android method instead of the extension method, which would be weird, but would make me feel better nonetheless.)

Alas, the semantics of generateAsyncWrapper can't be changed.

---

Which brings us to this PR: is it "future proof"? Is there any conceivable way for Google to add a method to a future Android version which would collide with the methods here?

I believe the answer is yes: C# doesn't care about return types in method overload resolution, only parameter types, and for all of these overloads the parameter types used either involve Java bindings (e.g. Java.Security.IPrincipal[], Android.Net.Uri) or have builtin overload support with generator (string). Nothing, to my eyes, prevents Google from adding this method overload in a future Android version:

/* partial */ class KeyChain {
    public static SomeFutureType choosePrivateKeyAlias(Activity activity, String[] keyTypes, Principal issuers, Uri uri, String alias) {
        // …
    }
}

which will immediately cause an ABI break.

---

Can the above "method scope" issue be addressed? Of course! The questions are how, and which is "best", and what are the criteria to determine "best"?

Various solutions that come to my mind include:

1. Add these methods "elsewhere." Mono.Android.dll is a place to have them, certainly, but is it the "best" place? Would something within MAUI be better? If Mono.Android.dll is the best place, is the existing KeyChain type the appropriate location? Or would a new Microsoft.Android.Security namespace and new type be better?

2. Make them extension methods on the first parameter type:

   namespace Android.Content;
   partial static class ContextExtensions {
       public static X509Certificate2? GetCertificateWithPrivateKey (
               this Android.Content.Context context,
               string alias) => …;
   }

   Pros: Nominally future-proof (unless/until Google adds an android.content.ContextExtensions class, which feels unlikely).
   Cons: Discoverability? We also have the "upgrade" issue mentioned above with generateAsyncWrapper: this preserves ABI compatibility -- existing binaries use, and will continue to use, the extension method -- but it isn't API compatible, as if Google adds a KeyChain.getCertificateWithPrivateKey() method which we bind as KeyChain.GetCertificateWithPrivateKey(), the new instance method "wins" over the extension method.

3. C# 13 extension types!

   public implicit extension KeyChainExtensions for KeyChain {
       public static X509Certificate2? GetCertificateWithPrivateKey (Context context, string alias) => …;
   }

   Pros: It's cool!
   Cons: It's harder to rely on C#13 right now. I also have no idea about the versionability of this: what happens if/when Google adds a KeyChain.getCertificateWithPrivateKey(Context, String) overload? The KeyChain binding would then also gain that method, so what happens when there are two applicable GetCertificateWithPrivateKey() methods? Is this like the extension method scenario, and KeyChain.GetCertificateWithPrivateKey() "wins"? Or does something else happen?

My current preference is toward (1) or (2).

[grendello]

@jonpryor I'm not sure the "threat" of Google adding conflicting methods is high in this case, but it's certainly a possibility. I think another solution would be to add an "unlikely" prefix to method names (e.g. back in the day it would be XA, today probably MS) and keep them in the type as they are now. Extension methods are fine if you don't own the code, but we kind of do (kind of because we generate it based on code we don't own) and the new methods logically belong in this type - I believe in creating as little cognitive noise as possible. A prefix for such methods could become our standard of implementing them, too.

[simonrozsival]

@jonpryor @grendello thanks for the feedback. My intent was to make these APIs discoverable through intellisense so that when the developer types KeyChain.Get or KeyChain.Choose, they will see our .NET APIs there in addition to the Google APIs.

We could also use a parameter with a .NET type in the method signature so that when Google accidentally introduces methods with the same name, there is no collision in method signatures:

static Task<X509Certificate2?> ChoosePrivateKeyAliasAsync(..., System.Uri? uri, ...)
{
    // when on API < 23, call the older overload with uri.Host and uri.Port
}

Admittedly, I don't know how to ensure the uniqueness of the GetCertificateWithPrivateKey method this way.

Instead of the "unlikely" prefix such as MS, we could make the method name unique by including the name of the .NET type X509Certificate2 which isn't used anywhere in Java and it seems highly unlikely that Google would be forced to use a method with such ugly name when they have a lot of room for more reasonable names:

KeyChain.GetX509Certificate2WithPrivateKey(context, alias);
KeyChain.ChooseX509Certificate2WithPrivateKeyAsync(activity, ...);

---

Maybe the option (1) is the best after all. The only really important method is GetCertificateWithPrivateKey and we don't need to really introduce the *Async methods. Maybe we can turn that into a C# 13 implicit static extension method X509Certificate2.FromPrivateKeyAlias(context, alias) or X509CertificateLoader.LoadFromKeyChain(context, alias).

I agree that requiring C# 13 might be limiting for some customers, so maybe we should just add a completely custom static class System.Security.Cryptography.X509Certificates.X509CertificateAndroidKeyChainLoader with some of the methods that I suggested for KeyChain. In this case there would still be some discoverability when the developer types X509Certificate and the suggestions would show the Android-specific class.

[jonpryor]

@simonrozsival wrote:

My intent was to make these APIs discoverable through intellisense so that when the developer types KeyChain.Get or KeyChain.Choose…

which prompts a question: how/why would a developer be writing KeyChain. in the first place? What tutorial are they following? Sample? Other?

Is there a "better" scope that a developer could/should know about?

Would it be "better" to instead have a C#13 extension on X509Certificate2, permitting X509Certificate2.FromAndroidContext(context, alias)? Would that be reasonable?

I have no idea about any of this, because I don't understand what led the developer to be typing KeyChain. in the first place. As such, I have no idea what would be most discoverable. I suspect that keying off System.Security "somehow" would make more sense, but… I really have no idea.

---

Assuming that KeyChain is in fact the most obvious place for such functionality, I like the idea of GetX509Certificate2…-style methods. Google doesn't (yet) provide an X509Certificate2 type -- they only have java.security.cert.X509Certificate -- so X509Certificate2 should be a useful disambiguation. If also horrifically verbose (but that's what code completion is for, right?).

[simonrozsival]

how/why would a developer be writing KeyChain. in the first place? What tutorial are they following? Sample? Other?

My thinking was that the developer could be looking for native APIs to access certificates installed on the device and learn about KeyChain.choosePrivateKeyAlias for example in the official Android docs. Then they would look if the same works in .NET for Android and they would either look into the KeyChain docs or by trying to port the Java/Kotlin code into .NET and in both cases, they could discover these convenience APIs.

It is possible that the APIs wouldn't be discoverable this way. In that case, maybe we don't need these APIs at all, and what we need is a .NET specific guide that would help the developers use the KeyChain directly and correctly set up the X509Certificate2 instance via the new IntPtr constructor before using it with SslStream or SocketsHttpHandler.

[dotMorten]

I don't understand what led the developer to be typing KeyChain.

Because when I googled to figure out how to use the installed certificates, this is where I ended up. It's pretty common that when I need to do something Android specific I look through the android doc instead, and then translate that to C#, while looking for .NET specific overloads to help that work (like what is proposed here).
My guess is if you put it somewhere else, I probably wouldn't have found these helpers and would be rummaging around the keychain APIs.

[jonpryor]

@dotMorten: thank you for the feedback! KeyChain might be the best place then.

[dotMorten]

Or perhaps the new X509CertificateLoader APIs might be the better place? dotnet/docs#41662

[simonrozsival]

I don't think the X509CertificateLoader is the right place for these new APIs:

• These APIs are Android specific
• The certificate loader contains methods that load certificates from raw bytes (from memory or files)

I still think the KeyChain is the best place to put these methods.