-
Notifications
You must be signed in to change notification settings - Fork 511
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] Implement the server certificate custom validation callback usage in NSUrlSessionHandler #15117
[Foundation] Implement the server certificate custom validation callback usage in NSUrlSessionHandler #15117
Conversation
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
var trustSec = (trustCallbackForUrl?.Invoke (sessionHandler, inflight.RequestUrl, challenge.ProtectionSpace.ServerSecTrust) ?? false) || | ||
(InvokeServerCertificateCustomValidationCallback (inflight.Request, challenge.ProtectionSpace.ServerSecTrust)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
🔥 [PR Build] Build failed 🔥Build failed for the job 'Generate API diff' Pipeline on Agent |
🔥 [PR Build] Build failed 🔥Build failed for the job 'Build packages' Pipeline on Agent |
The build fails with:
|
… nsurlsessionhandler-server-certificate-custom-validation-callback
…m-validation-callback
/azp run |
Commenter does not have sufficient privileges for PR 15117 in repo xamarin/xamarin-macios |
@mandel-macaque I don't have permissions to run CI in this repo. Could you please re-run the tests for my latest changes? |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
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.
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.
So it looks like we'll have to look into how to improve this size-wise. |
@rolfbjarne in theory if 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. |
@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 |
Alexander had a solution:
From https://discord.com/channels/732297728826277939/751137004007456849/1029424275788148776 |
@rolfbjarne could you please re-run the tests after the latest changes? |
…erver-certificate-custom-validation-callback
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 |
@rolfbjarne fyi |
/azp run |
Thanks for the improvements, @rolfbjarne. Looks good to me. |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
https://gist.github.com/rolfbjarne/615422d95d869a9d57c5b3c5a2e9c955! |
✅ API diff for current PR / commitLegacy Xamarin (No breaking changes)
NET (empty diffs)
✅ API diff vs stableLegacy Xamarin (No breaking changes).NET (No breaking changes)✅ Generator diffGenerator diff is empty Pipeline on Agent |
💻 [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 |
🚀 [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 Pipeline on Agent |
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.