You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Pavel Moravec <pm...@redhat.com> on 2014/03/10 15:20:16 UTC
Review Request 18968: [C++ broker] userId is not passed to ACL when
DIGEST-MD5 is used while creating link
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18968/
-----------------------------------------------------------
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
Re: Review Request 18968: [C++ broker] userId is not passed to ACL when
DIGEST-MD5 is used while creating link
Posted by mick goulish <mi...@redhat.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 18968: [C++ broker] userId is not passed to ACL when
DIGEST-MD5 is used while creating link
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18968/#review36767
-----------------------------------------------------------
Ship it!
Ship It!
- Gordon Sim
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
>
>
Re: Review Request 18968: [C++ broker] userId is not passed to ACL when
DIGEST-MD5 is used while creating link
Posted by Pavel Moravec <pm...@redhat.com>.
-----------------------------------------------------------
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.
Changes
-------
Great point, Gordon. This patch is straightforward then..
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 (updated)
-----
/trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.cpp 1576205
Diff: https://reviews.apache.org/r/18968/diff/
Testing
-------
Thanks,
Pavel Moravec
Re: Review Request 18968: [C++ broker] userId is not passed to ACL when
DIGEST-MD5 is used while creating link
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 18968: [C++ broker] userId is not passed to ACL when
DIGEST-MD5 is used while creating link
Posted by Pavel Moravec <pm...@redhat.com>.
-----------------------------------------------------------
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