Throws a specific exception when a certificate is needed but not provided #24544
Conversation
…marin-macios into detect_missing_cert
There was a problem hiding this comment.
Pull request overview
This PR adds specific exception handling for scenarios where a server requests a client certificate but none is provided by the application. The implementation throws a structured exception chain (HttpRequestException → WebException → AuthenticationException) to help developers detect and handle missing certificate scenarios programmatically.
Changes:
- Modified NSUrlSessionHandler to throw a specific exception when a client certificate is requested but not available
- Added an AppContext switch to allow disabling the new behavior for backward compatibility
- Added two test methods to validate optional and required certificate scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/Foundation/NSUrlSessionHandler.cs | Adds exception handling logic when client certificate is missing, with AppContext switch for backward compatibility |
| tests/monotouch-test/System.Net.Http/MessageHandlers.cs | Adds two test methods: one for optional certificates (should succeed) and one for required certificates (should throw specific exception) |
| // The server requested a certificate, but we don't have one to provide. Fail the request with a meaningful exception | ||
| // that allows the developer to identify this, ask the user for a certificate, add it to the ClientCertificates collection | ||
| // and then re-try the request. | ||
| lock (inflight.Lock) { | ||
| inflight.Exception = new HttpRequestException ("An error occurred while sending the request.", | ||
| new WebException ("Error: Certificate Required", | ||
| new AuthenticationException ("Error: Certificate Required"), | ||
| WebExceptionStatus.SecureChannelFailure, null)); | ||
| } |
There was a problem hiding this comment.
Setting the exception preemptively here could mask other unrelated errors. If the request fails due to a different reason (e.g., network timeout, DNS failure, server error), the pre-set "Certificate Required" exception will be thrown instead of the actual error, misleading developers about the real cause of failure. Consider only setting this exception if the error is specifically related to certificate authentication, or clearing the exception if the request proceeds successfully past a certain point.
| // The server requested a certificate, but we don't have one to provide. Fail the request with a meaningful exception | |
| // that allows the developer to identify this, ask the user for a certificate, add it to the ClientCertificates collection | |
| // and then re-try the request. | |
| lock (inflight.Lock) { | |
| inflight.Exception = new HttpRequestException ("An error occurred while sending the request.", | |
| new WebException ("Error: Certificate Required", | |
| new AuthenticationException ("Error: Certificate Required"), | |
| WebExceptionStatus.SecureChannelFailure, null)); | |
| } | |
| // The server requested a certificate, but we don't have one to provide. |
| string content = ""; | ||
| var done = TestRuntime.TryRunAsync (TimeSpan.FromSeconds (30), async () => { | ||
| using var handler = new NSUrlSessionHandler (); | ||
| using var client = new HttpClient (handler); | ||
| // This service doesn't require a certificate and should succeed even if one isn't provided | ||
| var response = await client.GetAsync (NetworkResources.EchoClientCertificateUrl); | ||
| content = await response.EnsureSuccessStatusCode ().Content.ReadAsStringAsync (); |
There was a problem hiding this comment.
The variable 'content' is declared but never used. Consider removing it or using it in assertions to verify the response content.
| string content = ""; | |
| var done = TestRuntime.TryRunAsync (TimeSpan.FromSeconds (30), async () => { | |
| using var handler = new NSUrlSessionHandler (); | |
| using var client = new HttpClient (handler); | |
| // This service doesn't require a certificate and should succeed even if one isn't provided | |
| var response = await client.GetAsync (NetworkResources.EchoClientCertificateUrl); | |
| content = await response.EnsureSuccessStatusCode ().Content.ReadAsStringAsync (); | |
| var done = TestRuntime.TryRunAsync (TimeSpan.FromSeconds (30), async () => { | |
| using var handler = new NSUrlSessionHandler (); | |
| using var client = new HttpClient (handler); | |
| // This service doesn't require a certificate and should succeed even if one isn't provided | |
| var response = await client.GetAsync (NetworkResources.EchoClientCertificateUrl); | |
| await response.EnsureSuccessStatusCode ().Content.ReadAsStringAsync (); |
| string content = ""; | ||
| var done = TestRuntime.TryRunAsync (TimeSpan.FromSeconds (30), async () => { | ||
| using var handler = new NSUrlSessionHandler (); | ||
| using var client = new HttpClient (handler); | ||
| // TODO: Replace with a service that actually requires a certificate and uses TLS1.2 | ||
| var response = await client.GetAsync (NetworkResources.EchoClientCertificateUrl); | ||
| content = await response.EnsureSuccessStatusCode ().Content.ReadAsStringAsync (); |
There was a problem hiding this comment.
The variable 'content' is declared but never used. Consider removing it or using it in assertions to verify the response content.
| string content = ""; | |
| var done = TestRuntime.TryRunAsync (TimeSpan.FromSeconds (30), async () => { | |
| using var handler = new NSUrlSessionHandler (); | |
| using var client = new HttpClient (handler); | |
| // TODO: Replace with a service that actually requires a certificate and uses TLS1.2 | |
| var response = await client.GetAsync (NetworkResources.EchoClientCertificateUrl); | |
| content = await response.EnsureSuccessStatusCode ().Content.ReadAsStringAsync (); | |
| var done = TestRuntime.TryRunAsync (TimeSpan.FromSeconds (30), async () => { | |
| using var handler = new NSUrlSessionHandler (); | |
| using var client = new HttpClient (handler); | |
| // TODO: Replace with a service that actually requires a certificate and uses TLS1.2 | |
| var response = await client.GetAsync (NetworkResources.EchoClientCertificateUrl); | |
| await response.EnsureSuccessStatusCode ().Content.ReadAsStringAsync (); |
| // TODO: Replace with a service that actually requires a certificate and uses TLS1.2 | ||
| var response = await client.GetAsync (NetworkResources.EchoClientCertificateUrl); | ||
| content = await response.EnsureSuccessStatusCode ().Content.ReadAsStringAsync (); |
There was a problem hiding this comment.
This test has a TODO comment indicating it needs a proper test URL that actually requires a certificate and uses TLS1.2. The current test URL (NetworkResources.EchoClientCertificateUrl) appears to be the same endpoint used for optional certificates, which means this test may not actually verify the missing certificate exception handling. The test logic expects an exception but uses a URL that doesn't require a certificate, making the test ineffective.
| new AuthenticationException ("Error: Certificate Required"), | ||
| WebExceptionStatus.SecureChannelFailure, null)); | ||
| } | ||
| // We will still continue with a null credential, since some services uses optional client certificates and this will still let it succeed |
There was a problem hiding this comment.
The comment states "some services uses optional client certificates" but there's a grammatical error. It should be "some services use optional client certificates" (use, not uses).
| // We will still continue with a null credential, since some services uses optional client certificates and this will still let it succeed | |
| // We will still continue with a null credential, since some services use optional client certificates and this will still let it succeed |
✅ [CI Build #426db2e] Build passed (Build packages) ✅Pipeline on Agent |
✅ [PR Build #426db2e] Build passed (Detect API changes) ✅Pipeline on Agent |
✅ API diff for current PR / commitNET (empty diffs)✅ API diff vs stableNET (empty diffs)ℹ️ Generator diffGenerator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes) Pipeline on Agent |
✅ [CI Build #426db2e] Build passed (Build macOS tests) ✅Pipeline on Agent |
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.
❌ [CI Build #426db2e] Tests on macOS X64 - Mac Sonoma (14) failed ❌Failed tests are:
Pipeline on Agent |
❌ [CI Build #426db2e] Tests on macOS M1 - Mac Monterey (12) failed ❌Failed tests are:
Pipeline on Agent |
❌ [CI Build #426db2e] Tests on macOS M1 - Mac Ventura (13) failed ❌Failed tests are:
Pipeline on Agent |
❌ [CI Build #426db2e] Tests on macOS arm64 - Mac Tahoe (26) failed ❌Failed tests are:
Pipeline on Agent |
❌ [CI Build #426db2e] Tests on macOS arm64 - Mac Sequoia (15) failed ❌Failed tests are:
Pipeline on Agent |
🔥 [CI Build #426db2e] Test results 🔥Test results❌ Tests failed on VSTS: test results 0 tests crashed, 39 tests failed, 78 tests passed. Failures❌ fsharp tests [attempt 2]1 tests failed, 3 tests passed.Failed tests
Html Report (VSDrops) Download ❌ monotouch tests (iOS) [attempt 2]9 tests failed, 0 tests passed.Failed tests
Html Report (VSDrops) Download ❌ monotouch tests (MacCatalyst) [attempt 2]11 tests failed, 0 tests passed.Failed tests
Html Report (VSDrops) Download ❌ monotouch tests (macOS) [attempt 2]9 tests failed, 0 tests passed.Failed tests
Html Report (VSDrops) Download ❌ monotouch tests (tvOS) [attempt 2]9 tests failed, 0 tests passed.Failed tests
Html Report (VSDrops) Download Successes✅ cecil: All 1 tests passed. Html Report (VSDrops) Download Pipeline on Agent |
Fixes #21688
This allows the user to detect the specific exception when a certificate is needed and react to it. Exceptions thrown are very similar to what
SocketsHttpHandlerand other handlers throw, while also following the pattern of other exceptions thrown byNSUrlSessionHandler.This is a re-creation of #24532 from @dotMorten (due to our CI not being able to build PRs from forks).