.NET 10 build fixes #4636

pepone · 2025-11-06T12:49:36Z

This PR updates C# sources to compile with .NET 10 (tested with RC2).

Copilot

Pull Request Overview

This PR modernizes SSL/TLS certificate handling in the C# Ice implementation by migrating from the deprecated X509Certificate2 constructor to the recommended X509CertificateLoader API. The changes improve code quality by using the modern .NET cryptography APIs and streamline certificate loading patterns.

Key changes:

Replaced X509Certificate2 constructor calls with X509CertificateLoader.LoadPkcs12FromFile() and X509CertificateLoader.LoadCertificateFromFile() methods
Updated cipher information retrieval to use NegotiatedCipherSuite instead of deprecated CipherAlgorithm property
Added Microsoft.Bcl.Cryptography package dependency for .NET 8 to support X509CertificateLoader

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
csharp/src/Ice/Ice.csproj	Added conditional package reference to Microsoft.Bcl.Cryptography for .NET 8 support
csharp/test/IceSSL/configuration/msbuild/client/client.csproj	Added conditional package reference to Microsoft.Bcl.Cryptography for .NET 8 test client
csharp/src/Ice/SSL/SSLEngine.cs	Migrated certificate loading from X509Certificate2 constructor to X509CertificateLoader API, updated cipher suite logging, and modernized StoreName parsing
csharp/src/Ice/SSL/TransceiverI.cs	Replaced CipherAlgorithm with NegotiatedCipherSuite for cipher information
csharp/test/IceSSL/configuration/PlatformTests.cs	Updated all test certificate loading to use X509CertificateLoader
csharp/test/IceSSL/configuration/AllTests.cs	Updated all test certificate loading to use X509CertificateLoader
csharp/src/Ice/UtilInternal/StringUtil.cs	Simplified string comparison using range syntax
csharp/src/iceboxnet/Server.cs	Changed Server class visibility from public to internal
csharp/msbuild/CodeAnalysis.Tests.globalconfig	Added suppression for CA1515 rule in tests
csharp/msbuild/CodeAnalysis.Src.globalconfig	Removed CA1515 suppression from source code

Comments suppressed due to low confidence (1)

csharp/src/Ice/SSL/SSLEngine.cs:504

The createSecureString method is no longer used after migrating to X509CertificateLoader.LoadPkcs12FromFile. This unused private method should be removed to improve code maintainability.

    private static SecureString createSecureString(string s)
    {
        var result = new SecureString();
        foreach (char ch in s)
        {
            result.AppendChar(ch);
        }
        return result;
    }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-06T12:53:50Z

csharp/test/IceSSL/configuration/PlatformTests.cs

-        using var serverCertificate =
-            new X509Certificate2(Path.Combine(certificatesPath, "ca1/server.p12"), "password");
+        using X509Certificate2 serverCertificate =
+            X509CertificateLoader.LoadPkcs12FromFile(Path.Combine(certificatesPath, "ca1/server.p12"), "password");


Call to 'System.IO.Path.Combine'.

Suggested change

X509CertificateLoader.LoadPkcs12FromFile(Path.Combine(certificatesPath, "ca1/server.p12"), "password");

X509CertificateLoader.LoadPkcs12FromFile(Path.Combine(certificatesPath, "ca1", "server.p12"), "password");

Sounds correct.

Copilot · 2025-11-06T12:53:50Z

csharp/test/IceSSL/configuration/PlatformTests.cs

-        using var serverCertificate =
-            new X509Certificate2(Path.Combine(certificatesPath, "ca1/server.p12"), "password");
+        using X509Certificate2 serverCertificate =
+            X509CertificateLoader.LoadPkcs12FromFile(Path.Combine(certificatesPath, "ca1/server.p12"), "password");


Call to 'System.IO.Path.Combine'.

Suggested change

X509CertificateLoader.LoadPkcs12FromFile(Path.Combine(certificatesPath, "ca1/server.p12"), "password");

X509CertificateLoader.LoadPkcs12FromFile(Path.Combine(certificatesPath, "ca1", "server.p12"), "password");

Copilot · 2025-11-06T12:53:51Z

csharp/test/IceSSL/configuration/PlatformTests.cs

-        using var serverCertificate =
-            new X509Certificate2(Path.Combine(certificatesPath, "ca1/server.p12"), "password");
+        using X509Certificate2 serverCertificate =
+            X509CertificateLoader.LoadPkcs12FromFile(Path.Combine(certificatesPath, "ca1/server.p12"), "password");


Call to 'System.IO.Path.Combine'.

Copilot · 2025-11-06T12:53:51Z

csharp/test/IceSSL/configuration/PlatformTests.cs

-        using var clientCertificate =
-            new X509Certificate2(Path.Combine(certificatesPath, "ca1/client.p12"), "password");
+        using X509Certificate2 serverCertificate =
+            X509CertificateLoader.LoadPkcs12FromFile(Path.Combine(certificatesPath, "ca1/server.p12"), "password");


Call to 'System.IO.Path.Combine'.

Copilot · 2025-11-06T12:53:51Z

csharp/test/IceSSL/configuration/PlatformTests.cs

+        using X509Certificate2 serverCertificate =
+            X509CertificateLoader.LoadPkcs12FromFile(Path.Combine(certificatesPath, "ca1/server.p12"), "password");
+        using X509Certificate2 clientCertificate =
+            X509CertificateLoader.LoadPkcs12FromFile(Path.Combine(certificatesPath, "ca1/client.p12"), "password");


Call to 'System.IO.Path.Combine'.

Copilot · 2025-11-06T12:53:52Z

csharp/test/IceSSL/configuration/PlatformTests.cs

+        using X509Certificate2 serverCertificate =
+            X509CertificateLoader.LoadPkcs12FromFile(Path.Combine(certificatesPath, "ca1/server.p12"), "password");
+        using X509Certificate2 clientCertificate =
+            X509CertificateLoader.LoadPkcs12FromFile(Path.Combine(certificatesPath, "ca1/client.p12"), "password");


Call to 'System.IO.Path.Combine'.

Copilot · 2025-11-06T12:53:52Z

csharp/test/IceSSL/configuration/PlatformTests.cs

-        using var serverCertificate = new X509Certificate2(Path.Combine(certificatesPath, "ca1/server.p12"), "password");
-        using var clientCertificate = new X509Certificate2(Path.Combine(certificatesPath, "ca1/client.p12"), "password");
+        using X509Certificate2 serverCertificate =
+            X509CertificateLoader.LoadPkcs12FromFile(Path.Combine(certificatesPath, "ca1/server.p12"), "password");


Call to 'System.IO.Path.Combine'.

Copilot · 2025-11-06T12:53:52Z

csharp/test/IceSSL/configuration/PlatformTests.cs

+        using X509Certificate2 serverCertificate =
+            X509CertificateLoader.LoadPkcs12FromFile(Path.Combine(certificatesPath, "ca1/server.p12"), "password");
+        using X509Certificate2 clientCertificate =
+            X509CertificateLoader.LoadPkcs12FromFile(Path.Combine(certificatesPath, "ca1/client.p12"), "password");


Call to 'System.IO.Path.Combine'.

Copilot · 2025-11-06T12:53:53Z

csharp/test/IceSSL/configuration/PlatformTests.cs

-        using var trustedRootCertificatesCA1 = new X509Certificate2(Path.Combine(certificatesPath, "ca1/ca1_cert.pem"));
-        using var trustedRootCertificatesCA2 = new X509Certificate2(Path.Combine(certificatesPath, "ca2/ca2_cert.pem"));
+        using X509Certificate2 trustedRootCertificatesCA1 =
+            X509CertificateLoader.LoadCertificateFromFile(Path.Combine(certificatesPath, "ca1/ca1_cert.pem"));


Call to 'System.IO.Path.Combine'.

Copilot · 2025-11-06T12:53:53Z

csharp/test/IceSSL/configuration/PlatformTests.cs

+        using X509Certificate2 trustedRootCertificatesCA1 =
+            X509CertificateLoader.LoadCertificateFromFile(Path.Combine(certificatesPath, "ca1/ca1_cert.pem"));
+        using X509Certificate2 trustedRootCertificatesCA2 =
+            X509CertificateLoader.LoadCertificateFromFile(Path.Combine(certificatesPath, "ca2/ca2_cert.pem"));


Call to 'System.IO.Path.Combine'.

Suggested change

X509CertificateLoader.LoadCertificateFromFile(Path.Combine(certificatesPath, "ca2/ca2_cert.pem"));

X509CertificateLoader.LoadCertificateFromFile(Path.Combine(certificatesPath, "ca2", "ca2_cert.pem"));

pepone · 2025-11-06T17:30:09Z

@externl can you take another look? I updated CI to run tests with .NET 10 too

externl · 2025-11-06T18:16:27Z

.github/workflows/ci.yml

      - name: Setup .NET
        uses: ./.github/actions/setup-dotnet

+      - uses: actions/setup-dotnet@v4


We already have setup-botnet above, we should integrate theses settings into it

Not sure we only need this for .NET 10 tests, the other we run in a few more places.

What do you suggest add an extra parameter to setup-dotnet?

externl · 2025-11-07T16:01:03Z

.github/actions/setup-dotnet/action.yml

Do we need both .NET 8 and .NET 10 for these builds?

Yes, Ice assemblies always target .NET 8, only the tests are built with .NET 10 target framework.

bernardnormier

The dependency on a 9.0.* Microsoft Bcl library when using .NET 8 sounds a bit odd, but I guess that's the recommended way to write code that works with both .NET 8 and .NET 10?

bernardnormier · 2025-11-11T15:16:23Z

csharp/msbuild/CodeAnalysis.Tests.globalconfig

 # CA2008: Do not create tasks without passing a TaskScheduler
 dotnet_diagnostic.CA2008.severity = none
+
+# CA1515: Because an application's API isn't typically referenced from outside the assembly, types can be made internal


You moved this rule because you were getting this warning for tests too?

Yes with .NET 10 this is enabled by default, I want to fix this in a separate PR.

bernardnormier · 2025-11-11T15:21:33Z

csharp/test/IceSSL/configuration/PlatformTests.cs

-        using var serverCertificate =
-            new X509Certificate2(Path.Combine(certificatesPath, "ca1/server.p12"), "password");
+        using X509Certificate2 serverCertificate =
+            X509CertificateLoader.LoadPkcs12FromFile(Path.Combine(certificatesPath, "ca1/server.p12"), "password");


Sounds correct.

pepone · 2025-11-11T15:30:37Z

write code that works with both .NET 8 and .NET 10?

Yes, this allow to use the new API and still target .NET 8. We can also keep the old code a disable the deprecation warnings when targeting .NET 9+ in the corresponding files.

I think using the new dependency is clear.

.NET 10 build fixes

e4a84f0

pepone requested review from bernardnormier, Copilot and externl November 6, 2025 12:49

Copilot AI reviewed Nov 6, 2025

View reviewed changes

Fix for password less certificate

655ed63

externl approved these changes Nov 6, 2025

View reviewed changes

pepone added 3 commits November 6, 2025 16:43

Run tests with .NET 10

5c69630

Review fix

8456b0f

Fix Windows build failure

e83d6a4

pepone requested a review from externl November 6, 2025 17:29

Fix windows build

23b0c59

externl reviewed Nov 6, 2025

View reviewed changes

pepone added 4 commits November 7, 2025 10:15

Use dotnet msbuild for C# .NET 10 build on Windows

fd97ddf

pass msbuild command to build action

4d6eb51

Update setup-dotnet to include .NET 10 option

800641c

Remove dead code

c76c8e3

externl reviewed Nov 7, 2025

View reviewed changes

bernardnormier approved these changes Nov 11, 2025

View reviewed changes

	X509CertificateLoader.LoadPkcs12FromFile(Path.Combine(certificatesPath, "ca1/server.p12"), "password");
	X509CertificateLoader.LoadPkcs12FromFile(Path.Combine(certificatesPath, "ca1", "server.p12"), "password");

	X509CertificateLoader.LoadCertificateFromFile(Path.Combine(certificatesPath, "ca2/ca2_cert.pem"));
	X509CertificateLoader.LoadCertificateFromFile(Path.Combine(certificatesPath, "ca2", "ca2_cert.pem"));

.NET 10 build fixes #4636

Are you sure you want to change the base?

.NET 10 build fixes #4636

Conversation

pepone commented Nov 6, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

pepone commented Nov 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bernardnormier left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pepone commented Nov 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants