You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Kenneth Giusti <kg...@apache.org> on 2015/10/09 17:33:09 UTC
Review Request 39173: qpidd should dynamically set the SASL service
name based on protocol.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39173/
-----------------------------------------------------------
Review request for qpid, Andrew Stitcher and Gordon Sim.
Bugs: QPID-6783
https://issues.apache.org/jira/browse/QPID-6783
Repository: qpid
Description
-------
See related JIRA
Diffs
-----
trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1706484
trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.cpp 1706484
trunk/qpid/cpp/src/qpid/broker/amqp/ProtocolPlugin.cpp 1706484
trunk/qpid/cpp/src/qpid/messaging/ConnectionOptions.cpp 1706484
Diff: https://reviews.apache.org/r/39173/diff/
Testing
-------
Unit tests.
Thanks,
Kenneth Giusti
Re: Review Request 39173: qpidd should dynamically set the SASL
service name based on protocol.
Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39173/#review102113
-----------------------------------------------------------
Ship it!
Now that we have AMQP_SASL_SERVICENAME in a bunch of places we should really put the constant in a single place too.
(But all these comments are mere quibbling)
trunk/qpid/cpp/src/qpid/broker/amqp/Domain.cpp (line 225)
<https://reviews.apache.org/r/39173/#comment159636>
I think you could have the desired effect by only changing this to:
... service(AMQP_SASL_SERVICENAME)
trunk/qpid/cpp/src/qpid/broker/amqp/Domain.cpp (line 237)
<https://reviews.apache.org/r/39173/#comment159637>
And reverting this change, since the previous code preset the name to "qpidd" via qpid::saslName, then allowed that to be overridden by the SASL_SERVICE property.
In that case I think the complex new conditional is just unnecessary.
- Andrew Stitcher
On Oct. 9, 2015, 8:33 p.m., Kenneth Giusti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39173/
> -----------------------------------------------------------
>
> (Updated Oct. 9, 2015, 8:33 p.m.)
>
>
> Review request for qpid, Andrew Stitcher and Gordon Sim.
>
>
> Bugs: QPID-6783
> https://issues.apache.org/jira/browse/QPID-6783
>
>
> Repository: qpid
>
>
> Description
> -------
>
> See related JIRA
>
>
> Diffs
> -----
>
> trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1706484
> trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.cpp 1706484
> trunk/qpid/cpp/src/qpid/broker/amqp/Domain.cpp 1706484
> trunk/qpid/cpp/src/qpid/broker/amqp/ProtocolPlugin.cpp 1706484
> trunk/qpid/cpp/src/qpid/messaging/ConnectionOptions.cpp 1706484
>
> Diff: https://reviews.apache.org/r/39173/diff/
>
>
> Testing
> -------
>
> Unit tests.
>
>
> Thanks,
>
> Kenneth Giusti
>
>
Re: Review Request 39173: qpidd should dynamically set the SASL
service name based on protocol.
Posted by Andrew Stitcher <as...@apache.org>.
> On Oct. 12, 2015, 2:57 p.m., Kenneth Giusti wrote:
> > trunk/qpid/cpp/src/qpid/broker/amqp/Domain.cpp, line 237
> > <https://reviews.apache.org/r/39173/diff/2/?file=1094232#file1094232line237>
> >
> > Hrmmm... not sure I follow.
> >
> > There are 3 potential sources for the sasl service name. In priority:
> >
> > 1) passed via the domain create command option --sasl-service-name
> > 2) lacking that, use the value configured via the broker wide configuration option --sasl-service-name
> > 3) lacking that, default to 'amqp'
> >
> > If we remove the conditional, the code would ignore the broker configuration setting.
yes, that is correct - the previous code never tried to use the command line option, so why should this change?
It was getting the configured property to use from somewhere and it will still do that.
Given that the command line option is the sasl service name to use when *listening*, I don't think it is correct to be using that name to connect outbound.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39173/#review102254
-----------------------------------------------------------
On Oct. 9, 2015, 8:33 p.m., Kenneth Giusti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39173/
> -----------------------------------------------------------
>
> (Updated Oct. 9, 2015, 8:33 p.m.)
>
>
> Review request for qpid, Andrew Stitcher and Gordon Sim.
>
>
> Bugs: QPID-6783
> https://issues.apache.org/jira/browse/QPID-6783
>
>
> Repository: qpid
>
>
> Description
> -------
>
> See related JIRA
>
>
> Diffs
> -----
>
> trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1706484
> trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.cpp 1706484
> trunk/qpid/cpp/src/qpid/broker/amqp/Domain.cpp 1706484
> trunk/qpid/cpp/src/qpid/broker/amqp/ProtocolPlugin.cpp 1706484
> trunk/qpid/cpp/src/qpid/messaging/ConnectionOptions.cpp 1706484
>
> Diff: https://reviews.apache.org/r/39173/diff/
>
>
> Testing
> -------
>
> Unit tests.
>
>
> Thanks,
>
> Kenneth Giusti
>
>
Re: Review Request 39173: qpidd should dynamically set the SASL
service name based on protocol.
Posted by Kenneth Giusti <kg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39173/#review102254
-----------------------------------------------------------
trunk/qpid/cpp/src/qpid/broker/amqp/Domain.cpp (line 237)
<https://reviews.apache.org/r/39173/#comment159834>
Hrmmm... not sure I follow.
There are 3 potential sources for the sasl service name. In priority:
1) passed via the domain create command option --sasl-service-name
2) lacking that, use the value configured via the broker wide configuration option --sasl-service-name
3) lacking that, default to 'amqp'
If we remove the conditional, the code would ignore the broker configuration setting.
- Kenneth Giusti
On Oct. 9, 2015, 8:33 p.m., Kenneth Giusti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39173/
> -----------------------------------------------------------
>
> (Updated Oct. 9, 2015, 8:33 p.m.)
>
>
> Review request for qpid, Andrew Stitcher and Gordon Sim.
>
>
> Bugs: QPID-6783
> https://issues.apache.org/jira/browse/QPID-6783
>
>
> Repository: qpid
>
>
> Description
> -------
>
> See related JIRA
>
>
> Diffs
> -----
>
> trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1706484
> trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.cpp 1706484
> trunk/qpid/cpp/src/qpid/broker/amqp/Domain.cpp 1706484
> trunk/qpid/cpp/src/qpid/broker/amqp/ProtocolPlugin.cpp 1706484
> trunk/qpid/cpp/src/qpid/messaging/ConnectionOptions.cpp 1706484
>
> Diff: https://reviews.apache.org/r/39173/diff/
>
>
> Testing
> -------
>
> Unit tests.
>
>
> Thanks,
>
> Kenneth Giusti
>
>
Re: Review Request 39173: qpidd should dynamically set the SASL
service name based on protocol.
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39173/#review102222
-----------------------------------------------------------
Ship it!
Ship It!
- Gordon Sim
On Oct. 9, 2015, 8:33 p.m., Kenneth Giusti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39173/
> -----------------------------------------------------------
>
> (Updated Oct. 9, 2015, 8:33 p.m.)
>
>
> Review request for qpid, Andrew Stitcher and Gordon Sim.
>
>
> Bugs: QPID-6783
> https://issues.apache.org/jira/browse/QPID-6783
>
>
> Repository: qpid
>
>
> Description
> -------
>
> See related JIRA
>
>
> Diffs
> -----
>
> trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1706484
> trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.cpp 1706484
> trunk/qpid/cpp/src/qpid/broker/amqp/Domain.cpp 1706484
> trunk/qpid/cpp/src/qpid/broker/amqp/ProtocolPlugin.cpp 1706484
> trunk/qpid/cpp/src/qpid/messaging/ConnectionOptions.cpp 1706484
>
> Diff: https://reviews.apache.org/r/39173/diff/
>
>
> Testing
> -------
>
> Unit tests.
>
>
> Thanks,
>
> Kenneth Giusti
>
>
Re: Review Request 39173: qpidd should dynamically set the SASL
service name based on protocol.
Posted by Kenneth Giusti <kg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39173/
-----------------------------------------------------------
(Updated Oct. 9, 2015, 8:33 p.m.)
Review request for qpid, Andrew Stitcher and Gordon Sim.
Changes
-------
Added Gordon's fix for domains
Bugs: QPID-6783
https://issues.apache.org/jira/browse/QPID-6783
Repository: qpid
Description
-------
See related JIRA
Diffs (updated)
-----
trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1706484
trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.cpp 1706484
trunk/qpid/cpp/src/qpid/broker/amqp/Domain.cpp 1706484
trunk/qpid/cpp/src/qpid/broker/amqp/ProtocolPlugin.cpp 1706484
trunk/qpid/cpp/src/qpid/messaging/ConnectionOptions.cpp 1706484
Diff: https://reviews.apache.org/r/39173/diff/
Testing
-------
Unit tests.
Thanks,
Kenneth Giusti
Re: Review Request 39173: qpidd should dynamically set the SASL
service name based on protocol.
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39173/#review102062
-----------------------------------------------------------
Ship it!
Ideally we would modify the default value for service in Domain - ~line 222 of qpid/broker/amqp/Domain.cpp - which is used for broker initiated connections. That value can be set explicitly per domain, but since 1.0 defaulst to 'amqp' on the broker side it would be nice to change there as well.
- Gordon Sim
On Oct. 9, 2015, 3:33 p.m., Kenneth Giusti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39173/
> -----------------------------------------------------------
>
> (Updated Oct. 9, 2015, 3:33 p.m.)
>
>
> Review request for qpid, Andrew Stitcher and Gordon Sim.
>
>
> Bugs: QPID-6783
> https://issues.apache.org/jira/browse/QPID-6783
>
>
> Repository: qpid
>
>
> Description
> -------
>
> See related JIRA
>
>
> Diffs
> -----
>
> trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1706484
> trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.cpp 1706484
> trunk/qpid/cpp/src/qpid/broker/amqp/ProtocolPlugin.cpp 1706484
> trunk/qpid/cpp/src/qpid/messaging/ConnectionOptions.cpp 1706484
>
> Diff: https://reviews.apache.org/r/39173/diff/
>
>
> Testing
> -------
>
> Unit tests.
>
>
> Thanks,
>
> Kenneth Giusti
>
>