----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55585/#review161793 -----------------------------------------------------------
I'm a little puzzled how it is possible for client code to be receiving input before it has sent the outcome frame; similarly I'm a little puzzled that the other end can be sending any encoded frames before it receives the outcome. On first impression, this seems like it shouldn't happen! Having said that, this fix looks basically good, although I'd love to be able to simplify the logic somehow. proton-c/src/sasl/sasl.c (line 304) <https://reviews.apache.org/r/55585/#comment233100> I think that you are now going to get the logging happening twice. Once for the encryption before switching layers and again when the encryption layer is switched in. I suggest changing this log message to specify that this is input only encryption before the layers get switched to avoid logging confusion. Or maybe just removing the message completely. - Andrew Stitcher On Jan. 16, 2017, 7:55 p.m., Gordon Sim wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55585/ > ----------------------------------------------------------- > > (Updated Jan. 16, 2017, 7:55 p.m.) > > > Review request for qpid and Andrew Stitcher. > > > Repository: qpid-proton-git > > > Description > ------- > > In pn_input_read_sasl, when a successful outcome frame is read, it will set > the desired_state to SASL_RECVED_OUTCOME_SUCCEED. This means that > pni_sasl_is_final_input_state() will return true. However from what I can > tell, the last_state, which is what is checked by > pni_sasl_is_final_output_state(), is only set to this same value when > pni_post_sasl_frame() is called, and the client will never need to call that > after receiving an outcome (as the sasl exchange is then over). > > So if we get two calls to pn_input_read_sasl(), one to read the outcome, the > next to read the first encrypted data, *without* a pn_output_write_sasl() > between them, pni_sasl_is_final_input_state() will return true but > pni_sasl_is_final_output_state() will return false on the second read, which > results in the second recv call passing it to the passthru layer even though > it may be encrypted. > > This patch ensures that if the final input state has been reached for sasl, > and encryption was negotiated, further incoming data is always decoded (even > if we have not yet reached the final output state and so have not set > io_layers). > > > Diffs > ----- > > proton-c/src/sasl/sasl.c 69fb6b2 > > Diff: https://reviews.apache.org/r/55585/diff/ > > > Testing > ------- > > Existing tests pass. Rerproducer for original issue - occasional failure of > proton client using DIGEST-MD5 against qpidd - passes reliably (i.e. while > ./simple_send.py -a guest:guest@localhost -m 1; do echo ok; done against > qpidd, with only DIGEST-MD5 allowed). > > > Thanks, > > Gordon Sim > >
