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.

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

NSUrlSessionHandler: Adds support for X509 client certificates #20434

Open
wants to merge 30 commits into
base: main
Choose a base branch
from
Open
Changes from 6 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
be5b13f
Adds support for X509 client certificates
Apr 11, 2024
3d580dc
Remove non-browsable attribute
dotMorten Apr 11, 2024
4be0f2e
Add certicate helper from .NET Runtime repro
dotMorten Apr 12, 2024
757e266
Auto-format source code
Apr 15, 2024
7671348
Move certificate helper in as a private local class
dotMorten Apr 15, 2024
87f8250
Auto-format source code
Apr 15, 2024
1904d64
Fully qualify type
dotMorten Apr 15, 2024
423a995
Only include new functionality for NET builds
dotMorten Apr 15, 2024
7d70e95
Apply suggestions from code review
dotMorten Apr 16, 2024
759703a
Merge remote-tracking branch 'origin/main' into dotMorten/x509
rolfbjarne Apr 18, 2024
6d2d25b
Format code.
rolfbjarne Apr 18, 2024
7a92d92
Add unit test
dotMorten Apr 19, 2024
05a5488
Auto-format source code
Apr 22, 2024
71c0a31
Merge branch 'main' into dotMorten/x509
mandel-macaque May 6, 2024
d67f4e1
Fix tests on macOS by not using the default keyset of the system by a…
mandel-macaque May 6, 2024
d77ddc6
Move to sportable to fix the tests.
mandel-macaque May 6, 2024
6b9b9ce
Fix dum typo.
mandel-macaque May 7, 2024
48f1e50
Merge branch 'main' into dotMorten/x509
mandel-macaque May 7, 2024
674a8e1
Add the persistent storage.
mandel-macaque May 7, 2024
3245b94
Merge branch 'dotMorten/x509' of github.com:dotMorten/xamarin-macios …
mandel-macaque May 7, 2024
93d0e47
Merge branch 'main' into dotMorten/x509
mandel-macaque May 7, 2024
8a88172
Add documentation.
rolfbjarne May 8, 2024
8d3017b
Merge branch 'main' into dotMorten/x509
mandel-macaque May 8, 2024
8285149
Seems to be that just adding exportable is enough.
mandel-macaque May 10, 2024
04d41a6
Merge branch 'main' into dotMorten/x509
rolfbjarne May 20, 2024
1afd980
Use platform-specific x509 flags, macOS isn't like other platforms.
rolfbjarne May 21, 2024
21a0c69
Merge remote-tracking branch 'origin/main' into dotMorten/x509
rolfbjarne May 21, 2024
59ad500
[devops] Unlock the default keychain when removing UI prompts.
rolfbjarne May 23, 2024
fadf9b9
Merge remote-tracking branch 'origin/main' into dotMorten/x509
rolfbjarne May 23, 2024
58a6a26
[devops] Increase debug spew.
rolfbjarne May 24, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
100 changes: 91 additions & 9 deletions src/Foundation/NSUrlSessionHandler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -567,15 +567,21 @@ protected override async Task<HttpResponseMessage> SendAsync (HttpRequestMessage
[EditorBrowsable (EditorBrowsableState.Never)]
public bool CheckCertificateRevocationList { get; set; } = false;

// We're ignoring this property, just like Xamarin.Android does:
// https://github.com/xamarin/xamarin-android/blob/09e8cb5c07ea6c39383185a3f90e53186749b802/src/Mono.Android/Xamarin.Android.Net/AndroidMessageHandler.cs#L150
// Note: we can't return null (like Xamarin.Android does), because the return type isn't nullable.
[UnsupportedOSPlatform ("ios")]
[UnsupportedOSPlatform ("maccatalyst")]
[UnsupportedOSPlatform ("tvos")]
[UnsupportedOSPlatform ("macos")]
[EditorBrowsable (EditorBrowsableState.Never)]
public X509CertificateCollection ClientCertificates { get { return new X509CertificateCollection (); } }

private X509CertificateCollection? _clientCertificates;
dotMorten marked this conversation as resolved.
Show resolved Hide resolved

public X509CertificateCollection ClientCertificates
{
get
{
if (ClientCertificateOptions != ClientCertificateOption.Manual)
{
throw new InvalidOperationException($"Enable manual options first on {nameof(ClientCertificateOptions)}");
}

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

// We're ignoring this property, just like Xamarin.Android does:
// https://github.com/xamarin/xamarin-android/blob/09e8cb5c07ea6c39383185a3f90e53186749b802/src/Mono.Android/Xamarin.Android.Net/AndroidMessageHandler.cs#L148
Expand Down Expand Up @@ -1083,6 +1089,16 @@ void DidReceiveChallengeImpl (NSUrlSession session, NSUrlSessionTask task, NSUrl
}
return;
}
if (sessionHandler.ClientCertificateOptions == ClientCertificateOption.Manual && challenge.ProtectionSpace.AuthenticationMethod == NSUrlProtectionSpace.AuthenticationMethodClientCertificate) {
var certificate = CertificateHelper.GetEligibleClientCertificate (sessionHandler.ClientCertificates);
if (certificate is not null) {
var cert = new SecCertificate (certificate);
var identity = SecIdentity.Import (certificate);
var credential = new NSUrlCredential (identity, new SecCertificate [] { cert }, NSUrlCredentialPersistence.ForSession);
rolfbjarne marked this conversation as resolved.
Show resolved Hide resolved
completionHandler (NSUrlSessionAuthChallengeDisposition.UseCredential, credential);
return;
}
}
dotMorten marked this conversation as resolved.
Show resolved Hide resolved
// case for the basic auth failing up front. As per apple documentation:
// The URL Loading System is designed to handle various aspects of the HTTP protocol for you. As a result, you should not modify the following headers using
// the addValue(_:forHTTPHeaderField:) or setValue(_:forHTTPHeaderField:) methods:
Expand Down Expand Up @@ -1587,5 +1603,71 @@ protected override void Dispose (bool disposing)
stream?.Dispose ();
}
}

private static class CertificateHelper {
dotMorten marked this conversation as resolved.
Show resolved Hide resolved
// Based on https://github.com/dotnet/runtime/blob/c2848c582f5d6ae42c89f5bfe0818687ab3345f0/src/libraries/Common/src/System/Net/Security/CertificateHelper.cs
// with the NetEventSource code removed and namespace changed.
private const string ClientAuthenticationOID = "1.3.6.1.5.5.7.3.2";
dotMorten marked this conversation as resolved.
Show resolved Hide resolved

internal static X509Certificate2? GetEligibleClientCertificate (X509CertificateCollection? candidateCerts)
{
if (candidateCerts is null || candidateCerts.Count == 0) {
return null;
}

var certs = new X509Certificate2Collection ();
certs.AddRange (candidateCerts);

return GetEligibleClientCertificate (certs);
}

internal static X509Certificate2? GetEligibleClientCertificate (X509Certificate2Collection? candidateCerts)
{
if (candidateCerts is null || candidateCerts.Count == 0) {
return null;
}

foreach (X509Certificate2 cert in candidateCerts) {
if (!cert.HasPrivateKey) {
continue;
}

if (IsValidClientCertificate (cert)) {
return cert;
}
}
return null;
}

private static bool IsValidClientCertificate (X509Certificate2 cert)
dotMorten marked this conversation as resolved.
Show resolved Hide resolved
{
foreach (X509Extension extension in cert.Extensions) {
if ((extension is X509EnhancedKeyUsageExtension eku) && !IsValidForClientAuthenticationEKU (eku)) {
return false;
} else if ((extension is X509KeyUsageExtension ku) && !IsValidForDigitalSignatureUsage (ku)) {
return false;
}
}

return true;
}

private static bool IsValidForClientAuthenticationEKU (X509EnhancedKeyUsageExtension eku)
dotMorten marked this conversation as resolved.
Show resolved Hide resolved
{
foreach (Oid oid in eku.EnhancedKeyUsages) {
if (oid.Value == ClientAuthenticationOID) {
return true;
}
}

return false;
}

private static bool IsValidForDigitalSignatureUsage (X509KeyUsageExtension ku)
dotMorten marked this conversation as resolved.
Show resolved Hide resolved
{
const X509KeyUsageFlags RequiredUsages = X509KeyUsageFlags.DigitalSignature;
return (ku.KeyUsages & RequiredUsages) == RequiredUsages;
Copy link
Member

Choose a reason for hiding this comment

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

I have always preferred the HasFlag method over the &, but is a matter of taste. I just think it is easier to read.

Suggested change
return (ku.KeyUsages & RequiredUsages) == RequiredUsages;
return (ku.KeyUsages.HasFlag(X509KeyUsageFlags.DigitalSignature);

Copy link
Author

Choose a reason for hiding this comment

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

Since this is code taken from the .NET Runtime I tried not changing the logic from its origin.

}
}
}
}