You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Chug Rolke <cr...@redhat.com> on 2012/04/24 22:26:17 UTC
Review Request: C++ Broker: Add limits to connections from users/hosts. Add
timers to discard stalled connection attempts.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4857/
-----------------------------------------------------------
Review request for qpid, Andrew Stitcher, Gordon Sim, and Ted Ross.
Summary
-------
One user can consume all connections to the broker as a denial of service attack. This patch provides command line limits to the number of connections made by an individual user or by a host computer.
The user is tracked by the connection user name and hosts are tracked by the client computer's IP address as seen in the connection's management ID.
This code uses the broker::ConnectionObserver facility.
This patch does NOT time out lower level socket connections such as when a user telnets in to the qpid broker socket and then transfers no data. To effect this function requires the addition of a transport/socket observer facility similar to the ConnectionObserver or to have those functions built into the lower layers.
This code is added as part of the ACL plugin. If the ACL plugin is not loaded then the functions are unavaliable and there is zero performance impact. Individual tracking limits may be disabled by setting their AclOptions values to 0.
This addresses bug QPID-2616.
https://issues.apache.org/jira/browse/QPID-2616
Diffs
-----
trunk/qpid/cpp/src/CMakeLists.txt 1329920
trunk/qpid/cpp/src/qpid/acl/Acl.h 1329920
trunk/qpid/cpp/src/qpid/acl/Acl.cpp 1329920
trunk/qpid/cpp/src/qpid/acl/AclConnectionCounter.h PRE-CREATION
trunk/qpid/cpp/src/qpid/acl/AclConnectionCounter.cpp PRE-CREATION
trunk/qpid/cpp/src/qpid/acl/AclPlugin.cpp 1329920
Diff: https://reviews.apache.org/r/4857/diff
Testing
-------
in the works - to be tested as part of acl.py suite.
Thanks,
Chug
Re: Review Request: C++ Broker: Add limits to connections from users/hosts.
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4857/#review7296
-----------------------------------------------------------
Ship it!
Not entirely sure this belongs in ACL at present, but otherwise looks good.
- Gordon
On 2012-04-26 20:08:30, Chug Rolke wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4857/
> -----------------------------------------------------------
>
> (Updated 2012-04-26 20:08:30)
>
>
> Review request for qpid, Andrew Stitcher, Gordon Sim, and Ted Ross.
>
>
> Summary
> -------
>
> One user can consume all connections to the broker as a denial of service attack. This patch provides command line limits to the number of connections made by an individual user or by a host computer.
>
> The user is tracked by the connection user name and hosts are tracked by the client computer's IP address as seen in the connection's management ID.
>
> This code uses the broker::ConnectionObserver facility.
>
> This patch does NOT time out lower level socket connections such as when a user telnets in to the qpid broker socket and then transfers no data. To effect this function requires the addition of a transport/socket observer facility similar to the ConnectionObserver or to have those functions built into the lower layers.
>
> This code is added as part of the ACL plugin. If the ACL plugin is not loaded then the functions are unavaliable and there is zero performance impact. Individual tracking limits may be disabled by setting their AclOptions values to 0.
>
>
> This addresses bug QPID-2616.
> https://issues.apache.org/jira/browse/QPID-2616
>
>
> Diffs
> -----
>
> trunk/qpid/cpp/src/qpid/acl/AclPlugin.cpp 1330296
> trunk/qpid/cpp/src/qpid/acl/AclConnectionCounter.h PRE-CREATION
> trunk/qpid/cpp/src/qpid/acl/AclConnectionCounter.cpp PRE-CREATION
> trunk/qpid/cpp/src/CMakeLists.txt 1330296
> trunk/qpid/cpp/src/acl.mk 1330296
> trunk/qpid/cpp/src/qpid/acl/Acl.h 1330296
> trunk/qpid/cpp/src/qpid/acl/Acl.cpp 1330296
>
> Diff: https://reviews.apache.org/r/4857/diff
>
>
> Testing
> -------
>
> in the works - to be tested as part of acl.py suite.
>
>
> Thanks,
>
> Chug
>
>
Re: Review Request: C++ Broker: Add limits to connections from users/hosts.
Posted by Chug Rolke <cr...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4857/
-----------------------------------------------------------
(Updated 2012-04-26 20:08:30.010970)
Review request for qpid, Andrew Stitcher, Gordon Sim, and Ted Ross.
Changes
-------
Incorporate review comments.
Remove trying to specify or track low level connections in this patch.
Summary (updated)
-------
One user can consume all connections to the broker as a denial of service attack. This patch provides command line limits to the number of connections made by an individual user or by a host computer.
The user is tracked by the connection user name and hosts are tracked by the client computer's IP address as seen in the connection's management ID.
This code uses the broker::ConnectionObserver facility.
This patch does NOT time out lower level socket connections such as when a user telnets in to the qpid broker socket and then transfers no data. To effect this function requires the addition of a transport/socket observer facility similar to the ConnectionObserver or to have those functions built into the lower layers.
This code is added as part of the ACL plugin. If the ACL plugin is not loaded then the functions are unavaliable and there is zero performance impact. Individual tracking limits may be disabled by setting their AclOptions values to 0.
This addresses bug QPID-2616.
https://issues.apache.org/jira/browse/QPID-2616
Diffs (updated)
-----
trunk/qpid/cpp/src/qpid/acl/AclPlugin.cpp 1330296
trunk/qpid/cpp/src/qpid/acl/AclConnectionCounter.h PRE-CREATION
trunk/qpid/cpp/src/qpid/acl/AclConnectionCounter.cpp PRE-CREATION
trunk/qpid/cpp/src/CMakeLists.txt 1330296
trunk/qpid/cpp/src/acl.mk 1330296
trunk/qpid/cpp/src/qpid/acl/Acl.h 1330296
trunk/qpid/cpp/src/qpid/acl/Acl.cpp 1330296
Diff: https://reviews.apache.org/r/4857/diff
Testing
-------
in the works - to be tested as part of acl.py suite.
Thanks,
Chug
Re: Review Request: C++ Broker: Add limits to connections from users/hosts.
Add timers to discard stalled connection attempts.
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4857/#review7264
-----------------------------------------------------------
trunk/qpid/cpp/src/qpid/acl/AclConnectionCounter.h
<https://reviews.apache.org/r/4857/#comment16049>
does this typedef need to be public?
trunk/qpid/cpp/src/qpid/acl/AclConnectionCounter.h
<https://reviews.apache.org/r/4857/#comment16048>
(very minor not: why mutable, since there appear to be no const methods exposed?)
trunk/qpid/cpp/src/qpid/acl/AclConnectionCounter.h
<https://reviews.apache.org/r/4857/#comment16047>
why use shared pointers here?
trunk/qpid/cpp/src/qpid/acl/AclConnectionCounter.cpp
<https://reviews.apache.org/r/4857/#comment16050>
This suggests to me that perhaps a better solution for the timeout would indeed be at a lower level. One of the concerns for example is around SSL handshakes, which would need to complete before the protocol versions (in 0-10).
trunk/qpid/cpp/src/qpid/acl/AclConnectionCounter.cpp
<https://reviews.apache.org/r/4857/#comment16051>
There is some duplication between this chunk of code and the very similar code above for user names... perhaps this could be encapsulated in a generic incrementing method that takes a key, a map and returns a bool indicating success or failure?
trunk/qpid/cpp/src/qpid/acl/AclConnectionCounter.cpp
<https://reviews.apache.org/r/4857/#comment16052>
again, feels like we could have a little less duplication
- Gordon
On 2012-04-24 20:26:17, Chug Rolke wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4857/
> -----------------------------------------------------------
>
> (Updated 2012-04-24 20:26:17)
>
>
> Review request for qpid, Andrew Stitcher, Gordon Sim, and Ted Ross.
>
>
> Summary
> -------
>
> One user can consume all connections to the broker as a denial of service attack. This patch provides command line limits to the number of connections made by an individual user or by a host computer.
>
> The user is tracked by the connection user name and hosts are tracked by the client computer's IP address as seen in the connection's management ID.
>
> This code uses the broker::ConnectionObserver facility.
>
> This patch does NOT time out lower level socket connections such as when a user telnets in to the qpid broker socket and then transfers no data. To effect this function requires the addition of a transport/socket observer facility similar to the ConnectionObserver or to have those functions built into the lower layers.
>
> This code is added as part of the ACL plugin. If the ACL plugin is not loaded then the functions are unavaliable and there is zero performance impact. Individual tracking limits may be disabled by setting their AclOptions values to 0.
>
>
> This addresses bug QPID-2616.
> https://issues.apache.org/jira/browse/QPID-2616
>
>
> Diffs
> -----
>
> trunk/qpid/cpp/src/CMakeLists.txt 1329920
> trunk/qpid/cpp/src/qpid/acl/Acl.h 1329920
> trunk/qpid/cpp/src/qpid/acl/Acl.cpp 1329920
> trunk/qpid/cpp/src/qpid/acl/AclConnectionCounter.h PRE-CREATION
> trunk/qpid/cpp/src/qpid/acl/AclConnectionCounter.cpp PRE-CREATION
> trunk/qpid/cpp/src/qpid/acl/AclPlugin.cpp 1329920
>
> Diff: https://reviews.apache.org/r/4857/diff
>
>
> Testing
> -------
>
> in the works - to be tested as part of acl.py suite.
>
>
> Thanks,
>
> Chug
>
>