Skip to content

Don't use IgnoreRootRevocationUnknown when processing intermediate CA re-key certificates - #133763

Merged
bartonjs merged 8 commits into
dotnet:mainfrom
bartonjs:sillycaname
Sep 19, 2026
Merged

bartonjs merged 8 commits into
dotnet:mainfrom
bartonjs:sillycaname

Conversation

@bartonjs

Copy link
Copy Markdown
Member

This change ends up with a lot of test infra boilerplate to let DynamicRevocationTests work with a 4-chain instead of a 3-chain, and to specify the names of the certificates to build the re-key state.

@bartonjs bartonjs added this to the 12.0.0 milestone Sep 11, 2026
@bartonjs bartonjs self-assigned this Sep 11, 2026
Copilot AI lite review requested due to automatic review settings September 11, 2026 23:02
@azure-pipelines

Copy link
Copy Markdown
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.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @bartonjs, @vcsjones, @dotnet/area-system-security
See info in area-owners.md if you want to be subscribed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Updates Unix certificate-chain verification so signed self-issued re-key certificates are treated as intermediates for revocation suppression.

Changes:

  • Determines root status by chain position and partial-chain state.
  • Adds four-certificate PKI test infrastructure and regression tests.
  • Adds distinguished-name overloads to certificate helpers.
File summaries
File Description
src/libraries/System.Security.Cryptography/tests/X509Certificates/RevocationTests/DynamicRevocationTests.cs Updated as part of this pull request.
src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/UnixChainVerifier.cs Updated as part of this pull request.
src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/CertificateAuthority.cs Updated as part of this pull request.
Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 2
  • Review effort level: Lite

Copilot AI review requested due to automatic review settings September 12, 2026 00:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Address the Android revocation-status test failure and add coverage for the partial-chain branch.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/UnixChainVerifier.cs:13

  • Please add coverage for the new partial-chain branch. The added tests always provide a custom root and assert a complete four-element chain, so rootIndex = -1 is never exercised; a self-issued, differently-keyed CA at the top of an incomplete chain could therefore regress to being treated as a root without any test detecting that IgnoreRootRevocationUnknown must not suppress its unknown revocation status. The existing AIA partial-chain tests only use RevocationMode.NoCheck (RevocationTests/AiaTests.cs:48-51).
            int rootIndex = HasPartialChain(chainElements) ? -1 : chainElements.Length - 1;
  • Files reviewed: 3/3 changed files
  • Comments generated: 1
  • Review effort level: Lite

@bartonjs

Copy link
Copy Markdown
Member Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

It looks like a number of OSes special case a self-issued child for revocation,
and so long as the parent was known don't complain about the child.

Rather than play whack-a-mole, just limit it to Linux for now.
Copilot AI review requested due to automatic review settings September 14, 2026 22:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

One or more issues must be addressed before approval.

Review details

Suppressed comments (2)

src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/UnixChainVerifier.cs:13

  • HasPartialChain changes the suppression classification for every chain containing a PartialChain status, but the new tests only exercise the complete four-element chain. The existing partial-chain tests do not combine that path with revocation verification flags, so a regression in the new rootIndex = -1 branch would go unnoticed. Please add a partial-chain case that verifies the root and CA ignore flags separately.
            int rootIndex = HasPartialChain(chainElements) ? -1 : chainElements.Length - 1;

src/libraries/System.Security.Cryptography/tests/X509Certificates/RevocationTests/DynamicRevocationTests.cs:1258

  • The Android-specific expectation below is unreachable because [PlatformSpecific(TestPlatforms.Linux)] excludes Android, so the shared ChainPal.Android path is not regression-tested by these new cases. Either include Android if this dynamic responder setup is supported there, or remove the dead branch and add separate Android coverage.
        [Fact]
        [PlatformSpecific(TestPlatforms.Linux)]
        public static void SelfIssuedButNotSelfSignedRevocationUnknown_IgnoreIntermediateUnknown()
  • Files reviewed: 3/3 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Copilot AI review requested due to automatic review settings September 19, 2026 00:11
@bartonjs
bartonjs enabled auto-merge (squash) September 19, 2026 00:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

No unresolved issues were identified that would block approval.

Review effort: Lite
Findings: None

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants