From 47f55aeee06952c0911e95113c089f3637798f9e Mon Sep 17 00:00:00 2001 From: Chenna Keshava Date: Tue, 6 Jun 2023 17:02:09 -0700 Subject: [PATCH 1/5] Fix #4015, #4557: Do not validate GRPC IP/Port/Protocol information in ServerHandler. Pass the responsibility of grpc port information validation to GRPCServer constructor. Both GRPCServer and ServerHandler are created during the construction of Application instance. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduce a constant (SECTION_PORT_GRPC) for port_grpc configuration section Replace all usages of "port_grpc" raw string with a constant Created macros for "port_ws", "port_rpc" and "port_peer" for unit test files. Removed unused imports in some files --- src/ripple/app/main/GRPCServer.cpp | 7 +-- src/ripple/app/misc/NetworkOPs.cpp | 4 +- src/ripple/core/ConfigSections.h | 2 + src/ripple/rpc/impl/ServerHandler.cpp | 9 +++- src/test/jtx/envconfig.h | 5 ++ src/test/jtx/impl/envconfig.cpp | 67 +++++++++++++-------------- src/test/rpc/ReportingETL_test.cpp | 21 +++++---- src/test/rpc/ServerInfo_test.cpp | 2 +- 8 files changed, 67 insertions(+), 50 deletions(-) diff --git a/src/ripple/app/main/GRPCServer.cpp b/src/ripple/app/main/GRPCServer.cpp index 3a5e96b0ed9..12456cc74d8 100644 --- a/src/ripple/app/main/GRPCServer.cpp +++ b/src/ripple/app/main/GRPCServer.cpp @@ -23,6 +23,7 @@ #include #include +#include namespace ripple { @@ -427,9 +428,9 @@ GRPCServerImpl::GRPCServerImpl(Application& app) : app_(app), journal_(app_.journal("gRPC Server")) { // if present, get endpoint from config - if (app_.config().exists("port_grpc")) + if (app_.config().exists(SECTION_PORT_GRPC)) { - const auto& section = app_.config().section("port_grpc"); + Section section = app_.config().section(SECTION_PORT_GRPC); auto const optIp = section.get("ip"); if (!optIp) @@ -659,7 +660,7 @@ GRPCServerImpl::setupListeners() secureGatewayIPs_)); } return requests; -}; +} bool GRPCServerImpl::start() diff --git a/src/ripple/app/misc/NetworkOPs.cpp b/src/ripple/app/misc/NetworkOPs.cpp index 4e91a9d32f6..d1748d85b04 100644 --- a/src/ripple/app/misc/NetworkOPs.cpp +++ b/src/ripple/app/misc/NetworkOPs.cpp @@ -2707,9 +2707,9 @@ NetworkOPsImp::getServerInfo(bool human, bool admin, bool counters) } } - if (app_.config().exists("port_grpc")) + if (app_.config().exists(SECTION_PORT_GRPC)) { - auto const& grpcSection = app_.config().section("port_grpc"); + auto const& grpcSection = app_.config().section(SECTION_PORT_GRPC); auto const optPort = grpcSection.get("port"); if (optPort && grpcSection.get("ip")) { diff --git a/src/ripple/core/ConfigSections.h b/src/ripple/core/ConfigSections.h index 03c702f9a52..a4f18cfa3bd 100644 --- a/src/ripple/core/ConfigSections.h +++ b/src/ripple/core/ConfigSections.h @@ -101,6 +101,8 @@ struct ConfigSection #define SECTION_SWEEP_INTERVAL "sweep_interval" #define SECTION_NETWORK_ID "network_id" +#define SECTION_PORT_GRPC "port_grpc" + } // namespace ripple #endif diff --git a/src/ripple/rpc/impl/ServerHandler.cpp b/src/ripple/rpc/impl/ServerHandler.cpp index f33ecd625aa..424f2f8d001 100644 --- a/src/ripple/rpc/impl/ServerHandler.cpp +++ b/src/ripple/rpc/impl/ServerHandler.cpp @@ -27,6 +27,7 @@ #include #include #include +#include #include #include #include @@ -46,9 +47,7 @@ #include #include #include -#include #include -#include #include namespace ripple { @@ -1149,6 +1148,12 @@ parse_Ports(Config const& config, std::ostream& log) log << "Missing section: [" << name << "]"; Throw(); } + + // grpc ports are parsed by GRPCServer class. Do not validate + // grpc port information in this file. + if (name == SECTION_PORT_GRPC) + continue; + ParsedPort parsed = common; parsed.name = name; parse_Port(parsed, config[name], log); diff --git a/src/test/jtx/envconfig.h b/src/test/jtx/envconfig.h index 59b881539e5..02c99c52f09 100644 --- a/src/test/jtx/envconfig.h +++ b/src/test/jtx/envconfig.h @@ -25,6 +25,11 @@ namespace ripple { namespace test { +// frequently used macros defined here for convinience. +#define PORT_WS "port_ws" +#define PORT_RPC "port_rpc" +#define PORT_PEER "port_peer" + extern std::atomic envUseIPv4; inline const char* diff --git a/src/test/jtx/impl/envconfig.cpp b/src/test/jtx/impl/envconfig.cpp index 7f8163f5ee7..cbca244be6d 100644 --- a/src/test/jtx/impl/envconfig.cpp +++ b/src/test/jtx/impl/envconfig.cpp @@ -20,7 +20,6 @@ #include #include -#include #include namespace ripple { @@ -57,22 +56,22 @@ setupConfigForUnitTests(Config& cfg) cfg.deprecatedClearSection(ConfigSection::importNodeDatabase()); cfg.legacy("database_path", ""); cfg.setupControl(true, true, true); - cfg["server"].append("port_peer"); - cfg["port_peer"].set("ip", getEnvLocalhostAddr()); - cfg["port_peer"].set("port", port_peer); - cfg["port_peer"].set("protocol", "peer"); - - cfg["server"].append("port_rpc"); - cfg["port_rpc"].set("ip", getEnvLocalhostAddr()); - cfg["port_rpc"].set("admin", getEnvLocalhostAddr()); - cfg["port_rpc"].set("port", port_rpc); - cfg["port_rpc"].set("protocol", "http,ws2"); - - cfg["server"].append("port_ws"); - cfg["port_ws"].set("ip", getEnvLocalhostAddr()); - cfg["port_ws"].set("admin", getEnvLocalhostAddr()); - cfg["port_ws"].set("port", port_ws); - cfg["port_ws"].set("protocol", "ws"); + cfg["server"].append(PORT_PEER); + cfg[PORT_PEER].set("ip", getEnvLocalhostAddr()); + cfg[PORT_PEER].set("port", port_peer); + cfg[PORT_PEER].set("protocol", "peer"); + + cfg["server"].append(PORT_RPC); + cfg[PORT_RPC].set("ip", getEnvLocalhostAddr()); + cfg[PORT_RPC].set("admin", getEnvLocalhostAddr()); + cfg[PORT_RPC].set("port", port_rpc); + cfg[PORT_RPC].set("protocol", "http,ws2"); + + cfg["server"].append(PORT_WS); + cfg[PORT_WS].set("ip", getEnvLocalhostAddr()); + cfg[PORT_WS].set("admin", getEnvLocalhostAddr()); + cfg[PORT_WS].set("port", port_ws); + cfg[PORT_WS].set("protocol", "ws"); cfg.SSL_VERIFY = false; } @@ -81,35 +80,35 @@ namespace jtx { std::unique_ptr no_admin(std::unique_ptr cfg) { - (*cfg)["port_rpc"].set("admin", ""); - (*cfg)["port_ws"].set("admin", ""); + (*cfg)[PORT_RPC].set("admin", ""); + (*cfg)[PORT_WS].set("admin", ""); return cfg; } std::unique_ptr secure_gateway(std::unique_ptr cfg) { - (*cfg)["port_rpc"].set("admin", ""); - (*cfg)["port_ws"].set("admin", ""); - (*cfg)["port_rpc"].set("secure_gateway", getEnvLocalhostAddr()); + (*cfg)[PORT_RPC].set("admin", ""); + (*cfg)[PORT_WS].set("admin", ""); + (*cfg)[PORT_RPC].set("secure_gateway", getEnvLocalhostAddr()); return cfg; } std::unique_ptr admin_localnet(std::unique_ptr cfg) { - (*cfg)["port_rpc"].set("admin", "127.0.0.0/8"); - (*cfg)["port_ws"].set("admin", "127.0.0.0/8"); + (*cfg)[PORT_RPC].set("admin", "127.0.0.0/8"); + (*cfg)[PORT_WS].set("admin", "127.0.0.0/8"); return cfg; } std::unique_ptr secure_gateway_localnet(std::unique_ptr cfg) { - (*cfg)["port_rpc"].set("admin", ""); - (*cfg)["port_ws"].set("admin", ""); - (*cfg)["port_rpc"].set("secure_gateway", "127.0.0.0/8"); - (*cfg)["port_ws"].set("secure_gateway", "127.0.0.0/8"); + (*cfg)[PORT_RPC].set("admin", ""); + (*cfg)[PORT_WS].set("admin", ""); + (*cfg)[PORT_RPC].set("secure_gateway", "127.0.0.0/8"); + (*cfg)[PORT_WS].set("secure_gateway", "127.0.0.0/8"); return cfg; } @@ -127,7 +126,7 @@ validator(std::unique_ptr cfg, std::string const& seed) std::unique_ptr port_increment(std::unique_ptr cfg, int increment) { - for (auto const sectionName : {"port_peer", "port_rpc", "port_ws"}) + for (auto const sectionName : {PORT_PEER, PORT_RPC, PORT_WS}) { Section& s = (*cfg)[sectionName]; auto const port = s.get("port"); @@ -143,8 +142,8 @@ std::unique_ptr addGrpcConfig(std::unique_ptr cfg) { std::string port_grpc = std::to_string(port_base + 3); - (*cfg)["port_grpc"].set("ip", getEnvLocalhostAddr()); - (*cfg)["port_grpc"].set("port", port_grpc); + (*cfg)[SECTION_PORT_GRPC].set("ip", getEnvLocalhostAddr()); + (*cfg)[SECTION_PORT_GRPC].set("port", port_grpc); return cfg; } @@ -154,9 +153,9 @@ addGrpcConfigWithSecureGateway( std::string const& secureGateway) { std::string port_grpc = std::to_string(port_base + 3); - (*cfg)["port_grpc"].set("ip", getEnvLocalhostAddr()); - (*cfg)["port_grpc"].set("port", port_grpc); - (*cfg)["port_grpc"].set("secure_gateway", secureGateway); + (*cfg)[SECTION_PORT_GRPC].set("ip", getEnvLocalhostAddr()); + (*cfg)[SECTION_PORT_GRPC].set("port", port_grpc); + (*cfg)[SECTION_PORT_GRPC].set("secure_gateway", secureGateway); return cfg; } diff --git a/src/test/rpc/ReportingETL_test.cpp b/src/test/rpc/ReportingETL_test.cpp index ed055d0fd93..078a51d7bcd 100644 --- a/src/test/rpc/ReportingETL_test.cpp +++ b/src/test/rpc/ReportingETL_test.cpp @@ -22,6 +22,7 @@ #include #include +#include #include #include #include @@ -56,7 +57,8 @@ class ReportingETL_test : public beast::unit_test::suite testcase("GetLedger"); using namespace test::jtx; std::unique_ptr config = envconfig(addGrpcConfig); - std::string grpcPort = *(*config)["port_grpc"].get("port"); + std::string grpcPort = + *(*config)[SECTION_PORT_GRPC].get("port"); Env env(*this, std::move(config)); env.close(); @@ -498,7 +500,8 @@ class ReportingETL_test : public beast::unit_test::suite testcase("GetLedgerData"); using namespace test::jtx; std::unique_ptr config = envconfig(addGrpcConfig); - std::string grpcPort = *(*config)["port_grpc"].get("port"); + std::string grpcPort = + *(*config)[SECTION_PORT_GRPC].get("port"); Env env(*this, std::move(config)); auto grpcLedgerData = [&grpcPort]( auto sequence, std::string marker = "") { @@ -620,7 +623,8 @@ class ReportingETL_test : public beast::unit_test::suite testcase("GetLedgerDiff"); using namespace test::jtx; std::unique_ptr config = envconfig(addGrpcConfig); - std::string grpcPort = *(*config)["port_grpc"].get("port"); + std::string grpcPort = + *(*config)[SECTION_PORT_GRPC].get("port"); Env env(*this, std::move(config)); auto grpcLedgerDiff = [&grpcPort]( @@ -735,7 +739,8 @@ class ReportingETL_test : public beast::unit_test::suite testcase("GetLedgerDiff"); using namespace test::jtx; std::unique_ptr config = envconfig(addGrpcConfig); - std::string grpcPort = *(*config)["port_grpc"].get("port"); + std::string grpcPort = + *(*config)[SECTION_PORT_GRPC].get("port"); Env env(*this, std::move(config)); auto grpcLedgerEntry = [&grpcPort](auto sequence, auto key) { @@ -895,7 +900,7 @@ class ReportingETL_test : public beast::unit_test::suite std::unique_ptr config = envconfig( addGrpcConfigWithSecureGateway, getEnvLocalhostAddr()); std::string grpcPort = - *(*config)["port_grpc"].get("port"); + *(*config)[SECTION_PORT_GRPC].get("port"); Env env(*this, std::move(config)); env.close(); @@ -955,7 +960,7 @@ class ReportingETL_test : public beast::unit_test::suite std::unique_ptr config = envconfig(addGrpcConfigWithSecureGateway, secureGatewayIp); std::string grpcPort = - *(*config)["port_grpc"].get("port"); + *(*config)[SECTION_PORT_GRPC].get("port"); Env env(*this, std::move(config)); env.close(); @@ -1008,7 +1013,7 @@ class ReportingETL_test : public beast::unit_test::suite std::unique_ptr config = envconfig( addGrpcConfigWithSecureGateway, getEnvLocalhostAddr()); std::string grpcPort = - *(*config)["port_grpc"].get("port"); + *(*config)[SECTION_PORT_GRPC].get("port"); Env env(*this, std::move(config)); env.close(); @@ -1065,7 +1070,7 @@ class ReportingETL_test : public beast::unit_test::suite std::unique_ptr config = envconfig(addGrpcConfigWithSecureGateway, secureGatewayIp); std::string grpcPort = - *(*config)["port_grpc"].get("port"); + *(*config)[SECTION_PORT_GRPC].get("port"); Env env(*this, std::move(config)); env.close(); diff --git a/src/test/rpc/ServerInfo_test.cpp b/src/test/rpc/ServerInfo_test.cpp index a69483cb130..72231a3c8e9 100644 --- a/src/test/rpc/ServerInfo_test.cpp +++ b/src/test/rpc/ServerInfo_test.cpp @@ -105,7 +105,7 @@ admin = 127.0.0.1 auto const rpc_port = (*config)["port_rpc"].get("port"); auto const grpc_port = - (*config)["port_grpc"].get("port"); + (*config)[SECTION_PORT_GRPC].get("port"); auto const ws_port = (*config)["port_ws"].get("port"); BEAST_EXPECT(grpc_port); BEAST_EXPECT(rpc_port); From 54d443aed869b217f2d341bc87864e4cd18a46ed Mon Sep 17 00:00:00 2001 From: Chenna Keshava Date: Mon, 25 Sep 2023 15:39:16 -0700 Subject: [PATCH 2/5] fix the compiler error by including ConfigSections in ServerInfo unit test --- src/test/rpc/ServerInfo_test.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/test/rpc/ServerInfo_test.cpp b/src/test/rpc/ServerInfo_test.cpp index 72231a3c8e9..f6edcd39a33 100644 --- a/src/test/rpc/ServerInfo_test.cpp +++ b/src/test/rpc/ServerInfo_test.cpp @@ -19,6 +19,7 @@ #include #include +#include #include #include From dc4a1beb066afbce6ed123923b1d134aea68e372 Mon Sep 17 00:00:00 2001 From: Chenna Keshava B S <21219765+ckeshava@users.noreply.github.com> Date: Wed, 27 Sep 2023 12:12:27 -0700 Subject: [PATCH 3/5] use const& whereever possible, addressing Ed's suggestions --- src/ripple/app/main/GRPCServer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ripple/app/main/GRPCServer.cpp b/src/ripple/app/main/GRPCServer.cpp index 12456cc74d8..a535a4a1a53 100644 --- a/src/ripple/app/main/GRPCServer.cpp +++ b/src/ripple/app/main/GRPCServer.cpp @@ -430,7 +430,7 @@ GRPCServerImpl::GRPCServerImpl(Application& app) // if present, get endpoint from config if (app_.config().exists(SECTION_PORT_GRPC)) { - Section section = app_.config().section(SECTION_PORT_GRPC); + Section const& section = app_.config().section(SECTION_PORT_GRPC); auto const optIp = section.get("ip"); if (!optIp) From 429593d1f7391359f853c188c3947a5efce302ca Mon Sep 17 00:00:00 2001 From: Chenna Keshava Date: Tue, 10 Oct 2023 12:47:35 -0700 Subject: [PATCH 4/5] alphabetically order the #defines --- src/ripple/core/ConfigSections.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/ripple/core/ConfigSections.h b/src/ripple/core/ConfigSections.h index a4f18cfa3bd..8e52d452459 100644 --- a/src/ripple/core/ConfigSections.h +++ b/src/ripple/core/ConfigSections.h @@ -73,6 +73,7 @@ struct ConfigSection #define SECTION_PEERS_MAX "peers_max" #define SECTION_PEERS_IN_MAX "peers_in_max" #define SECTION_PEERS_OUT_MAX "peers_out_max" +#define SECTION_PORT_GRPC "port_grpc" #define SECTION_REDUCE_RELAY "reduce_relay" #define SECTION_RELATIONAL_DB "relational_db" #define SECTION_RELAY_PROPOSALS "relay_proposals" @@ -101,8 +102,6 @@ struct ConfigSection #define SECTION_SWEEP_INTERVAL "sweep_interval" #define SECTION_NETWORK_ID "network_id" -#define SECTION_PORT_GRPC "port_grpc" - } // namespace ripple #endif From f61e7185a0f02919349f20b3a3034b7fc9d03043 Mon Sep 17 00:00:00 2001 From: Chenna Keshava Date: Tue, 10 Oct 2023 12:50:32 -0700 Subject: [PATCH 5/5] alphabetically order all the #defines (not relevant to the GRPCServer issue at hand) --- src/ripple/core/ConfigSections.h | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/ripple/core/ConfigSections.h b/src/ripple/core/ConfigSections.h index 8e52d452459..b4e460f1cfc 100644 --- a/src/ripple/core/ConfigSections.h +++ b/src/ripple/core/ConfigSections.h @@ -49,6 +49,7 @@ struct ConfigSection // VFALCO TODO Rename and replace these macros with variables. #define SECTION_AMENDMENTS "amendments" #define SECTION_AMENDMENT_MAJORITY_TIME "amendment_majority_time" +#define SECTION_BETA_RPC_API "beta_rpc_api" #define SECTION_CLUSTER_NODES "cluster_nodes" #define SECTION_COMPRESSION "compression" #define SECTION_DEBUG_LOGFILE "debug_logfile" @@ -57,10 +58,13 @@ struct ConfigSection #define SECTION_FETCH_DEPTH "fetch_depth" #define SECTION_HISTORICAL_SHARD_PATHS "historical_shard_paths" #define SECTION_INSIGHT "insight" +#define SECTION_IO_WORKERS "io_workers" #define SECTION_IPS "ips" #define SECTION_IPS_FIXED "ips_fixed" #define SECTION_LEDGER_HISTORY "ledger_history" +#define SECTION_LEDGER_REPLAY "ledger_replay" #define SECTION_MAX_TRANSACTIONS "max_transactions" +#define SECTION_NETWORK_ID "network_id" #define SECTION_NETWORK_QUORUM "network_quorum" #define SECTION_NODE_SEED "node_seed" #define SECTION_NODE_SIZE "node_size" @@ -74,6 +78,7 @@ struct ConfigSection #define SECTION_PEERS_IN_MAX "peers_in_max" #define SECTION_PEERS_OUT_MAX "peers_out_max" #define SECTION_PORT_GRPC "port_grpc" +#define SECTION_PREFETCH_WORKERS "prefetch_workers" #define SECTION_REDUCE_RELAY "reduce_relay" #define SECTION_RELATIONAL_DB "relational_db" #define SECTION_RELAY_PROPOSALS "relay_proposals" @@ -85,6 +90,7 @@ struct ConfigSection #define SECTION_SSL_VERIFY_FILE "ssl_verify_file" #define SECTION_SSL_VERIFY_DIR "ssl_verify_dir" #define SECTION_SERVER_DOMAIN "server_domain" +#define SECTION_SWEEP_INTERVAL "sweep_interval" #define SECTION_VALIDATORS_FILE "validators_file" #define SECTION_VALIDATION_SEED "validation_seed" #define SECTION_VALIDATOR_KEYS "validator_keys" @@ -95,12 +101,6 @@ struct ConfigSection #define SECTION_VALIDATOR_TOKEN "validator_token" #define SECTION_VETO_AMENDMENTS "veto_amendments" #define SECTION_WORKERS "workers" -#define SECTION_IO_WORKERS "io_workers" -#define SECTION_PREFETCH_WORKERS "prefetch_workers" -#define SECTION_LEDGER_REPLAY "ledger_replay" -#define SECTION_BETA_RPC_API "beta_rpc_api" -#define SECTION_SWEEP_INTERVAL "sweep_interval" -#define SECTION_NETWORK_ID "network_id" } // namespace ripple