use ROSpan in SslStreamCertificateContext #83485
Conversation
|
Tagging subscribers to this area: @dotnet/ncl, @vcsjones Issue Detailscontributes to #67552 and #72226.
|
| foreach (X509Certificate2 ca in intermediates) | ||
| { | ||
| collection.Add(ca); | ||
| } |
There was a problem hiding this comment.
This could be slower if we expected a lot of certs. I assume we don't?
There was a problem hiding this comment.
typically this would be 1-3 items. I was looking at #1530 but it did not seems like I can use the AddRage for generic collection. We actually allocate it just to make the export easy e.g. produce the Pkcs12 blob. If needed we may suck in some crypto primitives directly.
There was a problem hiding this comment.
Should be fine as you have it.
There was a problem hiding this comment.
Have we considered adding AddRange(RoS) overload into X509Certificate2Collection if we're introducing RoS type on SslStreamCertificateContext?
There was a problem hiding this comment.
I'm not sure that would make measurable improvement. There was some discussion on the issue linked above.
If anything I would push to avoid allocation of the collection.
| @@ -85,7 +85,7 @@ public static MsQuicSafeHandle Create(QuicServerConnectionOptions options, strin | |||
| } | |||
|
|
|||
| X509Certificate? certificate = null; | |||
| X509Certificate[]? intermediates = null; | |||
| ReadOnlySpan<X509Certificate2> intermediates = ReadOnlySpan<X509Certificate2>.Empty; | |||
There was a problem hiding this comment.
Can you also switch certificate to X509Certificate2? I don't like the asymmetry here, it think we should decide and stick with one type.
Also would default look better than ReadOnlySpan<X509Certificate2>.Empty ?
There was a problem hiding this comment.
We should prefer X509Certificate2 where possible.
There was a problem hiding this comment.
that one is tricky. Several of the underlying types in callbacks is X509Certificate for historical reasons. So we are possibly up to casting and cast exceptions. I'm not sure if it is worth it since all we do is serialization to MsQuic.
There was a problem hiding this comment.
BTW as far as the default I have no stone preference. The Empty span seems more explicit but either one should work
src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.cs
Show resolved
Hide resolved
src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.cs
Outdated
Show resolved
Hide resolved
| foreach (X509Certificate2 ca in intermediates) | ||
| { | ||
| collection.Add(ca); | ||
| } |
There was a problem hiding this comment.
Have we considered adding AddRange(RoS) overload into X509Certificate2Collection if we're introducing RoS type on SslStreamCertificateContext?
|
I will merge this as work-in-progress. There will be opportunity to do more touches if/when #72226 is approved. |
ghost
locked as resolved and limited conversation to collaborators
contributes to #67552 and #72226.
If we want to make it public, we should probably make the intermediate certificates immutable.
This change does it to make sure it would work OK.
Since the API is not really public I would probably move forward as preparation.