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

Consider adding SslStream.Connect #63663

Open
wfurt opened this issue Jan 12, 2022 · 5 comments
Open

Consider adding SslStream.Connect #63663

wfurt opened this issue Jan 12, 2022 · 5 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net.Security
Milestone

Comments

@wfurt
Copy link
Member

wfurt commented Jan 12, 2022

Background and motivation

This is somewhat similar to #63162 but works at different level and somewhat inspired by Network.framework @filipnavara mentioned in #63162 (comment).

Typical use of SslStream now looks like:

   var clientSocket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
   clientSocket.Connect(listener.LocalEndPoint);
   var ns = new NetworkStream(clientSocket, ownsSocket: true)
   SslStream ssl = new SslStream(ns);
   ssl.AuthenticateAsClient(options, cancellationToken); 

While this is not terribly long but it forces user to deal with Sockets, AddressFamily and none of it is really necessary if all they want is encrypted safe channel.

With the current layering, some actions would be very difficult. For example, TCP Fast Open described in #1476 requires first chunk of data for connect and For TLS that would be the initial hello. Together with parallel connect and TLS 1.3 this could be big win for anybody who cares about initial connect latency.

Another example may be kernel TLS https://people.freebsd.org/~gallatin/talks/euro2019-ktls.pdf
While this is not available in Linux (yet?), this allows to do handshake in user mode but then switch to HW accelerated encryption for bulk data. If SslStream "owns" the underlying transport, we would be able to explore options like this.

Proposed API

namespace System.Net.Security
{
    [Flags]
    public enum ConnectOptions
    {
       Default = 0,
       IPv4Only,
       IPv6Only,
       Parallel, // Happy-Eyeball
       FastOpen,
       ...
    }
    // New class
    public class SslStream
    {
        public Stream InnerStream;
        public static Task<SslStream> ConnectAsync(string target, int port, ConnectOptions connectOptions, SslClientAuthenticationOptions sslClientAuthenticationOptions, cancellationToken cancellationToken = default);
        public static SslStream Connect(string target, int port, ConnectOptions connectOptions, SslClientAuthenticationOptions sslClientAuthenticationOptions);
    }
}

Now, aside from the FastOption option to decrease latency, it would be great if the rest comes from underlying networking code.
However, since this is high level convenience API, I think it is ok if it does not e.g. we leave Socket as simple low-level primitive and we either add it to #63162 or we would keep it here to match Network.Framework.

I added `InnerStream' property so there is some access to underlying transport. That can be useful (aside from this) for audit logs.

Alternative Designs

I'm wondering if this could be merged with #63162 to something like

 public static Task<Stream> ConnectAsync(string target, int port, ConnectOptions connectOptions = ConnectOptions.Default, SslClientAuthenticationOptions? sslClientAuthenticationOptions = null, cancellationToken cancellationToken = default);

and negotiate TLS if sslClientAuthenticationOptions is set.

@wfurt wfurt added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net.Security labels Jan 12, 2022
@ghost
Copy link

ghost commented Jan 12, 2022

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

Issue Details

Background and motivation

This is somewhat similar to #63162 but works at different level and somewhat inspired by Network.framework @filipnavara mentioned in #63162 (comment).

Typical use of SslStream now looks like:

   var clientSocket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
   clientSocket.Connect(listener.LocalEndPoint);
   var ns = new NetworkStream(clientSocket, ownsSocket: true)
   SslStream ssl = new SslStream(ns);
   ssl.AuthenticateAsClient(options, cancellationToken); 

While this is not terribly long but it forces user to deal with Sockets, AddressFamily and none of it is really necessary if all they want is encrypted safe channel.

With the current layering, some actions would be very difficult. For example, TCP Fast Open described in #1476 requires first chunk of data for connect and For TLS that would be the initial hello. Together with parallel connect and TLS 1.3 this could be big win for anybody who cares about initial connect latency.

Another example may be kernel TLS https://people.freebsd.org/~gallatin/talks/euro2019-ktls.pdf
While this is not available in Linux (yet?), this allows to do handshake in user mode but then switch to HW accelerated encryption for bulk data. If SslStream "owns" the underlying transport, we would be able to explore options like this.

Proposed API

namespace System.Net.Security
{
    [Flags]
    public enum ConnectOptions
    {
       Default = 0,
       IPv4Only,
       IPv6Only,
       Parallel, // Happy-Eyeball
       FastOpen,
       ...
    }
    // New class
    public class SslStream
    {
        public Stream InnerStream;
        public static Task<SslStream> ConnectAsync(string target, int port, ConnectOptions connectOptions, SslClientAuthenticationOptions sslClientAuthenticationOptions, cancellationToken cancellationToken = default);
        public static SslStream Connect(string target, int port, ConnectOptions connectOptions, SslClientAuthenticationOptions sslClientAuthenticationOptions);
    }
}

Now, aside from the FastOption option to decrease latency, it would be great if the rest comes from underlying networking code.
However, since this is high level convenience API, I think it is ok if it does not e.g. we leave Socket as simple low-level primitive and we either add it to #63162 or we would keep it here to match Network.Framework.

I added `InnerStream' property so there is some access to underlying transport. That can be useful (aside from this) for audit logs.

Alternative Designs

I'm wondering if this could be merged with #63162 to something like

 public static Task<Stream> ConnectAsync(string target, int port, ConnectOptions connectOptions = ConnectOptions.Default, SslClientAuthenticationOptions? sslClientAuthenticationOptions = null, cancellationToken cancellationToken = default);

and negotiate TLS if sslClientAuthenticationOptions is set.

Author: wfurt
Assignees: -
Labels:

api-suggestion, area-System.Net.Security

Milestone: -

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Jan 12, 2022
@wfurt
Copy link
Member Author

wfurt commented Jan 12, 2022

Actually, OpenSSL has SSL_OP_ENABLE_KTLS and reference to Linux 4.20. So Kernel TLS should be possible. (we should investigate separately)

@filipnavara
Copy link
Member

Generally looks reasonable although I didn't check all the nooks and crannies. One question that I would love to have answered though is whether this API provides all the features necessary for using it in SocketsHttpHandler.

@geoffkizer
Copy link
Contributor

I agree with the direction here.

I think it's worth considering something like this just for simplicity and ease of use. But also, fast open and kernel TLS are good justification as well.

I wonder if we want a similar Listen/Accept API for acting as an SSL server.

If we decide to do something here, we should probably do it in conjunction with #63162 so that we end up with a consistent story here.

@wfurt
Copy link
Member Author

wfurt commented Jan 12, 2022

Possibly in

private async ValueTask<(Socket?, Stream, TransportContext?)> ConnectAsync(HttpRequestMessage request, bool async, CancellationToken cancellationToken)

Tricky with custom streams.

@karelz karelz removed the untriaged New issue has not been triaged by the area owner label Jan 13, 2022
@karelz karelz added this to the Future milestone Jan 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net.Security
Projects
None yet
Development

No branches or pull requests

4 participants