Feature request: SecureSocket.secure() should support non-_Socket Sockets

[GreenAppers]

The _ in _detachRaw precludes using SecureSocket with a Socket implemented outside of dart:io:
https://github.com/dart-lang/sdk/blob/master/sdk/lib/io/secure_socket.dart#L116

I'm working on a Dart SSH package that implements Socket over an SSH tunnel: https://github.com/GreenAppers/dartssh/blob/master/lib/socket_io.dart#L110

I would like to be able to make SSL connections over an SSH tunnel using SecureSocket, but this is not currently possible.

I think simply renaming _detachRaw to detachRaw would be one fix.

However perhaps a better fix would be to expose the _Socket constructor. Something like Socket socketFromRaw(RawSocket raw) => _Socket(raw); And then I would implement RawSocket instead of Socket.

Perhaps someone will recommended a better solution. (If it turns out the SSL implementation under SecureSocket only supports a native file descriptor, the fix would be more involved)

In the mean time, I will test the fixes I've proposed and update the bug. Thanks!

[a-siva]

/cc @zichangg

[GreenAppers]

I've confirmed that the first proposed fix does indeed work!

Simply renaming the 3 occurrences in the SDK of _detachRaw to detachRaw allows using SecureSocket for https and wss connections over an SSH tunnel.

There are probably a variety of ways to add support for this feature. I'm happy to send a PR. Thanks!

[sortie]

Hi @GreenAppers

Unfortunately adding detachRaw to the public API of Socket (if it makes sense to expose it) would be a breaking change for implementations of Socket per our breaking change policy. Breaking changes should have a certain importance to be worth the breakage, and I doubt this case would be worth it.

Yes, what you propose isn't a breaking change and technically lets you sneak a _detachRaw into your own Socket implementation that would get used, but it's not a great design. Indeed the bug seems to be SecureSocket.secure accepting a Socket but secretly expecting a _Socket to do the detach dance.

I think the main question is whether the detach can be done using just a plain Socket, or whether a reasonable fallback can be implemented if the Socket is not a _Socket. That would be the best solution in my opinion.

Adding a constructor to Socket from RawSocket could potentially work too and wouldn't be a breaking change.

@lrhn What do you thnink?

Jonas

[GreenAppers]

Hi @sortie

Ahh! Ok. It wasn't clear to me that this would be a breaking change. Just for my own understanding, can you explain a little more why it's breaking? Is it related to the fact that the changes would span the VM and SDK? (If there is expectation of mixing versions of VM and SDK then I understand. But I'm not sure that there is. I'm still very new to getting under the hood of Dart.)

It seems to me that any fallback implementation for non-_Sockets would still involve exposing a new interface like detachRaw. I don't know if we can render the change non-breaking with special handling like if (socket is _Socket) {} else {}.

Thanks for looking into this issue!

[GreenAppers]

I verified the second solution of adding Socket socketFromRaw(RawSocket raw) => _Socket(raw).

I think it's actually the better solution, because RawSocket is simpler to implement than Socket.

The requirements of SecureSocket.secure are effectively relaxed. The _Socket argument requirement should still be commented, but then we could direct to socketFromRaw.

It could also be Socket.fromRaw(). I'm not sure best place to add the function.