-
Notifications
You must be signed in to change notification settings - Fork 703
Add new System
and None
scopes for certificate trust mode, use System
by default for Python
#12103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 12103 Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 12103" |
src/Aspire.Hosting.Python/PythonAppResourceBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
src/Aspire.Hosting.Python/PythonAppResourceBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
System
and None
scopes for certificate trust mode, use System
by default for Python
There was a problem hiding this 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 introduces enhanced certificate trust configuration for Aspire resources by adding two new scope values (System
and None
) to CustomCertificateAuthoritiesScope
. The System
scope enables resources to trust both custom certificates and system root certificates, while None
disables custom certificate trust entirely. Python applications now default to System
scope since they cannot easily append to their default certificate stores (like certifi).
Key changes:
- Added
System
andNone
enum values toCustomCertificateAuthoritiesScope
with comprehensive documentation - Updated DCP executor logic to handle the new scopes, including system certificate retrieval for containers and executables
- Changed Python apps to default to
System
scope and addedSSL_CERT_FILE
environment variable support
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
src/Aspire.Hosting/ApplicationModel/CertificateAuthorityCollectionAnnotation.cs |
Added System and None enum values with XML documentation explaining their behavior |
src/Aspire.Hosting/ResourceBuilderExtensions.cs |
Updated documentation to clarify default scope behavior |
src/Aspire.Hosting/Dcp/DcpExecutor.cs |
Implemented handling for new scopes in both executable and container certificate trust logic |
src/Aspire.Hosting.Python/PythonAppResourceBuilderExtensions.cs |
Changed default scope to System for Python apps and added SSL_CERT_FILE support |
src/Aspire.Hosting.NodeJs/NodeExtensions.cs |
Added SSL_CERT_FILE environment variable for Node.js apps using OpenSSL CA |
src/Aspire.Hosting/ApplicationModel/CertificateAuthorityCollectionAnnotation.cs
Outdated
Show resolved
Hide resolved
Code looks good, need to get sign off from a security POV. |
I’ll schedule a review (and let Steve know we specifically need his eyes on it since otherwise it just goes to the general on rotation reviewers). |
Renames
CustomCertificateAuthoritiesScope
toCertificateTrustScope
and adds newSystem
andNone
values.System
behaves similar to the existingOverride
type, but automatically includes system trusted root certificates.None
disables custom certificate trust for a resource.Because Python doesn't support appending to default certificate trust (such as certifi certificates), this PR also makes the default for Python apps
System
.