Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Mono.Android] Add support for AndroidMessageHandler ClientCertificates #8961

Merged
merged 15 commits into from
Jul 10, 2024
Merged

[Mono.Android] Add support for AndroidMessageHandler ClientCertificates #8961

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
945991f
Implement support for client certificates
simonrozsival May 17, 2024
e486d33
Add test
simonrozsival May 17, 2024
a5f6f12
Suppress credscan error caused by a self-signed test certificate
simonrozsival May 20, 2024
15d77a4
Update apkdesc file
simonrozsival May 20, 2024
d183594
Addressing review comments
simonrozsival May 23, 2024
7dabc57
Revert unnecessary change
simonrozsival May 24, 2024
fc41236
Fix conversion of X509Certificate2 to Java Certificate
simonrozsival Jun 14, 2024
50d06db
Update apk size
simonrozsival Jun 14, 2024
7ae6527
Update gdn suppressions file
simonrozsival Jun 14, 2024
3b8f788
Merge branch 'main' of https://github.com/dotnet/android into android…
simonrozsival Jun 14, 2024
78afa3c
Merge branch 'main' into android-message-handler-support-client-certi…
jonathanpeppers Jun 26, 2024
e6aa1ef
Update apk sizes
simonrozsival Jun 27, 2024
9233d67
Merge branch 'main' of https://github.com/dotnet/android into android…
simonrozsival Jul 1, 2024
25e3eeb
Merge remote-tracking branch 'origin/main' into android-message-handl…
jonpryor Jul 8, 2024
108b196
Flush .apkdesc files
jonpryor Jul 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 63 additions & 0 deletions .gdn/.gdnsuppress
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,69 @@
],
"justification": "Dummy test.keystore file used for testing.",
"createdDate": "2024-02-21 20:58:02Z"
},
"ad733d624486984da63461d2a23f266714f76e1788c271d90d45687579f51099": {
"signature": "ad733d624486984da63461d2a23f266714f76e1788c271d90d45687579f51099",
"alternativeSignatures": [],
"memberOf": [
"default"
],
"justification": "release.keystore file created during test run.",
"createdDate": "2024-06-14 18:52:00Z"
},
"e10f89d02383ffef3bdbf9c048a9e0f3bdab956a8e6e49817780b0c837a5bd6d": {
"signature": "e10f89d02383ffef3bdbf9c048a9e0f3bdab956a8e6e49817780b0c837a5bd6d",
"alternativeSignatures": [],
"memberOf": [
"default"
],
"justification": "False positive in linker-dependencies.xml file.",
"createdDate": "2024-06-14 18:52:00Z"
},
"e73b15633b7cb1e9e735ce0fe78a6ce3c95c11a8888181eb3b0cb50c191da19e": {
"signature": "e73b15633b7cb1e9e735ce0fe78a6ce3c95c11a8888181eb3b0cb50c191da19e",
"alternativeSignatures": [],
"memberOf": [
"default"
],
"justification": "False positive in linker-dependencies.xml file.",
"createdDate": "2024-06-14 18:52:00Z"
},
"e622e6a9a73c1856d399e753105be517d62ec1e62d13a15ab9ecef43e15590a9": {
"signature": "e622e6a9a73c1856d399e753105be517d62ec1e62d13a15ab9ecef43e15590a9",
"alternativeSignatures": [],
"memberOf": [
"default"
],
"justification": "False positive in linker-dependencies.xml file.",
"createdDate": "2024-06-14 18:52:00Z"
},
"df428be5ce5ef90685e15981cf49e2af10de6d87544f437aa1722f84516d6fef": {
"signature": "df428be5ce5ef90685e15981cf49e2af10de6d87544f437aa1722f84516d6fef",
"alternativeSignatures": [],
"memberOf": [
"default"
],
"justification": "False positive in linker-dependencies.xml file.",
"createdDate": "2024-06-14 18:52:00Z"
},
"247325bc1f0ff6899ae09b13e006ac35c7cae4ffee0749f139fd5100f85a162f": {
"signature": "247325bc1f0ff6899ae09b13e006ac35c7cae4ffee0749f139fd5100f85a162f",
"alternativeSignatures": [],
"memberOf": [
"default"
],
"justification": "False positive in linker-dependencies.xml file.",
"createdDate": "2024-06-14 18:52:00Z"
},
"6d53f09942503c3f7eeccf23af43ae976431e8dbf2ad3d32be8af5bd37068d4d": {
"signature": "6d53f09942503c3f7eeccf23af43ae976431e8dbf2ad3d32be8af5bd37068d4d",
"alternativeSignatures": [],
"memberOf": [
"default"
],
"justification": "False positive in linker-dependencies.xml file.",
"createdDate": "2024-06-14 18:52:00Z"
}
}
}
147 changes: 114 additions & 33 deletions src/Mono.Android/Xamarin.Android.Net/AndroidMessageHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using System.Net.Http.Headers;
using System.Net.Security;
using System.Security.Authentication;
using System.Security.Cryptography;
using System.Security.Cryptography.X509Certificates;
using System.Text;
using System.Threading;
Expand All @@ -21,6 +22,7 @@
using Java.Security;
using Java.Security.Cert;
using Javax.Net.Ssl;
using JavaX509Certificate = Java.Security.Cert.X509Certificate;

namespace Xamarin.Android.Net
{
Expand Down Expand Up @@ -206,9 +208,22 @@ public CookieContainer CookieContainer

public bool AllowAutoRedirect { get; set; } = true;

public ClientCertificateOption ClientCertificateOptions { get; set; }
public ClientCertificateOption ClientCertificateOptions { get; set; } = ClientCertificateOption.Manual;

public X509CertificateCollection? ClientCertificates { get; set; }
private X509CertificateCollection? _clientCertificates;
public X509CertificateCollection? ClientCertificates
{
get
{
if (ClientCertificateOptions != ClientCertificateOption.Manual) {
throw new InvalidOperationException ($"Enable manual options first on {nameof (ClientCertificateOptions)}");
Copy link
Member

@jonpryor jonpryor May 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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…

Copy link
Member Author

@simonrozsival simonrozsival May 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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…

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

return _clientCertificates ?? (_clientCertificates = new X509CertificateCollection ());
}

set => _clientCertificates = value;
}

public ICredentials? DefaultProxyCredentials { get; set; }

Expand Down Expand Up @@ -1151,49 +1166,115 @@ void SetupSSL (HttpsURLConnection? httpsConnection, HttpRequestMessage requestMe
return;
}

var keyStore = InitializeKeyStore (out bool gotCerts);
keyStore = ConfigureKeyStore (keyStore);
var kmf = ConfigureKeyManagerFactory (keyStore);
var tmf = ConfigureTrustManagerFactory (keyStore);
KeyStore keyStore = GetConfiguredKeyStoreInstance ();
KeyManagerFactory? kmf = GetConfiguredKeyManagerFactory (keyStore);
TrustManagerFactory? tmf = ConfigureTrustManagerFactory (keyStore);

// If there is no customization there is no point in changing the behavior of the default SSL socket factory.
if (tmf is null && kmf is null && !HasTrustedCerts && !HasServerCertificateCustomValidationCallback && !HasClientCertificates) {
return;
}

if (tmf == null) {
// If there are no trusted certs, no custom trust manager factory or custom certificate validation callback
// there is no point in changing the behavior of the default SSL socket factory
if (!gotCerts && _serverCertificateCustomValidator is null)
return;
var context = SSLContext.GetInstance ("TLS") ?? throw new InvalidOperationException ("Failed to get the SSLContext instance for TLS");
var trustManagers = GetTrustManagers (tmf, keyStore, requestMessage);
context.Init (kmf?.GetKeyManagers (), trustManagers, null);
httpsConnection.SSLSocketFactory = context.SocketFactory;
}

tmf = TrustManagerFactory.GetInstance (TrustManagerFactory.DefaultAlgorithm);
tmf?.Init (gotCerts ? keyStore : null); // only use the custom key store if the user defined any trusted certs
[MemberNotNullWhen (true, nameof(TrustedCerts))]
bool HasTrustedCerts => TrustedCerts?.Count > 0;

[MemberNotNullWhen (true, nameof(_serverCertificateCustomValidator))]
bool HasServerCertificateCustomValidationCallback => _serverCertificateCustomValidator is not null;

[MemberNotNullWhen (true, nameof(_clientCertificates))]
bool HasClientCertificates => _clientCertificates?.Count > 0;

KeyManagerFactory? GetConfiguredKeyManagerFactory (KeyStore keyStore)
{
var kmf = ConfigureKeyManagerFactory (keyStore);

if (kmf is null && HasClientCertificates) {
kmf = KeyManagerFactory.GetInstance ("PKIX") ?? throw new InvalidOperationException ("Failed to get the KeyManagerFactory instance for PKIX");
kmf.Init (keyStore, null);
}

ITrustManager[]? trustManagers = tmf?.GetTrustManagers ();
return kmf;
}

KeyStore GetConfiguredKeyStoreInstance ()
{
var keyStore = KeyStore.GetInstance (KeyStore.DefaultType) ?? throw new InvalidOperationException ("Failed to get the default KeyStore instance");
keyStore.Load (null, null);

var customValidator = _serverCertificateCustomValidator;
if (customValidator is not null) {
trustManagers = customValidator.ReplaceX509TrustManager (trustManagers, requestMessage);
if (HasTrustedCerts) {
for (int i = 0; i < TrustedCerts!.Count; i++) {
if (TrustedCerts [i] is Certificate cert) {
keyStore.SetCertificateEntry ($"ca{i}", cert);
}
}
}

var context = SSLContext.GetInstance ("TLS");
context?.Init (kmf?.GetKeyManagers (), trustManagers, null);
httpsConnection.SSLSocketFactory = context?.SocketFactory;
if (HasClientCertificates) {
if (ClientCertificateOptions != ClientCertificateOption.Manual) {
throw new InvalidOperationException ($"Use of {nameof(ClientCertificates)} requires that {nameof(ClientCertificateOptions)} be set to ClientCertificateOption.Manual");
}

KeyStore? InitializeKeyStore (out bool gotCerts)
{
var keyStore = KeyStore.GetInstance (KeyStore.DefaultType);
keyStore?.Load (null, null);
gotCerts = TrustedCerts?.Count > 0;

if (gotCerts) {
for (int i = 0; i < TrustedCerts!.Count; i++) {
Certificate cert = TrustedCerts [i];
if (cert == null)
continue;
keyStore?.SetCertificateEntry ($"ca{i}", cert);
for (int i = 0; i < _clientCertificates.Count; i++) {
var keyEntry = GetKeyEntry (new X509Certificate2 (_clientCertificates [i]));
if (keyEntry is var (key, chain)) {
keyStore.SetKeyEntry ($"key{i}", key, null, chain);
}
}
}

return ConfigureKeyStore (keyStore) ?? throw new InvalidOperationException ($"{nameof(ConfigureKeyStore)} unexpectedly returned null");
}

ITrustManager[]? GetTrustManagers (TrustManagerFactory? tmf, KeyStore keyStore, HttpRequestMessage requestMessage)
{
if (tmf is null) {
tmf = TrustManagerFactory.GetInstance (TrustManagerFactory.DefaultAlgorithm) ?? throw new InvalidOperationException ("Failed to get the default TrustManagerFactory instance");
tmf.Init (HasTrustedCerts ? keyStore : null); // only use the custom key store if the user defined any trusted certs
}

ITrustManager[]? trustManagers = tmf.GetTrustManagers ();

if (HasServerCertificateCustomValidationCallback) {
trustManagers = _serverCertificateCustomValidator.ReplaceX509TrustManager (trustManagers, requestMessage);
}

return keyStore;
return trustManagers;
}

static (IPrivateKey, Certificate[])? GetKeyEntry (X509Certificate2 clientCertificate)
{
if (!clientCertificate.HasPrivateKey) {
return null;
}

AsymmetricAlgorithm? key = null;
string? algorithmName = null;

if (clientCertificate.GetRSAPrivateKey () is {} rsa) {
(key, algorithmName) = (rsa, "RSA");
} else if (clientCertificate.GetECDsaPrivateKey () is {} ec) {
(key, algorithmName) = (ec, "EC");
} else if (clientCertificate.GetDSAPrivateKey () is {} dsa) {
(key, algorithmName) = (dsa, "DSA");
} else {
return null;
}

var keyFactory = KeyFactory.GetInstance (algorithmName) ?? throw new InvalidOperationException ($"Failed to get the KeyFactory instance for algorithm {algorithmName}");
var privateKey = keyFactory.GeneratePrivate (new Java.Security.Spec.PKCS8EncodedKeySpec (key.ExportPkcs8PrivateKey ()));
var certificate = Java.Lang.Object.GetObject<Certificate> (clientCertificate.Handle, JniHandleOwnership.DoNotTransfer);

if (privateKey is null || certificate is null) {
return null;
}

return (privateKey, new Certificate [] { certificate });
}

void HandlePreAuthentication (HttpURLConnection httpConnection)
Expand Down
14 changes: 7 additions & 7 deletions ...ild.Tasks/Tests/Xamarin.ProjectTools/Resources/Base/BuildReleaseArm64SimpleDotNet.apkdesc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,13 @@
"Size": 1114
},
"lib/arm64-v8a/lib_Java.Interop.dll.so": {
"Size": 66243
"Size": 66250
},
"lib/arm64-v8a/lib_Mono.Android.dll.so": {
"Size": 94712
"Size": 94741
},
"lib/arm64-v8a/lib_Mono.Android.Runtime.dll.so": {
"Size": 5320
"Size": 5367
},
"lib/arm64-v8a/lib_System.Console.dll.so": {
"Size": 7226
Expand All @@ -35,7 +35,7 @@
"Size": 4475
},
"lib/arm64-v8a/lib_UnnamedProject.dll.so": {
"Size": 2932
"Size": 3059
},
"lib/arm64-v8a/libarc.bin.so": {
"Size": 1546
Expand All @@ -44,7 +44,7 @@
"Size": 87432
},
"lib/arm64-v8a/libmonodroid.so": {
"Size": 492344
"Size": 492280
},
"lib/arm64-v8a/libmonosgen-2.0.so": {
"Size": 3163208
Expand All @@ -62,10 +62,10 @@
"Size": 159544
},
"lib/arm64-v8a/libxamarin-app.so": {
"Size": 17960
"Size": 18008
},
"META-INF/BNDLTOOL.RSA": {
"Size": 1221
"Size": 1213
},
"META-INF/BNDLTOOL.SF": {
"Size": 3266
Expand Down
Loading