Skip to content
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

UnixCertificateManager: implement IsTrusted. #55335

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tmds
Copy link
Member

@tmds tmds commented Apr 24, 2024

Consider the certificate trusted when .NET trusts it.

@davidfowl ptal.

Consider the certificate trusted when .NET trusts it.
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-commandlinetools Includes: Command line tools, dotnet-dev-certs, dotnet-user-jwts, and OpenAPI label Apr 24, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Apr 24, 2024
@tmds
Copy link
Member Author

tmds commented Apr 24, 2024

This is to omit the following warning when a development cert has been installed through https://github.com/tmds/linux-dev-certs.

The ASP.NET Core developer certificate is not trusted. For information about trusting the ASP.NET Core developer certificate, see https://aka.ms/aspnet/https-trust-dev-cert.

When UnixCertificateManager.TrustCertificateCore is implemented, the IsTrusted implementation can be changed to match.
Until then, using the .NET API can be a meaningful implementation.

@amcasey
Copy link
Member

amcasey commented Apr 24, 2024

What validation does this perform? Just that the certificates in the chain are available and unexpired?

@amcasey
Copy link
Member

amcasey commented Apr 24, 2024

Wait, is this check used exclusively to decide whether or not to print that warning?
https://source.dot.net/#Microsoft.AspNetCore.Server.Kestrel.Core/src/Shared/CertificateGeneration/CertificateManager.cs,a513168894adcdad,references

@tmds
Copy link
Member Author

tmds commented Apr 25, 2024

What validation does this perform? Just that the certificates in the chain are available and unexpired?

As implemented, this check tells us if HttpClient will accept the development certificate for localhost or not.

Wait, is this check used exclusively to decide whether or not to print that warning?

That, and there is another call in dev-certs itself:

if (!RuntimeInformation.IsOSPlatform(OSPlatform.Linux))
{
var trustedCertificates = certificates.Where(certificateManager.IsTrusted).ToList();
if (!trustedCertificates.Any())
{
reporter.Output($@"The following certificates were found, but none of them is trusted: {CertificateManager.ToCertificateDescription(certificates)}");
return ErrorCertificateNotTrusted;
}
else
{
ReportCertificates(reporter, trustedCertificates, "trusted");
}
}
else
{
reporter.Warn("Checking the HTTPS development certificate trust status was requested. Checking whether the certificate is trusted or not is not supported on Linux distributions." +
"For instructions on how to manually validate the certificate is trusted on your Linux distribution, go to https://aka.ms/dev-certs-trust");
}

I don't have a strong opinion on whether this one should be updated (to no longer special case Linux) or not.

public override bool IsTrusted(X509Certificate2 certificate)
{
using X509Chain chain = new X509Chain();
chain.ChainPolicy.RevocationMode = X509RevocationMode.NoCheck;
Copy link
Member

Choose a reason for hiding this comment

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

Why disable revocation checking? Perf?

Copy link
Member Author

Choose a reason for hiding this comment

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

X509Chain uses Online as the default and SocketsHttpHandler uses NoCheck as the default.

If we don't set this to NoCheck then verification fails because the local certificates have no online revocation list to check against.

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-commandlinetools Includes: Command line tools, dotnet-dev-certs, dotnet-user-jwts, and OpenAPI community-contribution Indicates that the PR has been added by a community member pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants