From 3c4d3b10c1299607e7d889e83fe74ed5d7425d48 Mon Sep 17 00:00:00 2001 From: Brad Chase Date: Wed, 7 Dec 2016 11:59:44 -0500 Subject: [PATCH] Update RPC handler role/usage (RIPD-557): * Properly use the RPC method to determine required role for HTTP/S RPC calls. * Charge for malformed RPC calls over HTTP/S --- Builds/VisualStudio2015/RippleD.vcxproj | 4 + .../VisualStudio2015/RippleD.vcxproj.filters | 3 + src/ripple/rpc/impl/ServerHandlerImp.cpp | 77 ++++++++-------- src/ripple/server/impl/JSONRPCUtil.cpp | 1 + src/test/rpc/LedgerRequestRPC_test.cpp | 10 ++- src/test/rpc/RPCOverload_test.cpp | 90 +++++++++++++++++++ src/test/support/impl/WSClient.cpp | 11 ++- src/test/unity/rpc_test_unity.cpp | 1 + 8 files changed, 156 insertions(+), 41 deletions(-) create mode 100644 src/test/rpc/RPCOverload_test.cpp diff --git a/Builds/VisualStudio2015/RippleD.vcxproj b/Builds/VisualStudio2015/RippleD.vcxproj index 6958f7cec..4cb7ceeb5 100644 --- a/Builds/VisualStudio2015/RippleD.vcxproj +++ b/Builds/VisualStudio2015/RippleD.vcxproj @@ -4620,6 +4620,10 @@ True True + + True + True + True True diff --git a/Builds/VisualStudio2015/RippleD.vcxproj.filters b/Builds/VisualStudio2015/RippleD.vcxproj.filters index 02ce1446e..df307fb76 100644 --- a/Builds/VisualStudio2015/RippleD.vcxproj.filters +++ b/Builds/VisualStudio2015/RippleD.vcxproj.filters @@ -5262,6 +5262,9 @@ test\rpc + + test\rpc + test\rpc diff --git a/src/ripple/rpc/impl/ServerHandlerImp.cpp b/src/ripple/rpc/impl/ServerHandlerImp.cpp index 8e6d14b6c..7f5341ba7 100644 --- a/src/ripple/rpc/impl/ServerHandlerImp.cpp +++ b/src/ripple/rpc/impl/ServerHandlerImp.cpp @@ -402,6 +402,8 @@ ServerHandlerImp::processSession( if (is->getConsumer().disconnect()) { session->close(); + // FIX: This rpcError is not delivered since the session + // was just closed. return rpcError(rpcSLOW_DOWN); } @@ -438,6 +440,7 @@ ServerHandlerImp::processSession( is->user()); if (Role::FORBID == role) { + loadType = Resource::feeInvalidRPC; jr[jss::result] = rpcError (rpcFORBIDDEN); } else @@ -550,26 +553,12 @@ ServerHandlerImp::processRequest (Port const& port, } } - // Parse id now so errors from here on will have the id - // - // VFALCO NOTE Except that "id" isn't included in the following errors. - // - Json::Value const& id = jsonRPC [jss::id]; + /* ---------------------------------------------------------------------- */ + // Determine role/usage so we can charge for invalid requests Json::Value const& method = jsonRPC [jss::method]; - if (! method) { - HTTPReply (400, "Null method", output, rpcJ); - return; - } - - if (!method.isString ()) { - HTTPReply (400, "method is not string", output, rpcJ); - return; - } - - /* ---------------------------------------------------------------------- */ auto role = Role::FORBID; - auto required = RPC::roleRequired(id.asString()); + auto required = RPC::roleRequired(method.asString()); if (jsonRPC.isMember(jss::params) && jsonRPC[jss::params].isArray() && jsonRPC[jss::params].size() > 0 && @@ -584,16 +573,6 @@ ServerHandlerImp::processRequest (Port const& port, remoteIPAddress, user); } - /** - * Clear header-assigned values if not positively identified from a - * secure_gateway. - */ - if (role != Role::IDENTIFIED) - { - forwardedFor.clear(); - user.clear(); - } - Resource::Consumer usage; if (isUnlimited(role)) { @@ -610,9 +589,31 @@ ServerHandlerImp::processRequest (Port const& port, } } + if (role == Role::FORBID) + { + usage.charge(Resource::feeInvalidRPC); + HTTPReply (403, "Forbidden", output, rpcJ); + return; + } + + if (! method) + { + usage.charge(Resource::feeInvalidRPC); + HTTPReply (400, "Null method", output, rpcJ); + return; + } + + if (! method.isString ()) + { + usage.charge(Resource::feeInvalidRPC); + HTTPReply (400, "method is not string", output, rpcJ); + return; + } + std::string strMethod = method.asString (); if (strMethod.empty()) { + usage.charge(Resource::feeInvalidRPC); HTTPReply (400, "method is empty", output, rpcJ); return; } @@ -630,6 +631,7 @@ ServerHandlerImp::processRequest (Port const& port, else if (!params.isArray () || params.size() != 1) { + usage.charge(Resource::feeInvalidRPC); HTTPReply (400, "params unparseable", output, rpcJ); return; } @@ -638,20 +640,20 @@ ServerHandlerImp::processRequest (Port const& port, params = std::move (params[0u]); if (!params.isObject()) { + usage.charge(Resource::feeInvalidRPC); HTTPReply (400, "params unparseable", output, rpcJ); return; } } - // VFALCO TODO Shouldn't we handle this earlier? - // - if (role == Role::FORBID) + /** + * Clear header-assigned values if not positively identified from a + * secure_gateway. + */ + if (role != Role::IDENTIFIED) { - // VFALCO TODO Needs implementing - // FIXME Needs implementing - // XXX This needs rate limiting to prevent brute forcing password. - HTTPReply (403, "Forbidden", output, rpcJ); - return; + forwardedFor.clear(); + user.clear(); } JLOG(m_journal.debug()) << "Query: " << strMethod << params; @@ -684,6 +686,10 @@ ServerHandlerImp::processRequest (Port const& port, result[jss::status] = jss::success; } + usage.charge (loadType); + if (usage.warn()) + result[jss::warning] = jss::load; + Json::Value reply (Json::objectValue); reply[jss::result] = std::move (result); if (jsonRPC.isMember(jss::jsonrpc)) @@ -702,7 +708,6 @@ ServerHandlerImp::processRequest (Port const& port, response.size ())); response += '\n'; - usage.charge (loadType); if (auto stream = m_journal.debug()) { diff --git a/src/ripple/server/impl/JSONRPCUtil.cpp b/src/ripple/server/impl/JSONRPCUtil.cpp index d0c7d57df..1017a592d 100644 --- a/src/ripple/server/impl/JSONRPCUtil.cpp +++ b/src/ripple/server/impl/JSONRPCUtil.cpp @@ -94,6 +94,7 @@ void HTTPReply ( case 403: output ("HTTP/1.1 403 Forbidden\r\n"); break; case 404: output ("HTTP/1.1 404 Not Found\r\n"); break; case 500: output ("HTTP/1.1 500 Internal Server Error\r\n"); break; + case 503: output ("HTTP/1.1 503 Server is overloaded\r\n"); break; } output (getHTTPHeaderTimestamp ()); diff --git a/src/test/rpc/LedgerRequestRPC_test.cpp b/src/test/rpc/LedgerRequestRPC_test.cpp index fbc3b910e..48b86f3ec 100644 --- a/src/test/rpc/LedgerRequestRPC_test.cpp +++ b/src/test/rpc/LedgerRequestRPC_test.cpp @@ -294,10 +294,12 @@ public: env.fund(XRP(100000), gw); env.close(); - auto const result = env.rpc ( "ledger_request", "1" ) [jss::result]; - BEAST_EXPECT(result[jss::error] == "noPermission"); - BEAST_EXPECT(result[jss::status] == "error"); - BEAST_EXPECT(result[jss::error_message] == "You don't have permission for this command."); + auto const result = env.rpc ( "ledger_request", "1" ) [jss::result]; + // The current HTTP/S ServerHandler returns an HTTP 403 error code here + // rather than a noPermission JSON error. The JSONRPCClient just eats that + // error and returns an null result. + BEAST_EXPECT(result.type() == Json::nullValue); + } void run () diff --git a/src/test/rpc/RPCOverload_test.cpp b/src/test/rpc/RPCOverload_test.cpp new file mode 100644 index 000000000..967ecfac3 --- /dev/null +++ b/src/test/rpc/RPCOverload_test.cpp @@ -0,0 +1,90 @@ +//------------------------------------------------------------------------------ +/* + This file is part of rippled: https://github.com/ripple/rippled + Copyright (c) 2012, 2013 Ripple Labs Inc. + Permission to use, copy, modify, and/or distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#include +#include +#include +#include +#include +#include + +namespace ripple { +namespace test { + +class RPCOverload_test : public beast::unit_test::suite +{ +public: + void testOverload(bool useWS) + { + testcase << "Overload " << (useWS ? "WS" : "HTTP") << " RPC client"; + using namespace jtx; + Env env(*this, []() + { + auto p = std::make_unique(); + setupConfigForUnitTests(*p); + (*p)["port_rpc"].set("admin",""); + (*p)["port_ws"].set("admin",""); + return p; + }()); + + Account const alice {"alice"}; + Account const bob {"bob"}; + env.fund (XRP (10000), alice, bob); + + std::unique_ptr client = useWS ? + makeWSClient(env.app().config()) + : makeJSONRPCClient(env.app().config()); + + Json::Value tx = Json::objectValue; + tx[jss::tx_json] = pay(alice, bob, XRP(1)); + tx[jss::secret] = toBase58(generateSeed("alice")); + + // Ask the server to repeatedly sign this transaction + // Signing is a resource heavy transaction, so we want the server + // to warn and eventually boot us. + bool warned = false, booted = false; + for(int i = 0 ; i < 500 && !booted; ++i) + { + auto jv = client->invoke("sign", tx); + if(!useWS) + jv = jv[jss::result]; + // When booted, we just get a null json response + if(jv.isNull()) + booted = true; + else + BEAST_EXPECT(jv.isMember(jss::status) + && (jv[jss::status] == "success")); + + if(jv.isMember(jss::warning)) + warned = jv[jss::warning] == jss::load; + } + BEAST_EXPECT(warned && booted); + } + + + + void run() override + { + testOverload(false /* http */); + testOverload(true /* ws */); + } +}; + +BEAST_DEFINE_TESTSUITE(RPCOverload,app,ripple); + +} // test +} // ripple diff --git a/src/test/support/impl/WSClient.cpp b/src/test/support/impl/WSClient.cpp index 45592c560..34d771f49 100644 --- a/src/test/support/impl/WSClient.cpp +++ b/src/test/support/impl/WSClient.cpp @@ -27,6 +27,7 @@ #include #include #include + #include #include @@ -97,6 +98,8 @@ class WSClientImpl : public WSClient beast::websocket::opcode op_; beast::streambuf rb_; + bool peerClosed_ = false; + // synchronize destructor bool b0_ = false; std::mutex m0_; @@ -112,7 +115,8 @@ class WSClientImpl : public WSClient void cleanup() { error_code ec; - ws_.close({}, ec); + if(!peerClosed_) + ws_.close({}, ec); stream_.close(ec); work_ = boost::none; thread_.join(); @@ -255,7 +259,12 @@ private: on_read_msg(error_code const& ec) { if(ec) + { + if(ec == beast::websocket::error::closed) + peerClosed_ = true; return; + } + Json::Value jv; Json::Reader jr; jr.parse(buffer_string(rb_.data()), jv); diff --git a/src/test/unity/rpc_test_unity.cpp b/src/test/unity/rpc_test_unity.cpp index 9b31bbbbf..eecd72d94 100644 --- a/src/test/unity/rpc_test_unity.cpp +++ b/src/test/unity/rpc_test_unity.cpp @@ -33,6 +33,7 @@ #include #include #include +#include #include #include #include