Qpid
  1. Qpid
  2. QPID-4854

max-negotiate-time feature breaks AMQP 1.0 (and arguably doesn't achieve the desired objective anyway)

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.20, 0.22
    • Fix Version/s: 0.23
    • Component/s: C++ Broker
    • Labels:
      None

      Description

      It has assumptions based on 0-10 (and a particular pattern for 0-10 at that) built into the underlying transport code (which should be protocol agnostic).

      As AMQP 1.0 makes it much simpler and more likely for less synchronous handshaking, the '3 reads' magic doesn't work and causes 1.0 connections to be terminated incorrectly. Either the original solution needs to be reimplemented or it needs to be possible to disable it for use with 1.0.

      See QPID-4021 and QPID-2518.

        Activity

        Hide
        Andrew Stitcher added a comment - - edited

        The original max-negotiate-time code was never a solution (which is why the original bug remains open) it is just a heuristic that works with the qpid implementation of amqp 0-10 (which is all we supported then).

        The real issue with the current code is that there is no semantic object that exists from the point that the low level connection is accepted until the point that the connection is authenticated, which is when there is no longer any chance of an unauthenticated DoS. This object is what needs to hold the timeout code, not the low-level code which has, as Gordon correctly points out, no idea of the underlying semantics and shouldn't have any idea of it.

        The only solution that I can see that can be made to work is to refactor the Connection object creation so that it happens much earlier in the accepting a connection process and for the Connection object to hold the timeout logic. However this object is currently created as part of the process of making the codec for the specific protocol and so the refactor is larger than I'd wish.

        Any other suggestions welcome.

        Show
        Andrew Stitcher added a comment - - edited The original max-negotiate-time code was never a solution (which is why the original bug remains open) it is just a heuristic that works with the qpid implementation of amqp 0-10 (which is all we supported then). The real issue with the current code is that there is no semantic object that exists from the point that the low level connection is accepted until the point that the connection is authenticated, which is when there is no longer any chance of an unauthenticated DoS. This object is what needs to hold the timeout code, not the low-level code which has, as Gordon correctly points out, no idea of the underlying semantics and shouldn't have any idea of it. The only solution that I can see that can be made to work is to refactor the Connection object creation so that it happens much earlier in the accepting a connection process and for the Connection object to hold the timeout logic. However this object is currently created as part of the process of making the codec for the specific protocol and so the refactor is larger than I'd wish. Any other suggestions welcome.
        Hide
        Gordon Sim added a comment -

        An alternative might be to add an isHandshakeComplete() (or similar) method to the Codec interface allowing the lower layer to ask the upper layer for information about when the timer can be cancelled rather than guessing based on a read count.

        Show
        Gordon Sim added a comment - An alternative might be to add an isHandshakeComplete() (or similar) method to the Codec interface allowing the lower layer to ask the upper layer for information about when the timer can be cancelled rather than guessing based on a read count.
        Hide
        Gordon Sim added a comment -

        Actually, the current heuristic doesn't really work - at least not as a DOS prevention. It simply counts the number of reads, but with very little effort at all you can create a client that write the protocol header, writes three bytes each with a slight interval between them, and thus tricks the current solution into cancelling the timeout while leaving a socket open without having gone through any handshaking.

        This makes the value of the current hack even more questionable in my opinion.

        Show
        Gordon Sim added a comment - Actually, the current heuristic doesn't really work - at least not as a DOS prevention. It simply counts the number of reads, but with very little effort at all you can create a client that write the protocol header, writes three bytes each with a slight interval between them, and thus tricks the current solution into cancelling the timeout while leaving a socket open without having gone through any handshaking. This makes the value of the current hack even more questionable in my opinion.
        Hide
        ASF subversion and git services added a comment -

        Commit 1489458 from Andrew Stitcher
        [ https://svn.apache.org/r1489458 ]

        QPID-4854: Make the protocol negotiation timeout actually relate to
        the protocol negotiation!

        Show
        ASF subversion and git services added a comment - Commit 1489458 from Andrew Stitcher [ https://svn.apache.org/r1489458 ] QPID-4854 : Make the protocol negotiation timeout actually relate to the protocol negotiation!
        Hide
        Andrew Stitcher added a comment -

        Gordon's comment above made me think of an approach similar to his but adding a new hook into the OutputControl interface to tell the lower layer when to consider the connection fully established and so abort the potential timeout.

        This change is dependent on one of the cleanups in QPID-4905 which stops extra unnecessary classes from implementing the OutputControl interface and stops this change from affecting seemingly irrelevant code.

        Show
        Andrew Stitcher added a comment - Gordon's comment above made me think of an approach similar to his but adding a new hook into the OutputControl interface to tell the lower layer when to consider the connection fully established and so abort the potential timeout. This change is dependent on one of the cleanups in QPID-4905 which stops extra unnecessary classes from implementing the OutputControl interface and stops this change from affecting seemingly irrelevant code.
        Hide
        ASF subversion and git services added a comment -

        Commit 1489545 from Andrew Stitcher
        [ https://svn.apache.org/r1489545 ]

        QPID-4854: Removed code now unused since this change

        Show
        ASF subversion and git services added a comment - Commit 1489545 from Andrew Stitcher [ https://svn.apache.org/r1489545 ] QPID-4854 : Removed code now unused since this change
        Hide
        ASF subversion and git services added a comment -

        Commit 1489986 from Andrew Stitcher
        [ https://svn.apache.org/r1489986 ]

        QPID-4854: Removed use of intrusive_ptr::reset to work with older boost versions

        Show
        ASF subversion and git services added a comment - Commit 1489986 from Andrew Stitcher [ https://svn.apache.org/r1489986 ] QPID-4854 : Removed use of intrusive_ptr::reset to work with older boost versions
        Hide
        ASF subversion and git services added a comment -

        Commit 1494645 from Gordon Sim
        [ https://svn.apache.org/r1494645 ]

        QPID-4854: signal connection establishment for broker initiated connections also

        Show
        ASF subversion and git services added a comment - Commit 1494645 from Gordon Sim [ https://svn.apache.org/r1494645 ] QPID-4854 : signal connection establishment for broker initiated connections also
        Hide
        Justin Ross added a comment -
        Show
        Justin Ross added a comment - Released in Qpid 0.24, http://qpid.apache.org/releases/qpid-0.24/index.html

          People

          • Assignee:
            Andrew Stitcher
            Reporter:
            Gordon Sim
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development