fix ConnectWithCertificateChain quic test #54026
Conversation
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsIt seems like ordering of the additionalCertificates X509Certificate2Collection is not quite predicable as I assumed. With this change we would always get peer certificate from PlatformCertificateHandle and we would add all extra certificates to ChainPolicy.ExtraStore. That may contain peer certificate it self but it does not matter as that is just hint and chain.Build will create the chain as needed. I aslo added ITestOutputHelper to the test so it is easier to collect useful information on test failures.
|
| @@ -345,27 +345,18 @@ private static uint HandleEventPeerCertificateReceived(State state, ref Connecti | |||
| { | |||
| unsafe | |||
| { | |||
| ReadOnlySpan<QuicBuffer> quicBuffer; | |||
| ReadOnlySpan<QuicBuffer> quicBuffer = new ReadOnlySpan<QuicBuffer>((void*)connectionEvent.Data.PeerCertificateReceived.PlatformCertificateHandle, sizeof(QuicBuffer)); | |||
| certificate = new X509Certificate2(new ReadOnlySpan<byte>(quicBuffer[0].Buffer, (int)quicBuffer[0].Length)); | |||
There was a problem hiding this comment.
We're guaranteed quicBuffer.Length > 0?
There was a problem hiding this comment.
Nevermind, I missed the sizeof(QuicBuffer) as the length above.
There was a problem hiding this comment.
BTW the relevant code is here:
https://github.com/microsoft/msquic/blob/5f9b5482f0fcabb970033cf72dfcde2f472f0262/src/platform/tls_openssl.c#L218-L228
It does not check for length but since certificate exist and i2d_X509 did not fail I assume the length is positive.
Similar for the chain aka pkcs7 blob.
ghost
locked as resolved and limited conversation to collaborators
It seems like ordering of the additionalCertificates X509Certificate2Collection is not quite predicable as I assumed.
With that the validation callback can get wrong certificate and/or incomplete chain.
With this change we would always get peer certificate from PlatformCertificateHandle and we would add all extra certificates to ChainPolicy.ExtraStore. That may contain peer certificate it self but it does not matter as that is just hint and chain.Build will create the chain as needed.
I aslo added ITestOutputHelper to the test so it is easier to collect useful information on test failures.