-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Use forTls parameter to enable TLS-compatible certificate key types while maintaining platform compatibility #119265
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
Conversation
Co-authored-by: rzikm <[email protected]>
… vs networking tests Co-authored-by: rzikm <[email protected]>
src/libraries/System.Net.Security/tests/FunctionalTests/NetworkingCertificateKeySizeTests.cs
Outdated
Show resolved
Hide resolved
@copilot instead of CRYPTO_TESTS directive, use NETWORKING_TESTS, and set it from relevant networking projects. Comment that increased size of the RSA key in networking test is required as some TLS implementations don't work with weaker keys. Also, there is no need to add tests. |
Switched to NETWORKING_TESTS directive instead of CRYPTO_TESTS as requested. Added the directive to System.Net.Security.Tests.csproj and removed it from System.Security.Cryptography.Tests.csproj. Updated the comment to explain that networking tests require larger RSA keys as some TLS implementations don't work with weaker keys. Test files have been removed as requested. Changes made in b6b5db8. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Co-authored-by: rzikm <[email protected]>
You also need to remove the explicit keyfactory so that random key type is being used in tests. runtime/src/libraries/Common/tests/System/Net/Configuration.Certificates.Dynamic.cs Line 185 in 44a6dd2
|
Co-authored-by: rzikm <[email protected]>
Removed the explicit keyFactory parameter from BuildPrivatePki call. Now uses the default random key type selection which respects the NETWORKING_TESTS conditional compilation for RSA key sizes. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
src/libraries/System.Net.Security/tests/FunctionalTests/System.Net.Security.Tests.csproj
Outdated
Show resolved
Hide resolved
src/libraries/Common/tests/System/Net/Configuration.Certificates.Dynamic.cs
Show resolved
Hide resolved
@copilot you need to pass the |
…se CertificateAuthority Co-authored-by: rzikm <[email protected]>
Added the NETWORKING_TESTS directive to the missing networking test projects: System.Net.Http.Functional.Tests, System.Net.Mail.Functional.Tests, and System.Net.Security.Unit.Tests in 45a2b47. |
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 change addresses performance vs security tradeoffs in test certificate generation by introducing a runtime parameter approach to select appropriate key types and sizes for different test contexts. The main goal is to enable TLS-compatible certificate key types while maintaining platform compatibility.
- Adds a
forTls
parameter to certificate generation methods to differentiate between TLS and crypto test contexts - Implements platform-specific key type filtering to exclude ML-DSA on Windows due to S/Channel incompatibility
- Uses 2048-bit RSA keys for TLS tests and maintains 1024-bit for crypto tests for performance
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
File | Description |
---|---|
CertificateAuthority.cs | Adds forTls parameter and BuildTlsVariantFactories() method for platform-aware key type selection |
Configuration.Certificates.Dynamic.cs | Updates networking certificate generation to use forTls: true instead of hardcoded RSA key size |
@rzikm There are untagged failures here. Since this is actually changing how some of these tests work it might be related. Can you take a look? ( |
This is weird, seems like Alpine and CentOS only are affected I will find out which cert type is that |
Seems like issue with the new PQC certs |
Got it, the OpenSSL provider does not advertise support for SLH-DSA-* family in its TLS-SIGALG capability, only ML-DSA I think we need to wait with testing these cert in TLS until they are advertised by the common OpenSSL provider. in the meanwhile, we can keep ML-DSA. |
This reverts commit 11d2ce2.
Disabled SLH-DSA for linux for now, Opened #119573 to track that (and so that the issue is discoverable if anyone hits it) |
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
Looks like maybe QUIC on Alpine/CentOS doesn't agree that ML-DSA works? |
Right, the libssl linked to MsQuic does not support even ML-DSA as the fork is pre-3.5.0 OpenSSL. This is rather unfortunate, because now we are in a territory where ML-DSA certs work with HTTP/1.1 and HTTP/2, but will fail with HTTP/3.... MsQuic recently did some effort to ditch the fork and use (dynamically-linked?) OpenSSL 3.5.0 but I don't know if that made into any releases, or whether it would work with .NET runtime, cc @ManickaP. |
It's in the main only and there is an open issue against it: microsoft/msquic#5210 - statically linking, including crypto - so even if it got released, we cannot consume it. |
/ba-g Test failure in System.Numerics.Tensors.Tests is unrelated |
@bartonjs Thanks a lot for helping with this. |
This change addresses the performance vs security tradeoff in dynamically generated test certificates by using a runtime parameter approach to select appropriate key types and sizes for TLS vs crypto test contexts while ensuring platform compatibility.
Problem
The shared
CertificateAuthority
class was using both:Configuration.Certificates.Dynamic.cs
While the hardcoded size provided fast test execution for cryptography tests, it was too small for networking tests where larger key sizes are needed for TLS compatibility. Additionally, the explicit key factory prevented the use of other key types (ECDsa, MLDsa, SlhDsa) that the test infrastructure supports. Furthermore, Windows S/Channel doesn't support ML-DSA certificates yet, creating platform compatibility issues.
Solution
Implemented a
forTls
parameter approach that differentiates between test contexts while enabling appropriate key type selection based on platform capabilities:forTls: true
): Use 2048-bit RSA keys and platform-appropriate key typesChanges
bool forTls = false
parameter toCertificateAuthority.BuildPrivatePki
methodss_tlsVariantKeyFactories
static field andBuildTlsVariantFactories()
methodBuildTlsVariantFactories
:forTls
parameterConfiguration.Certificates.Dynamic.cs
to useforTls: true
for networking certificate generationImplementation
The runtime selection logic uses the
forTls
parameter to choose between key factory arrays:The
BuildTlsVariantFactories
method ensures platform compatibility:This approach provides optimal performance for crypto tests, secure and compatible networking test certificates, and broader key type coverage while respecting platform limitations.
Addresses #116365.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.