H/3 Server Cert validation callback exception fix #55526
Conversation
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsThis is an easy fix for the problem. It solves the exception encountered in the issue, but it doesn't solve it properly for all possible callbacks. In case the callback is registered through This is because we're receiving the event on connection where we don't have any way to pair it with the request that triggered this ATM. I assume, it could be possible, but it'll need more digging and potentially a change in msquic (@wfurt). If this is something that could unblock you @JamesNK, we can merge this "quick fix". If not and you really need the request in the callback, this might take a bit longer. I'm not closing the issue, to track the proper solution to this.
|
|
|
ManickaP
force-pushed
the
mapichov/55192_cert_callback_fix
branch
from
1118449
to
0c78487
Compare
ManickaP
force-pushed
the
mapichov/55192_cert_callback_fix
branch
from
0c78487
to
f867d19
Compare
| bool result = localFromHttpClientHandler(request, certificate as X509Certificate2, chain, sslPolicyErrors); | ||
| return result; | ||
| }; | ||
| } |
There was a problem hiding this comment.
This looks like an almost duplicate of https://github.com/dotnet/runtime/pull/55526/files#diff-9eb150e0af6ce559b3ef3e99949ba3512594fe4c55fbf10e6b739d315899d469R40-R57. Why are we duplicating the code?
It also looks like as part of copying/pasting that here, you removed the localRequest... why?
There was a problem hiding this comment.
For msquic, the callback gets called multiple times when the connection is established, so I couldn't unset it in the callback itself, but after the connection is opened.
So I moved the "unsetting" to finally block, where I discard the whole custom callback wrapper and put back the original, thus removing reference to the request.
There was a problem hiding this comment.
Also there's a similar issue with SslStream as well: #55334
I plan to put up an additional PR with the similar fix, where I could unify the logic and remove the code duplication. Unless there's something fundamentally wrong with the way I unset the callback and release the hold of the request.
There was a problem hiding this comment.
Ok. Note the localRequest was also there as an optimization. Right now you're paying for the closure even if there is no callback.
There was a problem hiding this comment.
That's something I was not aware of and I'll probably bring the localRequest back, thanks.
| if (clientAuthenticationOptions is not null && callback is not null) | ||
| { | ||
| // We cannot unset the request closure variable after the first callaback invocation as we do with SslStream | ||
| // since the callback will get triggered multiple times during connection establishment. |
There was a problem hiding this comment.
Ah, is this why you duplicated the code?
|
The underlying issue is tracked by microsoft/msquic#1810 We could put workaround to MsQuicConnection and ignore additional validation callbacks. Since TLS 13 does not support renegotiation there should never be case when we need to do validation more than once. |
|
@wfurt will do as you suggest, if feasible. If not, I'll push this through as is. |
…oved code duplication
ManickaP
force-pushed
the
mapichov/55192_cert_callback_fix
branch
from
ba48796
to
6857e7a
Compare
|
In the end, I prevented |
ghost
locked as resolved and limited conversation to collaborators
Sets and unsets
HttpMessageRequestto the cert callback when opening newQuicConnection.Fixes #55192