----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18968/#review36778 -----------------------------------------------------------
Ship it! I can't think of any reason why this check should not be moved to this point. ( And it looks like there is an excellent reason why it *must* be moved here... ) - mick goulish On March 11, 2014, 8:06 a.m., Pavel Moravec wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18968/ > ----------------------------------------------------------- > > (Updated March 11, 2014, 8:06 a.m.) > > > Review request for qpid, Gordon Sim and mick goulish. > > > Bugs: QPID-5621 > https://issues.apache.org/jira/browse/QPID-5621 > > > Repository: qpid > > > Description > ------- > > Root cause of the problem: ACL for links is checked after getting > connection.startOk AMQP method. While DIGEST-MD5 (and other auth.methods) > provide userId later on - during connection.secureOk AMQP method. > > So the ACL check for the SASL mechanisms relying on challenge & response > should be postponed until ConnectionHandler::Handler::secureOk method. > > I have two issues with the patch: > > > 1) How to identify SASL methods relying on challenge & response? I used > "((body.getMechanism()=="ANONYMOUS")||(body.getMechanism()=="PLAIN"))" test > there but dont like the explicit SASL mechs comparison.. > (And I am not even 100% sure the list of mechanisms is correct - I just > *guess* SSL or GSSAPI sends challenge and response as well. > > > 2) Can a user have empty username? If so, then in the test: > > if ((connection.getUserId()!="") && (connection.isFederationLink())) > > the first condition will never match - while the condition is necessary as > usually SASL authentication requires several challenge+response exchanges, > i.e. several connection.secureOk methods received, while only the latest one > has userId finally set. > > > Diffs > ----- > > /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.cpp 1576205 > > Diff: https://reviews.apache.org/r/18968/diff/ > > > Testing > ------- > > > Thanks, > > Pavel Moravec > >
