Deprecate AndroidClientHandler in .NET 6

[simonrozsival]

After talking to @steveisok we decided to seal deprecate AndroidClientHandler in .NET 6. Users should prefer deriving from AndroidMessageHandler.

There are several changes in this PR:

• AndroidMessageHandler had internal virtual methods, these have to be changed to protected virtual
• The usage of AndroidClientHandler should be discouraged, so we added the [Obsolete] attribute
• There were many places in the codebase that refer to the AndroidClientHandler so these needed to be changed to AndroidMessageHandler but only for the .NET 6 variant
• There was a bug in AndroidClientHandler.GetUnderlyingHandler() which prevented users from extending the class. Since we're not sealing it after all, I fixed it.

Closes #6526

[grendello]

Thanks @simonrozsival for the PR, but I'm afraid it's not enough just to make the class sealed :( If you read the docstring above the class, it talks about a method to implement SSL certificate validation by deriving a class from AndroidClientHandler. Could you make changes to address that scenario, also updating the docs as needed? Thanks!

[steveisok]

Thanks @simonrozsival for the PR, but I'm afraid it's not enough just to make the class sealed :( If you read the docstring above the class, it talks about a method to implement SSL certificate validation by deriving a class from AndroidClientHandler. Could you make changes to address that scenario, also updating the docs as needed? Thanks!

@grendello Given the changes in net6, AndroidMessageHandler is the class to derive from.

[grendello]

@steveisok Yes, I know, but the doc string (at the very least) needs to be updated to reflect that change, so that it's not misleading.

[simonrozsival]

Thanks for the feedback. I'll make those changes 👍

[emorell96]

So before net6.0 if you wanted to do client certificates on Xamarin you would do something like below where you would override ConfigureCustomSSLSocketFactory, how would you do the same thing with AndroidMessageHandler?

public class AndroidHttpsClientHandler : AndroidClientHandler
    {
        private SSLContext sslContext;
        private ITrustManager[] trustManagers;
        private IKeyManager[] keyManagers = null;

       

        public void Initialize()
        {
            trustManagers = GetTrustManagers();
            sslContext = GetSSLContext();
        }

        private SSLContext GetSSLContext()
        {
            string protocol;
            if (SslProtocols == SslProtocols.Tls11)
            {
                protocol = "TLSv1.1";
            }
            else if (SslProtocols == SslProtocols.Tls || SslProtocols == SslProtocols.Tls12)
            {
                protocol = "TLSv1.2";
            }
            else
            {
                throw new IOException("unsupported ssl protocol: " + SslProtocols.ToString());
            }
            SSLContext ctx = SSLContext.GetInstance(protocol);
            ctx.Init(keyManagers, trustManagers, null);
            return ctx;
        }

        public new SslProtocols SslProtocols { get; set; } = SslProtocols.Tls12;

        public void SetClientCertificate(byte[] pkcs12, char[] password)
        {
            keyManagers = GetKeyManagersFromClientCert(pkcs12, password);
            SSLContext newContext = GetSSLContext();
            sslContext = newContext;
        }

        private ITrustManager[] GetTrustManagers()
        {
            TrustManagerFactory trustManagerFactory = TrustManagerFactory.GetInstance(TrustManagerFactory.DefaultAlgorithm);
            trustManagerFactory.Init((KeyStore)null);
            return trustManagerFactory.GetTrustManagers();
        }

        private IKeyManager[] GetKeyManagersFromClientCert(byte[] pkcs12, char[] password)
        {
            if (pkcs12 != null)
            {
                using (MemoryStream memoryStream = new MemoryStream(pkcs12))
                {
                    KeyStore keyStore = KeyStore.GetInstance("pkcs12");
                    
                    keyStore.Load(memoryStream, password);
                    KeyManagerFactory kmf = KeyManagerFactory.GetInstance("x509");
                    kmf.Init(keyStore, password);
                    return kmf.GetKeyManagers();
                }
            }
            return null;
        }

        protected override SSLSocketFactory ConfigureCustomSSLSocketFactory(HttpsURLConnection connection)
        {
            SSLSocketFactory socketFactory = sslContext.SocketFactory;
            if (connection != null)
            {
                connection.SSLSocketFactory = socketFactory;
            }
            return socketFactory;
        }
    }

Thank you!

[simonrozsival]

/azp run

[azure-pipelines]

Commenter does not have sufficient privileges for PR 6541 in repo xamarin/xamarin-android

[simonrozsival]

@emorell96 it seems that right now it's not possible (the ConfigureCustomSSLSocketFactory method is internal virtual in AndroidMessageHandler) but if this change is approved then you'll be able to extend it the same way you did with AndroidClientHandler.

[jpobst]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[jonpryor]

work-in-progress Squash-and-merge commit message:

[One .NET] Deprecate Xamarin.Android.Net.AndroidClientHandler (#6541)

Fixes: https://github.com/xamarin/xamarin-android/issues/6526

The .NET 6+ `AndroidClientHandler` type ("added" in 1e5bfa3f) couldn't
be used as a base class.  Attempting to do so [results in][0] an
`InvalidOperationException`

	class DerivedHandler : Xamarin.Android.Net.AndroidClientHandler {
	}
	class DerivedHandler2 : DerivedHandler {
	}

	// …
	new DerivedHandler2();
	// throws InvalidOperationException:
	//   Field '_nativeHandler' is missing from type 'DerivedHandler'.

Fix this bug by updating `AndroidClientHandler.GetUnderlyingHandler()`
to check all base classes, not just the immediate base class, for the
`_nativeHandler` field.

Additionally, `[Obsolete]` the `AndroidClientHandler` type.

In commit 1e5bfa3f, we "moved" the functionality that was previously
in `Xamarin.Android.Net.AndroidClientHandler` into a new type,
`Xamarin.Android.Net.AndroidMessageHandler`.  This was done because
in .NET 6, the `HttpClientHandler` type -- which `AndroidClientHandler`
derives from -- has an underlying `HttpMessageHandler` type used to
do all of the work.  For Android, there are actually two underlying
handler types to choose from: `AndroidMessageHandler` and
`HttpClientHandler`.  If `AndroidClientHandler` had stayed the same,
we would have a circular dependency and that created the need for a
more independent `AndroidMessageHandler` type.

.NET 6 users should move away from `AndroidClientHandler` to
`AndroidMessageHandler`, as `AndroidClientHandler` remains only for
backward compatibility purposes.  Any types that previously dervied
from `AndroidClientHandler` should instead inherit from
`AndroidMessageHandler`.

Administrivia:

  * `AndroidMessageHandler` had `internal virtual` methods.  These
    have to be changed to `protected virtual`.

  * There were many places in the codebase that refer to the
    `AndroidClientHandler`; these have been changed to refer to
    `AndroidMessageHandler` in .NET 6+ builds.

[0]: https://github.com/xamarin/xamarin-android/blob/e24ea21b77353f458633e781d2f02a97fa3c96ad/src/Mono.Android/Xamarin.Android.Net/AndroidClientHandler.cs#L317-L322

[jonpryor]

When this while loops is re-enabled, I think a more "conventional" approach would be to instead do:

FieldInfo? field = null;
for (var type = GetType (); type != null; type = type.BaseType) {
    field = type.GetField (fieldName, BindingFlags.Instance | BindingFlags.NonPublic);
    if (field != null)
        break;
}
if (field == null)
    throw new InvalidOperationException (…);
…

The benefit to this approach is that it's conceptually simpler (yay maintenance), and also means that if someone knows what they're doing (possible 🤣), they can "simply" add a new _nativeHandler instance field and things will (more or less) Just Work™.

[simonrozsival]

I like this approach better, thanks for the suggestion!

[simonrozsival]

This commit fixed the stuck Xamarin.Android-PR (Smoke Tests APKs .NET - macOS) (interpreter): 142a0d5

I'm not sure why but this code was causing it:

var obj = CreateHandler ();
if (obj is not HttpClientHandler h) {
	Assert.Ignore ($"{obj.GetType()} is not a HttpClientHandler.");
	return;
}

Now there's just Xamarin.Android-PR (Linux Build and Smoke Test) that's failing and it's because of Error MSB3030: Could not copy the file "/mnt/vss/_work/1/s/xamarin-android/bin/Release/lib/xamarin.android/xbuild/Xamarin/Android//de/Xamarin.Android.Build.Debugging.Tasks.resources.dll" because it was not found.. Does anyone know what could be the cause of that? Is that a known issue?

[simonrozsival]

I think there's not much I can do about the failing test (it seems completely unrelated to my PR) and so I think this PR is done.