[Mono.Android] Add support for AndroidMessageHandler ClientCertificates

[simonrozsival]

This PR updates the SSLSetup method of AndroidMessageHandler to load ClientCertificates into a KeyStore before making the request.

Closes #7274
Closes dotnet/runtime#78933
Related to and inspired by xamarin/xamarin-macios#20434 (cc @dotMorten)

[simonrozsival]

The expected apk size increased, this is expected I think:

apkdiff regression test failed with exit code: 3
context: https://github.com/xamarin/xamarin-android/blob/main/Documentation/project-docs/ApkSizeRegressionChecks.md
stdOut: Saving apk description to '/Users/runner/work/1/a/TestRelease/05-20_13.20.25/BuildReleaseArm64XFormsDotNet.apkdesc'
Size difference in bytes ([*1] apk1 only, [*2] apk2 only):

 70,654 lib/arm64-v8a/lib_System.Security.Cryptography.dll.so
 36,057 lib/arm64-v8a/lib_System.Runtime.Numerics.dll.so *2
 31,805 lib/arm64-v8a/lib_System.Formats.Asn1.dll.so *2
 15,672 lib/arm64-v8a/libmonodroid.so
 15,536 lib/arm64-v8a/libmonosgen-2.0.so
  4,124 lib/arm64-v8a/lib_Mono.Android.dll.so
  3,946 lib/arm64-v8a/lib_System.Private.CoreLib.dll.so
  2,230 lib/arm64-v8a/lib_System.Collections.dll.so
  1,080 lib/arm64-v8a/libxamarin-app.so
    576 lib/arm64-v8a/libSystem.Native.so
    407 lib/arm64-v8a/lib_Java.Interop.dll.so
    236 META-INF/BNDLTOOL.SF
    236 META-INF/MANIFEST.MF
    152 lib/arm64-v8a/lib_System.Private.Xml.dll.so
     39 lib/arm64-v8a/lib_System.Text.RegularExpressions.dll.so
     28 lib/arm64-v8a/lib_System.Net.Primitives.dll.so
     21 lib/arm64-v8a/lib_System.Collections.Concurrent.dll.so
      6 lib/arm64-v8a/lib_System.Private.Uri.dll.so
      4 lib/arm64-v8a/lib_System.Net.Http.dll.so
      2 lib/arm64-v8a/lib_System.Console.dll.so
      2 lib/arm64-v8a/lib_System.IO.Compression.dll.so
      1 lib/arm64-v8a/lib_System.ComponentModel.TypeConverter.dll.so
      1 lib/arm64-v8a/lib_System.Private.DataContractSerialization.dll.so
      1 lib/arm64-v8a/lib_System.ComponentModel.dll.so
      1 lib/arm64-v8a/lib_System.Core.dll.so
      1 lib/arm64-v8a/lib_System.Diagnostics.DiagnosticSource.dll.so
      1 lib/arm64-v8a/lib_System.Diagnostics.TraceSource.dll.so
      1 lib/arm64-v8a/lib_System.Drawing.Primitives.dll.so
      1 lib/arm64-v8a/lib_System.IO.IsolatedStorage.dll.so
      1 lib/arm64-v8a/lib_System.Linq.dll.so
      1 lib/arm64-v8a/lib_System.Net.Requests.dll.so
      1 lib/arm64-v8a/lib_System.Runtime.dll.so
      1 lib/arm64-v8a/lib_System.Runtime.Serialization.dll.so
      1 lib/arm64-v8a/lib_System.Runtime.Serialization.Primitives.dll.so
      1 lib/arm64-v8a/lib_System.Xml.Linq.dll.so
      2 lib/arm64-v8a/lib_mscorlib.dll.so
      2 lib/arm64-v8a/lib_netstandard.dll.so
      2 lib/arm64-v8a/lib_System.ObjectModel.dll.so
      2 lib/arm64-v8a/lib_System.Private.Xml.Linq.dll.so
      3 lib/arm64-v8a/lib_System.Collections.NonGeneric.dll.so
     40 lib/arm64-v8a/lib_System.IO.Compression.Brotli.dll.so
     47 lib/arm64-v8a/lib_System.Linq.Expressions.dll.so
     53 lib/arm64-v8a/lib_Mono.Android.Runtime.dll.so

Summary:

    472 Other entries 0.03% (of 1,566,528)
      0 Dalvik executables 0.00% (of 9,591,100)
182,180 Shared libraries 2.02% (of 9,011,330)
131,256 Package size difference 1.28% (of 10,271,784)

stdErr: Error: apkdiff: File 'lib/arm64-v8a/lib_System.Security.Cryptography.dll.so' has changed by 70,654 bytes (89.71 %). This exceeds the threshold of 5.00 %.
Error: apkdiff: File 'lib/arm64-v8a/lib_System.Runtime.Numerics.dll.so' has changed by 36,057 bytes (100.00 %). This exceeds the threshold of 5.00 %.
Error: apkdiff: File 'lib/arm64-v8a/lib_System.Formats.Asn1.dll.so' has changed by 31,805 bytes (100.00 %). This exceeds the threshold of 5.00 %.
Error: apkdiff: File 'lib/arm64-v8a/lib_System.Collections.dll.so' has changed by 2,230 bytes (12.64 %). This exceeds the threshold of 5.00 %.
Error: apkdiff: Size regression occured, 4 check(s) failed.
Expected: True
But was:  False

Is this increase acceptable?

[jonpryor]

I believe that this is somewhat contrary to the Framework design guidelines; from Property Design:

✔️ DO allow properties to be set in any order even if this results in a temporary invalid state of the object.

It is common for two or more properties to be interrelated to a point where some values of one property might be invalid given the values of other properties on the same object. In such cases, exceptions resulting from the invalid state should be postponed until the interrelated properties are actually used together by the object.

For example, consider this snippet:

var handler = new AndroidMessageHandler(…) {
    ClientCertificates = {
        certificate,
    },
    ClientCertificateOptions = ClientCertificateOption.Automatic,
};

Because C# collection initializer syntax is repeated .Add() calls, the above is equivalent to:

var handler = new AndroidMessageHandler(…);
handler.ClientCertificates.Add(certificate);
handler.ClientCertificateOptions = ClientCertificateOption.Automatic,

which is an invalid state: having any ClientCertificates should require ClientCertificateOption.Manual, as per the current check, but (as far as I can tell) nothing actually validates this ordering.

(And nothing should validate "ordering"!)

I think it would thus be preferable to delay the "consistency check" until…

[simonrozsival]

First of all, thanks for the detailed feedback, @jonpryor!

You have a great point here, this design choice isn't great. I'm following HttpClientHandler which also throws. I would personally keep this in line with the runtime, but I don't have a strong preference and I can definitely modify the code.

[jonpryor]

On the one hand, following what HttpClientHandler is doing makes sense.

On the other hand, I'm not convinced that actually fixes the implicit ordering issues.

A quick glance through the HttpClientHandler unit tests for ClientCertificates suggests that this ordering issue is not tested at all. Fortunately, I don't see any issues on dotnet/runtime about this either…

[dotMorten]

The required order makes sense to me. The error you get is helpful too and expected

[jonpryor]

…here?

if (ClientCertificateOptions != ClientCertificateOption.Manual) {
    throw new InvalidOperationException ($"Use of {nameof(ClientCertificates)} requires that {nameof(ClientCertificateOptions)} be set to ClientCertificateOption.Manual");
}

[jonpryor]

…and this should probably be:

if (_clientCertificates is null || _clientCertificates.Count == 0) {
    return;
}

[jonpryor]

Why out bool gotCerts instead of bool return type?

[jonpryor]

Obvious response: because GetConfiguredKeyStoreInstance() has an out parameter.

Obvious counter-response: so?

KeyStore GetConfiguredKeyStoreInstance (out bool gotTrustedCerts, out bool gotClientCerts)
{
    …
    gotClientCerts = LoadClientCertificates (keyStore);
}

I'm not sure what the out parameter is actually buying us.

[simonrozsival]

I mostly just shuffled existing code around here, so I kept the out bool we already had. I'm not a big fan of out arguments myself so I'll look into ways of getting rid of it.

[jonpryor]

I'm not sure I like the implied semantics of the "wtf" path.

Assume that ClientCertificateOptions=ClientCertificateOptions.Manual and ClientCertificates is set to a non-empty collection. This presumably means "the certificates should be used".

Execution eventually hits this method, which calls KeyStore.GetInstance(KeyStore.DefaultType), which could return null. If that happens, then:

• keyStore?.Load() is ignored
• LoadClientCertificates(keyStore, out gotClientCerts) becomes an effective no-op, ignoring the contents of ClientCertificates (okay, not quite, X509Certificate2 instances will be created, other logic happens, but nothing important happens to the keyStore instance, as it's null)
• …and then…???

Should this happen? Of course not.

But if it does happen -- something goes FUBAR, reasons unknown -- then the downstream effect will be "as if" ClientCertificates weren't set at all, which in turn will likely result in a bunch of head scratching and cursing, or a security vulnerability. (I have an overactive imagination.)

I would prefer that we instead require that things work:

var keyStore = KeyStore.GetInstance (KeyStore.DefaultType) ??
    throw new InvalidOperationException($"KeyStore.GetInstance(\"{KeyStore.DefaultType}\") returned null!");

and then replace most of the keyStore?.Whatever with keyStore.Whatever, as we are requiring that KeyStore be non-null.

[simonrozsival]

Thans for pointing this out. The GetInstance(DefaultType) call never returns null AFAIK (at least on any existing Android version), so ensuring it is null will clean up the code and make the intent much clearer. We might also avoid weird silent errors at some later point if the behavior of the method changes.

[jonpryor]

This change seems odd; what's pulling in System.Runtime.Numerics.dll?

[simonrozsival]

This the Asn1 DLLs are both dependencies of System.Security.Cryptography (https://github.com/dotnet/runtime/blob/e8b89a3fde2911c6cbac0488bf82c74329a7224a/src/libraries/System.Security.Cryptography/src/System.Security.Cryptography.csproj#L1785-L1798).

[jonpryor]

This change seems odd; what's pulling in System.Formats.Asn1.dll?

[jonpryor]

a'la my previous comment, we should update this test to include setting ClientCertificateOptions:

[Test]
public async Task AndroidMessageHandlerSendsClientCertificate([Values(true, false)] bool manualOptions)
{
    string testClientCertificate = …;
    using var certificate = new X509Certificate(Convert.FromBase64String (testClientCertificate), "pass");
    using var handler = new AndroidMessageHandler {
        ClientCertificates = { certificate },
        ClientCertificateOptions = manualOptions ? ClientCertificateOptions.Manual : ClientCertificateOptions.Automatic,
    };
    using var client = new HttpClient (handler);

    try {
        var response = await client.GetAsync ("https://corefx-net-tls.azurewebsites.net/EchoClientCertificate.ashx");
        var content = await response.EnsureSuccessStatusCode ().Content.ReadAsStringAsync ();
        if (!manualOptions) AssertNotReached();
    }
    catch (Exception e) {
        if (manualOptions) AssertNotReached();

        // assert that e is whatever it should be (InvalidOperationException?)
    }
}

[simonrozsival]

@jonpryor could you please take another look? I hope we could get this ready in time for the next preview release.

[jonpryor]

This test concerns me, as it's (kinda, sorta) the "inverse" of my previous comment. This test asserts that when AndroidMessageHandler.ClientCertificateOptions is ClientCertificateOption.Automatic, accessing AndroidMessageHandler.ClientCertificates will throw.

Fair enough.

But what should happen when ClientCertificateOptions is set last?

var h = new AndroidMessageHandler {
    ClientCertificates = { BuildClientCertificate () },
    ClientCertificateOptions = ClientCertificateOption.Automatic,
};
var c = new HttpClient(h);
var r = await c.GetAsync (…);

Because you've updated the default value of AndroidMessageHandler.ClientCertificateOptions to ClientCertificateOption.Manual, I believe that h will be constructed without error, because when h.ClientCertificates is accessed, ClientCertificateOptions will still be .Manual, so h.ClientCertificates shouldn't throw.

The result is that after construction, h is in an invalid state.

What will happen when we try to use h? What should happen?

[simonrozsival]

What will happen when you try to use h? What should happen?

1. When we set ClientCertificateOptions.Automatic first and then we set ClientCertificates, we know we're in an invalid state as soon as we set ClientCertificates (they would be ignored). HttpClientHandler throws which breaks the design guideline that we should allow being temporarily in an invalid state.

   • I think we should follow the design of HttpClientHandler to keep the behavior consistent accross platforms since AndroidMessageHandler is commonly used through HttpClientHandler.

2. In the other scenario, when we set ClientCertificates first and then we set ClientCertificateOptions.Automatic, the HttpClientHandler/SocketsHttpHandler is not in an invalid state because it chooses to ignore the ClientCertificates completely (certificates from the current user's X509 store are used instead).

   • We could maybe support the Automatic mode as well (using KeyChain.choosePrivateKeyAlias to show the user a UI to choose a certificate?) but definitely not in this PR. Until we support the Automatic mode, I think it makes sense to throw once we are preparing the request here: https://github.com/xamarin/xamarin-android/pull/8961/files?diff=split&w=0#diff-5db393c58158459e46590be1423607c52488da9182e9e5e929d7b79e7179d13dR1219-R1221 Maybe this deserves its own unit test.
   • Alternatively, we could choose to throw as early as when the developer sets ClientCertificateOptions.Automatic because we don't support it at all. If we want to go in this direction, we should coordinate this with the iOS/Mac implementation (cc @dotMorten)

[jonpryor]

That makes sense: ClientCertificateOptions = ClientCertificateOption.Automatic means "ignore ClientCertificates", so the resulting state isn't "invalid", it's just "weird" (contains data that cannot be used until ClientCertificateOptions is changed again).

@simonrozsival: what's the process for getting documentation updated? The documentation for HttpClientHandler.ClientCertificates does not mention that it could throw an exception depending on the value of HttpClientHandler.ClientCertificateOptions.

[simonrozsival]

what's the process for getting documentation updated?

AFAIK the source of the documentation is here: https://github.com/dotnet/dotnet-api-docs/blob/2cd326e62adfa53ec1df761f571d1f2b94c263ab/xml/System.Net.Http/HttpClientHandler.xml#L256-L353 and the process is described here: https://learn.microsoft.com/en-gb/contribute/content/dotnet/dotnet-contribute

[jonpryor]

@simonrozsival: please rebase this branch and resolve the merge conflict.

[simonrozsival]

@jonpryor please take another look