NSUrlSessionHandler: Adds support for X509 client certificates #20434

dotMorten · 2024-04-11T20:41:37Z

Addresses #13856

There's a couple of outstanding questions for this PR, so keeping it in draft for now:

This relies on System.Net.Security.CertificateHelper which is internal. I'm not sure if xamarinios has access to this. If not, should I just copy the file over? It is for instance used here to pick the correct certificate in SocketsHttpHandler and HttpClientHandler.. Update: Imported a copy.
I didn't mark ClientCertificateOptions supported. However, I rely on it being set to Manual (which is the default). I also duplicated the HttpClientHandler behavior for requiring Manual to be set, so I'm sort of using a property we say isn't supported, but Automatic would never be supported.

I also toyed with having a public callback for users to do more native certificate handling, and modeled around the ServerCertificateCustomValidationCallback callback:

	public Func<HttpRequestMessage, NSUrlAuthenticationChallenge, NSUrlCredential?>? ClientCertificateChallengeCallback
	{
		get; set;
	}

	private bool TryInvokeClientCertificateChallengeCallback(HttpRequestMessage request, NSUrlAuthenticationChallenge challenge, [NotNullWhen(true)] out NSUrlCredential? credential)
	{            
		var callback = ClientCertificateChallengeCallback;
		credential = null;
		if (callback is null)
			return false;
		credential = callback(request, challenge);
		return credential != null;
	}

This isn't in the PR, but more than happy to also add that. My thinking was that might enable some scenarios where you can't use the .NET X509 certificates but need to pull native certificate types in

src/Foundation/NSUrlSessionHandler.cs

rolfbjarne · 2024-04-12T08:33:23Z

This relies on System.Net.Security.CertificateHelper which is internal. I'm not sure if xamarinios has access to this. If not, should I just copy the file over?

We don't have access to that class, so please just copy it over (and add a comment explaining where it came from).

2. I didn't mark ClientCertificateOptions supported. However, I rely on it being set to Manual (which is the default). I also duplicated the HttpClientHandler behavior for requiring Manual to be set, so I'm sort of using a property we say isn't supported, but Automatic would never be supported.

I think it's fine to not mark it as supported (an alternative approach would be to mark it as supported, but throw some sort of "not supported exception" if someone tries to set it to Automatic).

rolfbjarne · 2024-04-15T06:57:31Z

/azp run

azure-pipelines · 2024-04-15T06:57:49Z

Azure Pipelines successfully started running 1 pipeline(s).

github-actions · 2024-04-15T07:01:01Z

⚠️ Your code has been reformatted. ⚠️

If this is not desired, add the actions-disable-autoformat label, and revert the reformatting commit.

If files unrelated to your change were modified, try reverting the reformatting commit + merging with the target branch (and push those changes).

rolfbjarne · 2024-04-15T07:13:01Z

/azp run

azure-pipelines · 2024-04-15T07:13:21Z

Azure Pipelines successfully started running 1 pipeline(s).

rolfbjarne · 2024-04-15T08:24:59Z

@dotMorten it fails to build:

Foundation/NSUrlSessionHandler.cs(1092,24): error CS1061: 'NSUrlSessionHandler' does not contain a definition for 'ClientCertificateOptions' and no accessible extension method 'ClientCertificateOptions' accepting a first argument of type 'NSUrlSessionHandler' could be found (are you missing a using directive or an assembly reference?)
Foundation/NSUrlSessionHandler.cs(1093,87): error CS1061: 'NSUrlSessionHandler' does not contain a definition for 'ClientCertificates' and no accessible extension method 'ClientCertificates' accepting a first argument of type 'NSUrlSessionHandler' could be found (are you missing a using directive or an assembly reference?)
make[1]: *** [build/watch/watch-32/Xamarin.WatchOS.dll] Error 1
Foundation/NSUrlSessionHandler.cs(1093,24): error CS0103: The name 'CertificateHelper' does not exist in the current context
make[1]: *** [build/dotnet/tvos/ref/Microsoft.tvOS.dll] Error 1

dotMorten · 2024-04-15T15:58:28Z

@rolfbjarne Fixed. Could you retry?

github-actions · 2024-04-15T16:55:04Z

⚠️ Your code has been reformatted. ⚠️

If this is not desired, add the actions-disable-autoformat label, and revert the reformatting commit.

If files unrelated to your change were modified, try reverting the reformatting commit + merging with the target branch (and push those changes).

rolfbjarne · 2024-04-15T16:56:21Z

/azp run

azure-pipelines · 2024-04-15T16:56:42Z

Azure Pipelines successfully started running 1 pipeline(s).

src/Foundation/NSUrlSessionHandler.cs

rolfbjarne · 2024-05-21T07:51:14Z

/azp run

azure-pipelines · 2024-05-21T07:51:36Z

Azure Pipelines successfully started running 1 pipeline(s).

rolfbjarne · 2024-05-23T09:42:11Z

This is turning rather obnoxious to figure out, there seems to be platform differences between macOS and the other platforms that are unnecessary, so I've filed to issues:

Can't export X509Certificate on macOS (CoreCLR) dotnet/runtime#102599
Can't create X509Certificate using X509KeyStorageFlags.Exportable on Mac Catalyst or iOS dotnet/runtime#102600

I've also found a problem with SecIdentity.Import, where the Apple API we use will put the imported identity into the default keychain (but only on macOS, not the other platforms). This us troublesome on bots, because the default keychain might not be unlocked, and a dialog pops up, and the tests hang.

Fortunately there seems to be a different Apple API we can use to import an identity without needing a keychain, so I'm looking into this at the moment.

rolfbjarne · 2024-05-23T13:58:34Z

I've also found a problem with SecIdentity.Import, where the Apple API we use will put the imported identity into the default keychain (but only on macOS, not the other platforms). This us troublesome on bots, because the default keychain might not be unlocked, and a dialog pops up, and the tests hang.

Fortunately there seems to be a different Apple API we can use to import an identity without needing a keychain, so I'm looking into this at the moment.

OK, this turned into a rabbit hole...

The new code in this PR calls SecIdentity.Import to convert the X509Certificate provided in ClientCertificates collection into a SecIdentity we can pass on to NSUrlSession:

var identity = SecIdentity.Import (certificate);

SecIdentity.Import calls the native SecPKCS12Import function to convert the certificate into a SecIdentity:

xamarin-macios/src/Security/ImportExport.cs

Line 59 in 76cef6c

code = SecPKCS12Import (data.Handle, options.Handle, &handle);

SecPKCS12Import will, on macOS only, not other platforms, add the imported certificate + private key into the default keychain.

Apple's documentation for SecPKCS12Import implies importing into the keychain is optional ("[...] You can then use the Keychain Services API (see Keychain services) to put the identities and associated certificates in the keychain."), but that doesn't match the behavior we're seeing, neither 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 instead, which works, with a few caveats:

Importing a PKCS#12 blob only returns the certificate, not the private key. This is a bug, as confirmed 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.
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?
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, and we're back to where we started.
OK, what about creating a temporary SecKeychain, add the SecKey there, create the SecIdentity, then delete the SecKeychain?
- SecKeyChain was deprecated in macOS 10.10 :/
- curl had the same problem:
  - PKCS#12 certificate does not work with Secure Transport unless itself or private key is in keychain curl/curl#10038
  - Unable to use PKCS12 certificate for TLS under macOSX curl/curl#5403)
  There was PR to use a temporary keychain (Use SecPKCS12Import on temporary keychain if p12 certificate not found curl/curl#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 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.

So I've given up about the idea of not using the keychain in tests, and instead trying to make the keychain work somehow.

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...

rolfbjarne · 2024-05-23T13:59:43Z

/azp run

azure-pipelines · 2024-05-23T14:00:05Z

Azure Pipelines successfully started running 1 pipeline(s).

vs-mobiletools-engineering-service2 · 2024-05-23T15:38:19Z

❌ [PR Build] Tests on macOS X64 - Mac Sonoma (14) failed ❌

Failed tests are:

introspection
xammac_tests
monotouch-test

Pipeline on Agent
Hash: [PR build]

vs-mobiletools-engineering-service2 · 2024-05-23T15:51:06Z

❌ [PR Build] Tests on macOS M1 - Mac Big Sur (11) failed ❌

Failed tests are:

monotouch-test

Pipeline on Agent
Hash: [PR build]

vs-mobiletools-engineering-service2 · 2024-05-23T15:59:13Z

❌ [PR Build] Tests on macOS M1 - Mac Ventura (13) failed ❌

Failed tests are:

monotouch-test

Pipeline on Agent
Hash: [PR build]

vs-mobiletools-engineering-service2 · 2024-05-23T15:59:47Z

❌ [PR Build] Tests on macOS M1 - Mac Monterey (12) failed ❌

Failed tests are:

monotouch-test

Pipeline on Agent
Hash: [PR build]

vs-mobiletools-engineering-service2 · 2024-05-23T16:32:35Z

📚 [PR Build] Artifacts 📚

Packages generated

View packages

Pipeline on Agent
Hash: [PR build]

vs-mobiletools-engineering-service2 · 2024-05-23T16:54:26Z

✅ API diff for current PR / commit

Legacy Xamarin (No breaking changes)

~~iOS~~ (no change detected)
~~tvOS~~ (no change detected)
~~watchOS~~ (no change detected)
~~macOS~~ (no change detected)

NET (empty diffs)

~~iOS~~: (empty diff detected)
~~tvOS~~: (empty diff detected)
~~MacCatalyst~~: (empty diff detected)
~~macOS~~: (empty diff detected)

✅ API diff vs stable

Legacy Xamarin (No breaking changes)

iOS: vsdrops gist (No breaking changes)
tvOS: vsdrops gist (No breaking changes)
watchOS: vsdrops gist (No breaking changes)
macOS: vsdrops gist (No breaking changes)

.NET (No breaking changes)

iOS: vsdrops gist (No breaking changes)
tvOS: vsdrops gist (No breaking changes)
MacCatalyst: vsdrops gist (No breaking changes)
macOS: vsdrops gist (No breaking changes)
Microsoft.iOS vs Microsoft.MacCatalyst: vsdrops gist

Legacy Xamarin (stable) vs .NET

iOS: vsdrops gist
tvOS: vsdrops gist
macOS: vsdrops gist

ℹ️ Generator diff

Generator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes)

Pipeline on Agent
Hash: fadf9b931fd66fceaf70e670e3d5532dc4b895d6 [PR build]

vs-mobiletools-engineering-service2 · 2024-05-23T19:44:15Z

🔥 [CI Build] Test results 🔥

Test results

❌ Tests failed on VSTS: test results

1 tests crashed, 1 tests failed, 161 tests passed.

Failures

❌ cecil tests

1 tests failed, 0 tests passed.

Cecil-based tests: TimedOut (Execution timed out after 10 minutes.)

Html Report (VSDrops) Download

❌ monotouch tests (macOS)

🔥 Failed catastrophically on VSTS: test results - monotouch_macos (no summary found).

Html Report (VSDrops) Download

Successes

✅ dotnettests (iOS): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (MacCatalyst): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (macOS): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (Multiple platforms): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (tvOS): All 1 tests passed. Html Report (VSDrops) Download
✅ framework: All 8 tests passed. Html Report (VSDrops) Download
✅ fsharp: All 7 tests passed. Html Report (VSDrops) Download
✅ generator: All 2 tests passed. Html Report (VSDrops) Download
✅ install-source: All 1 tests passed. Html Report (VSDrops) Download
✅ interdependent-binding-projects: All 7 tests passed. Html Report (VSDrops) Download
✅ introspection: All 8 tests passed. Html Report (VSDrops) Download
✅ linker: All 65 tests passed. Html Report (VSDrops) Download
✅ mac-binding-project: All 1 tests passed. Html Report (VSDrops) Download
✅ mmp: All 2 tests passed. Html Report (VSDrops) Download
✅ mononative: All 6 tests passed. Html Report (VSDrops) Download
✅ monotouch (iOS): All 11 tests passed. Html Report (VSDrops) Download
✅ monotouch (MacCatalyst): All 7 tests passed. Html Report (VSDrops) Download
✅ monotouch (tvOS): All 11 tests passed. Html Report (VSDrops) Download
✅ monotouch (watchOS): All 4 tests passed. Html Report (VSDrops) Download
✅ msbuild: All 2 tests passed. Html Report (VSDrops) Download
✅ mtouch: All 1 tests passed. Html Report (VSDrops) Download
✅ xammac: All 3 tests passed. Html Report (VSDrops) Download
✅ xcframework: All 8 tests passed. Html Report (VSDrops) Download
✅ xtro: All 2 tests passed. Html Report (VSDrops) Download

Pipeline on Agent
Hash: [PR build]

Morten Nielsen and others added 2 commits April 11, 2024 13:34

Adds support for X509 client certificates

be5b13f

Remove non-browsable attribute

3d580dc

riverar reviewed Apr 11, 2024

View reviewed changes

src/Foundation/NSUrlSessionHandler.cs Outdated Show resolved Hide resolved

riverar reviewed Apr 11, 2024

View reviewed changes

src/Foundation/NSUrlSessionHandler.cs Outdated Show resolved Hide resolved

Add certicate helper from .NET Runtime repro

4be0f2e

dotMorten marked this pull request as ready for review April 12, 2024 19:01

dotMorten requested review from mandel-macaque and rolfbjarne as code owners April 12, 2024 19:01

dotMorten changed the title ~~Adds support for X509 client certificates~~ NSUrlSessionHandler: Adds support for X509 client certificates Apr 12, 2024

Auto-format source code

757e266

This comment has been minimized.

Sign in to view

Move certificate helper in as a private local class

7671348

Auto-format source code

87f8250

riverar reviewed Apr 15, 2024

View reviewed changes

src/Foundation/NSUrlSessionHandler.cs Show resolved Hide resolved

This comment has been minimized.

Sign in to view

rolfbjarne added 2 commits May 21, 2024 09:50

Use platform-specific x509 flags, macOS isn't like other platforms.

1afd980

Merge remote-tracking branch 'origin/main' into dotMorten/x509

21a0c69

This comment has been minimized.

Sign in to view

rolfbjarne added 2 commits May 23, 2024 13:24

[devops] Unlock the default keychain when removing UI prompts.

59ad500

Merge remote-tracking branch 'origin/main' into dotMorten/x509

fadf9b9

NSUrlSessionHandler: Adds support for X509 client certificates #20434

Are you sure you want to change the base?

NSUrlSessionHandler: Adds support for X509 client certificates #20434

Conversation

dotMorten commented Apr 11, 2024 • edited

rolfbjarne commented Apr 12, 2024

rolfbjarne commented Apr 15, 2024

azure-pipelines bot commented Apr 15, 2024

github-actions bot commented Apr 15, 2024

⚠️ Your code has been reformatted. ⚠️

rolfbjarne commented Apr 15, 2024

azure-pipelines bot commented Apr 15, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

rolfbjarne commented Apr 15, 2024

dotMorten commented Apr 15, 2024

github-actions bot commented Apr 15, 2024

⚠️ Your code has been reformatted. ⚠️

rolfbjarne commented Apr 15, 2024

azure-pipelines bot commented Apr 15, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

rolfbjarne commented May 21, 2024

azure-pipelines bot commented May 21, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

rolfbjarne commented May 23, 2024

rolfbjarne commented May 23, 2024

rolfbjarne commented May 23, 2024

azure-pipelines bot commented May 23, 2024

vs-mobiletools-engineering-service2 commented May 23, 2024

❌ [PR Build] Tests on macOS X64 - Mac Sonoma (14) failed ❌

vs-mobiletools-engineering-service2 commented May 23, 2024

❌ [PR Build] Tests on macOS M1 - Mac Big Sur (11) failed ❌

vs-mobiletools-engineering-service2 commented May 23, 2024

❌ [PR Build] Tests on macOS M1 - Mac Ventura (13) failed ❌

vs-mobiletools-engineering-service2 commented May 23, 2024

❌ [PR Build] Tests on macOS M1 - Mac Monterey (12) failed ❌

vs-mobiletools-engineering-service2 commented May 23, 2024

📚 [PR Build] Artifacts 📚

Packages generated

vs-mobiletools-engineering-service2 commented May 23, 2024

✅ API diff for current PR / commit

✅ API diff vs stable

ℹ️ Generator diff

vs-mobiletools-engineering-service2 commented May 23, 2024

🔥 [CI Build] Test results 🔥

Test results

Failures

❌ cecil tests

❌ monotouch tests (macOS)

Successes

dotMorten commented Apr 11, 2024 •

edited