----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18968/#review36653 -----------------------------------------------------------
Perhaps the cleanest thing is just to move any ACl check on the connection to the handling of open(). That avoids different paths for different mechanisms which seems error prone, and also any special tricks for determining whether username is yet available (it *must* be available by open if one is set). - Gordon Sim On March 10, 2014, 2:20 p.m., Pavel Moravec wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18968/ > ----------------------------------------------------------- > > (Updated March 10, 2014, 2:20 p.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 1575923 > > Diff: https://reviews.apache.org/r/18968/diff/ > > > Testing > ------- > > > Thanks, > > Pavel Moravec > >
