[ci-fix] Needs review: Fix Android X509 DynamicChainTests name-constraint expectations (refs #128890)#129651
Draft
github-actions[bot] wants to merge 2 commits into
Draft
Conversation
…128890) Update test expectations to match current Android behavior: - DNS name constraints: Android now reports InvalidNameConstraints directly instead of PartialChain - UPN name constraints: Android does not enforce UPN (OtherName) constraints, so chain.Build() returns true Remove [ActiveIssue] annotations that were skipping these tests on Android. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
Tagging subscribers to this area: @bartonjs, @vcsjones, @dotnet/area-system-security |
Member
|
This looks like a duplicate of #129523 |
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.
Workflow artifact: ci-fix
Artifact kind: help
Linked KBE: #128890
Note
This is an AI/Copilot-generated best-effort fix attempt that I could not fully validate. It is a starting point for a maintainer, not a finished change. Please review the analysis below before merging.
Root cause (best analysis)
DynamicChainTests.NameConstraintViolation_*tests fail on Android because the test expectations for Android's X509 chain-building behavior are out of date:DNS name constraints (
PermittedTree_Dns,ExcludedTree_Dns): Android now reportsInvalidNameConstraintsdirectly in the chain status flags, rather than the previously observedPartialChain. The chain still fails to build (returnsfalse), but the status flag differs.UPN name constraints (
PermittedTree_Upn,ExcludedTree_Upn): Android does not enforce UPN (OtherName) name constraints at all —chain.Build()returnstrue(no violation detected), whereas the tests expectedfalse.The
PlatformNameConstraints()helper (line 1172) returnsPartialChainfor all Android name constraint cases, but this blanket behavior no longer matches Android's actual behavior for DNS and UPN constraint types. Other constraint types (e.g.HasMin) still returnPartialChain, so the helper itself is left unchanged.Attempted fix
This change updates the four affected test methods to match the actual Android behavior:
InvalidNameConstraintson Android instead of usingPlatformNameConstraints()(which returnsPartialChain)chain.Build()returnstrueon Android (it doesn't enforce UPN) with early return[ActiveIssue("...#128890", TestPlatforms.Android)]annotations that were skipping these testsThis approach was guided by
@bartonjs's comment pointing to this section as the fix location.What is unverified / where I need help
InvalidNameConstraintsis the correct expected status for DNS violations on the current Android test devicesPlatformNameConstraintshelper is intentionally left unchanged sinceNameConstraintViolation_PermittedTree_HasMinstill passes withPartialChain— this needs confirmationValidation
dotnet build src/libraries/System.Security.Cryptography/tests/System.Security.Cryptography.Tests.csprojEvidence
Help wanted
area-System.Security):@bartonjs,@vcsjones,@dotnet/area-system-securityFiled by
ci-failure-fix. Comment here or on the workflow file to suggest changes;ci-failure-scan-feedbackreads in-scope feedback daily and opens (or updates) a PR with prompt edits.Note
🔒 Integrity filter blocked 2 items
The following items were blocked because they don't meet the GitHub integrity level.
issue_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".issue_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".To allow these resources, lower
min-integrityin your GitHub frontmatter: