From 89b58da1e85777827e5bdb46e30813105ed07202 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 20 Aug 2026 16:14:56 +0100 Subject: [PATCH] fix: Report telemetry config errors instead of aborting at startup makeTelemetrySetup() rejects a contradictory [telemetry] mutual-TLS setup by throwing, but it is called from ApplicationImp's member-initializer list. A try/catch in the constructor body cannot reach a throw from there, and nothing further up the stack caught it either, so a config mistake reached std::terminate: the default handler printed a terminate dump and raised SIGABRT, leaving a core file instead of a startup error. Catch std::exception around makeApplication() in run(), report the reason on stderr and return -1, so the failure is a clean non-zero exit with a message an operator can act on. Only the construction is wrapped. setup() starts subsystems whose shutdown order is delicate and is left outside deliberately, because unwinding a half-started Application would skip the normal stop sequence. Gate both validation guards on enabled. A node with telemetry switched off previously refused to start over certificate paths that nothing would read. Document both throws on makeTelemetrySetup(), state in cfg/xrpld-example.cfg and the configuration reference that a partial mutual-TLS setup is fatal and that the checks apply only when enabled=1, and add a runbook troubleshooting entry keyed on the two error messages. Tests cover both guards with the message asserted so the two are told apart, both enabled=0 paths, and the default plaintext configuration. --- .../05-configuration-reference.md | 10 +- cfg/xrpld-example.cfg | 12 +- docs/telemetry-runbook.md | 30 ++- include/xrpl/telemetry/Telemetry.h | 14 +- src/libxrpl/telemetry/TelemetryConfig.cpp | 45 +++-- .../libxrpl/telemetry/TelemetryConfig.cpp | 174 ++++++++++++++++-- src/xrpld/app/main/Main.cpp | 33 +++- 7 files changed, 273 insertions(+), 45 deletions(-) diff --git a/OpenTelemetryPlan/05-configuration-reference.md b/OpenTelemetryPlan/05-configuration-reference.md index 2b8b969fec..0b5d5fafda 100644 --- a/OpenTelemetryPlan/05-configuration-reference.md +++ b/OpenTelemetryPlan/05-configuration-reference.md @@ -7,7 +7,7 @@ ## 5.1 xrpld Configuration -> **OTLP** = OpenTelemetry Protocol | **TxQ** = Transaction Queue +> **OTLP** = OpenTelemetry Protocol | **TxQ** = Transaction Queue | **mTLS** = mutual TLS ### 5.1.1 Configuration File Section @@ -17,12 +17,12 @@ The authoritative `[telemetry]` example lives in `cfg/xrpld-example.cfg`. Teleme | Option | Type | Default | Description | | -------------------------- | ------ | --------------------------------- | ---------------------------------------------------------------------------------------------------------- | -| `enabled` | bool | `false` | Enable/disable telemetry | +| `enabled` | 0 or 1 | `0` | Enable/disable telemetry | | `endpoint` | string | `http://localhost:4318/v1/traces` | OTLP/HTTP collector endpoint | -| `use_tls` | bool | `false` | Enable TLS for exporter connection | +| `use_tls` | 0 or 1 | `0` | Enable TLS for exporter connection | | `tls_ca_cert` | string | `""` | Path to CA certificate file | -| `tls_client_cert` | string | `""` | Path to node's client certificate (PEM) for mutual TLS; requires `use_tls=1`; empty = one-way TLS | -| `tls_client_key` | string | `""` | Path to private key (PEM) for `tls_client_cert`; requires `use_tls=1`; required when the cert is set | +| `tls_client_cert` | string | `""` | Client cert (PEM) for mTLS; empty = one-way; if `enabled=1`, needs key + `use_tls=1` or startup fails | +| `tls_client_key` | string | `""` | Private key (PEM) for `tls_client_cert`; if set with `enabled=1`, needs the cert + `use_tls=1` or fails | | `batch_size` | uint | `512` | Spans per export batch | | `batch_delay_ms` | uint | `5000` | Max delay before sending batch (ms) | | `max_queue_size` | uint | `2048` | Maximum queued spans | diff --git a/cfg/xrpld-example.cfg b/cfg/xrpld-example.cfg index 9c7616a5ef..a3bd8673ce 100644 --- a/cfg/xrpld-example.cfg +++ b/cfg/xrpld-example.cfg @@ -1717,13 +1717,21 @@ validators.txt # tls_client_cert= # # Path to this node's PEM-encoded client certificate, presented to the -# collector for mutual TLS (mTLS). Only used when use_tls=1. Leave empty +# collector for mutual TLS (mTLS). Requires use_tls=1. Leave empty # for one-way (server-only) TLS. Default: empty. # +# To enable mTLS, both tls_client_cert and tls_client_key must be +# specified. If only one is provided, xrpld will fail to start. Providing +# them while use_tls=0 also fails to start, rather than being ignored. +# Both checks apply only when enabled=1; with telemetry disabled these +# settings are read but never validated. +# # tls_client_key= # # Path to the PEM-encoded private key for tls_client_cert. Required -# whenever tls_client_cert is set. Only used when use_tls=1. +# whenever tls_client_cert is set. Requires use_tls=1. Both conditions +# are enforced exactly as described under tls_client_cert above: when +# enabled=1, breaking either one makes xrpld fail to start. # Default: empty. # # Head sampling is intentionally fixed at 1.0 (sample everything) and is diff --git a/docs/telemetry-runbook.md b/docs/telemetry-runbook.md index 3d5d0d9e76..f30b87eaea 100644 --- a/docs/telemetry-runbook.md +++ b/docs/telemetry-runbook.md @@ -59,8 +59,10 @@ cmake --build --preset default | `max_queue_size` | `2048` | Max spans queued before dropping | | `use_tls` | `0` | Use TLS for exporter connection | | `tls_ca_cert` | (empty) | Path to CA certificate bundle | -| `tls_client_cert` | (empty) | Client cert (PEM) for mutual TLS; empty = one-way TLS | -| `tls_client_key` | (empty) | Private key (PEM) for `tls_client_cert` | +| `tls_client_cert` | (empty) | Client cert (PEM) for mTLS; empty = one-way. See note | +| `tls_client_key` | (empty) | Private key (PEM) for `tls_client_cert`. See note | + +> **mTLS (mutual TLS) note**: `tls_client_cert` and `tls_client_key` are optional — leaving both empty gives one-way (server-only) TLS. **If either one is set**, `enabled=1` requires both of them **and** `use_tls=1`, or the node exits at startup; see the Troubleshooting entry for `Unable to start ...: [telemetry] ...`. When `enabled=0` they are read but never validated. ## Span Reference @@ -603,6 +605,30 @@ Three dashboards are pre-provisioned in `docker/telemetry/grafana/dashboards/`: - Check firewall rules for ports 4317/4318 - If using TLS, verify certificate path with `tls_ca_cert` +### Node exits at startup with `Unable to start ...: [telemetry] ...` + +- Symptom: the process exits immediately with a non-zero status (255 on POSIX) + — a clean exit, not a crash — after printing that line on stderr. Any + exception thrown while the `Application` object is constructed prints the same + `Unable to start` prefix, so confirm the text after the colon begins with + `[telemetry]` before using this entry +- Cause: the `[telemetry]` mTLS keys (`tls_client_cert` and `tls_client_key`) + contradict each other. Only these two mTLS checks are gated on `enabled=1`; + the rest of the section is still read when telemetry is off, so a malformed + value in any key — including `enabled` itself, which is read before the gate + — still fails startup with a different message +- Fix: the two checks need different remedies, and the printed message says + which one fired + - `tls_client_cert and tls_client_key must be set together` — exactly one of + the two paths is set. Either delete the one that is set, or add the missing + one **and** set `use_tls=1`. Unless `use_tls=1` is already set, adding the + missing path on its own just moves the failure to the second check + - `tls_client_cert/tls_client_key require use_tls=1` — both paths are set but + TLS is off. Either set `use_tls=1`, or delete **both** paths. Deleting only + one of them trips the first check + - If you did not mean to enable telemetry at all, set `enabled=0` — that + clears both checks whichever one fired + ## Performance Tuning | Scenario | Recommendation | diff --git a/include/xrpl/telemetry/Telemetry.h b/include/xrpl/telemetry/Telemetry.h index 831ad5b464..a34e3cf19b 100644 --- a/include/xrpl/telemetry/Telemetry.h +++ b/include/xrpl/telemetry/Telemetry.h @@ -409,8 +409,10 @@ public: /** * Create a Telemetry instance. * - * Returns a TelemetryImpl when setup.enabled is true, or a - * NullTelemetry no-op stub otherwise. + * With XRPL_ENABLE_TELEMETRY defined, returns a TelemetryImpl when + * setup.enabled is true, or a no-op stub otherwise. Without it, the only + * definition of this factory always returns the no-op stub and never reads + * setup.enabled. * * @param setup Configuration from the [telemetry] config section. * @param journal Journal for log output during initialization. @@ -427,6 +429,14 @@ makeTelemetry(Telemetry::Setup const& setup, beast::Journal journal); * @param networkId Network identifier from [network_id] config * (0 = mainnet, 1 = testnet, 2 = devnet). * @return A populated Setup struct with defaults for missing values. + * @throws std::runtime_error If `enabled` is set and the mutual TLS (mTLS) + * settings contradict each other: only one of `tls_client_cert`/`tls_client_key` + * is given, or a client certificate is given while `use_tls` is 0. Those two + * checks are skipped when `enabled` is 0. + * @throws boost::bad_lexical_cast If any numeric key (`enabled`, `use_tls`, + * `batch_size`, the trace switches, ...) holds a value Section::valueOr cannot + * convert. None of the numeric reads sit inside the `enabled` branch, so this + * escapes whether telemetry is on or off. */ Telemetry::Setup makeTelemetrySetup( diff --git a/src/libxrpl/telemetry/TelemetryConfig.cpp b/src/libxrpl/telemetry/TelemetryConfig.cpp index 9544c71345..006f062009 100644 --- a/src/libxrpl/telemetry/TelemetryConfig.cpp +++ b/src/libxrpl/telemetry/TelemetryConfig.cpp @@ -108,24 +108,37 @@ makeTelemetrySetup( setup.tlsClientCertPath = section.valueOr(key::tlsClientCert, ""); setup.tlsClientKeyPath = section.valueOr(key::tlsClientKey, ""); - // Mutual TLS needs both the client certificate and its private key. - // Supplying only one fails later with a cryptic SSL handshake error, so - // reject the partial configuration here with an actionable message. - if (setup.tlsClientCertPath.empty() != setup.tlsClientKeyPath.empty()) + // The mutual TLS (mTLS) checks below are fatal, so gate them on the one + // thing this parser can know: `enabled` is 1. With `enabled` 0 a leftover + // cert line must never stop the node from booting. + // + // The predicate is only that config switch, not whether an exporter can + // exist. This file has no preprocessor guard, so both checks also run in a + // -Dtelemetry=OFF build, where makeTelemetry() returns the null + // implementation whatever `enabled` says. + if (setup.enabled) { - Throw( - "[telemetry] tls_client_cert and tls_client_key must be set together " - "(set both for mutual TLS, or neither for one-way TLS)."); - } + // mTLS needs both the client certificate and its private key. + // Supplying only one fails later with a cryptic SSL handshake error, so + // reject the partial configuration here with an actionable message. + if (setup.tlsClientCertPath.empty() != setup.tlsClientKeyPath.empty()) + { + Throw( + "[telemetry] tls_client_cert and tls_client_key must be set together " + "(set both for mutual TLS, or neither for one-way TLS)."); + } - // Mutual TLS only takes effect when TLS is on. Certificate paths set with - // use_tls=0 would be silently ignored and the exporter would connect in - // plaintext, so reject that contradiction instead of failing open. - if (!setup.tlsClientCertPath.empty() && !setup.useTls) - { - Throw( - "[telemetry] tls_client_cert/tls_client_key require use_tls=1 " - "(set use_tls=1 to enable mutual TLS, or remove the cert paths)."); + // Still inside the enabled branch. mTLS only takes effect when TLS is + // on, so a client certificate set with use_tls=0 would be ignored and + // any exporter that did run would connect in plaintext. Reject that + // contradiction instead of failing open. tls_ca_cert is deliberately + // not checked this way. + if (!setup.tlsClientCertPath.empty() && !setup.useTls) + { + Throw( + "[telemetry] tls_client_cert/tls_client_key require use_tls=1 " + "(set use_tls=1 to enable mutual TLS, or remove the cert paths)."); + } } // Head sampling is intentionally fixed at 1.0 (sample everything) and is diff --git a/src/tests/libxrpl/telemetry/TelemetryConfig.cpp b/src/tests/libxrpl/telemetry/TelemetryConfig.cpp index bfa0b85149..2aff850c84 100644 --- a/src/tests/libxrpl/telemetry/TelemetryConfig.cpp +++ b/src/tests/libxrpl/telemetry/TelemetryConfig.cpp @@ -2,12 +2,83 @@ #include #include +#include #include #include using namespace xrpl; +using ::testing::HasSubstr; +using ::testing::ThrowsMessage; + +namespace { + +/** + * Shared inputs for the mutual TLS (mTLS) tests of makeTelemetrySetup(). + * + * keyClientCert and keyClientKey are the config key names, named once so every + * test below spells them the same way, mirroring the `key::` constants the + * parser itself uses. A misspelling cannot hide here: the throwing case that + * names the misspelled key stops throwing, the use_tls case throws the pairing + * message instead and fails its matcher, and the value cases see an empty path + * or an unexpected throw. Tests that never set the key are unaffected. One + * source of truth still keeps the two files from drifting apart. + * + * clientCert and clientKey are the paths written to those keys. They are + * declared as `char const*` so they pass to Section::set() (which takes + * `std::string const&`) and compare against the parsed std::string members + * without an explicit conversion, exactly as a literal would. + * + * pairingError and useTlsError are message fragments. Both guards throw + * std::runtime_error, so the exception type alone cannot tell them apart. + * Each fragment occurs in exactly one of the two messages, so matching it + * proves which guard fired. + */ +namespace mtls { +constexpr char const* keyClientCert = "tls_client_cert"; +constexpr char const* keyClientKey = "tls_client_key"; +constexpr char const* clientCert = "/etc/ssl/client.pem"; +constexpr char const* clientKey = "/etc/ssl/client.key"; +constexpr char const* pairingError = "must be set together"; +constexpr char const* useTlsError = "require use_tls=1"; + +/** + * Build a [telemetry] section carrying only the `enabled` key. + * + * Every mTLS test states `enabled` explicitly, because the validation + * guards run only when telemetry is on. Each test then adds the TLS keys its + * own case needs on top of the returned section. + * + * @param telemetryEnabled Value written to the `enabled` key. + * @return The section, ready for further set() calls. + */ +Section +makeSection(bool telemetryEnabled) +{ + Section section; + section.set("enabled", telemetryEnabled ? "1" : "0"); + return section; +} + +/** + * Parse a [telemetry] section with a fixed placeholder node identity. + * + * Keeps the node key, version and network ID out of the individual cases, + * which vary only in their TLS keys. + * + * @param section The section to parse. + * @return The populated Setup struct. + */ +telemetry::Telemetry::Setup +parseSection(Section const& section) +{ + return telemetry::makeTelemetrySetup(section, "nHUtest123", "2.0.0", 0); +} +} // namespace mtls + +} // namespace + TEST(TelemetryConfig, setup_defaults) { telemetry::Telemetry::Setup const s; @@ -87,39 +158,110 @@ TEST(TelemetryConfig, parse_full_section) TEST(TelemetryConfig, mtls_cert_and_key_both_set) { - Section section; + // Telemetry on and use_tls=1, so both guards run and neither may fire. + Section section = mtls::makeSection(true); section.set("use_tls", "1"); - section.set("tls_client_cert", "/etc/ssl/client.pem"); - section.set("tls_client_key", "/etc/ssl/client.key"); + section.set(mtls::keyClientCert, mtls::clientCert); + section.set(mtls::keyClientKey, mtls::clientKey); - auto setup = telemetry::makeTelemetrySetup(section, "nHUtest123", "2.0.0", 0); - EXPECT_EQ(setup.tlsClientCertPath, "/etc/ssl/client.pem"); - EXPECT_EQ(setup.tlsClientKeyPath, "/etc/ssl/client.key"); + auto const setup = mtls::parseSection(section); + EXPECT_TRUE(setup.enabled); + EXPECT_TRUE(setup.useTls); + EXPECT_EQ(setup.tlsClientCertPath, mtls::clientCert); + EXPECT_EQ(setup.tlsClientKeyPath, mtls::clientKey); } TEST(TelemetryConfig, mtls_cert_without_key_throws) { - Section section; - section.set("tls_client_cert", "/etc/ssl/client.pem"); - EXPECT_THROW( - telemetry::makeTelemetrySetup(section, "nHUtest123", "2.0.0", 0), std::runtime_error); + // Only the cert is set, so the pairing guard is the one that must fire. + Section section = mtls::makeSection(true); + section.set(mtls::keyClientCert, mtls::clientCert); + + EXPECT_THAT( + [§ion] { mtls::parseSection(section); }, + ThrowsMessage(HasSubstr(mtls::pairingError))); } TEST(TelemetryConfig, mtls_key_without_cert_throws) { - Section section; - section.set("tls_client_key", "/etc/ssl/client.key"); - EXPECT_THROW( - telemetry::makeTelemetrySetup(section, "nHUtest123", "2.0.0", 0), std::runtime_error); + // Only the key is set, the mirror image of the case above. + Section section = mtls::makeSection(true); + section.set(mtls::keyClientKey, mtls::clientKey); + + EXPECT_THAT( + [§ion] { mtls::parseSection(section); }, + ThrowsMessage(HasSubstr(mtls::pairingError))); +} + +TEST(TelemetryConfig, mtls_cert_key_without_use_tls_throws) +{ + // Both paths are set, so the pairing guard cannot fire; use_tls is absent + // and defaults to 0, so the use_tls guard is the only reachable throw. + Section section = mtls::makeSection(true); + section.set(mtls::keyClientCert, mtls::clientCert); + section.set(mtls::keyClientKey, mtls::clientKey); + + EXPECT_THAT( + [§ion] { mtls::parseSection(section); }, + ThrowsMessage(HasSubstr(mtls::useTlsError))); +} + +TEST(TelemetryConfig, mtls_contradiction_ignored_when_telemetry_disabled) +{ + // The use_tls contradiction with telemetry off: parsing must succeed so a + // stale cert line cannot stop the node from booting. + Section section = mtls::makeSection(false); + section.set(mtls::keyClientCert, mtls::clientCert); + section.set(mtls::keyClientKey, mtls::clientKey); + + auto const setup = mtls::parseSection(section); + EXPECT_FALSE(setup.enabled); + EXPECT_FALSE(setup.useTls); + EXPECT_EQ(setup.tlsClientCertPath, mtls::clientCert); + EXPECT_EQ(setup.tlsClientKeyPath, mtls::clientKey); +} + +TEST(TelemetryConfig, mtls_cert_without_key_ignored_when_telemetry_disabled) +{ + // The pairing violation with telemetry off: also parsed, not rejected. + Section section = mtls::makeSection(false); + section.set(mtls::keyClientCert, mtls::clientCert); + + auto const setup = mtls::parseSection(section); + EXPECT_FALSE(setup.enabled); + EXPECT_FALSE(setup.useTls); + EXPECT_EQ(setup.tlsClientCertPath, mtls::clientCert); + EXPECT_TRUE(setup.tlsClientKeyPath.empty()); +} + +TEST(TelemetryConfig, mtls_default_no_client_tls_is_accepted) +{ + // The documented default with telemetry on: no client certificate, and + // use_tls absent so it defaults to 0. Both guards run and neither may + // fire. The use_tls guard tests the certificate path first; drop that + // conjunct and this config is rejected, so no default node could boot. + Section const section = mtls::makeSection(true); + + telemetry::Telemetry::Setup setup; + ASSERT_NO_THROW(setup = mtls::parseSection(section)); + EXPECT_TRUE(setup.enabled); + EXPECT_FALSE(setup.useTls); + EXPECT_TRUE(setup.tlsClientCertPath.empty()); + EXPECT_TRUE(setup.tlsClientKeyPath.empty()); } TEST(TelemetryConfig, mtls_neither_set_is_one_way_tls) { - Section section; + // Telemetry is on so the guards run, and this config must pass both: + // one-way TLS with a CA bundle and no client certificate. + Section section = mtls::makeSection(true); section.set("use_tls", "1"); section.set("tls_ca_cert", "/etc/ssl/ca.pem"); - auto setup = telemetry::makeTelemetrySetup(section, "nHUtest123", "2.0.0", 0); + auto const setup = mtls::parseSection(section); + EXPECT_TRUE(setup.enabled); + EXPECT_TRUE(setup.useTls); + EXPECT_EQ(setup.tlsCertPath, "/etc/ssl/ca.pem"); EXPECT_TRUE(setup.tlsClientCertPath.empty()); EXPECT_TRUE(setup.tlsClientKeyPath.empty()); } diff --git a/src/xrpld/app/main/Main.cpp b/src/xrpld/app/main/Main.cpp index ba6520db5f..9fe2bb31ef 100644 --- a/src/xrpld/app/main/Main.cpp +++ b/src/xrpld/app/main/Main.cpp @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -804,8 +805,36 @@ run(int argc, char** argv) if (vm.contains("debug")) setDebugLogSink(logs->makeSink("Debug", beast::Severity::Trace)); - auto app = - makeApplication(std::move(config), std::move(logs), std::make_unique()); + // Application construction runs member initializers that validate + // config (for example the [telemetry] section) and can throw. A throw + // from a member-initializer list cannot be recovered inside the + // constructor, so catch it here. Left uncaught it reaches + // std::terminate, whose default handler prints a C++ terminate dump + // and raises SIGABRT, leaving a core file where the system allows one; + // the catch replaces that with two operator-readable lines on stderr + // and a non-zero exit status. + // + // Only the construction is covered. The [telemetry] section is parsed + // near the top of the member list, before the job queue and node store + // are built, so unwinding that throw destroys very little. setup() is + // left outside deliberately: it starts subsystems whose shutdown order + // is delicate, and only the normal stop sequence gets that order right. + std::unique_ptr app; + try + { + app = + makeApplication(std::move(config), std::move(logs), std::make_unique()); + } + catch (std::exception const& e) + { + std::cerr << "Unable to start " << systemName() << ": " << e.what() << std::endl; + std::cerr << "Fix the reported problem and start again." << std::endl; + return -1; + } + + // Construction succeeded, so app holds an object: makeApplication never + // returns null and the catch above is the only other way out. + XRPL_ASSERT(app, "xrpl::run : non-null application"); if (!app->setup(vm)) return -1;