Skip to content

Use the cancellation token in AuthenticateAsServerAsync - #15426

Merged
davidfowl merged 1 commit into
masterfrom
davidfowl/https-token
Oct 29, 2019
Merged

Use the cancellation token in AuthenticateAsServerAsync#15426
davidfowl merged 1 commit into
masterfrom
davidfowl/https-token

Conversation

@davidfowl

@davidfowl davidfowl commented Oct 26, 2019

Copy link
Copy Markdown
Member

Instead of aborting the connection, just handle the OCE (which already happens)

_options.OnAuthenticate?.Invoke(context, sslOptions);

await sslStream.AuthenticateAsServerAsync(sslOptions, CancellationToken.None);
await sslStream.AuthenticateAsServerAsync(sslOptions, cancellationTokeSource.Token);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't we remove this because it didn't work well?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep in .net Framework and early versions of core. It was fixed in 2.1 or so but we never tried to use it

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. It didn't work at all perviously. It does make me nervous that we're not explicitly aborting the underlying connection at all anymore with this PR. This means if there's any point in the handshake where SslStream isn't correctly observing the cancellation token, the handshake could hang indefinitely.

@stephentoub Do you think it's safe to rely on cancelling AuthenticateAsServerAsync with a token, or should we continue to abort the underlying connection as well?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it's safe to rely on cancelling AuthenticateAsServerAsync with a token, or should we continue to abort the underlying connection as well?

It should work reliably (as of dotnet/corefx#37259 in 3.0). If you find problems I'd like us to fix it asap.

@davidfowl davidfowl added this to the 5.0.0-preview1 milestone Oct 28, 2019
@davidfowl
davidfowl merged commit b0d6b0e into master Oct 29, 2019
@davidfowl
davidfowl deleted the davidfowl/https-token branch October 29, 2019 18:08
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants