MB-28825: Let command timings be protected by SimpleStats privilege

trondn · trondn · commit d4c951fb9d65 · 2018-03-23T13:41:57.000Z
Users with SimpleStats (assigned by DataMonitoring role) may query the bucket for command timings for all of the buckets they have SimpleStats privilege. This means that if user foo with access to bucket1 and bucket2, but not bucket3 tries to run: mctimings -v GET or mctimings -v -b /all/ GET The aggregated stats of bucket1 and bucket2 is returned Change-Id: Ia69fc1582cd7add4d972bb9bf99a84181f7330c5 Reviewed-on: http://review.couchbase.org/91494 Well-Formed: Build Bot <build@couchbase.com> Reviewed-by: Dave Rigby <daver@couchbase.com> Tested-by: Build Bot <build@couchbase.com>
diff --git a/daemon/mcbp_privileges.cc b/daemon/mcbp_privileges.cc
@@ -283,7 +283,7 @@ McbpPrivilegeChains::McbpPrivilegeChains() {
     setup(PROTOCOL_BINARY_CMD_SSL_CERTS_REFRESH,
           require<Privilege::SecurityManagement>);
     /* Internal timer ioctl */
-    setup(PROTOCOL_BINARY_CMD_GET_CMD_TIMER, require<Privilege::NodeManagement>);
+    setup(PROTOCOL_BINARY_CMD_GET_CMD_TIMER, empty);
     /* ns_server - memcached session validation */
     setup(PROTOCOL_BINARY_CMD_SET_CTRL_TOKEN, require<Privilege::SessionManagement>);
     setup(PROTOCOL_BINARY_CMD_GET_CTRL_TOKEN, require<Privilege::SessionManagement>);
diff --git a/daemon/protocol/mcbp/get_cmd_timer_executor.cc b/daemon/protocol/mcbp/get_cmd_timer_executor.cc
@@ -21,6 +21,105 @@
 #include <daemon/mcbp.h>
 #include <daemon/buckets.h>
 
+/**
+ * Get the timing histogram for the specified bucket if we've got access
+ * to the bucket.
+ *
+ * @param connection The connection executing the command
+ * @param bucket The bucket to get the timing data from
+ * @param opcode The opcode to get the timing histogram for
+ * @return A std::pair with the first being the error code for the operation
+ *         and the second being the histogram (only valid if the first
+ *         parameter is ENGINE_SUCCESS)
+ */
+static std::pair<ENGINE_ERROR_CODE, TimingHistogram> get_timings(
+        McbpConnection& connection, const Bucket& bucket, uint8_t opcode) {
+    // Don't creata a new privilege context if the one we've got is for the
+    // connected bucket:
+    if (bucket.name == connection.getBucket().name) {
+        auto ret = mcbp::checkPrivilege(connection,
+                                        cb::rbac::Privilege::SimpleStats);
+        if (ret != ENGINE_SUCCESS) {
+            return std::make_pair(ENGINE_EACCESS, TimingHistogram{});
+        }
+    } else {
+        // Check to see if we've got access to the bucket
+        bool access = false;
+        try {
+            auto context = cb::rbac::createContext(connection.getUsername(),
+                                                   bucket.name);
+            const auto check = context.check(cb::rbac::Privilege::SimpleStats);
+            if (check == cb::rbac::PrivilegeAccess::Ok) {
+                access = true;
+            }
+        } catch (const cb::rbac::Exception& e) {
+            // We don't have access to that bucket
+        }
+
+        if (!access) {
+            return std::make_pair(ENGINE_EACCESS, TimingHistogram{});
+        }
+    }
+
+    return std::make_pair(ENGINE_SUCCESS,
+                          bucket.timings.get_timing_histogram(opcode));
+}
+
+/**
+ * Get the command timings for the provided bucket if:
+ *    it's not NoBucket
+ *    it is running
+ *    it has the given name
+ *
+ * @param connection The connection requesting access
+ * @param bucket The bucket to look at
+ * @param opcode The opcode we're interested in
+ * @param bucketname The name of the bucket we want
+ */
+static std::pair<ENGINE_ERROR_CODE, TimingHistogram> maybe_get_timings(
+    McbpConnection& connection, const Bucket& bucket, uint8_t opcode, const std::string& bucketname) {
+
+    std::pair<ENGINE_ERROR_CODE, TimingHistogram> ret = std::make_pair(ENGINE_KEY_ENOENT, TimingHistogram{});
+
+    cb_mutex_enter(&bucket.mutex);
+    try {
+        if (bucket.type != BucketType::NoBucket &&
+            bucket.state == BucketState::Ready && bucketname == bucket.name) {
+            ret = get_timings(connection, bucket, opcode);
+        }
+    } catch (...) {
+        // we don't want to leave the mutex locked
+    }
+    cb_mutex_exit(&bucket.mutex);
+
+    return ret;
+}
+
+/**
+ * Get the aggregated timings across "all" buckets that the connected
+ * client has access to.
+ */
+static std::pair<ENGINE_ERROR_CODE, std::string> get_aggregated_timings(
+        McbpConnection& connection, uint8_t opcode) {
+    TimingHistogram timings;
+    bool found = false;
+
+    for (auto& bucket : all_buckets) {
+        auto bt = maybe_get_timings(connection, bucket, opcode, bucket.name);
+        if (bt.first == ENGINE_SUCCESS) {
+            timings += bt.second;
+            found = true;
+        }
+    }
+
+    if (found) {
+        return std::make_pair(ENGINE_SUCCESS, timings.to_string());
+    }
+
+    // We didn't have access to any buckets!
+    return std::make_pair(ENGINE_EACCESS, std::string{});
+}
+
 std::pair<ENGINE_ERROR_CODE, std::string> get_cmd_timer(
         McbpConnection& connection,
         const protocol_binary_request_get_cmd_timer* req) {
@@ -30,52 +129,44 @@ std::pair<ENGINE_ERROR_CODE, std::string> get_cmd_timer(
     const std::string bucket(key, keylen);
     const auto opcode = req->message.body.opcode;
 
-    if (keylen > 0 && bucket != all_buckets[index].name) {
-        // The user specified the current selected bucket
-        keylen = 0;
+    if (bucket == "/all/") {
+        index = 0;
     }
 
-    if (keylen > 0 || index == 0) {
-        // You need the Stats privilege in order to specify a bucket
-        auto ret = mcbp::checkPrivilege(connection, cb::rbac::Privilege::Stats);
-        if (ret != ENGINE_SUCCESS) {
-            return std::make_pair(ret, "");
-        }
+    if (index == 0) {
+        return get_aggregated_timings(connection, opcode);
     }
 
-    // At this point we know that the user have the appropriate access
-    // and should be permitted to perform the action
-    if (bucket == "/all/") {
-        // The aggregated timings is stored in index 0 (no bucket)
-        index = 0;
-        keylen = 0;
-    }
+    if (bucket.empty() || bucket == all_buckets[index].name) {
+        // The current selected bucket
+        auto bt = get_timings(connection, connection.getBucket(), opcode);
+        if (bt.first == ENGINE_SUCCESS) {
+            return std::make_pair(ENGINE_SUCCESS, bt.second.to_string());
+        }
 
-    if (keylen == 0) {
-        return std::make_pair(ENGINE_SUCCESS,
-                              all_buckets[index].timings.generate(opcode));
+        return std::make_pair(bt.first, std::string{});
     }
 
     // The user specified a bucket... let's locate the bucket
-    std::string str;
+    std::pair<ENGINE_ERROR_CODE, TimingHistogram> ret;
 
-    bool found = false;
-    for (size_t ii = 1; ii < all_buckets.size() && !found; ++ii) {
-        // Need the lock to get the bucket state and name
-        cb_mutex_enter(&all_buckets[ii].mutex);
-        if ((all_buckets[ii].state == BucketState::Ready) &&
-            (bucket == all_buckets[ii].name)) {
-            str = all_buckets[ii].timings.generate(opcode);
-            found = true;
+    for (auto& b : all_buckets) {
+        ret = maybe_get_timings(connection, b, opcode, bucket);
+        if (ret.first != ENGINE_KEY_ENOENT && ret.first != ENGINE_SUCCESS) {
+            break;
         }
-        cb_mutex_exit(&all_buckets[ii].mutex);
     }
 
-    if (found) {
-        return std::make_pair(ENGINE_SUCCESS, str);
+    if (ret.first == ENGINE_SUCCESS) {
+        return std::make_pair(ENGINE_SUCCESS, ret.second.to_string());
+    }
+
+    if (ret.first == ENGINE_KEY_ENOENT) {
+        // Don't tell the user that the bucket doesn't exist
+        ret.first = ENGINE_EACCESS;
     }
 
-    return std::make_pair(ENGINE_KEY_ENOENT, "");
+    return std::make_pair(ret.first, std::string{});
 }
 
 void get_cmd_timer_executor(McbpConnection* c, void* packet) {
diff --git a/daemon/timings.h b/daemon/timings.h
@@ -46,6 +46,13 @@ class Timings {
     cb::sampling::Interval get_interval_mutation_latency();
     cb::sampling::Interval get_interval_lookup_latency();
 
+    /**
+     * Get the underlying timings histogram for the specified opcode
+     */
+    TimingHistogram get_timing_histogram(uint8_t opcode) const {
+        return timings[opcode];
+    }
+
 private:
     // This lock is only held by sample() and some blocks within generate().
     // It guards the various IntervalSeries variables which internally
diff --git a/programs/mctimings/mctimings.cc b/programs/mctimings/mctimings.cc
@@ -381,7 +381,7 @@ static void request_stat_timings(MemcachedBinprotConnection& connection,
 void usage() {
     std::cerr << "Usage mctimings [-h host[:port]] [-p port] [-u user]"
               << " [-P pass] [-S passFromStdin] [-b bucket] [-s]"
-              << "-v [opcode / stat_name]*" << std::endl
+              << " -v [opcode / stat_name]*" << std::endl
               << std::endl
               << "Example:" << std::endl
               << "    mctimings -h localhost:11210 -v GET SET"
diff --git a/protocol/connection/client_mcbp_commands.h b/protocol/connection/client_mcbp_commands.h
@@ -899,6 +899,15 @@ using BinprotTouchResponse = BinprotResponse;
 class BinprotGetCmdTimerCommand
     : public BinprotCommandT<BinprotGetCmdTimerCommand, PROTOCOL_BINARY_CMD_GET_CMD_TIMER> {
 public:
+    BinprotGetCmdTimerCommand() = default;
+    BinprotGetCmdTimerCommand(uint8_t opcode)
+        : BinprotCommandT(), opcode(opcode) {
+    }
+    BinprotGetCmdTimerCommand(const std::string& bucket, uint8_t opcode)
+        : BinprotCommandT(), opcode(opcode) {
+        setKey(bucket);
+    }
+
     void encode(std::vector<uint8_t>& buf) const override;
 
     void setOpcode(uint8_t opcode) {
diff --git a/tests/testapp/CMakeLists.txt b/tests/testapp/CMakeLists.txt
@@ -22,6 +22,7 @@ list(APPEND TESTAPP_SOURCES
      testapp_cert_tests.cc
      testapp_client_test.cc
      testapp_client_test.h
+     testapp_cmd_timers.cc
      testapp_dcp.cc
      testapp_environment.cc
      testapp_environment.h
@@ -250,3 +251,8 @@ ADD_TEST(NAME memcached-regression-tests
          WORKING_DIRECTORY ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}
          COMMAND memcached_testapp --gtest_filter=TransportProtocols/RegressionTest.*)
 SET_TESTS_PROPERTIES(memcached-regression-tests PROPERTIES TIMEOUT 60)
+
+ADD_TEST(NAME memcached-cmd-timers
+         WORKING_DIRECTORY ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}
+         COMMAND memcached_testapp --gtest_filter=CmdTimerTest.*)
+SET_TESTS_PROPERTIES(memcached-cmd-timers PROPERTIES TIMEOUT 200)
diff --git a/tests/testapp/rbac.json b/tests/testapp/rbac.json
@@ -24,7 +24,8 @@
     "buckets": {
       "rbac_test": [
         "Read",
-        "XattrRead"
+        "XattrRead",
+        "SimpleStats"
       ]
     },
     "privileges": [],
diff --git a/tests/testapp/testapp_cmd_timers.cc b/tests/testapp/testapp_cmd_timers.cc
