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-24T12:56:06Z

/azp run

azure-pipelines · 2024-05-24T12:56:27Z

Azure Pipelines successfully started running 1 pipeline(s).

vs-mobiletools-engineering-service2 · 2024-05-24T14:40:13Z

💻 [PR Build] Tests on macOS X64 - Mac Sonoma (14) passed 💻

✅ All tests on macOS X64 - Mac Sonoma (14) passed.

Pipeline on Agent
Hash: [PR build]

vs-mobiletools-engineering-service2 · 2024-05-24T15:02:15Z

❌ [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-24T15:02:24Z

❌ [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-24T15:02:31Z

❌ [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-24T15:07:50Z

📚 [PR Build] Artifacts 📚

Packages generated

View packages

Pipeline on Agent
Hash: [PR build]

vs-mobiletools-engineering-service2 · 2024-05-24T15:33:46Z

✅ 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: 58a6a26a135d89b0ebe114631f6ecca17d226e0d [PR build]

vs-mobiletools-engineering-service2 · 2024-05-24T17:37:50Z

🔥 [CI Build] Test results 🔥

Test results

❌ Tests failed on VSTS: test results

1 tests crashed, 8 tests failed, 159 tests passed.

Failures

❌ monotouch tests (macOS)

8 tests failed, 0 tests passed.

monotouch-test/Mac [dotnet]/Debug [dotnet]: Failed (Test run failed.
Tests run: 2866 Passed: 2770 Inconclusive: 5 Failed: 1 Ignored: 95)
monotouch-test/Mac [dotnet]/Debug (static registrar) [dotnet]: TimedOut (Execution timed out after 1200 seconds.
Test run crashed)
monotouch-test/Mac [dotnet]/Release (NativeAOT, x64) [dotnet]: TimedOut (Execution timed out after 1200 seconds.
Test run crashed)
monotouch-test/Mac [dotnet]/Release [dotnet]: TimedOut (Execution timed out after 1200 seconds.
Test run crashed)
monotouch-test/Mac [dotnet]/Release (NativeAOT) [dotnet]: TimedOut (Execution timed out after 1200 seconds.
Test run crashed)
monotouch-test/Mac [dotnet]/Release (all optimizations) [dotnet]: TimedOut (Execution timed out after 1200 seconds.
Test run crashed)
monotouch-test/Mac [dotnet]/Debug (managed static registrar) [dotnet]: TimedOut (Execution timed out after 1200 seconds.
Test run crashed)
monotouch-test/Mac [dotnet]/Release (managed static registrar, all optimizations) [dotnet]: TimedOut (Execution timed out after 1200 seconds.
Test run crashed)

Html Report (VSDrops) Download

❌ xammac tests

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

Html Report (VSDrops) Download

Successes

✅ cecil: All 1 tests passed. Html Report (VSDrops) Download
✅ 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
✅ xcframework: All 8 tests passed. Html Report (VSDrops) Download
✅ xtro: All 2 tests passed. Html Report (VSDrops) Download

Pipeline on Agent
Hash: [PR build]

microsoft-github-policy-service · 2024-06-17T06:22:46Z

Hi @dotMorten. Due to inactivity, we will close this pull request in 7 days.

rolfbjarne · 2024-06-17T11:20:39Z

Hi @dotMorten. Due to inactivity, we will close this pull request in 7 days.

Never mind that, we're still working on this.

rolfbjarne · 2024-06-25T07:58:13Z

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

Looks like Apple is fixing this in macOS 15, Xcode 16 beta 1's header for SecPKCS12Import says:

Starting with macOS 15 and iOS 18, kSecImportToMemoryOnly (with a value of kCFBooleanTrue) allows you to skip importing to the keychain on macOS and explicitly specify iOS behavior.

…es (#8961) Fixes: #7274 Fixes: dotnet/runtime#78933 Context: xamarin/xamarin-macios#20434 Update `AndroidMessageHandler.SetupSSL()` to use the `AndroidMessageHandler.ClientCertificates` collection when `AndroidMessageHandler.ClientCertificateOptions` is `ClientCertificateOption.Manual`, which is now the default. This allows the following code to work as expected: var certificate = … var handler = new AndroidMessageHandler { ClientCertificates = { certificate, }, }; var client = new HttpClient(handler); var result = await client.GetAsync(…); !!API BREAK!! the `AndroidMessageHandler.ClientCertificates` property is updated to now throw an `InvalidOperationException` when `AndroidMessageHandler.ClientCertificateOptions` is `ClientCertificateOption.Automatic`, meaning the following code will now throw, whereas it does *not* throw in .NET 8: var certificate = … var handler = new AndroidMessageHandler(); handler.ClientCertificateOptions = ClientCertificateOption.Automatic; // this now throws InvalidOperationException, new to .NET 9 handler.ClientCertificates.Add(certificate); This updated behavior is consistent with iOS (xamarin/xamarin-macios#20434) and [dotnet/runtime][0]: handler.ClientCertificateOptions = ClientCertificateOption.Automatic; Assert.Throws<InvalidOperationException>(() => handler.ClientCertificates); [0]: https://github.com/dotnet/runtime/blob/2ea6ae57874c452923af059cbcb57d109564353c/src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.ClientCertificates.cs#L60-L68

dalexsoto · 2024-08-28T03:08:33Z

/azp run

azure-pipelines · 2024-08-28T03:08:40Z

No pipelines are associated with this pull request.

microsoft-github-policy-service · 2024-09-23T03:10:10Z

Hi @dotMorten. Due to inactivity, we will close this pull request in 7 days.

dotMorten · 2024-09-23T03:12:16Z

@rolfbjarne @mandel-macaque bot likes to close this. Curious what the hold up is at this point? This is a quite important feature for several of our customers

rolfbjarne · 2024-09-23T10:25:28Z

@dotMorten It turns out the big problem with testing on macOS is gone now, because Apple added API to import certificates without needing access to the default keychain in macOS 15 (this was a big part of the delay, I wanted to test if Apple's new API worked, and it does, at least locally for me). I've updated the code accordingly.

On the other hand, due to security changes/restrictions, our CI can't build PRs from forks anymore, so I re-created this PR here: #21284. Any further discussion should be done there.

@dotMorten

…icates (#21284) Addresses #13856 This was originally created by @dotMorten in #20434. Also make SecIdentity.Import use an in-memory keychain on macOS 15+, so that SecIdentity.Import works like all othe other platforms (i.e. not requiring access to the default keychain, which, among other things, is not ideal on bots). --------- Co-authored-by: Morten Nielsen <[email protected]> Co-authored-by: dotMorten <[email protected]> Co-authored-by: Manuel de la Pena <[email protected]>

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

[devops] Increase debug spew.

58a6a26

This comment was marked as outdated.

Sign in to view

Merge branch 'main' into dotMorten/x509

35d6a45

This comment was marked as outdated.

Sign in to view

rolfbjarne and others added 3 commits August 19, 2024 14:08

Merge branch 'main' into dotMorten/x509

85a6cb6

Merge branch 'main' into dotMorten/x509

ebc030b

Merge branch 'main' into dotMorten/x509

7de13b2

rolfbjarne mentioned this pull request Sep 23, 2024

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

Merged

rolfbjarne closed this Sep 23, 2024

NSUrlSessionHandler: Adds support for X509 client certificates #20434

NSUrlSessionHandler: Adds support for X509 client certificates #20434

Conversation

dotMorten commented Apr 11, 2024 • edited Loading

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.

rolfbjarne commented May 24, 2024

azure-pipelines bot commented May 24, 2024

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

💻 [PR Build] Tests on macOS X64 - Mac Sonoma (14) passed 💻

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

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

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

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

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

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

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

📚 [PR Build] Artifacts 📚

Packages generated

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

✅ API diff for current PR / commit

✅ API diff vs stable

ℹ️ Generator diff

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

🔥 [CI Build] Test results 🔥

Test results

Failures

❌ monotouch tests (macOS)

❌ xammac tests

Successes

microsoft-github-policy-service bot commented Jun 17, 2024

rolfbjarne commented Jun 17, 2024

rolfbjarne commented Jun 25, 2024

This comment was marked as outdated.

This comment was marked as outdated.

dalexsoto commented Aug 28, 2024

azure-pipelines bot commented Aug 28, 2024

microsoft-github-policy-service bot commented Sep 23, 2024

dotMorten commented Sep 23, 2024

rolfbjarne commented Sep 23, 2024

dotMorten commented Apr 11, 2024 •

edited

Loading