Qpid
  1. Qpid
  2. QPID-4022

C++ Broker connection limits by host ip and by user name can get confused

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.16
    • Fix Version/s: 0.17
    • Component/s: C++ Broker
    • Labels:
      None

      Description

      The current ACL module uses the ConnectionObserver to watch the life cycle of connections. It tries to disallow the creation of too many connections by a user or from an IP address. However, the method is uses is flawed especially in the cluster case.

      A better strategy to use it to provide approvers in the ConnectionObserver scheme and then to call them:
      1. Limits by IP address are disapproved in the ConnectionFactories. If the limit is reached then the factory does not create the connection codec and the connection never begins a life cycle. This is enforced at the same point in code as the per-broker --max-connection limit using similar enforcement methods.

      2. Limits by user name are disapproved at the same point as user authentication happens. Details to follow.

        Activity

        Hide
        Chuck Rolke added a comment -

        The connection limit specifications SHOULD be the same for all members of a cluster but this is just to make management and log correlation easier. There is no check for the limits being the same or not. The logic used to allow or deny connections should not fail if the cluster members are configured with different limits.

        In practice a connection to a node is allowed or denied based on the current number of connections known by this node. The number of current connections includes the local connections made directly to this node and the shadowed connections made to other cluster members.

        • If a connection is denied due to configured connection limits then none of the other cluster members sees the connection. The connection and its denial are all handled on the local node.

        *If the connection is allowed then the connection is shadowed to the other cluster members who count the connection against their configured limits. Shadowed and local connections count against all three connection limits: --max-connections, --max-connections-per-user, and max-connections-per-ip. Shadowed connections that cause a node to exceed a configured connection limit only emit a warning log message and allow the connection to persist.

        Connections are shadowed among cluster members. If node A has three local connections and node B has three local connections then each node has six connections in total. Configured connection limits are applied to new connections being created on the local node. If nodes A and B have different limits then each node will use a different limit when allowing connections to itself.

        The code in its current state also allows clusters to create more connections than specified on the command line. For instance suppose you have a three node cluster, --max-connections-per-user=4, and user bob is connected three times. Next suppose each cluster member simultaneously receives a new connection from bob. Each cluster member will see three connections from bob and allow the connection. Each shadowed connection propagates to cluster peers and is observed but not denied and bob may now have 6 connections. The rule is that a cluster of M member nodes that specify N connections may have up to (N + M-1) connections.

        Show
        Chuck Rolke added a comment - The connection limit specifications SHOULD be the same for all members of a cluster but this is just to make management and log correlation easier. There is no check for the limits being the same or not. The logic used to allow or deny connections should not fail if the cluster members are configured with different limits. In practice a connection to a node is allowed or denied based on the current number of connections known by this node. The number of current connections includes the local connections made directly to this node and the shadowed connections made to other cluster members. If a connection is denied due to configured connection limits then none of the other cluster members sees the connection. The connection and its denial are all handled on the local node. *If the connection is allowed then the connection is shadowed to the other cluster members who count the connection against their configured limits. Shadowed and local connections count against all three connection limits: --max-connections, --max-connections-per-user, and max-connections-per-ip. Shadowed connections that cause a node to exceed a configured connection limit only emit a warning log message and allow the connection to persist. Connections are shadowed among cluster members. If node A has three local connections and node B has three local connections then each node has six connections in total. Configured connection limits are applied to new connections being created on the local node. If nodes A and B have different limits then each node will use a different limit when allowing connections to itself. The code in its current state also allows clusters to create more connections than specified on the command line. For instance suppose you have a three node cluster, --max-connections-per-user=4, and user bob is connected three times. Next suppose each cluster member simultaneously receives a new connection from bob. Each cluster member will see three connections from bob and allow the connection. Each shadowed connection propagates to cluster peers and is observed but not denied and bob may now have 6 connections. The rule is that a cluster of M member nodes that specify N connections may have up to (N + M-1) connections.
        Hide
        Andrew Stitcher added a comment -

        This patch looks good.

        Just one thing: It looks like ConnectionCounter::connection(broker::Connection&) now has no real function - did I miss something?

        Show
        Andrew Stitcher added a comment - This patch looks good. Just one thing: It looks like ConnectionCounter::connection(broker::Connection&) now has no real function - did I miss something?
        Hide
        Chuck Rolke added a comment -

        I put the last patch up for review a week ago but reviews are in Maintenance.

        Here is the proposed patch, including fixes to the self tests.

        Show
        Chuck Rolke added a comment - I put the last patch up for review a week ago but reviews are in Maintenance. Here is the proposed patch, including fixes to the self tests.

          People

          • Assignee:
            Chuck Rolke
            Reporter:
            Chuck Rolke
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development