Minimal changes to support certificate chain-preloading at startup #24934
Conversation
davidfowl
requested review from
halter73,
jkotalik and
Tratcher
as code owners
ghost
added
the
| @@ -68,6 +68,13 @@ internal class SniOptionsSelector | |||
| } | |||
| } | |||
|
|
|||
| if (sslOptions.ServerCertificate != null) | |||
| { | |||
| // This might be do blocking IO but it'll resolve the certificate chain up front before any connections are | |||
There was a problem hiding this comment.
| // This might be do blocking IO but it'll resolve the certificate chain up front before any connections are | |
| // This might do blocking IO but it'll resolve the certificate chain up front before any connections are |
| @@ -761,7 +760,7 @@ public X509Certificate2 LoadCertificate(CertificateConfig certInfo, string endpo | |||
| return null; | |||
| } | |||
|
|
|||
| var cert = new X509Certificate2(); | |||
| var cert = TestResources.GetTestCertificate(); | |||
There was a problem hiding this comment.
@halter73 introduced IsTestMock because he wasn't using a real cert here. Something about not wanting to create a large number of real certs.
There was a problem hiding this comment.
Is that a real problem?
There was a problem hiding this comment.
When @halter73 is back, if the test haven't exploded by then can give me some feedback.
There was a problem hiding this comment.
I'd recommend storing the test cert in a static like we do for some other tests and removing IsTestMock since there's no longer an issue with this returning invalid certs. I guess it's no big deal if it isn't causing any slowness or reliability issues though.
There was a problem hiding this comment.
The second PR does it. I'll do the work in there.
amcasey
added
area-networking
This is a pre-cursor to supporting preloaded certificate chains in configuration. We resolve the certificate context outside of the connection establishment phase.
Contributes to #21512