Qpid
  1. Qpid
  2. QPID-2616

Qpid C++ broker: disconnect client when handshake incomplete

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Duplicate
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: C++ Broker
    • Labels:
      None
    • Environment:

      Red Hat Enterprise MRG 1.2

      Description

      The broker should disconnect clients if the connection handshake doesn't complete after a configurable time (both for SSL and for non-SSL connections).

      This feature has already been mentioned by G. Sim in the JIRA QPID-2518.

      We are looking for an implementation of this feature and will provide it as soon as we are done.

        Issue Links

          Activity

          Armin Noll created issue -
          Armin Noll made changes -
          Field Original Value New Value
          Description The broker should disconnect clients if the connection handshake doesn't complete after a configurable time (both for SSL and for non-SSL connections).

          We are looking for an implementation of this feature and will provide it as soon as we are done.
          The broker should disconnect clients if the connection handshake doesn't complete after a configurable time (both for SSL and for non-SSL connections).

          This feature has already been mentioned by G. Sim in the JIRA QPID-2518.

          We are looking for an implementation of this feature and will provide it as soon as we are done.
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- 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
          Hide
          Jason Dillaman added a comment -

          It looks like the maximum connection count is managed globally. Would it make more sense to integrate the connection limit into the ACL file (i.e. something along the lines of 'acl allow <user> connection maximum=100') to permit more granular setup?

          Show
          Jason Dillaman added a comment - It looks like the maximum connection count is managed globally. Would it make more sense to integrate the connection limit into the ACL file (i.e. something along the lines of 'acl allow <user> connection maximum=100') to permit more granular setup?
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- 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
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- 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
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- 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
          Justin Ross made changes -
          Assignee Andrew Stitcher [ astitcher ]
          Hide
          Andrew Stitcher added a comment -

          I'm not sure if this bug has always been the same bug as qpid-2518, but with the current title/description it seems to be - this is one of a related set of issues ending and finally (I hope) resolved in qpid-4854

          Show
          Andrew Stitcher added a comment - I'm not sure if this bug has always been the same bug as qpid-2518, but with the current title/description it seems to be - this is one of a related set of issues ending and finally (I hope) resolved in qpid-4854
          Andrew Stitcher made changes -
          Link This issue relates to QPID-2518 [ QPID-2518 ]
          Andrew Stitcher made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Resolution Duplicate [ 3 ]
          Justin Ross made changes -
          Status Resolved [ 5 ] Closed [ 6 ]

            People

            • Assignee:
              Andrew Stitcher
              Reporter:
              Armin Noll
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development