Re: [MINA 3] SSL and fragmentation
On extending java.nio.ByteBuffer : it's not doable because the constructor is private :( On Fri, May 3, 2013 at 3:40 PM, Jeff MAURY wrote: > +1 > > I think we shall rewrite the code and let be driven by the underlying > SSLengine status. > Now, if after processing there are still remaining data in the input > buffer, we must keep it until the next buffer is received. > In order to preserve performances, as we need to assemble the input buffer > with the rest of the previous input buffer, I had the idea of having a > CompositeReadOnlyByteBuffer that extends ByteBuffer and that will be > composed of the two buffers. This will prevent to perform costly copies > while allowing the SSLEngine to see the whole data. > Any thoughts ? > > Jeff > > > On Fri, May 3, 2013 at 3:19 PM, Emmanuel Lécharny >wrote: > > > Hi guys, > > > > the way we handle incomming SSL data on the server will not work if the > > data are fragmented. > > > > Let's say we are receiving some Handshake data. This is typically a few > > hundreds of bytes, that usually comes in one block. In this case, we are > > all good. But if those data aren't read in one block, then we are dead. > > > > I have simulated such a fragmentation by sending a ClientHello block of > > data in three parts : as soon as the SslEngine if given the second block > > of data, it throws an exception : > > 2013-05-03 15:07:15,546 ERROR [SelectorWorker Server-I/O-1] > > tcp.NioTcpSession (NioTcpSession.java:330) - Exception while reading : > > javax.net.ssl.SSLException: Unsupported record version Unknown-0.0 > > at > > > > > sun.security.ssl.EngineInputRecord.bytesInCompletePacket(EngineInputRecord.java:116) > > at > sun.security.ssl.SSLEngineImpl.readNetRecord(SSLEngineImpl.java:845) > > at sun.security.ssl.SSLEngineImpl.unwrap(SSLEngineImpl.java:758) > > at javax.net.ssl.SSLEngine.unwrap(SSLEngine.java:624) > > at org.apache.mina.session.SslHelper.unwrap(SslHelper.java:190) > > > > > > This is plain normal : when we read the first block of data, we unwrap > > it, and the SslEngine resturn with a BUFFER_UNDERFLOW status (expected). > > Now, we return back to the main loop, and we *clear* the reaBuffer. We > > then lose the first bytes we have read, parts that the SslEngine is > > expecting to have the next time we call it. > > > > There is one way to avoid such a problem : we just have to compact the > > buffer instead of clearing it. > > > > Now, regarding the SSL code, it's really messy atm. The fact that we > > separate the handshake from the normal processing of data is wrong. When > > we do an unwrap, it will initiate a handshake if needed. The fact is we > > have to review the full code and rewrite it... > > > > I'm currently doing some experiments here, but I would be please to > > share ideas, suggestion, proposals from any of you... > > > > > > -- > Jeff MAURY > > > "Legacy code" often differs from its suggested alternative by actually > working and scaling. > - Bjarne Stroustrup > > http://www.jeffmaury.com > http://riadiscuss.jeffmaury.com > http://www.twitter.com/jeffmaury >
Re: [MINA 3] SSL and fragmentation
Le 5/3/13 4:45 PM, Jeff MAURY a écrit :
> On Fri, May 3, 2013 at 3:47 PM, Emmanuel Lécharny wrote:
>
>> Le 5/3/13 3:40 PM, Jeff MAURY a écrit :
>>> +1
>>>
>>> I think we shall rewrite the code and let be driven by the underlying
>>> SSLengine status.
>> Absolutely. I have created a branch to play a bit with SSL.
>>> Now, if after processing there are still remaining data in the input
>>> buffer, we must keep it until the next buffer is received.
>> Thus the compact().
>>
>> The pb is for the normal calls (ie, not SSL- as the buffer can be
>> propagated up to the user. In this case, if we compact the buffer, we
>> may get some weaird side effect if the user has done some weird things
>> wih the buffer.
>>
> Why do we need to take care of non SSL configuration in this SSL discussion
> ?
Sorry, I was unclear. I meant when we have already processed the
Handshake. The unwrapped buffer will be probagated to the user, and can
potentially be modified (even if it's a mistake).
Not sure this is a big issue atm...
>
>
>>> In order to preserve performances, as we need to assemble the input
>> buffer
>>> with the rest of the previous input buffer, I had the idea of having a
>>> CompositeReadOnlyByteBuffer that extends ByteBuffer and that will be
>>> composed of the two buffers. This will prevent to perform costly copies
>>> while allowing the SSLEngine to see the whole data.
>>> Any thoughts ?
>> That's an option.
>>
>> However, internally, the SslEngine does copy the ByteBuffer if it's not
>> a Direct buffer. So I'm not sure we can spare any CPU time by
>> concatenating the received ByteBuffer into a composite structure.
>>
> I think so as I suspect the SSLEngine to be message oriented so when an
> input buffer is given, then the SLL engine will read the first bytes to
> decode the length and if not enough data is present, it will throw a
> BufferUnderflow so only the first bytes of the input buffer will be read
> and taht's why we need to concatenate.
This is most certainly what the SSLEngine does. A SSL message typically
starts with a heade :
b[0] : ContentType {0x14:ChangeCipherSpec, 0x15:Alert, 0x16:Handshake,
0x17:ApplicationData}
b[1-2] : Version {3.0 : SSL 3.0, 3.1 : TLS 1.0, 3.2 : TLS 1.1, 3.3 : TLS
1.2}
b[3-4] : length <= 2^14 (16Kb)
So if it gets at least 5 bytes, the SslEngine can know how many bytes to
expect.
>
>> I mean, if we pass a DirectBuffer to the SslEngine, it will be faster.
>> If this DirectBuffer does not contain enough data to produce an
>> unwrapped buffer, the we will have to pass a new DirectBuffer containing
>> the previous data (copied from the previous DirectBuffer) plus the newly
>> read data. In any case, we have some copy...
>>
> I don't think so.
If we reuse the readBuffer (which is allocated once per session),
without clearing it between reads, and if it's a DirectBuffer, then I
think there is no copy done.
--
Regards,
Cordialement,
Emmanuel Lécharny
www.iktek.com
Re: [MINA 3] SSL and fragmentation
On Fri, May 3, 2013 at 3:47 PM, Emmanuel Lécharny wrote: > Le 5/3/13 3:40 PM, Jeff MAURY a écrit : > > +1 > > > > I think we shall rewrite the code and let be driven by the underlying > > SSLengine status. > Absolutely. I have created a branch to play a bit with SSL. > > Now, if after processing there are still remaining data in the input > > buffer, we must keep it until the next buffer is received. > Thus the compact(). > > The pb is for the normal calls (ie, not SSL- as the buffer can be > propagated up to the user. In this case, if we compact the buffer, we > may get some weaird side effect if the user has done some weird things > wih the buffer. > Why do we need to take care of non SSL configuration in this SSL discussion ? > > In order to preserve performances, as we need to assemble the input > buffer > > with the rest of the previous input buffer, I had the idea of having a > > CompositeReadOnlyByteBuffer that extends ByteBuffer and that will be > > composed of the two buffers. This will prevent to perform costly copies > > while allowing the SSLEngine to see the whole data. > > Any thoughts ? > That's an option. > > However, internally, the SslEngine does copy the ByteBuffer if it's not > a Direct buffer. So I'm not sure we can spare any CPU time by > concatenating the received ByteBuffer into a composite structure. > I think so as I suspect the SSLEngine to be message oriented so when an input buffer is given, then the SLL engine will read the first bytes to decode the length and if not enough data is present, it will throw a BufferUnderflow so only the first bytes of the input buffer will be read and taht's why we need to concatenate. > > I mean, if we pass a DirectBuffer to the SslEngine, it will be faster. > If this DirectBuffer does not contain enough data to produce an > unwrapped buffer, the we will have to pass a new DirectBuffer containing > the previous data (copied from the previous DirectBuffer) plus the newly > read data. In any case, we have some copy... > I don't think so. Jeff > > > -- > Regards, > Cordialement, > Emmanuel Lécharny > www.iktek.com > > -- Jeff MAURY "Legacy code" often differs from its suggested alternative by actually working and scaling. - Bjarne Stroustrup http://www.jeffmaury.com http://riadiscuss.jeffmaury.com http://www.twitter.com/jeffmaury
Re: [MINA 3] SSL and fragmentation
Le 5/3/13 3:40 PM, Jeff MAURY a écrit : > +1 > > I think we shall rewrite the code and let be driven by the underlying > SSLengine status. Absolutely. I have created a branch to play a bit with SSL. > Now, if after processing there are still remaining data in the input > buffer, we must keep it until the next buffer is received. Thus the compact(). The pb is for the normal calls (ie, not SSL- as the buffer can be propagated up to the user. In this case, if we compact the buffer, we may get some weaird side effect if the user has done some weird things wih the buffer. > In order to preserve performances, as we need to assemble the input buffer > with the rest of the previous input buffer, I had the idea of having a > CompositeReadOnlyByteBuffer that extends ByteBuffer and that will be > composed of the two buffers. This will prevent to perform costly copies > while allowing the SSLEngine to see the whole data. > Any thoughts ? That's an option. However, internally, the SslEngine does copy the ByteBuffer if it's not a Direct buffer. So I'm not sure we can spare any CPU time by concatenating the received ByteBuffer into a composite structure. I mean, if we pass a DirectBuffer to the SslEngine, it will be faster. If this DirectBuffer does not contain enough data to produce an unwrapped buffer, the we will have to pass a new DirectBuffer containing the previous data (copied from the previous DirectBuffer) plus the newly read data. In any case, we have some copy... -- Regards, Cordialement, Emmanuel Lécharny www.iktek.com
Re: [MINA 3] SSL and fragmentation
+1 I think we shall rewrite the code and let be driven by the underlying SSLengine status. Now, if after processing there are still remaining data in the input buffer, we must keep it until the next buffer is received. In order to preserve performances, as we need to assemble the input buffer with the rest of the previous input buffer, I had the idea of having a CompositeReadOnlyByteBuffer that extends ByteBuffer and that will be composed of the two buffers. This will prevent to perform costly copies while allowing the SSLEngine to see the whole data. Any thoughts ? Jeff On Fri, May 3, 2013 at 3:19 PM, Emmanuel Lécharny wrote: > Hi guys, > > the way we handle incomming SSL data on the server will not work if the > data are fragmented. > > Let's say we are receiving some Handshake data. This is typically a few > hundreds of bytes, that usually comes in one block. In this case, we are > all good. But if those data aren't read in one block, then we are dead. > > I have simulated such a fragmentation by sending a ClientHello block of > data in three parts : as soon as the SslEngine if given the second block > of data, it throws an exception : > 2013-05-03 15:07:15,546 ERROR [SelectorWorker Server-I/O-1] > tcp.NioTcpSession (NioTcpSession.java:330) - Exception while reading : > javax.net.ssl.SSLException: Unsupported record version Unknown-0.0 > at > > sun.security.ssl.EngineInputRecord.bytesInCompletePacket(EngineInputRecord.java:116) > at sun.security.ssl.SSLEngineImpl.readNetRecord(SSLEngineImpl.java:845) > at sun.security.ssl.SSLEngineImpl.unwrap(SSLEngineImpl.java:758) > at javax.net.ssl.SSLEngine.unwrap(SSLEngine.java:624) > at org.apache.mina.session.SslHelper.unwrap(SslHelper.java:190) > > > This is plain normal : when we read the first block of data, we unwrap > it, and the SslEngine resturn with a BUFFER_UNDERFLOW status (expected). > Now, we return back to the main loop, and we *clear* the reaBuffer. We > then lose the first bytes we have read, parts that the SslEngine is > expecting to have the next time we call it. > > There is one way to avoid such a problem : we just have to compact the > buffer instead of clearing it. > > Now, regarding the SSL code, it's really messy atm. The fact that we > separate the handshake from the normal processing of data is wrong. When > we do an unwrap, it will initiate a handshake if needed. The fact is we > have to review the full code and rewrite it... > > I'm currently doing some experiments here, but I would be please to > share ideas, suggestion, proposals from any of you... > -- Jeff MAURY "Legacy code" often differs from its suggested alternative by actually working and scaling. - Bjarne Stroustrup http://www.jeffmaury.com http://riadiscuss.jeffmaury.com http://www.twitter.com/jeffmaury
