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

[Foundation] NSUrlSessionHandler: Adds support for X509 client certificates #21284

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
104 changes: 95 additions & 9 deletions src/Foundation/NSUrlSessionHandler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -567,15 +567,20 @@ public DecompressionMethods AutomaticDecompression {
[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 (); } }

X509CertificateCollection? _clientCertificates;

/// <summary>Gets the collection of security certificates that are associated with requests to the server.</summary>
/// <remarks>Client certificates are only supported when ClientCertificateOptions is set to ClientCertificateOptions.Manual.</remarks>
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 @@ -1088,6 +1093,19 @@ void DidReceiveChallengeImpl (NSUrlSession session, NSUrlSessionTask task, NSUrl
}
return;
}
#if NET
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);
completionHandler (NSUrlSessionAuthChallengeDisposition.UseCredential, credential);
return;
}
}
#endif

// 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 @@ -1592,5 +1610,73 @@ protected override void Dispose (bool disposing)
stream?.Dispose ();
}
}

#if NET
static class CertificateHelper {
// 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.
const string ClientAuthenticationOID = "1.3.6.1.5.5.7.3.2";

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;
}

static bool IsValidClientCertificate (X509Certificate2 cert)
{
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;
}

static bool IsValidForClientAuthenticationEKU (X509EnhancedKeyUsageExtension eku)
{
foreach (System.Security.Cryptography.Oid oid in eku.EnhancedKeyUsages) {
if (oid.Value == ClientAuthenticationOID) {
return true;
}
}

return false;
}

static bool IsValidForDigitalSignatureUsage (X509KeyUsageExtension ku)
{
const X509KeyUsageFlags RequiredUsages = X509KeyUsageFlags.DigitalSignature;
return (ku.KeyUsages & RequiredUsages) == RequiredUsages;
}
}
#endif
}
}
83 changes: 82 additions & 1 deletion src/Security/Certificate.cs
Original file line number Diff line number Diff line change
Expand Up @@ -583,15 +583,96 @@ public SecCertificate Certificate {
}
}

/// <summary>Create a <see cref="SecIdentity" /> from PKCS #12 data.</summary>
/// <param name="data">The PKCS #12 blob data as a byte array.</param>
/// <param name="password">The password for the private key in the PKCS #12 data. An empty password is not supported.</param>
/// <remarks>
/// <para>On macOS 14 or earlier this method requires access to the default keychain,where the private key + certificate will be stored.</para>
/// <para>On macOS 15 or later, as well as all other platforms (iOS, tvOS, Mac Catalyst), calling this method will not affect any keychains (a temporary, in-memory keychain is created for the duration of the import).</para>
/// </remarks>
public static SecIdentity Import (byte [] data, string password)
{
if (data is null)
ObjCRuntime.ThrowHelper.ThrowArgumentNullException (nameof (data));
if (string.IsNullOrEmpty (password)) // SecPKCS12Import() doesn't allow empty passwords.
throw new ArgumentException (nameof (password));
using (var pwstring = new NSString (password))
using (var options = NSDictionary.FromObjectAndKey (pwstring, SecImportExport.Passphrase)) {
using (var options = NSMutableDictionary.FromObjectAndKey (pwstring, SecImportExport.Passphrase)) {
NSDictionary [] array;
#if __MACOS__
/* There are unfortunate platform differences for SecPKCS12Import:
*
* `SecPKCS12Import` will, _on macOS only, not other platforms_, add the imported certificate + private key into the default keychain.
*
* Apple's documentation for [`SecPKCS12Import`](https://developer.apple.com/documentation/security/1396915-secpkcs12import?language=objc)
* implies importing into the keychain is optional ("[...] You can then use the Keychain Services API
* (see [Keychain services](https://developer.apple.com/documentation/security/keychain_services?language=objc))
* to put the identities and associated certificates in the keychain."), but that doesn't match the behavior we're seeing,
* either on the bots nor locally (if I lock the keychain before running the unit test on macOS, I get a dialog asking for
* my password to unlock the keychain when this method is called). Other people on StackOverflow has run into the same issue
* (https://stackoverflow.com/q/33181127), where one of the answers points to the source code (https://stackoverflow.com/a/66846609),
* confirming this behavior.
*
* StackOverflow also suggests using [`SecItemImport`](https://developer.apple.com/documentation/security/1395728-secitemimport)
* instead, which works, with a few caveats:
*
* 1. Importing a PKCS#12 blob only returns the certificate, not the private key. This is a bug, as [confirmed](https://forums.developer.apple.com/forums/thread/31711) by Quinn "The Eskimo!":
*
* > `SecItemImport` really does support importing private keys without putting them in the keychain,
* > and that code all runs and works in the PKCS#12 case; internally I see both the certificate and
* > the private key extracted from the PKCS#12. The problem arises when the code tries to match up
* > the certificate and private key to form an identity. That code is failing in the no-keychain case,
* > so you end up getting back just the certificate. Notably, in the PEM case no matching occurs and
* > thus you get back both the certificate and the private key.
* >
* > This is clearly a bug and I’ve filed it as such (r. 25,140,029).
*
* That was 8 years ago, and 6 years later it still hasn't been fixed (as confirmed by Quinn in the same thread), so it's unlikely it'll ever be fixed.
*
* 2. So I tried exporting the X509Certificate into a PEM string instead, and that works, I successfully
* get back a `SecKey` instance and a `SecCertificate` instance! Success?
*
* 3. Nope, because there's no way to create a `SecIdentity` from `SecKey`+`SecCertificate`. You have to put
* the `SecKey` into a keychain, and then pass the `SecCertificate` to [`SecIdentityCreateWithCertificate`](https://developer.apple.com/documentation/security/1401160-secidentitycreatewithcertificate?language=objc),
* and we're back to where we started.
*
* 4. OK, what about creating a temporary `SecKeychain`, add the `SecKey` there, create the `SecIdentity`, then delete the `SecKeychain`?
*
* * [`SecKeyChain`](https://developer.apple.com/documentation/security/1401214-seckeychaincreate) was deprecated in macOS 10.10 :/
* * curl had the same problem:
* * https://github.com/curl/curl/issues/10038
* * https://github.com/curl/curl/issues/5403)
*
* There was PR to use a temporary keychain (https://github.com/curl/curl/pull/10059), with a number of good reasons why it was a bad idea, so it was eventually rejected:
*
* * The temporary keychain is stored on disk, which isn't particularly fast.
* * The ownership/rights of the file must be considered to ensure there are no security issues.
* * Concurrency would have to be considered with regards to cleanup - what if there are multiple threads trying to use the same location on disk.
*
*
* In [this Apple forum thread](https://forums.developer.apple.com/forums/thread/31711) a user gripes about this exact problem:
*
* > I've resorted to using a private API to created a SecIdentity from a SecCertificate and a SecKey that I already have in memory.
*
* Quinn answers:
*
* > On the macOS front, there’s nothing stopping a command-line tool running on a CI machine using the keychain.
* There are some pain points but no showstoppers.
*
* FWIW Quinn “The Eskimo!” wrote a document explaining how to find and fix problems with regards to the keychain on CI machines
* (https://developer.apple.com/forums/thread/712005). The last sentence is a gem: "Resetting trust settings is more of a challenge.
* It’s probably possible to do this with the security tool but, honestly, if you think that your CI system has messed up trust settings
* it’s easiest to throw it away and start again from scratch." - in other words if something goes wrong, the easiest is to wipe the
* machine and start over again.
*
* "some pain points" is somewhat of an understatement...
*
* The good news is that on macOS 15+, Apple added an option to use a temporary, in-memory only keychain, avoiding the whole problem,
* so let's use that!
*/
if (OperatingSystem.IsMacOSVersionAtLeast (15, 0))
options.Add (SecImportExport.ToMemoryOnly, NSNumber.FromBoolean (true));
#endif
SecStatusCode result = SecImportExport.ImportPkcs12 (data, options, out array);
if (result != SecStatusCode.Success)
throw new InvalidOperationException (result.ToString ());
Expand Down
1 change: 0 additions & 1 deletion tests/cecil-tests/Documentation.KnownFailures.txt
Original file line number Diff line number Diff line change
Expand Up @@ -47220,7 +47220,6 @@ M:Security.SecCertificate.ToX509Certificate
M:Security.SecCertificate.ToX509Certificate2
M:Security.SecCertificate2.#ctor(Security.SecCertificate)
M:Security.SecIdentity.GetTypeID
M:Security.SecIdentity.Import(System.Byte[],System.String)
M:Security.SecIdentity.Import(System.Security.Cryptography.X509Certificates.X509Certificate2)
M:Security.SecIdentity2.#ctor(Security.SecIdentity,Security.SecCertificate[])
M:Security.SecIdentity2.#ctor(Security.SecIdentity)
Expand Down
Loading