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`.
2025-11-06 04:05:53 +00:00 · 2022-06-03 18:09:37 -07:00
parent d318ab612a
commit 5a15229eeb
15 changed files with 166 additions and 65 deletions
--- 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<std::uint64_t>(
-          1,
-          std::numeric_limits<std::uint64_t>::max())}
+    , valCookie_(
+          1 +
+          rand_int(
+              crypto_prng(),
+              std::numeric_limits<std::uint64_t>::max() - 1))
    , nUnlVote_(validatorKeys_.nodeID, j_)
 {
    assert(valCookie_ != 0);
--- a/src/ripple/app/main/Application.cpp
+++ b/src/ripple/app/main/Application.cpp
@@ -52,10 +52,12 @@
 #include <ripple/basics/ByteUtilities.h>
 #include <ripple/basics/PerfLog.h>
 #include <ripple/basics/ResolverAsio.h>
+#include <ripple/basics/random.h>
 #include <ripple/basics/safe_cast.h>
 #include <ripple/beast/asio/io_latency_probe.h>
 #include <ripple/beast/core/LexicalCast.h>
 #include <ripple/core/DatabaseCon.h>
+#include <ripple/crypto/csprng.h>
 #include <ripple/json/json_reader.h>
 #include <ripple/nodestore/DatabaseShard.h>
 #include <ripple/nodestore/DummyScheduler.h>
@@ -165,6 +167,8 @@ public:
    std::unique_ptr<Logs> logs_;
    std::unique_ptr<TimeKeeper> timeKeeper_;

+    std::uint64_t const instanceCookie_;
+
    beast::Journal m_journal;
    std::unique_ptr<perf::PerfLog> 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<std::uint64_t>::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
--- a/src/ripple/app/main/Application.h
+++ b/src/ripple/app/main/Application.h
@@ -28,6 +28,7 @@
 #include <ripple/shamap/FullBelowCache.h>
 #include <ripple/shamap/TreeNodeCache.h>
 #include <boost/asio.hpp>
+#include <boost/program_options.hpp>
 #include <memory>
 #include <mutex>

@@ -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&
--- 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<std::string>(), "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<std::string>(),
+        "Specify the node identity for this server.")(
        "quorum",
        po::value<std::size_t>(),
        "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
--- a/src/ripple/app/main/NodeIdentity.cpp
+++ b/src/ripple/app/main/NodeIdentity.cpp
@@ -20,27 +20,38 @@
 #include <ripple/app/main/Application.h>
 #include <ripple/app/main/NodeIdentity.h>
 #include <ripple/app/rdb/Wallet.h>
-#include <ripple/basics/Log.h>
 #include <ripple/core/Config.h>
 #include <ripple/core/ConfigSections.h>
-#include <boost/format.hpp>
 #include <boost/optional.hpp>

 namespace ripple {

 std::pair<PublicKey, SecretKey>
-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> seed;
+
+    if (cmdline.count("nodeid"))
    {
-        auto const seed = parseBase58<Seed>(
+        seed = parseGenericSeed(cmdline["nodeid"].as<std::string>(), false);
+
+        if (!seed)
+            Throw<std::runtime_error>("Invalid 'nodeid' in command line");
+    }
+    else if (app.config().exists(SECTION_NODE_SEED))
+    {
+        seed = parseBase58<Seed>(
            app.config().section(SECTION_NODE_SEED).lines().front());

        if (!seed)
-            Throw<std::runtime_error>("NodeIdentity: Bad [" SECTION_NODE_SEED
-                                      "] specified");
+            Throw<std::runtime_error>("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);
 }

--- a/src/ripple/app/main/NodeIdentity.h
+++ b/src/ripple/app/main/NodeIdentity.h
@@ -23,13 +23,20 @@
 #include <ripple/app/main/Application.h>
 #include <ripple/protocol/PublicKey.h>
 #include <ripple/protocol/SecretKey.h>
+#include <boost/program_options.hpp>
 #include <utility>

 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<PublicKey, SecretKey>
-getNodeIdentity(Application& app);
+getNodeIdentity(
+    Application& app,
+    boost::program_options::variables_map const& cmdline);

 }  // namespace ripple

--- 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<PublicKey, SecretKey>
 getNodeIdentity(soci::session& session);
--- 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<PublicKey, SecretKey>
 getNodeIdentity(soci::session& session)
 {
--- 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          	|
 |---------------------	|:-----------------:	|:-----------------:	|
--- 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:
    //
--- 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}
 };
--- a/src/ripple/protocol/Seed.h
+++ b/src/ripple/protocol/Seed.h
@@ -116,9 +116,13 @@ template <>
 std::optional<Seed>
 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<Seed>
-parseGenericSeed(std::string const& str);
+parseGenericSeed(std::string const& str, bool rfc1751 = true);

 /** Encode a Seed in RFC1751 format */
 std::string
--- 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<Seed>
-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<Seed>(str))
        return seed;

+    if (rfc1751)
    {
        std::string key;
        if (RFC1751::getKeyFromEnglish(key, str) == 1)
--- 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<std::runtime_error>("Env::AppBundle: setup failed");
    timeKeeper->set(app->getLedgerMaster().getClosedLedger()->info().closeTime);
    app->start(false /*don't start timers*/);
--- 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 <class FwdIt>
-    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);