-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
H/3 Server Cert validation callback exception fix #55526
Merged
ManickaP
merged 2 commits into
dotnet:main
from
ManickaP:mapichov/55192_cert_callback_fix
Jul 13, 2021
Merged
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -104,8 +104,22 @@ private static async ValueTask<SslStream> EstablishSslConnectionAsyncCore(bool a | |
[SupportedOSPlatform("windows")] | ||
[SupportedOSPlatform("linux")] | ||
[SupportedOSPlatform("macos")] | ||
public static async ValueTask<QuicConnection> ConnectQuicAsync(QuicImplementationProvider quicImplementationProvider, DnsEndPoint endPoint, SslClientAuthenticationOptions? clientAuthenticationOptions, CancellationToken cancellationToken) | ||
public static async ValueTask<QuicConnection> ConnectQuicAsync(HttpRequestMessage request, QuicImplementationProvider quicImplementationProvider, DnsEndPoint endPoint, SslClientAuthenticationOptions? clientAuthenticationOptions, CancellationToken cancellationToken) | ||
{ | ||
// If there's a cert validation callback, and if it came from HttpClientHandler, | ||
// wrap the original delegate in order to change the sender to be the request message (expected by HttpClientHandler's delegate). | ||
RemoteCertificateValidationCallback? callback = clientAuthenticationOptions?.RemoteCertificateValidationCallback; | ||
if (callback != null && callback.Target is CertificateCallbackMapper mapper) | ||
{ | ||
clientAuthenticationOptions = clientAuthenticationOptions!.ShallowClone(); // Clone as we're about to mutate it and don't want to affect the cached copy | ||
Func<HttpRequestMessage, X509Certificate2?, X509Chain?, SslPolicyErrors, bool> localFromHttpClientHandler = mapper.FromHttpClientHandler; | ||
clientAuthenticationOptions.RemoteCertificateValidationCallback = (object sender, X509Certificate? certificate, X509Chain? chain, SslPolicyErrors sslPolicyErrors) => | ||
{ | ||
bool result = localFromHttpClientHandler(request, certificate as X509Certificate2, chain, sslPolicyErrors); | ||
return result; | ||
}; | ||
} | ||
|
||
QuicConnection con = new QuicConnection(quicImplementationProvider, endPoint, clientAuthenticationOptions); | ||
try | ||
{ | ||
|
@@ -117,6 +131,17 @@ public static async ValueTask<QuicConnection> ConnectQuicAsync(QuicImplementatio | |
con.Dispose(); | ||
throw CreateWrappedException(ex, endPoint.Host, endPoint.Port, cancellationToken); | ||
} | ||
finally | ||
{ | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, is this why you duplicated the code? |
||
// So to ensure the callback don't keep the first HttpRequestMessage alive indefinitely, we reset the callback back to user's provided one. | ||
clientAuthenticationOptions.RemoteCertificateValidationCallback = callback; | ||
} | ||
|
||
} | ||
} | ||
|
||
internal static Exception CreateWrappedException(Exception error, string host, int port, CancellationToken cancellationToken) | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's something I was not aware of and I'll probably bring the
localRequest
back, thanks.