Skip to content

Filter dev certs without X509SubjectKeyIdentifierExtension when any cert has it#13667

Merged
davidfowl merged 2 commits intomainfrom
copilot/exclude-non-extension-certificates
Dec 20, 2025
Merged

Filter dev certs without X509SubjectKeyIdentifierExtension when any cert has it#13667
davidfowl merged 2 commits intomainfrom
copilot/exclude-non-extension-certificates

Conversation

Copy link
Contributor

Copilot AI commented Dec 20, 2025

Description

When loading developer certificates, if any certificate contains an X509SubjectKeyIdentifierExtension, exclude all certificates lacking this extension from the trust set. This prevents conflicts between legacy certificates and modern SKI-enabled certificates, particularly with OpenSSL's first-match validation behavior.

Changes:

  • Modified DeveloperCertificateService.cs certificate loading logic to filter certificates based on SKI extension presence
  • If any valid certificate has SKI extension, exclude all certificates without it before grouping and ordering
  • No behavior change when all certificates are homogeneous (all with or all without SKI)

Implementation:

// Get all valid ASP.NET Core development certificates
var validCerts = store.Certificates
    .Where(c => c.IsAspNetCoreDevelopmentCertificate())
    .Where(c => c.NotBefore <= now && now <= c.NotAfter)
    .ToList();

// If any certificate has a Subject Key Identifier extension, exclude certificates without it
if (validCerts.Any(c => c.HasSubjectKeyIdentifier()))
{
    validCerts = validCerts.Where(c => c.HasSubjectKeyIdentifier()).ToList();
}

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?
    • Yes
    • No
Original prompt

This section details on the original issue you should resolve

<issue_title>If there are dev certs with X509SubjectKeyIdentifierExtension, only include certificates with X509SubjectKeyIdentifierExtension in the trust set</issue_title>
<issue_description>In DeveloperCertificateService.cs, if any developer certificate includes an X509SubjectKeyIdentifierExtension, we need to exclude any dev certs that don't include the extension from the final set.</issue_description>

Comments on the Issue (you are @copilot in this section)


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

…ny cert has it

Co-authored-by: danegsta <50252651+danegsta@users.noreply.github.com>
@danegsta danegsta marked this pull request as ready for review December 20, 2025 01:07
Copilot AI review requested due to automatic review settings December 20, 2025 01:07
@github-actions
Copy link
Contributor

🚀 Dogfood this PR with:

⚠️ WARNING: Do not do this without first carefully reviewing the code of this PR to satisfy yourself it is safe.

curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 13667

Or

  • Run remotely in PowerShell:
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 13667"

Copilot AI changed the title [WIP] Fix developer certificate trust set to include only specific certificates Filter dev certs without X509SubjectKeyIdentifierExtension when any cert has it Dec 20, 2025
Copilot AI requested a review from danegsta December 20, 2025 01:11
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR implements a fix for filtering developer certificates to ensure that when any certificate has an X509SubjectKeyIdentifier extension, only certificates with this extension are included in the final trust set. This prevents legacy certificates without SKI from causing conflicts in OpenSSL certificate validation.

Key Changes

  • Extracted certificate filtering into an intermediate validCerts list to enable conditional filtering before grouping
  • Added logic to check if any valid certificate has a Subject Key Identifier extension
  • If SKI is present on any certificate, filters out all certificates without it

Comment on lines +43 to +47
// If any certificate has a Subject Key Identifier extension, exclude certificates without it
if (validCerts.Any(c => c.HasSubjectKeyIdentifier()))
{
validCerts = validCerts.Where(c => c.HasSubjectKeyIdentifier()).ToList();
}
Copy link

Copilot AI Dec 20, 2025

Choose a reason for hiding this comment

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

The new filtering logic that excludes certificates without Subject Key Identifier when at least one certificate has it lacks test coverage. Consider adding unit tests that verify the filtering behavior with scenarios such as:

  • All certificates have SKI: all should be included
  • Some certificates have SKI, some don't: only those with SKI should be included
  • No certificates have SKI: all should be included
  • Mixed valid/invalid SKI values

This would ensure the filtering logic works correctly across different certificate store states.

Copilot uses AI. Check for mistakes.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not quite ready to add tests that write certificates to the store to make these tests viable.

@davidfowl
Copy link
Member

Confirmed this works

@davidfowl davidfowl merged commit e2adbc2 into main Dec 20, 2025
291 of 292 checks passed
@davidfowl davidfowl deleted the copilot/exclude-non-extension-certificates branch December 20, 2025 03:24
@davidfowl
Copy link
Member

@danegsta What happens if they only have an older dev cert?

@dotnet-policy-service dotnet-policy-service bot added this to the 13.2 milestone Dec 20, 2025
@danegsta
Copy link
Member

@danegsta What happens if they only have an older dev cert?

If there’s only older dev certs (pre-10.0.101 patch), we’ll only trust a single older cert (the newest one). If there’s multiple certs from 10.0.101 or newer we can safely trust all of them.

@danegsta
Copy link
Member

/backport to release/13.1

@github-actions
Copy link
Contributor

Started backporting to release/13.1: https://github.com/dotnet/aspire/actions/runs/20414391919

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

If there are dev certs with X509SubjectKeyIdentifierExtension, only include certificates with X509SubjectKeyIdentifierExtension in the trust set

3 participants