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.

Sign up for GitHub

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

Replace use of obsolete X509Certificate2 API #5688

Closed
wants to merge 4 commits into from
Closed

Replace use of obsolete X509Certificate2 API #5688

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
56fcb6c
Replace use of obsolete X509Certificate2 API
drewnoakes Sep 12, 2024
3cd762a
Reuse certificate file contents
drewnoakes Sep 13, 2024
e513ee8
Add tests for reading certificates
drewnoakes Sep 13, 2024
5b6d159
Review feedback
drewnoakes Sep 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Directory.Packages.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@
<PackageVersion Include="Grpc.Tools" Version="2.65.0" />
<PackageVersion Include="Humanizer.Core" Version="2.14.1" />
<PackageVersion Include="KubernetesClient" Version="14.0.9" />
<PackageVersion Include="Microsoft.Bcl.Cryptography" Version="9.0.0-rc.1.24431.7" /> <!-- Can remove Microsoft.Bcl.Cryptography when the repo moves to .NET 9 or later -->
Copy link
Member

Choose a reason for hiding this comment

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

Can we can't get to the final version of the package for the .NET 9 release? Will timings work out?

If not, is it ok to ship a preview package?

Copy link
Member Author

Choose a reason for hiding this comment

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

@joperezr @davidfowl any concerns here?

Copy link
Member

Choose a reason for hiding this comment

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

There are some concerns, but perhaps not really ship-blockers. Starting from RC2, all of the packages from dotnet/runtime will only flow internally, and so far we are mostly just building externally. Our two options would be either ship depending on RC2 build (which while not ideal wouldn't have a big impact since the dashboard ships as a published app with no package references, and hence it can be stable still), or building internally so that we get the final 9.0 version that will get shipped. cc: @bartonjs

<PackageVersion Include="Microsoft.Data.SqlClient" Version="5.2.2" />
<PackageVersion Include="Microsoft.FluentUI.AspNetCore.Components" Version="4.9.3" />
<PackageVersion Include="Microsoft.FluentUI.AspNetCore.Components.Icons" Version="4.9.3" />
Expand Down
1 change: 1 addition & 0 deletions src/Aspire.Dashboard/Aspire.Dashboard.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
<PackageReference Include="Humanizer.Core" />
<PackageReference Include="Microsoft.AspNetCore.Authentication.Certificate" />
<PackageReference Include="Microsoft.AspNetCore.Authentication.OpenIdConnect" />
<PackageReference Include="Microsoft.Bcl.Cryptography" />
<PackageReference Include="Microsoft.FluentUI.AspNetCore.Components" />
<PackageReference Include="Microsoft.FluentUI.AspNetCore.Components.Icons" />
</ItemGroup>
Expand Down
18 changes: 17 additions & 1 deletion src/Aspire.Dashboard/ResourceService/DashboardClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,23 @@ X509CertificateCollection GetFileCertificate()
var filePath = _dashboardOptions.ResourceServiceClient.ClientCertificate.FilePath;
var password = _dashboardOptions.ResourceServiceClient.ClientCertificate.Password;

return [new X509Certificate2(filePath, password)];
Copy link
Member

Choose a reason for hiding this comment

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

"99%" of the reason to use the new X509CertificateLoader over the old constructors is to not load a PKCS#12/PFX when you weren't expecting to. Since you were expecting to load a PFX, and you're loading from a local file, it's not the end of the world if you continue calling the legacy constructor. (And since you're only loading it once you don't care about "we made parallel-loading the same PFX better" feature, etc.)

So, while it is forward looking to get off of this ctor and on to the new loader model, it's not "important" right now.

var certBytes = File.ReadAllBytes(filePath);

var certContentType = X509Certificate2.GetCertContentType(certBytes);

if (certContentType is X509ContentType.Pkcs12)
{
return [X509CertificateLoader.LoadPkcs12(certBytes, password)];
}
else
{
if (password is not null)
{
_logger.LogDebug("Resource service certificate {FilePath} has type {Type} which does not support passwords, yet a password was configured. The certificate password will be ignored.", filePath, certContentType);
}

return [X509CertificateLoader.LoadCertificate(certBytes)];
}
}

X509CertificateCollection GetKeyStoreCertificate()
Expand Down