[Foundation] Implement the server certificate custom validation callback usage in NSUrlSessionHandler #15117

simonrozsival · 2022-05-25T08:56:09Z

We recently implemented the ServerCertificateCustomValidationCallback in Xamarin.Android (dotnet/android#6665). It would be great to have feature parity and support the same callback in Xamarin.iOS and Xamarin.Mac.

Related to dotnet/runtime#68898.

Partial fix for #14632.

mandel-macaque · 2022-05-25T17:51:25Z

/azp run

azure-pipelines · 2022-05-25T17:52:49Z

Azure Pipelines successfully started running 1 pipeline(s).

src/Foundation/NSUrlSessionHandler.cs

mandel-macaque · 2022-05-25T18:07:32Z

src/Foundation/NSUrlSessionHandler.cs

+					var trustSec = (trustCallbackForUrl?.Invoke (sessionHandler, inflight.RequestUrl, challenge.ProtectionSpace.ServerSecTrust) ?? false) ||
+						(InvokeServerCertificateCustomValidationCallback (inflight.Request, challenge.ProtectionSpace.ServerSecTrust));


What happens if both are set? Do we trust the native one of the manage APIs? Should we always trust first the results from ServerCertificateCustomValidationCallback ?

I opted to trust the certificate if either of the callbacks trusts the certificate. It's basically the same in the #else branch. I guess the comment should be updated as well. Unless the callbacks contain some weird side effects, the order in which they are invoked doesn't matter. I'm not sure there's really a much better way of doing it.

vs-mobiletools-engineering-service2 · 2022-05-25T18:19:28Z

🔥 [PR Build] Build failed 🔥

Build failed for the job 'Generate API diff'

Pipeline on Agent
Hash:

vs-mobiletools-engineering-service2 · 2022-05-25T18:20:19Z

🔥 [PR Build] Build failed 🔥

Build failed for the job 'Build packages'

Pipeline on Agent
Hash:

rolfbjarne · 2022-05-25T21:36:29Z

The build fails with:

Foundation/NSUrlSessionHandler.cs(885,102): error CS1061: 'NSUrlSessionHandler' does not contain a definition for 'ServerCertificateCustomValidationCallback' and no accessible extension method 'ServerCertificateCustomValidationCallback' accepting a first argument of type 'NSUrlSessionHandler' could be found (are you missing a using directive or an assembly reference?)
Foundation/NSUrlSessionHandler.cs(974,24): error CS1061: 'NSUrlSessionHandler' does not contain a definition for 'ServerCertificateCustomValidationCallback' and no accessible extension method 'ServerCertificateCustomValidationCallback' accepting a first argument of type 'NSUrlSessionHandler' could be found (are you missing a using directive or an assembly reference?)
Foundation/NSUrlSessionHandler.cs(1007,27): error CS1061: 'NSUrlSessionHandler' does not contain a definition for 'ServerCertificateCustomValidationCallback' and no accessible extension method 'ServerCertificateCustomValidationCallback' accepting a first argument of type 'NSUrlSessionHandler' could be found (are you missing a using directive or an assembly reference?)
make[1]: *** [build/watch/reference/Xamarin.WatchOS.dll] Error 1

… nsurlsessionhandler-server-certificate-custom-validation-callback

…m-validation-callback

simonrozsival · 2022-09-05T10:07:16Z

/azp run

azure-pipelines · 2022-09-05T10:07:23Z

Commenter does not have sufficient privileges for PR 15117 in repo xamarin/xamarin-macios

simonrozsival · 2022-09-05T10:08:09Z

@mandel-macaque I don't have permissions to run CI in this repo. Could you please re-run the tests for my latest changes?

rolfbjarne · 2022-09-05T10:21:17Z

/azp run

azure-pipelines · 2022-09-05T10:21:56Z

Azure Pipelines successfully started running 1 pipeline(s).

rolfbjarne · 2022-10-11T15:01:38Z

@rolfbjarne @mandel-macaque without some linker/trimmer help this will likely bring a ton of additional code to any apps (most) that are using HttpClient - even if they don't use the new callback (since it will be marked and so everything it uses recursively).

You should compare the app size with and without the PR.

A very simple app has no real size diff (with LinkSdk): https://gist.github.com/rolfbjarne/31e10f85968df97c44f3dacfcbc48452
A more complex app there's a noticeable sizable difference - 3.6mb (8,5%) - with LinkSdk: https://gist.github.com/rolfbjarne/4c73703ca1a64184ba7e2cffb7875ca1
The complex app changed to LinkAll assemblies is back to no difference: https://gist.github.com/rolfbjarne/153894f29c50c81146a0d6a4547bfd5d
The simple app again, but adding a statement using HttpClient, and there's a 3.8mb increase (23,5%) - with LinkAll: https://gist.github.com/rolfbjarne/5c50f35da85ae8703acf6529bb6a5b33
```
var client = new HttpClient ();
client.GetStringAsync ("http://microsoft.com");
```

So it looks like we'll have to look into how to improve this size-wise.

spouliot · 2022-10-11T15:30:16Z

@rolfbjarne in theory if ServerCertificateCustomValidationCallback setter is not marked (not used) then you can nuke the content of InvokeServerCertificateCustomValidationCallback (to return false) which should avoid marking X509Chain which was (if I recall correctly) the main culprit in this scenario.

That would mean the cost of using the feature would only be paid by consumers of the API.

There were some similar cases (at least in XI old linker code base) where delaying the marking of some code was required.

rolfbjarne · 2022-10-11T15:56:00Z

@spouliot yeah, I asked here: https://discord.com/channels/732297728826277939/751137004007456849/1029421295873564683, let's see if the linker people come up with something. I'm not sure

rolfbjarne · 2022-10-11T21:24:43Z

Alexander had a solution:

Alexander Köplinger — Today at 6:04 PM
there is a way, we've done this in the android version of this change
@Rolf this is the change I made back then: dotnet/android@765c26e
basically move all of the heavy lifting into a method on a helper class, the linker will remove the helper method body if the property isn't set

From https://discord.com/channels/732297728826277939/751137004007456849/1029424275788148776

simonrozsival · 2022-10-12T12:28:36Z

@rolfbjarne could you please re-run the tests after the latest changes?

…erver-certificate-custom-validation-callback

rolfbjarne · 2022-10-13T15:38:43Z

I built the test app again, and got much better results for case 4 above: the app size only increased ~30kb (https://gist.github.com/rolfbjarne/3e96d3377dbb99195424d34acdef035b).

However, I figured that wasn't enough, so I made a few more changes, and now the app increase is 1.3kb (https://gist.github.com/rolfbjarne/09fbbe0bb1693144196c3d78119efcbe).

This is a complete diff of the IL of Microsoft.iOS.dll: https://gist.github.com/rolfbjarne/0c75b935ab70c929734eb011df84e3d5, I don't think we can remove much more at this point.

@simonrozsival can you review my changes?

There's one functional difference between the legacy Xamarin (non-.NET) code and my new implementation: previously if both TrustOverrideForUrl and ServerCertificateCustomValidationCallback were set, we'd pass the challenge (trust verified) if either callback said yay. This didn't seem quite right, so I changed it (and the code looks easier to understand too) to first check (and use) TrustOverrideForUrl, and if there's no such callback, only then call ServerCertificateCustomValidationCallback (and not if TrustOverrideForUrl returns false).

spouliot · 2022-10-13T15:49:20Z

@rolfbjarne fyi appcompare GUI can use ilspycmd to diff C# (instead of IL) 😄
See https://github.com/spouliot/appcompare/wiki/DiffCustomToolOutput

rolfbjarne · 2022-10-14T07:31:03Z

/azp run

simonrozsival · 2022-10-14T07:52:16Z

Thanks for the improvements, @rolfbjarne. Looks good to me.

rolfbjarne · 2022-10-14T13:46:16Z

/azp run

azure-pipelines · 2022-10-14T13:46:55Z

Azure Pipelines successfully started running 1 pipeline(s).

rolfbjarne · 2022-10-14T14:21:52Z

@rolfbjarne fyi appcompare GUI can use ilspycmd to diff C# (instead of IL) 😄 See spouliot/appcompare/wiki/DiffCustomToolOutput

https://gist.github.com/rolfbjarne/615422d95d869a9d57c5b3c5a2e9c955!

vs-mobiletools-engineering-service2 · 2022-10-14T15:23:30Z

✅ 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 is empty

Pipeline on Agent
Hash: df170fcc27ff8a14b4b7f7afc7b573d0e017c9ed [PR build]

vs-mobiletools-engineering-service2 · 2022-10-14T15:34:14Z

📚 [PR Build] Artifacts 📚

Packages generated

View packages

Pipeline on Agent XAMBOT-1107.Monterey'
Hash: df170fcc27ff8a14b4b7f7afc7b573d0e017c9ed [PR build]

vs-mobiletools-engineering-service2 · 2022-10-14T15:40:20Z

💻 [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) passed 💻

✅ All tests on macOS M1 - Mac Big Sur (11.5) passed.

Pipeline on Agent
Hash: df170fcc27ff8a14b4b7f7afc7b573d0e017c9ed [PR build]

vs-mobiletools-engineering-service2 · 2022-10-14T19:13:39Z

🚀 [CI Build] Test results 🚀

Test results

✅ All tests passed on VSTS: simulator tests.

🎉 All 223 tests passed 🎉

Tests counts

✅ bcl: All 69 tests passed. Html Report (VSDrops) Download
✅ cecil: All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests: All 1 tests passed. Html Report (VSDrops) Download
✅ fsharp: All 7 tests passed. Html Report (VSDrops) Download
✅ framework: All 8 tests passed. Html Report (VSDrops) Download
✅ generator: All 2 tests passed. Html Report (VSDrops) Download
✅ interdependent_binding_projects: All 7 tests passed. Html Report (VSDrops) Download
✅ install_source: All 1 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 12 tests passed. Html Report (VSDrops) Download
✅ monotouch: All 23 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]

simonrozsival and others added 5 commits May 4, 2022 17:41

WIP invoke server certificate custom validation callback

b0dbe59

WIP add tests

916b931

Fix sec trust param type

6bd110a

WIP improve chain building

6e20ea4

Make it build

f0b8dd0

mandel-macaque reviewed May 25, 2022

View reviewed changes

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

mandel-macaque reviewed May 25, 2022

View reviewed changes

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

mandel-macaque reviewed May 25, 2022

View reviewed changes

simonrozsival added 7 commits August 30, 2022 12:24

Merge branch 'main' of https://github.com/xamarin/xamarin-macios into…

c6ae804

… nsurlsessionhandler-server-certificate-custom-validation-callback

Update tests

4326052

Fix build and update implementation

78509b2

Run the new tests only in .NET builds

9a6deda

Fix default certificate evaluation

09ec0ce

Fix tests

eeb3495

Merge branch 'main' into nsurlsessionhandler-server-certificate-custo…

f9a46f4

…m-validation-callback

This comment has been minimized.

Sign in to view

Fix using GetCertificateChain on systems which don't support it

c9d822d

This comment has been minimized.

Sign in to view

simonrozsival added 3 commits October 12, 2022 11:54

Use helper class to fix trimming issues

27b368f

Fixes

40da74f

Add missing setter

db36a5d

rolfbjarne added 2 commits October 13, 2022 10:10

Merge remote-tracking branch 'origin/main' into nsurlsessionhandler-s…

d691f6d

…erver-certificate-custom-validation-callback

Improve code to make the linker able to remove more code.

7fb65da

Fix build.

df170fc

mandel-macaque approved these changes Oct 14, 2022

View reviewed changes

rolfbjarne approved these changes Oct 17, 2022

View reviewed changes

rolfbjarne changed the title ~~[Foundation] Implement the server certificate custom validation callback usage in NSUrlSessionHandler~~ [Foundation] Implement the server certificate custom validation callback usage in NSUrlSessionHandler Oct 17, 2022

rolfbjarne merged commit 7171baa into xamarin:main Oct 17, 2022

		var trustSec = (trustCallbackForUrl?.Invoke (sessionHandler, inflight.RequestUrl, challenge.ProtectionSpace.ServerSecTrust) ?? false) \|\|
		(InvokeServerCertificateCustomValidationCallback (inflight.Request, challenge.ProtectionSpace.ServerSecTrust));

[Foundation] Implement the server certificate custom validation callback usage in NSUrlSessionHandler #15117

[Foundation] Implement the server certificate custom validation callback usage in NSUrlSessionHandler #15117

Conversation

simonrozsival commented May 25, 2022 • edited by rolfbjarne Loading

mandel-macaque commented May 25, 2022

azure-pipelines bot commented May 25, 2022

mandel-macaque May 25, 2022

Choose a reason for hiding this comment

simonrozsival May 26, 2022

Choose a reason for hiding this comment

vs-mobiletools-engineering-service2 commented May 25, 2022

🔥 [PR Build] Build failed 🔥

vs-mobiletools-engineering-service2 commented May 25, 2022

🔥 [PR Build] Build failed 🔥

rolfbjarne commented May 25, 2022

simonrozsival commented Sep 5, 2022

azure-pipelines bot commented Sep 5, 2022

simonrozsival commented Sep 5, 2022

rolfbjarne commented Sep 5, 2022

azure-pipelines bot commented Sep 5, 2022

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 Oct 11, 2022

spouliot commented Oct 11, 2022

rolfbjarne commented Oct 11, 2022

rolfbjarne commented Oct 11, 2022

simonrozsival commented Oct 12, 2022

rolfbjarne commented Oct 13, 2022

spouliot commented Oct 13, 2022

rolfbjarne commented Oct 14, 2022

simonrozsival commented Oct 14, 2022

rolfbjarne commented Oct 14, 2022

azure-pipelines bot commented Oct 14, 2022

rolfbjarne commented Oct 14, 2022

vs-mobiletools-engineering-service2 commented Oct 14, 2022

✅ API diff for current PR / commit

✅ API diff vs stable

✅ Generator diff

vs-mobiletools-engineering-service2 commented Oct 14, 2022

📚 [PR Build] Artifacts 📚

Packages generated

vs-mobiletools-engineering-service2 commented Oct 14, 2022

💻 [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) passed 💻

vs-mobiletools-engineering-service2 commented Oct 14, 2022

🚀 [CI Build] Test results 🚀

Test results

Tests counts

simonrozsival commented May 25, 2022 •

edited by rolfbjarne

Loading