Uploaded image for project: 'Couchbase Server'
  1. Couchbase Server
  2. MB-34280

Memcached should disallow DCP connections with names greater than 200 characters (was: ns_server connections may be terminated if a client uses a long DCP_OPEN.name)

    XMLWordPrintable

Details

    • KV-Engine MH 2nd Beta

    Description

      Summary

      If a DCP client uses a long (exact length TBD) name when connecting, it can result in
      STATS calls (for example performed by ns_server to monitor rebalance) to fail:

       462445 2019-05-11T08:18:28.525392+00:00 WARNING 143: exception occurred in runloop during packet execution. Cookie info: {"packet":{"magic":"ClientRequest","opcode":"STAT","keylen":3,"extlen":0,"datatype":"raw","vbucket":0,"bodylen":3,"opaque":"0x0","cas":0},"connection":"[ 127.0.0.1:48735 - 127.0.0.1:11209 (<ud>@ns_server</ud>) ]"} - closing connection: Response::setKeylen: key cannot exceed 1 byte 
      

      Details

      Looks like problem is here - we cap the response keyLen to 1 byte for all response types, not just "Response with flex-framing extras" (0x18):

          void setKeylen(uint16_t value) {
              if (value > 0xff) {
                  throw std::invalid_argument(
                          "Response::setKeylen: key cannot exceed 1 byte");
              }
              keylen = uint8_t(value);
          }
      

      This change was introduced via: http://review.couchbase.org/#/c/86393/

      See https://github.com/couchbase/kv_engine/blob/master/docs/BinaryProtocol.md#response-header - it should be permitted to return keys up to 65,535 bytes long for "regular" response packets (0x81).

      Attachments

        Issue Links

          No reviews matched the request. Check your Options in the drop-down menu of this sections header.

          Activity

            drigby Dave Rigby created issue -
            drigby Dave Rigby made changes -
            Field Original Value New Value
            Description Looks like problem is here - we cap the response keyLen to 1 byte for all response types, not just "Response with flex-framing extras" (0x18):

            {code:cpp}
                void setKeylen(uint16_t value) {
                    if (value > 0xff) {
                        throw std::invalid_argument(
                                "Response::setKeylen: key cannot exceed 1 byte");
                    }
                    keylen = uint8_t(value);
                }
            {code}

            See https://github.com/couchbase/kv_engine/blob/master/docs/BinaryProtocol.md#response-header - it should be permitted to return keys up to 65,535 bytes long for "regular" response packets (0x81).
            h2. Summary

            If a DCP client uses a long (exact length TBD) name when connecting, it can result in
            STATS calls (for example performed by ns_server to monitor rebalance) to fail:

            {code}
             462445 2019-05-11T08:18:28.525392+00:00 WARNING 143: exception occurred in runloop during packet execution. Cookie info: {"packet":{"magic":"ClientRequest","opcode":"STAT","keylen":3,"extlen":0,"datatype":"raw","vbucket":0,"bodylen":3,"opaque":"0x0","cas":0},"connection":"[ 127.0.0.1:48735 - 127.0.0.1:11209 (<ud>@ns_server</ud>) ]"} - closing connection: Response::setKeylen: key cannot exceed 1 byte
            {code}

            h2. Details

            Looks like problem is here - we cap the response keyLen to 1 byte for all response types, not just "Response with flex-framing extras" (0x18):

            {code:cpp}
                void setKeylen(uint16_t value) {
                    if (value > 0xff) {
                        throw std::invalid_argument(
                                "Response::setKeylen: key cannot exceed 1 byte");
                    }
                    keylen = uint8_t(value);
                }
            {code}

            See https://github.com/couchbase/kv_engine/blob/master/docs/BinaryProtocol.md#response-header - it should be permitted to return keys up to 65,535 bytes long for "regular" response packets (0x81).
            drigby Dave Rigby made changes -
            Description h2. Summary

            If a DCP client uses a long (exact length TBD) name when connecting, it can result in
            STATS calls (for example performed by ns_server to monitor rebalance) to fail:

            {code}
             462445 2019-05-11T08:18:28.525392+00:00 WARNING 143: exception occurred in runloop during packet execution. Cookie info: {"packet":{"magic":"ClientRequest","opcode":"STAT","keylen":3,"extlen":0,"datatype":"raw","vbucket":0,"bodylen":3,"opaque":"0x0","cas":0},"connection":"[ 127.0.0.1:48735 - 127.0.0.1:11209 (<ud>@ns_server</ud>) ]"} - closing connection: Response::setKeylen: key cannot exceed 1 byte
            {code}

            h2. Details

            Looks like problem is here - we cap the response keyLen to 1 byte for all response types, not just "Response with flex-framing extras" (0x18):

            {code:cpp}
                void setKeylen(uint16_t value) {
                    if (value > 0xff) {
                        throw std::invalid_argument(
                                "Response::setKeylen: key cannot exceed 1 byte");
                    }
                    keylen = uint8_t(value);
                }
            {code}

            See https://github.com/couchbase/kv_engine/blob/master/docs/BinaryProtocol.md#response-header - it should be permitted to return keys up to 65,535 bytes long for "regular" response packets (0x81).
            h2. Summary

            If a DCP client uses a long (exact length TBD) name when connecting, it can result in
            STATS calls (for example performed by ns_server to monitor rebalance) to fail:

            {code}
             462445 2019-05-11T08:18:28.525392+00:00 WARNING 143: exception occurred in runloop during packet execution. Cookie info: {"packet":{"magic":"ClientRequest","opcode":"STAT","keylen":3,"extlen":0,"datatype":"raw","vbucket":0,"bodylen":3,"opaque":"0x0","cas":0},"connection":"[ 127.0.0.1:48735 - 127.0.0.1:11209 (<ud>@ns_server</ud>) ]"} - closing connection: Response::setKeylen: key cannot exceed 1 byte
            {code}

            h2. Details

            Looks like problem is here - we cap the response keyLen to 1 byte for all response types, not just "Response with flex-framing extras" (0x18):

            {code:cpp}
                void setKeylen(uint16_t value) {
                    if (value > 0xff) {
                        throw std::invalid_argument(
                                "Response::setKeylen: key cannot exceed 1 byte");
                    }
                    keylen = uint8_t(value);
                }
            {code}
            This change was introduced via: http://review.couchbase.org/#/c/86393/

            See https://github.com/couchbase/kv_engine/blob/master/docs/BinaryProtocol.md#response-header - it should be permitted to return keys up to 65,535 bytes long for "regular" response packets (0x81).
            drigby Dave Rigby made changes -
            Affects Version/s 5.5.4 [ 16003 ]
            Affects Version/s 6.0.1 [ 15522 ]
            Affects Version/s 5.5.3 [ 15520 ]
            Affects Version/s 5.5.2 [ 15412 ]
            Affects Version/s 5.5.1 [ 15159 ]
            Affects Version/s 5.5.0 [ 14610 ]
            Affects Version/s 6.0.0 [ 15048 ]
            drigby Dave Rigby made changes -
            Is this a Regression? Unknown [ 10452 ] Yes [ 10450 ]
            drigby Dave Rigby made changes -
            Link This issue causes CBSE-6804 [ CBSE-6804 ]
            drigby Dave Rigby made changes -
            Fix Version/s 6.0.3 [ 16164 ]
            trond Trond Norbye made changes -
            Status Open [ 1 ] In Progress [ 3 ]
            trond Trond Norbye made changes -
            Actual Start 2019-05-22 06:48 (issue has been started)
            owend Daniel Owen made changes -
            Rank Ranked lower
            drigby Dave Rigby added a comment -

            Raju Suravarjjala / Wayne Siu requesting for 6.0.3 (note this was initially targeted for 6.0.2 but moved out as too late).

            drigby Dave Rigby added a comment - Raju Suravarjjala / Wayne Siu requesting for 6.0.3 (note this was initially targeted for 6.0.2 but moved out as too late).
            drigby Dave Rigby made changes -
            Affects Version/s 6.0.2 [ 15919 ]
            drigby Dave Rigby made changes -
            Sprint KV-Engine MH Beta Refresh [ 872 ]
            drigby Dave Rigby made changes -
            Rank Ranked lower
            dhaikney David Haikney made changes -
            Link This issue relates to CBSE-7201 [ CBSE-7201 ]
            dfinlay Dave Finlay made changes -
            Assignee Trond Norbye [ trond ] Dave Finlay [ dfinlay ]
            dfinlay Dave Finlay made changes -
            Fix Version/s 6.0.3 [ 16164 ]
            dfinlay Dave Finlay made changes -
            Component/s ns_server [ 10019 ]
            dfinlay Dave Finlay added a comment - - edited

            We discovered this issue on a cluster where the Elastic Search connector connected to a cluster and ns_server "control" connections to memcached started dropping. The elastic search connector has nothing to do with ns_server DCP connections, however, ns_server does monitor the DCP replications it manages and the "dcp" stats command started failing as it retrieves information for all DCP stats, not just ns_server DCP replications. Separately, memcached should not allow DCP clients to connect with connection names that are long enough to cause a subsequent stats call to fail. And lastly clients should know about the name length limitation and ensure that their connection names are less than the limit.

            I would like to fix this issue in the following ways.

            1. Memcached should define the max length of a DCP stat name and disallow DCP connections to be established with names that have a length > 255 - MAX_DCP_STAT_NAME_LENGTH. Currently it looks like the longest stat name is stream_1024_cur_snapshot_prepare, which is 33 characters long. (This ticket now tracks this improvement)
            2. Memcached clients, including ns_server should modify the way their DCP connection names are constructed to ensure they are always less than the max allowed name. (Tracked by MB-35881.)
            3. Lastly, it seems like a good idea to enhance the "dcp" stats call to take an optional connection name prefix so that ns_server can query stats only on the connections it has set up. (Tracked by MB-35882).

            I'll file tickets on these issues and link here.

            dfinlay Dave Finlay added a comment - - edited We discovered this issue on a cluster where the Elastic Search connector connected to a cluster and ns_server "control" connections to memcached started dropping. The elastic search connector has nothing to do with ns_server DCP connections, however, ns_server does monitor the DCP replications it manages and the "dcp" stats command started failing as it retrieves information for all DCP stats, not just ns_server DCP replications. Separately, memcached should not allow DCP clients to connect with connection names that are long enough to cause a subsequent stats call to fail. And lastly clients should know about the name length limitation and ensure that their connection names are less than the limit. I would like to fix this issue in the following ways. Memcached should define the max length of a DCP stat name and disallow DCP connections to be established with names that have a length > 255 - MAX_DCP_STAT_NAME_LENGTH. Currently it looks like the longest stat name is stream_1024_cur_snapshot_prepare , which is 33 characters long. (This ticket now tracks this improvement) Memcached clients, including ns_server should modify the way their DCP connection names are constructed to ensure they are always less than the max allowed name. (Tracked by MB-35881 .) Lastly, it seems like a good idea to enhance the "dcp" stats call to take an optional connection name prefix so that ns_server can query stats only on the connections it has set up. (Tracked by MB-35882 ). I'll file tickets on these issues and link here.

            I took a quick look in mad hatter and I found at least one stats there which is 38 bytes long (plus the prefix: "eq_dcp:" and ":" after the name) which means we're (at least) around 46 characters. I'm tempted to set the max DCP name to 200 characters (looks better than a limit of lets say 207 characters )

            trond Trond Norbye added a comment - I took a quick look in mad hatter and I found at least one stats there which is 38 bytes long (plus the prefix: "eq_dcp:" and ":" after the name) which means we're (at least) around 46 characters. I'm tempted to set the max DCP name to 200 characters (looks better than a limit of lets say 207 characters )
            dfinlay Dave Finlay made changes -
            Summary ns_server connections may be terminated if a client uses a long DCP_OPEN.name Memcached should disallow DCP connections with names greater than 200 characters (was: ns_server connections may be terminated if a client uses a long DCP_OPEN.name)
            dfinlay Dave Finlay made changes -
            Component/s ns_server [ 10019 ]
            dfinlay Dave Finlay made changes -
            Issue Type Bug [ 1 ] Improvement [ 4 ]
            dfinlay Dave Finlay made changes -
            Fix Version/s Cheshire-Cat [ 15915 ]
            Fix Version/s Mad-Hatter [ 15037 ]
            dfinlay Dave Finlay made changes -
            Assignee Dave Finlay [ dfinlay ] Trond Norbye [ trond ]
            dfinlay Dave Finlay made changes -
            Link This issue depends on MB-35881 [ MB-35881 ]
            dfinlay Dave Finlay made changes -
            Link This issue depends on MB-35882 [ MB-35882 ]
            drigby Dave Rigby made changes -
            Sprint KV-Engine MH 2nd Beta [ 872 ] KV-Engine MH 2nd Beta, KV-Engine MH 2nd Beta 2 [ 872, 910 ]
            drigby Dave Rigby made changes -
            Sprint KV-Engine MH 2nd Beta, KV-Engine Mad-Hatter GA [ 872, 910 ] KV-Engine MH 2nd Beta [ 872 ]
            owend Daniel Owen made changes -
            Link This issue causes CBSE-7505 [ CBSE-7505 ]
            trond Trond Norbye made changes -
            Resolution Fixed [ 1 ]
            Status In Progress [ 3 ] Resolved [ 5 ]

            Build couchbase-server-7.0.0-1022 contains kv_engine commit 94b4570 with commit message:
            MB-34280: Set max DCP name to 200 characters

            build-team Couchbase Build Team added a comment - Build couchbase-server-7.0.0-1022 contains kv_engine commit 94b4570 with commit message: MB-34280 : Set max DCP name to 200 characters
            trond Trond Norbye made changes -
            Status Resolved [ 5 ] Closed [ 6 ]
            michael.zilberstein Michael Zilbertstein (Inactive) made changes -
            Link This issue backports to CBSE-7584 [ CBSE-7584 ]
            owend Daniel Owen made changes -
            Link This issue depends on MB-37241 [ MB-37241 ]
            owend Daniel Owen made changes -
            Link This issue depends on MB-37242 [ MB-37242 ]
            owend Daniel Owen made changes -
            Link This issue depends on MB-37243 [ MB-37243 ]
            owend Daniel Owen made changes -
            Link This issue depends on MB-37244 [ MB-37244 ]
            owend Daniel Owen made changes -
            Link This issue depends on MB-37245 [ MB-37245 ]
            owend Daniel Owen made changes -
            Link This issue depends on MB-37246 [ MB-37246 ]
            owend Daniel Owen made changes -
            Link This issue depends on CBG-626 [ CBG-626 ]
            owend Daniel Owen made changes -
            Link This issue depends on JDCP-136 [ JDCP-136 ]
            owend Daniel Owen made changes -
            Link This issue depends on KAFKAC-160 [ KAFKAC-160 ]
            owend Daniel Owen made changes -
            Link This issue depends on CBES-145 [ CBES-145 ]
            david.nault David Nault made changes -
            Link This issue relates to JDCP-137 [ JDCP-137 ]
            owend Daniel Owen made changes -
            Link This issue depends on MB-37283 [ MB-37283 ]
            owend Daniel Owen made changes -
            Resolution Fixed [ 1 ]
            Status Closed [ 6 ] Reopened [ 4 ]
            owend Daniel Owen made changes -
            Resolution Fixed [ 1 ]
            Status Reopened [ 4 ] Closed [ 6 ]
            owend Daniel Owen made changes -
            Link This issue relates to CBSE-10104 [ CBSE-10104 ]
            Elliot.goodwin Elliot Goodwin made changes -
            Link This issue relates to CBSP-3701 [ CBSP-3701 ]
            lynn.straus Lynn Straus made changes -
            Fix Version/s 7.0.0 [ 17233 ]
            lynn.straus Lynn Straus made changes -
            Fix Version/s Cheshire-Cat [ 15915 ]
            drigby Dave Rigby made changes -
            Labels releasenote
            owend Daniel Owen added a comment -

            Release note
            A new restriction has been added where a DCP connection name cannot exceed 200 characters.
            This restriction applies to all DCP connection requests to the data service (memcached).

            owend Daniel Owen added a comment - Release note A new restriction has been added where a DCP connection name cannot exceed 200 characters. This restriction applies to all DCP connection requests to the data service (memcached).
            david.nault David Nault added a comment -

            Hi Dave Rigby, can you clarify whether the limit is 200 characters, or 200 bytes?

            david.nault David Nault added a comment - Hi Dave Rigby , can you clarify whether the limit is 200 characters, or 200 bytes?
            drigby Dave Rigby added a comment -

            Bytes (pretty much everything in memcached binary protocol is byte counts)

            drigby Dave Rigby added a comment - Bytes (pretty much everything in memcached binary protocol is byte counts)
            owend Daniel Owen made changes -
            Fix Version/s 7.0.0 [ 17233 ]
            owend Daniel Owen made changes -
            Fix Version/s 7.0.0 [ 17233 ]
            owend Daniel Owen made changes -
            Resolution Fixed [ 1 ]
            Status Closed [ 6 ] Reopened [ 4 ]
            owend Daniel Owen made changes -
            Fix Version/s 6.6.4 [ 17614 ]

            Daniel Owen Should MB-35881 (ns_server should generate DCP connection names that are no longer than allowed by memcached) also be considered for inclusion in 6.6.4?

            steve.watanabe Steve Watanabe added a comment - Daniel Owen  Should MB-35881 (ns_server should generate DCP connection names that are no longer than allowed by memcached) also be considered for inclusion in 6.6.4?

            I opened MB-49271 to track getting the fix for MB-35881.

            steve.watanabe Steve Watanabe added a comment - I opened MB-49271 to track getting the fix for MB-35881 .
            wayne Wayne Siu made changes -
            Link This issue blocks MB-47673 [ MB-47673 ]
            wayne Wayne Siu made changes -
            Labels releasenote approved-for-6.6.4 releasenote
            meni.hillel Meni Hillel made changes -
            Link This issue depends on MB-49271 [ MB-49271 ]

            Build couchbase-server-6.6.4-9928 contains kv_engine commit b5fa131 with commit message:
            MB-34280: Set max DCP name to 200 characters

            build-team Couchbase Build Team added a comment - Build couchbase-server-6.6.4-9928 contains kv_engine commit b5fa131 with commit message: MB-34280 : Set max DCP name to 200 characters
            trond Trond Norbye made changes -
            Resolution Fixed [ 1 ]
            Status Reopened [ 4 ] Resolved [ 5 ]
            trond Trond Norbye made changes -
            Status Resolved [ 5 ] Closed [ 6 ]
            wayne Wayne Siu made changes -
            Assignee Trond Norbye [ trond ] Ritam Sharma [ ritam.sharma ]
            tony.hillman Tony Hillman made changes -
            Link This issue relates to DOC-9381 [ DOC-9381 ]

            Add to documentation for applicable releases.

            tony.hillman Tony Hillman added a comment - Add to documentation for applicable releases.
            marks.polakovs Marks Polakovs made changes -
            Link This issue is triggering CBSE-11034 [ CBSE-11034 ]
            marks.polakovs Marks Polakovs made changes -
            Link This issue relates to CMOS-154 [ CMOS-154 ]
            callum.majumdar Callum Majumdar made changes -
            Link This issue causes CBSE-11043 [ CBSE-11043 ]

            People

              ritam.sharma Ritam Sharma
              drigby Dave Rigby
              Votes:
              0 Vote for this issue
              Watchers:
              16 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                PagerDuty