Closed
Fix Android failure in SslAuthenticationOptionsTests.UpdateOptions_ServerCertificateContextProvided_DoesNotDisposeCallerContext#131929
Conversation
|
Azure Pipelines: 16 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
…uding root cert Co-authored-by: rzikm <32671551+rzikm@users.noreply.github.com>
Contributor
|
Tagging subscribers to this area: @dotnet/ncl, @bartonjs, @vcsjones |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/libraries/System.Net.Security/tests/UnitTests/SslAuthenticationOptionsTests.cs:234
- The new
Assert.NotEmptycheck is a bit too loose for this scenario and doesn’t really encode the stated platform variation (root trimmed vs not). You can keep this robust while still being specific by asserting the expected range for this chain: 1 intermediate on platforms that trim the root, and 2 (intermediate + root) on platforms that keep it.
// Verify that the caller's intermediate certificates were not disposed
// (the exact count varies by platform, some platforms keep the root in the chain)
Assert.NotEmpty(callerContext.IntermediateCertificates);
Copilot
AI
changed the title
[WIP] Fix test failure in SslAuthenticationOptionsTests for Android
Fix Android failure in SslAuthenticationOptionsTests.UpdateOptions_ServerCertificateContextProvided_DoesNotDisposeCallerContext
Aug 6, 2026
Member
|
/azp list |
Member
|
/azp run runtime-android |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
rzikm
marked this pull request as ready for review
August 10, 2026 10:16
|
Azure Pipelines: Successfully started running 4 pipeline(s). 12 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Member
|
/azp run runtime-android |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Member
|
/ba-g test failures are unrelated |
Member
|
The test in question did not fail on the last runtime-android run |
rzikm
approved these changes
Aug 12, 2026
rzikm
added a commit
that referenced
this pull request
Aug 14, 2026
…p only (#132277) Alternative to #131929 and #132125 for #131922. `System.Net.Security.Unit.Tests` crashes during runtime init on Apple-mobile CoreCLR (e.g. tvOS), before test discovery runs. The single test `UpdateOptions_ServerCertificateContextProvided_DoesNotDisposeCallerContext` exercises `SslStreamCertificateContext.Create` with intermediate certificates, which pulls in a generic `Dictionary` instantiation whose IL body is stripped from the Apple-mobile CoreCLR ReadyToRun composite image, crashing the whole app at startup. Because the app crashes before test discovery, per-test `[Fact]`/attribute-based skipping can't help. ## Approach Rather than disabling the entire test project on non-desktop platforms (as the alternatives do), this keeps the rest of the suite running on mobile and only excludes the one problematic test from the mobile build: - Make `SslAuthenticationOptionsTests` a `partial` class. - Move `UpdateOptions_ServerCertificateContextProvided_DoesNotDisposeCallerContext` verbatim into a new `SslAuthenticationOptionsTests.Desktop.cs`. - Register that file with an MSBuild `Compile` condition limited to desktop platforms (`windows`/`unix`/`osx`). An MSBuild condition is used rather than a `#if` because the `TARGET_*` C# defines aren't reliably present in this test project. `TargetFrameworks` and the existing `IgnoreForCI` line are untouched, so the app still builds and all other unit tests still run on mobile. ## Comparison to alternatives - #131929 changes the test to also supply the root cert so Android's chain build succeeds. That addresses the Android `PlatformNotSupportedException` but not the tvOS R2R startup crash, which is unrelated to the test's certificate inputs. - #132125 disables the whole `System.Net.Security.Unit.Tests` project in CI on non-desktop platforms, losing all mobile coverage of these unit tests. This PR is the most surgical option: it drops only the one test on mobile and preserves the remaining coverage. ## Validation Test-only change; no product code touched. Not built locally (test project requires the full libraries baseline build in this environment); the change is a mechanical move plus a build-condition addition. CI will exercise the desktop and mobile legs. > [!NOTE] > This PR was generated with the assistance of GitHub Copilot. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Member
|
replaced by #132277 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
UpdateOptions_ServerCertificateContextProvided_DoesNotDisposeCallerContextfails on all Android CoreCLR and NativeAOT legs withPlatformNotSupportedException: An empty custom trust store is not supported on this platform.Android sets
ChainBuildNeedsTrustedRoot = true, so when the firstX509Chain.Buildfails,SslStreamCertificateContext.Createretries with everyadditionalCertificatesentry inChainPolicy.CustomTrustStoreunderX509ChainTrustMode.CustomRootTrust.ChainPal.Androidonly accepts self-issued certificates as trust anchors and routes the rest to the extra store — the test passed only the (non-self-issued) intermediate, leaving the anchor set empty, which Android rejects.Changes
Supply the root as an additional certificate. Both
SslStreamCertificateContext.Createcalls now receive{ intermediateWithKey, rootCert }, giving Android's retry a self-issued trust anchor. This also matches how real callers hand over a chain (seeConfiguration.Certificates.GenerateCertificatesin the functional tests).Relax the intermediate count assertion.
Assert.Equal(1, callerContext.IntermediateCertificates.Count)→Assert.NotEmpty(...), sinceTrimRootCertificateisfalseon Windows and the root stays in the collection there. The per-certificateExport()check — the actual subject of the assertion — is unchanged.Test-only change; no product code touched. The underlying product behavior (Android throwing
PlatformNotSupportedExceptionwhen a caller supplies intermediates without a root) is pre-existing and left as-is.Verified on linux-x64:
System.Net.Security.Unit.Tests→ 137 total, 0 failed. The Android legs could not be exercised locally; the fix is reasoned fromChainPal.Androidand needs CI confirmation.