> On Jan. 16, 2017, 10:39 p.m., Andrew Stitcher wrote: > > 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. > > Gordon Sim wrote: > The client doesn't send the outcome frame, or any response to it. So the > server has sent the outcome, and the client has received it. The server may > also have sent the first part of the encrypted data since it has now > authenticated the client and is not expecting any further sasl frames from > the client. > > Andrew Stitcher wrote: > Looking at it some more istm that in the case you have found > pni_sasl_is_final_output_state() should actually be true. This is supposed to > mean that there will be no more output forthcoming. Which you have > demonstrated is true. > > Andrew Stitcher wrote: > In detail, I have a correction to your sequence of events though. > > The issue is that there is back to back input for the client so that > pn_input_read_sasl() gets called twice by the transport processing loop > before it calls pn_output_write_sasl() which it will always do at least once > after the outcome frame is received. > > So probably a neater fix is to amend pni_sasl_is_final_output_state() to > check desired_state against SASL_RECVED_OUTCOME_SUCCEED too. > > Gordon Sim wrote: > That could well be the case. It *isn't* true, because it checks the > 'last_state' which is only set (from the 'desired_state') when you do output. > So although the desired state is SASL_RECVED_OUTCOME_SUCCEED, the last_state > isn't set to that until output is done after that. > > Btw I created PROTON-1388 for this issue. > > Andrew Stitcher wrote: > Do you want to set the bugs field up top then - I think only the review > author can do that > > Andrew Stitcher wrote: > I've attached a possible different fix to the bug - it changes the > behaviour of pni_sasl_is_final_output_state(). I think it would be a somewhat > neater fix if it solves the issue.
I agree it is neater and it does indeed seem to fix the problem. Go ahead and commit that and I'll close this review. - Gordon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55585/#review161793 ----------------------------------------------------------- On Jan. 17, 2017, 9:05 p.m., Gordon Sim wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55585/ > ----------------------------------------------------------- > > (Updated Jan. 17, 2017, 9:05 p.m.) > > > Review request for qpid and Andrew Stitcher. > > > Bugs: PROTON-1388 > https://issues.apache.org/jira/browse/PROTON-1388 > > > 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 > >
