From 5a15229eeb13b69c8adf1f653b88a8f8b9480546 Mon Sep 17 00:00:00 2001 From: Nik Bougalis Date: Fri, 3 Jun 2022 18:09:37 -0700 Subject: [PATCH] Improve detection & handling of duplicate Node ID: Each node on the network is supposed to have a unique cryptographic identity. Typically, this identity is generated randomly at startup and stored for later reuse in the (poorly named) file `wallet.db`. If the file is copied, it is possible for two nodes to share the same node identity. This is generally not desirable and existing servers will detect and reject connections to other servers that have the same key. This commit achives three things: 1. It improves the detection code to pinpoint instances where two distinct servers with the same key connect with each other. In that case, servers will log an appropriate error and shut down pending intervention by the server's operator. 2. It makes it possible for server administrators to securely and easily generate new cryptographic identities for servers using the new `--newnodeid` command line arguments. When a server is started using this command, it will generate and save a random secure identity. 3. It makes it possible to configure the identity using a command line option, which makes it possible to derive it from data or parameters associated with the container or hardware where the instance is running by passing the `--nodeid` option, followed by a single argument identifying the infomation from which the node's identity is derived. For example, the following command will result in nodes with different hostnames having different node identities: `rippled --nodeid $HOSTNAME` The last option is particularly useful for automated cloud-based deployments that minimize the need for storing state and provide unique deployment identifiers. **Important note for server operators:** Depending on variables outside of the the control of this code, such as operating system version or configuration, permissions, and more, it may be possible for other users or programs to be able to access the command line arguments of other processes on the system. If you are operating in a shared environment, you should avoid using this option, preferring instead to use the `[node_seed]` option in the configuration file, and use permissions to limit exposure of the node seed. A user who gains access to the value used to derive the node's unique identity could impersonate that node. The commit also updates the minimum supported server protocol version to `XRPL/2.1`, which has been supported since version 1.5.0 and eliminates support for `XPRL/2.0`. --- src/ripple/app/consensus/RCLConsensus.cpp | 8 ++-- src/ripple/app/main/Application.cpp | 38 ++++++++++++++--- src/ripple/app/main/Application.h | 10 ++++- src/ripple/app/main/Main.cpp | 6 ++- src/ripple/app/main/NodeIdentity.cpp | 31 ++++++++++---- src/ripple/app/main/NodeIdentity.h | 11 ++++- src/ripple/app/rdb/Wallet.h | 17 ++++++-- src/ripple/app/rdb/impl/Wallet.cpp | 6 +++ src/ripple/overlay/README.md | 4 +- src/ripple/overlay/impl/Handshake.cpp | 39 +++++++++++++---- src/ripple/overlay/impl/ProtocolVersion.cpp | 1 - src/ripple/protocol/Seed.h | 8 +++- src/ripple/protocol/impl/Seed.cpp | 3 +- src/test/jtx/impl/Env.cpp | 2 +- src/test/overlay/ProtocolVersion_test.cpp | 47 +++++++++++---------- 15 files changed, 166 insertions(+), 65 deletions(-) diff --git a/src/ripple/app/consensus/RCLConsensus.cpp b/src/ripple/app/consensus/RCLConsensus.cpp index 31c851eb8..aec747e09 100644 --- a/src/ripple/app/consensus/RCLConsensus.cpp +++ b/src/ripple/app/consensus/RCLConsensus.cpp @@ -86,9 +86,11 @@ RCLConsensus::Adaptor::Adaptor( , inboundTransactions_{inboundTransactions} , j_(journal) , validatorKeys_(validatorKeys) - , valCookie_{rand_int( - 1, - std::numeric_limits::max())} + , valCookie_( + 1 + + rand_int( + crypto_prng(), + std::numeric_limits::max() - 1)) , nUnlVote_(validatorKeys_.nodeID, j_) { assert(valCookie_ != 0); diff --git a/src/ripple/app/main/Application.cpp b/src/ripple/app/main/Application.cpp index b7f18ba34..99f3b060b 100644 --- a/src/ripple/app/main/Application.cpp +++ b/src/ripple/app/main/Application.cpp @@ -52,10 +52,12 @@ #include #include #include +#include #include #include #include #include +#include #include #include #include @@ -165,6 +167,8 @@ public: std::unique_ptr logs_; std::unique_ptr timeKeeper_; + std::uint64_t const instanceCookie_; + beast::Journal m_journal; std::unique_ptr perfLog_; Application::MutexType m_masterMutex; @@ -274,6 +278,11 @@ public: , config_(std::move(config)) , logs_(std::move(logs)) , timeKeeper_(std::move(timeKeeper)) + , instanceCookie_( + 1 + + rand_int( + crypto_prng(), + std::numeric_limits::max() - 1)) , m_journal(logs_->journal("Application")) // PerfLog must be started before any other threads are launched. @@ -508,13 +517,13 @@ public: //-------------------------------------------------------------------------- bool - setup() override; + setup(boost::program_options::variables_map const& cmdline) override; void start(bool withTimers) override; void run() override; void - signalStop() override; + signalStop(std::string msg = "") override; bool checkSigs() const override; void @@ -526,6 +535,12 @@ public: //-------------------------------------------------------------------------- + std::uint64_t + instanceID() const override + { + return instanceCookie_; + } + Logs& logs() override { @@ -1108,7 +1123,7 @@ private: // TODO Break this up into smaller, more digestible initialization segments. bool -ApplicationImp::setup() +ApplicationImp::setup(boost::program_options::variables_map const& cmdline) { // We want to intercept CTRL-C and the standard termination signal SIGTERM // and terminate the process. This handler will NEVER be invoked twice. @@ -1146,8 +1161,10 @@ ApplicationImp::setup() if (logs_->threshold() > kDebug) logs_->threshold(kDebug); } - JLOG(m_journal.info()) << "process starting: " - << BuildInfo::getFullVersionString(); + + JLOG(m_journal.info()) << "Process starting: " + << BuildInfo::getFullVersionString() + << ", Instance Cookie: " << instanceCookie_; if (numberOfThreads(*config_) < 2) { @@ -1265,7 +1282,7 @@ ApplicationImp::setup() if (!config().reporting()) m_orderBookDB.setup(getLedgerMaster().getCurrentLedger()); - nodeIdentity_ = getNodeIdentity(*this); + nodeIdentity_ = getNodeIdentity(*this, cmdline); if (!cluster_->load(config().section(SECTION_CLUSTER_NODES))) { @@ -1627,10 +1644,17 @@ ApplicationImp::run() } void -ApplicationImp::signalStop() +ApplicationImp::signalStop(std::string msg) { if (!isTimeToStop.exchange(true)) + { + if (msg.empty()) + JLOG(m_journal.warn()) << "Server stopping"; + else + JLOG(m_journal.warn()) << "Server stopping: " << msg; + stoppingCondition_.notify_all(); + } } bool diff --git a/src/ripple/app/main/Application.h b/src/ripple/app/main/Application.h index 53155ca4f..3b357deef 100644 --- a/src/ripple/app/main/Application.h +++ b/src/ripple/app/main/Application.h @@ -28,6 +28,7 @@ #include #include #include +#include #include #include @@ -136,13 +137,14 @@ public: virtual ~Application() = default; virtual bool - setup() = 0; + setup(boost::program_options::variables_map const& options) = 0; + virtual void start(bool withTimers) = 0; virtual void run() = 0; virtual void - signalStop() = 0; + signalStop(std::string msg = "") = 0; virtual bool checkSigs() const = 0; virtual void @@ -154,6 +156,10 @@ public: // --- // + /** Returns a 64-bit instance identifier, generated at startup */ + virtual std::uint64_t + instanceID() const = 0; + virtual Logs& logs() = 0; virtual Config& diff --git a/src/ripple/app/main/Main.cpp b/src/ripple/app/main/Main.cpp index 70127afe0..f25b83fd5 100644 --- a/src/ripple/app/main/Main.cpp +++ b/src/ripple/app/main/Main.cpp @@ -372,6 +372,10 @@ run(int argc, char** argv) "conf", po::value(), "Specify the configuration file.")( "debug", "Enable normally suppressed debug logging")( "help,h", "Display this message.")( + "newnodeid", "Generate a new node identity for this server.")( + "nodeid", + po::value(), + "Specify the node identity for this server.")( "quorum", po::value(), "Override the minimum validation quorum.")( @@ -756,7 +760,7 @@ run(int argc, char** argv) auto app = make_Application( std::move(config), std::move(logs), std::move(timeKeeper)); - if (!app->setup()) + if (!app->setup(vm)) return -1; // With our configuration parsed, ensure we have diff --git a/src/ripple/app/main/NodeIdentity.cpp b/src/ripple/app/main/NodeIdentity.cpp index a2051bbb6..e66b9e840 100644 --- a/src/ripple/app/main/NodeIdentity.cpp +++ b/src/ripple/app/main/NodeIdentity.cpp @@ -20,27 +20,38 @@ #include #include #include -#include #include #include -#include #include namespace ripple { std::pair -getNodeIdentity(Application& app) +getNodeIdentity( + Application& app, + boost::program_options::variables_map const& cmdline) { - // If a seed is specified in the configuration file use that directly. - if (app.config().exists(SECTION_NODE_SEED)) + std::optional seed; + + if (cmdline.count("nodeid")) { - auto const seed = parseBase58( + seed = parseGenericSeed(cmdline["nodeid"].as(), false); + + if (!seed) + Throw("Invalid 'nodeid' in command line"); + } + else if (app.config().exists(SECTION_NODE_SEED)) + { + seed = parseBase58( app.config().section(SECTION_NODE_SEED).lines().front()); if (!seed) - Throw("NodeIdentity: Bad [" SECTION_NODE_SEED - "] specified"); + Throw("Invalid [" SECTION_NODE_SEED + "] in configuration file"); + } + if (seed) + { auto secretKey = generateSecretKey(KeyType::secp256k1, *seed); auto publicKey = derivePublicKey(KeyType::secp256k1, secretKey); @@ -48,6 +59,10 @@ getNodeIdentity(Application& app) } auto db = app.getWalletDB().checkoutDb(); + + if (cmdline.count("newnodeid") != 0) + clearNodeIdentity(*db); + return getNodeIdentity(*db); } diff --git a/src/ripple/app/main/NodeIdentity.h b/src/ripple/app/main/NodeIdentity.h index 60deeed85..b82b3657a 100644 --- a/src/ripple/app/main/NodeIdentity.h +++ b/src/ripple/app/main/NodeIdentity.h @@ -23,13 +23,20 @@ #include #include #include +#include #include namespace ripple { -/** The cryptographic credentials identifying this server instance. */ +/** The cryptographic credentials identifying this server instance. + + @param app The application object + @param cmdline The command line parameters passed into the application. + */ std::pair -getNodeIdentity(Application& app); +getNodeIdentity( + Application& app, + boost::program_options::variables_map const& cmdline); } // namespace ripple diff --git a/src/ripple/app/rdb/Wallet.h b/src/ripple/app/rdb/Wallet.h index 6bf6ca9ea..2769e459a 100644 --- a/src/ripple/app/rdb/Wallet.h +++ b/src/ripple/app/rdb/Wallet.h @@ -87,10 +87,19 @@ saveManifests( void addValidatorManifest(soci::session& session, std::string const& serialized); -/** - * @brief getNodeIdentity Returns the public and private keys of this node. - * @param session Session with the database. - * @return Pair of public and private keys. +/** Delete any saved public/private key associated with this node. */ +void +clearNodeIdentity(soci::session& session); + +/** Returns a stable public and private key for this node. + + The node's public identity is defined by a secp256k1 keypair + that is (normally) randomly generated. This function will + return such a keypair, securely generating one if needed. + + @param session Session with the database. + + @return Pair of public and private secp256k1 keys. */ std::pair getNodeIdentity(soci::session& session); diff --git a/src/ripple/app/rdb/impl/Wallet.cpp b/src/ripple/app/rdb/impl/Wallet.cpp index 24715404c..25a06bbd9 100644 --- a/src/ripple/app/rdb/impl/Wallet.cpp +++ b/src/ripple/app/rdb/impl/Wallet.cpp @@ -119,6 +119,12 @@ addValidatorManifest(soci::session& session, std::string const& serialized) tr.commit(); } +void +clearNodeIdentity(soci::session& session) +{ + session << "DELETE FROM NodeIdentity;"; +} + std::pair getNodeIdentity(soci::session& session) { diff --git a/src/ripple/overlay/README.md b/src/ripple/overlay/README.md index 289c5f707..8be890ef7 100644 --- a/src/ripple/overlay/README.md +++ b/src/ripple/overlay/README.md @@ -296,8 +296,8 @@ For more on the Peer Crawler, please visit https://xrpl.org/peer-crawler.html. If present, identifies the hash of the last ledger that the sending server considers to be closed. -The value is presently encoded using **Base64** encoding, but implementations -should support both **Base64** and **HEX** encoding for this value. +The value is encoded as **HEX**, but implementations should support both +**Base64** and **HEX** encoding for this value for legacy purposes. | Field Name | Request | Response | |--------------------- |:-----------------: |:-----------------: | diff --git a/src/ripple/overlay/impl/Handshake.cpp b/src/ripple/overlay/impl/Handshake.cpp index 2ea208f55..793dec19e 100644 --- a/src/ripple/overlay/impl/Handshake.cpp +++ b/src/ripple/overlay/impl/Handshake.cpp @@ -204,6 +204,8 @@ buildHandshake( h.insert("Session-Signature", base64_encode(sig.data(), sig.size())); } + h.insert("Instance-Cookie", std::to_string(app.instanceID())); + if (!app.config().SERVER_DOMAIN.empty()) h.insert("Server-Domain", app.config().SERVER_DOMAIN); @@ -215,14 +217,8 @@ buildHandshake( if (auto const cl = app.getLedgerMaster().getClosedLedger()) { - // TODO: Use hex for these - h.insert( - "Closed-Ledger", - base64_encode(cl->info().hash.begin(), cl->info().hash.size())); - h.insert( - "Previous-Ledger", - base64_encode( - cl->info().parentHash.begin(), cl->info().parentHash.size())); + h.insert("Closed-Ledger", strHex(cl->info().hash)); + h.insert("Previous-Ledger", strHex(cl->info().parentHash)); } } @@ -306,7 +302,34 @@ verifyHandshake( }(); if (publicKey == app.nodeIdentity().first) + { + auto const peerInstanceID = [&headers]() { + std::uint64_t iid = 0; + + if (auto const iter = headers.find("Instance-Cookie"); + iter != headers.end()) + { + if (!beast::lexicalCastChecked(iid, iter->value().to_string())) + throw std::runtime_error("Invalid instance cookie"); + + if (iid == 0) + throw std::runtime_error("Invalid instance cookie"); + } + + return iid; + }(); + + // Attempt to differentiate self-connections as opposed to accidental + // node identity reuse caused by accidental misconfiguration. When we + // detect this, we stop the process and log an error message. + if (peerInstanceID != app.instanceID()) + { + app.signalStop("Remote server is using our node identity"); + throw std::runtime_error("Node identity reuse detected"); + } + throw std::runtime_error("Self connection"); + } // This check gets two birds with one stone: // diff --git a/src/ripple/overlay/impl/ProtocolVersion.cpp b/src/ripple/overlay/impl/ProtocolVersion.cpp index 4931dacb4..9a549b563 100644 --- a/src/ripple/overlay/impl/ProtocolVersion.cpp +++ b/src/ripple/overlay/impl/ProtocolVersion.cpp @@ -36,7 +36,6 @@ namespace ripple { // clang-format off constexpr ProtocolVersion const supportedProtocolList[] { - {2, 0}, {2, 1}, {2, 2} }; diff --git a/src/ripple/protocol/Seed.h b/src/ripple/protocol/Seed.h index c1768d205..2ebc64970 100644 --- a/src/ripple/protocol/Seed.h +++ b/src/ripple/protocol/Seed.h @@ -116,9 +116,13 @@ template <> std::optional parseBase58(std::string const& s); -/** Attempt to parse a string as a seed */ +/** Attempt to parse a string as a seed. + + @param str the string to parse + @param rfc1751 true if we should attempt RFC1751 style parsing (deprecated) + * */ std::optional -parseGenericSeed(std::string const& str); +parseGenericSeed(std::string const& str, bool rfc1751 = true); /** Encode a Seed in RFC1751 format */ std::string diff --git a/src/ripple/protocol/impl/Seed.cpp b/src/ripple/protocol/impl/Seed.cpp index f4c6ee52b..49da20a42 100644 --- a/src/ripple/protocol/impl/Seed.cpp +++ b/src/ripple/protocol/impl/Seed.cpp @@ -87,7 +87,7 @@ parseBase58(std::string const& s) } std::optional -parseGenericSeed(std::string const& str) +parseGenericSeed(std::string const& str, bool rfc1751) { if (str.empty()) return std::nullopt; @@ -111,6 +111,7 @@ parseGenericSeed(std::string const& str) if (auto seed = parseBase58(str)) return seed; + if (rfc1751) { std::string key; if (RFC1751::getKeyFromEnglish(key, str) == 1) diff --git a/src/test/jtx/impl/Env.cpp b/src/test/jtx/impl/Env.cpp index 3445fd1c9..900b9812d 100644 --- a/src/test/jtx/impl/Env.cpp +++ b/src/test/jtx/impl/Env.cpp @@ -83,7 +83,7 @@ Env::AppBundle::AppBundle( std::move(config), std::move(logs), std::move(timeKeeper_)); app = owned.get(); app->logs().threshold(thresh); - if (!app->setup()) + if (!app->setup({})) Throw("Env::AppBundle: setup failed"); timeKeeper->set(app->getLedgerMaster().getClosedLedger()->info().closeTime); app->start(false /*don't start timers*/); diff --git a/src/test/overlay/ProtocolVersion_test.cpp b/src/test/overlay/ProtocolVersion_test.cpp index 81845345d..a5a26fe74 100644 --- a/src/test/overlay/ProtocolVersion_test.cpp +++ b/src/test/overlay/ProtocolVersion_test.cpp @@ -25,22 +25,21 @@ namespace ripple { class ProtocolVersion_test : public beast::unit_test::suite { private: - template - static std::string - join(FwdIt first, FwdIt last, char const* sep = ",") - { - std::string result; - if (first == last) - return result; - result = to_string(*first++); - while (first != last) - result += sep + to_string(*first++); - return result; - } - void check(std::string const& s, std::string const& answer) { + auto join = [](auto first, auto last) { + std::string result; + if (first != last) + { + result = to_string(*first++); + + while (first != last) + result += "," + to_string(*first++); + } + return result; + }; + auto const result = parseProtocolVersions(s); BEAST_EXPECT(join(result.begin(), result.end()) == answer); } @@ -60,20 +59,21 @@ public: // Empty string check("", ""); + + // clang-format off check( - "RTXP/1.1,RTXP/1.2,RTXP/1.3,XRPL/2.1,XRPL/2.0", + "RTXP/1.1,RTXP/1.2,RTXP/1.3,XRPL/2.1,XRPL/2.0,/XRPL/3.0", "XRPL/2.0,XRPL/2.1"); check( - "RTXP/0.9,RTXP/1.01,XRPL/0.3,XRPL/2.01,XRPL/19.04,Oscar/" - "123,NIKB", + "RTXP/0.9,RTXP/1.01,XRPL/0.3,XRPL/2.01,websocket", ""); check( - "XRPL/2.0,RTXP/1.2,XRPL/2.0,XRPL/19.4,XRPL/7.89,XRPL/" - "A.1,XRPL/2.01", + "XRPL/2.0,XRPL/2.0,XRPL/19.4,XRPL/7.89,XRPL/XRPL/3.0,XRPL/2.01", "XRPL/2.0,XRPL/7.89,XRPL/19.4"); check( "XRPL/2.0,XRPL/3.0,XRPL/4,XRPL/,XRPL,OPT XRPL/2.2,XRPL/5.67", "XRPL/2.0,XRPL/3.0,XRPL/5.67"); + // clang-format on } { @@ -81,13 +81,14 @@ public: BEAST_EXPECT(negotiateProtocolVersion("RTXP/1.2") == std::nullopt); BEAST_EXPECT( - negotiateProtocolVersion("RTXP/1.2, XRPL/2.0") == - make_protocol(2, 0)); + negotiateProtocolVersion("RTXP/1.2, XRPL/2.0, XRPL/2.1") == + make_protocol(2, 1)); BEAST_EXPECT( - negotiateProtocolVersion("XRPL/2.0") == make_protocol(2, 0)); + negotiateProtocolVersion("XRPL/2.2") == make_protocol(2, 2)); BEAST_EXPECT( - negotiateProtocolVersion("RTXP/1.2, XRPL/2.0, XRPL/999.999") == - make_protocol(2, 0)); + negotiateProtocolVersion( + "RTXP/1.2, XRPL/2.2, XRPL/2.3, XRPL/999.999") == + make_protocol(2, 2)); BEAST_EXPECT( negotiateProtocolVersion("XRPL/999.999, WebSocket/1.0") == std::nullopt);