Add support for X509ChainPolicy for QUIC #73056
Conversation
ghost
assigned 
|
Tagging subscribers to this area: @dotnet/ncl |
rzikm
changed the title
|
Test failures are unrelated |
There was a problem hiding this comment.
I think we should add also http test - to make sure we can catch missing entry in clone fixed by #72326. In that case we can also ensure consistency between http/3 and prior versions e.g. SslStream vs Quic.
It may be nice to take GenerateCertificates from System.Net.Security and move it to Common place.
| /// </summary> | ||
| private readonly X509ChainPolicy? _chainPolicy; | ||
|
|
||
| public SslConnectionOptions(QuicConnection connection, bool isClient, string? targetHost, bool certificateRequired, X509RevocationMode revocationMode, RemoteCertificateValidationCallback? validationCallback, X509ChainPolicy? chainPolicy) |
There was a problem hiding this comment.
nit: as this gets longer and longer it may be nice to split it to multiple lines.
| { | ||
| _connection = connection; | ||
| _isClient = isClient; | ||
| _targetHost = targetHost; | ||
| _certificateRequired = certificateRequired; | ||
| _revocationMode = revocationMode; | ||
| _validationCallback = validationCallback; | ||
| _chainPolicy = chainPolicy; |
There was a problem hiding this comment.
nit: I would probably name it _certificateChainPolicy to match the ssl options.
|
BTW it seems like the changes to partial chain broke |
Should I disable the tests for now or revert the change in SslStream tests? I don't have access to my Mac machine to debug it this week and I would prefer not to hold this change. |
I think that would be fine to separate the changes. I can sync & build your branch locally to see if this is something obvious and consistent or if this is some issue with CI machines or race condition. |
Closes #72873.
This PR adds support for Ssl(Server|Client)AuthenticationOptions.CertificateChainPolicy for QUIC. It essentially mirrors what #71961 did for SslStream.