Skip to content
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

Update SslStream to implement DuplexStream #51431

Closed
scalablecory opened this issue Apr 17, 2021 · 6 comments
Closed

Update SslStream to implement DuplexStream #51431

scalablecory opened this issue Apr 17, 2021 · 6 comments
Labels
api-approved API was approved in API review, it can be implemented area-System.Net.Security
Milestone

Comments

@scalablecory
Copy link
Contributor

We think TLS probably support half-closed connections. If so, it should be updated to derive from DuplexStream. API already approved in #43290.

@scalablecory scalablecory added this to the 6.0.0 milestone Apr 17, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Apr 17, 2021
@ghost
Copy link

ghost commented Apr 17, 2021

Tagging subscribers to this area: @dotnet/ncl, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

We think TLS probably support half-closed connections. If so, it should be updated to derive from DuplexStream. API already approved in #43290.

Author: scalablecory
Assignees: -
Labels:

area-System.Net.Security

Milestone: 6.0.0

@scalablecory scalablecory added the api-approved API was approved in API review, it can be implemented label Apr 17, 2021
@wfurt
Copy link
Member

wfurt commented Apr 20, 2021

This may be somewhat tricky. While user may not want to send any data, there still may be CloseNotification, other alerts or protocol responses to requests from other side. We could possibly derive from DuplexStream but I personally don't see big value.

@scalablecory
Copy link
Contributor Author

This may be somewhat tricky. While user may not want to send any data, there still may be CloseNotification, other alerts or protocol responses to requests from other side. We could possibly derive from DuplexStream but I personally don't see big value.

Note we would be shutting down TLS, not the socket, so we could still send any other alerts that TLS allows.

@scalablecory
Copy link
Contributor Author

scalablecory commented Apr 21, 2021

This may still be tricky. It looks like shutdown behavior is only supported in TLS 1.3:

https://tools.ietf.org/html/rfc8446#section-6

The
"close_notify" alert is used to indicate orderly closure of one
direction of the connection. Upon receiving such an alert, the TLS
implementation SHOULD indicate end-of-data to the application.

https://tools.ietf.org/html/rfc8446#section-6.1

Each party MUST send a "close_notify" alert before closing its write
side of the connection, unless it has already sent some error alert.
This does not have any effect on its read side of the connection.
Note that this is a change from versions of TLS prior to TLS 1.3 in
which implementations were required to react to a "close_notify" by
discarding pending writes and sending an immediate "close_notify"
alert of their own. That previous requirement could cause truncation
in the read side. Both parties need not wait to receive a
"close_notify" alert before closing their read side of the
connection, though doing so would introduce the possibility of
truncation.

I'm talking to Schannel folks to see if this is supported.

Also, we can not implement this only on SslStream but would need to add it to AuthenticatedStream. I'm still looking into if NegotiateStream supports this behavior.

@ManickaP ManickaP removed the untriaged New issue has not been triaged by the area owner label Apr 22, 2021
@ManickaP
Copy link
Member

Triage: we either implement this or clos this issue for 6.0. It seems like this would usable only for TLS 1.3 atm.

@karelz
Copy link
Member

karelz commented May 4, 2021

DuplexStream won't be part of 6.0. Closing.

@karelz karelz closed this as completed May 4, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Net.Security
Projects
None yet
Development

No branches or pull requests

4 participants