Replace socket with new engine-based version about conscrypt HOT 16 OPEN

@dave-r12 ah ok, makes sense. Yeah, the engine-based socket should make things easier for you then, since there's no black magic involved (e.g. writing to the FD directly via JNI).

If for some reason you decide to use Conscrypt's SSLEngine directly, it should handle all of the security concerns for you ... after all, that's its job :)

from conscrypt.

I briefly looked at this a while ago. There seem to be a lot of fiddly interaction with FileDescriptor instances to make sockets work correctly.

from conscrypt.

@kruton the new socket won't have a FileDescriptor, so I would think it should make things simpler, no? Or is something else trying to access the underlying file?

from conscrypt.

Maybe it won't matter since you're basically going to be only reading from the wrapped Socket's InputStream. However, this may be a performance regression because you'd have to make at least one more copy now. One to get data from the OS copied to the underlying Socket buffer and then another one to copy the received data into BoringSSL.

from conscrypt.

@kruton Agreed ... we need to benchmark the new socket on android to confirm there is no performance regression (#10)

from conscrypt.

Re-targeting for 1.0.0. We need this to properly handle closures of the underlying FD.

from conscrypt.

@flooey can we throw the flag and make this the default? It would be good to make the switch before 1.0.0 if possible.

from conscrypt.

Android needs the socket implementation to support renegotiation (which has no tests, so you need to fix that first... which will needs someone with TLS expertise and a lot of time to test, see #228 ). The engine implementation does not support renegotiation. Merely adding renegotiation support to the engine implementation will also take someone with a even more TLS expertise. (BoringSSL tightly limits renegotiation for security reasons and you all will need to think hard about how it is exposed via engine as a result of those.)

I don't think you'll be able to do this for 1.0.0 if you want that done soon.

[Edit: fixed bug number]

from conscrypt.

@davidben ok thanks, I guess we'll punt on this for now then.

from conscrypt.

Once this change is completed, will it then be possible to count the number of bytes used by the SSLSocket? Seems the OpenSSL implementation does a JNI call so there is no way to capture the number of bytes written/read to the underlying socket.

from conscrypt.

@dave-r12 could you just use a delegate socket that counts the bytes written?

from conscrypt.

@nmittler I think I've done just that. Is this what you mean? I created a DelegatingSocket class that extends Socket. All methods get delegated except getInputStream() and getOutputStream(). These get wrapped so the bytes get counted. But this will only work if the SSLSocket writes to the underlying Socket input/output streams.

from conscrypt.

@dave-r12 yeah that's right.

But this will only work if the SSLSocket writes to the underlying Socket input/output streams.

The engine-based socket currently uses the underlying socket's streams directly. I had explored writing to a channel (if the underlying socket has one), but the performance improvement seemed relatively minor and not worth the added complexity. Even if we were using channels, your delegation pattern could account for that as well.

I'm assuming that you are currently using the FD-based socket? What is your byte-counting strategy for that?

from conscrypt.

The engine-based socket currently uses the underlying socket's streams directly.

Gotcha, thanks. I believe that answers my question (assuming you don't change the implementation.)

I'm assuming that you are currently using the FD-based socket? What is your byte-counting strategy for that?

I'm stuck 'cause it's making JNI calls. The only other idea I had was to use an SSLEngine and do everything manually. I'm nervous though, I don't want to screw up security! And I've read it wasn't given much love in earlier versions of Android.

from conscrypt.

@flooey is this still on the radar? It would be good to have a single socke impl.

from conscrypt.

@nmittler It's still something I'd like to do but other things have been above it on the priority list. The main blocker is #433, which I haven't been able to make progress on diagnosing yet.

from conscrypt.