Hello, back to work, I have the following thoughts:
- encrypting just before we write to the socket may lead to other problems: if the resulting message is greater than the send buffer, then we would need to wait for the rest of the buffer. If between, we receive an handshake, we may risk the data to be sent with wrong key materials but if the handshake acknowledgment is queued after those application messages, this may be ok, we should look at what the spec says about this case. So maybe we should keep encryption performed when the message is queued, assumed we handle the case properly if handshake is not yet finished. - I don't see the purpose of the timestamp, do you want to timestamp/version the handshake acks to detect a message encrypted by key materials 1 when it was submitted will be sent as key materials 2 has been activated ? I think if we simply queue messages,we don't need it. WDYT ? Jeff On Thu, Jul 3, 2014 at 10:53 AM, Emmanuel Lécharny <elecha...@gmail.com> wrote: > Le 03/07/2014 07:56, Jeff MAURY a écrit : > > On Thu, Jul 3, 2014 at 1:19 AM, Emmanuel Lécharny <elecha...@gmail.com> > > wrote: > > > >> Le 03/07/2014 00:42, Jeff MAURY a écrit : > >>> Hello, > >>> > >>> I have spent some times reviewing the SSL code in order to add support > >>> client handshake start. > >>> I have seen several problems that I'd like to share: > >>> > >>> - messages sent are encrypted when they are submitted: I see two > >>> problems with that: if a message is submitted before the handshake > is > >>> completed, then behavior is probably error. If a message is > submitted > >> so > >>> queued but handshake happened between the data are sent to the > >> socket, then > >>> it's likely that remote won't be able to decode it. > >> How possibly ? Until the handshake is completed, you don't have the key > >> to encrypt anything... > >> > > That's the point. The algoritm when a message is submitted is the > following: > > > > - if the handhskake is complete, then message is encrypted and > processed > > like any other (sent on socket if no write current otherwhise queued > > - otherwise, processed like any other (sent on socket if no write > > current otherwhise queued <-- BIG PB > > > > > >> The way it works (or the way it should work) is that as soon as you > >> switch to SSL mode, then a handshake is done. That should block the > >> sending of any message until the handshake is done. One potential pb we > >> may have here is that swicthing from a normal state to SSL might be done > >> while we do have some pending messages, but the key here is that those > >> messages must be sent before we switch. > >> > > But ypu may have no control on the handshake switch as it may be > initiated > > by the remote. > > The server knows when it receives a CLIENT_REQUEST, so the server has > some control. > > > >>> - Regarding the handshake, it may lead to data sent. In our > >>> implementation, it queue another message to be sent which will not > be > >>> encrypted because the state is linked to the handshake but this > means > >> if > >>> the application submits a message to send, it will not be encrypted > >> as well. > >> > >> Not sure I get it... > >> > > Imagine that handshake processing may lead to messages generated that > need > > to be sent. If you apply the above algorithm, then it work because the > > message will be queued without encryption. But if you fix it (for > > application messages), then you need a flag. > > > > > >>> The changes I propose are the following: > >>> > >>> - encryption performed only when data is to be written to the socket > >> That what the spirit of the changes in SSL handling in MINA 3, compared > >> to MINA 2. MINA 2 inject the SSL filter in the middle of the chain, > >> making it extremely complex to deal with pênding messages and such. > >>> - messages are stored with a flag to tell if they have been already > >>> encrypted > >> IMO, messages don"t need to be flagged. They are either encrypted, or > >> they aren't, but in any case, we should send all the encrypted messages > >> to the remote peer before we switch off the SSL mode, or we should send > >> all teh unencrypted messages before we set up teh SSL mode. By all > >> means, it's really liek a global switch which should hold any new > >> messages until teh handshake is completed. > >>> - messages generated as part of the handshake should probably queued > >> at > >>> the top of the queue instead of the tail for application messages. > >> Not sure. As teh handshake will start by a CLIENT_HELLO message, until > >> this message is sent, all the pending messages in teh queue must have > >> been sent. That means we should not accept any other messages in teh > >> queue until the end of teh handshake is reached. This is the real key : > >> how to forbod the application to push messages in teh queue, as we will > >> have to push some SSL Handshake messages in this queue ? > >> > > That's why I propose to queue handshake messages in front of the queue > and > > to encrypt before writing to the socket. > Thinking about it a bit more, that may be the right thing to do. If we > block the session, forbidding it to push new messages in teh queue, then > we won't be able to process handshake messages anymore. Using a second > queue to stack messages while we are handshaking would be painful. > > Now, considering that when the client send a CLIEN_HELLO message, we > won't receive any message that is not SSL related (and even so, it will > be rejected by teh handshake state engine). The only problem is with the > session that could still have some messages to write (either because we > have some pending messages or because there is a separated thread for > this session that generate some messages). This is the tricky part, IMO. > While we are handshaking, the application messages should never be sent, > but the messages generated before we started the handshake should be sent. > > That requires mayb more than a flag, but also a timestamp (why should we > forbid the app to send a message which is a response to a request older > than the handshake ?) > > Regarding the encryption, it should obviously be done before writing to > the socket. We should even encrypt before pushing the messages into the > queue. > > -- 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
