----------------------------------------------------------- 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