Normalize X509Chain App/Cert policies across OSes - #132348
Open
bartonjs wants to merge 1 commit into
Open
Conversation
The original policy handling code was written before contextual trust, which made it hard to write good tests. Now that we have more tests, unify the behaviors as best we can, even across invalidly encoded extensions.
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 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
Author
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Member
Author
|
/azp run runtime-extra-platforms |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates X509Chain policy processing so ApplicationPolicy/CertificatePolicy handling (and invalid/undecodable policy-related extensions) is evaluated and reported more consistently across Windows/OpenSSL/Apple/Android, and adjusts/expands tests to match the unified behavior.
Changes:
- Refactors
CertificatePolicyChainto compute per-chain-element “encoding” vs “usage” errors and exposes helpers to reuse the same logic across platform chain processors. - Updates OpenSSL, Apple, and Android chain processors to merge policy/encoding errors into chain + element status consistently (and to detect encoding issues even when no explicit policy filtering is requested).
- Normalizes existing tests’ OS-conditional expectations and adds focused test suites for app-policy vs EKU behavior and corrupt policy-related extensions.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Security.Cryptography/tests/X509Certificates/RevocationTests/DynamicRevocationTests.cs | Removes OS-conditional expectations for NotValidForUsage at non-leaf levels. |
| src/libraries/System.Security.Cryptography/tests/X509Certificates/DynamicChainTests.cs | Refactors/makes helpers reusable by new tests; factors out shared DER encoding for policy extensions. |
| src/libraries/System.Security.Cryptography/tests/X509Certificates/CorruptPoliciesChainTests.cs | New coverage for corrupt/undecodable policy/EKU-related extensions and expected chain-element status behavior. |
| src/libraries/System.Security.Cryptography/tests/X509Certificates/ChainTests.cs | Normalizes expectations around NotValidForUsage across platforms. |
| src/libraries/System.Security.Cryptography/tests/X509Certificates/AppAndCertPoliciesChainTests.cs | New tests covering certificate policy constraints/mappings plus Application Policies vs EKU interactions. |
| src/libraries/System.Security.Cryptography/tests/System.Security.Cryptography.Tests.csproj | Adds the new test files to the test project. |
| src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/OpenSslX509ChainProcessor.cs | Splits policy evaluation into “merge errors” and “process policy”; adds encoding-only validation when no policy filters are requested. |
| src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/ChainPal.Apple.cs | Switches to shared policy-chain logic and per-element error attribution. |
| src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/ChainPal.Android.cs | Switches to shared policy-chain logic and per-element error attribution. |
| src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/CertificatePolicy.cs | Implements CertificatePolicyChain.Build/CheckEncodingOnly, per-element error vectors, and revised EKU/app-policy semantics. |
| src/libraries/Common/src/System/Security/Cryptography/Oids.cs | Adds AnyEnhancedKeyUsage constant used in policy evaluation. |
Comment on lines
+671
to
+686
| if (_vector is null) | ||
| { | ||
| return (_scalar & (1 << index)) != 0; | ||
| } | ||
|
|
||
| return _vector[index]; | ||
| } | ||
| } | ||
|
|
||
| internal void Set(int index) | ||
| { | ||
| if (_vector is null) | ||
| { | ||
| _scalar |= (1 << index); | ||
| } | ||
| else |
Comment on lines
+446
to
+450
| public static IEnumerable<object[]> ApplicationPolicyVsEkuMemberData() | ||
| { | ||
| const string ServerAuth = "1.3.6.1.5.5.7.3.2"; | ||
| const string ClientAuth = "1.3.6.1.5.5.7.3.1"; | ||
| const string TimeStamp = "1.3.6.1.5.5.7.3.8"; // RFC 3161, used only as a companion value |
Comment on lines
+138
to
+141
| // When the ApplicationPolicy is corrupt, it seems to treat | ||
| // the element as valid for all usages (but is still | ||
| // scoped by the issuers), so by checking for a | ||
|
|
| X509ChainStatus invalidPolicyConstraints = new X509ChainStatus | ||
| { | ||
| Status = X509ChainStatusFlags.InvalidPolicyConstraints, | ||
| // "NoPolicyMatch" sais that the policy is "invalid", which works for this one, too. |
|
|
||
| foreach (X509Certificate2 cert in chain) | ||
| { | ||
| // Windows ignores declared policcy corruption on the root cert. |
Comment on lines
640
to
642
| <Compile Include="X509Certificates\Cert.cs" /> | ||
| <Compile Include="X509Certificates\AppAndCertPoliciesChainTests.cs" /> | ||
| <Compile Include="X509Certificates\CertTests.cs" /> |
This was referenced Aug 15, 2026
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.
The original policy handling code was written before contextual trust, which made it hard to write good tests. Now that we have more tests, unify the behaviors as best we can, even across invalidly encoded extensions.
Fixes #31246 (and maybe others)