From bcf2d098d36833fe75aa7287e682fad141f487f3 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 9 Sep 2026 12:25:17 +0100 Subject: [PATCH] fix(telemetry): require https for metrics_endpoint under mTLS With tls_client_cert set, only traces_endpoint was checked for an https scheme. Telemetry::makeMetricExporter() attaches the client certificate and key to the metric exporter whenever use_tls=1, and metrics_endpoint defaults to a plain http URL, so an operator who set up mTLS and overrode only traces_endpoint exported every metric in the clear with the configured client identity unused. Check both endpoints, and state the requirement under metrics_endpoint and tls_client_cert in the example config. Four config tests cover an explicit http metrics endpoint, the omitted-key default, both endpoints on https, and a one-way-TLS control that must stay accepted. --- cfg/xrpld-example.cfg | 19 +++-- src/libxrpl/telemetry/TelemetryConfig.cpp | 15 ++-- .../libxrpl/telemetry/TelemetryConfig.cpp | 78 +++++++++++++++++++ 3 files changed, 101 insertions(+), 11 deletions(-) diff --git a/cfg/xrpld-example.cfg b/cfg/xrpld-example.cfg index 93c430b0c2..3116a01420 100644 --- a/cfg/xrpld-example.cfg +++ b/cfg/xrpld-example.cfg @@ -1762,12 +1762,13 @@ validators.txt # 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. -# traces_endpoint must be an https:// URL, because that scheme is what -# makes the exporter present the certificate at all. With use_tls=1 each -# path is opened at startup, so one that does not exist or cannot be read -# fails to start too, rather than failing later as an opaque TLS -# handshake error. All four checks apply only when enabled=1; with -# telemetry disabled these settings are read but never validated. +# traces_endpoint and metrics_endpoint must both be https:// URLs, because +# that scheme is what makes each exporter present the certificate at all. +# With use_tls=1 each path is opened at startup, so one that does not +# exist or cannot be read fails to start too, rather than failing later +# as an opaque TLS handshake error. All four checks apply only when +# enabled=1; with telemetry disabled these settings are read but never +# validated. # # tls_client_key= # @@ -1853,6 +1854,12 @@ validators.txt # defaults. # Default: http://localhost:4318/v1/metrics. # +# The scheme decides encryption here exactly as it does for +# traces_endpoint, so setting tls_client_cert requires this URL to start +# with https:// as well — including leaving it at the default above. +# Overriding only traces_endpoint therefore makes xrpld fail to start, +# rather than export metrics without the client identity. +# # metric_export_interval_ms=1000 # # Gap in milliseconds between two metric exports. Must be greater than 0. diff --git a/src/libxrpl/telemetry/TelemetryConfig.cpp b/src/libxrpl/telemetry/TelemetryConfig.cpp index 86fb632185..469ffc3fc9 100644 --- a/src/libxrpl/telemetry/TelemetryConfig.cpp +++ b/src/libxrpl/telemetry/TelemetryConfig.cpp @@ -387,13 +387,18 @@ makeTelemetrySetup( } // Still inside the enabled branch, and checked before the files are - // opened so a scheme problem is not hidden behind a path problem. The - // exporter reads TLS off the endpoint scheme, so a client certificate is - // only presented on an https endpoint. tls_ca_cert is left out of this - // check: it only names a trust store, while a client certificate is this - // node's own identity and has to reach the collector to mean anything. + // opened so a scheme problem is not hidden behind a path problem. Each + // exporter reads TLS off its own endpoint scheme, and both are handed + // the client certificate, so both endpoints have to be https. Checking + // only one leaves the other signal exporting in the clear without this + // node's identity. tls_ca_cert is left out of this check: it only names + // a trust store, while a client certificate is this node's own identity + // and has to reach the collector to mean anything. if (!setup.tlsClientCertPath.empty()) + { requireHttpsEndpoint(setup.tracesEndpoint, key::tracesEndpoint); + requireHttpsEndpoint(setup.metricsEndpoint, key::metricsEndpoint); + } // Still inside the enabled branch. The exporter opens these files only // when TLS is on, so check them only then: a bad path behind use_tls=0 diff --git a/src/tests/libxrpl/telemetry/TelemetryConfig.cpp b/src/tests/libxrpl/telemetry/TelemetryConfig.cpp index da39b6eab4..f8ecaa7de7 100644 --- a/src/tests/libxrpl/telemetry/TelemetryConfig.cpp +++ b/src/tests/libxrpl/telemetry/TelemetryConfig.cpp @@ -79,6 +79,19 @@ constexpr char const* httpsEndpoint = "https://collector:4318/v1/traces"; constexpr char const* defaultEndpoint = "http://localhost:4318/v1/traces"; constexpr char const* schemeError = "must start with 'https://'"; +/** + * The same four values for the metric signal. + * + * The guard covers both endpoints, so every case that expects parsing to + * succeed has to set this key too. Spelled separately from the trace values so + * a case can put one signal on https and the other on http, which is the + * configuration that used to pass. + */ +constexpr char const* keyMetricsEndpoint = "metrics_endpoint"; +constexpr char const* metricsHttpEndpoint = "http://collector:4318/v1/metrics"; +constexpr char const* metricsHttpsEndpoint = "https://collector:4318/v1/metrics"; +constexpr char const* defaultMetricsEndpoint = "http://localhost:4318/v1/metrics"; + /** * Build a [telemetry] section carrying only the `enabled` key. * @@ -366,6 +379,7 @@ TEST(TelemetryConfig, mtls_cert_and_key_both_set) Section section = mtls::makeSection(true); section.set("use_tls", "1"); section.set(mtls::keyEndpoint, mtls::httpsEndpoint); + section.set(mtls::keyMetricsEndpoint, mtls::metricsHttpsEndpoint); section.set(mtls::keyClientCert, cert); section.set(mtls::keyClientKey, key); @@ -568,6 +582,7 @@ TEST(TelemetryConfig, tls_readable_files_are_accepted) Section section = mtls::makeSection(true); section.set("use_tls", "1"); section.set(mtls::keyEndpoint, mtls::httpsEndpoint); + section.set(mtls::keyMetricsEndpoint, mtls::metricsHttpsEndpoint); section.set("tls_ca_cert", ca); section.set(mtls::keyClientCert, cert); section.set(mtls::keyClientKey, key); @@ -666,22 +681,85 @@ TEST(TelemetryConfig, mtls_client_cert_on_an_https_endpoint_is_accepted) { // The same configuration as the two cases above with only the scheme // changed, so nothing but the scheme can explain the different outcome. + // Both endpoints are https, which is the only shape the guard accepts. TempDir const dir; auto const cert = mtls::writeCertFile(dir.file("c.pem")); auto const key = mtls::writeCertFile(dir.file("k.pem")); Section section = mtls::makeSection(true); section.set("use_tls", "1"); section.set(mtls::keyEndpoint, mtls::httpsEndpoint); + section.set(mtls::keyMetricsEndpoint, mtls::metricsHttpsEndpoint); section.set(mtls::keyClientCert, cert); section.set(mtls::keyClientKey, key); telemetry::Telemetry::Setup setup; ASSERT_NO_THROW(setup = mtls::parseSection(section)); EXPECT_EQ(setup.tracesEndpoint, mtls::httpsEndpoint); + EXPECT_EQ(setup.metricsEndpoint, mtls::metricsHttpsEndpoint); EXPECT_EQ(setup.tlsClientCertPath, cert); EXPECT_EQ(setup.tlsClientKeyPath, key); } +TEST(TelemetryConfig, mtls_client_cert_on_a_plain_http_metrics_endpoint_throws) +{ + // traces_endpoint is https and only metrics_endpoint is not, so the trace + // guard cannot be what fires. Before the metric endpoint was checked this + // configuration started the node and exported every metric in the clear, + // with the client certificate attached to the exporter and never used. + TempDir const dir; + Section section = mtls::makeSection(true); + section.set("use_tls", "1"); + section.set(mtls::keyEndpoint, mtls::httpsEndpoint); + section.set(mtls::keyMetricsEndpoint, mtls::metricsHttpEndpoint); + section.set(mtls::keyClientCert, mtls::writeCertFile(dir.file("c.pem"))); + section.set(mtls::keyClientKey, mtls::writeCertFile(dir.file("k.pem"))); + + EXPECT_THAT( + [§ion] { mtls::parseSection(section); }, + ThrowsMessage(AllOf( + HasSubstr(mtls::schemeError), + HasSubstr(mtls::keyMetricsEndpoint), + HasSubstr(mtls::metricsHttpEndpoint)))); +} + +TEST(TelemetryConfig, mtls_client_cert_with_the_default_metrics_endpoint_throws) +{ + // The key is absent, so the built-in default applies, and that default is a + // plain http URL. This is the shape an operator reaches by setting up mTLS + // and overriding only traces_endpoint, which makes it the case worth having. + TempDir const dir; + Section section = mtls::makeSection(true); + section.set("use_tls", "1"); + section.set(mtls::keyEndpoint, mtls::httpsEndpoint); + section.set(mtls::keyClientCert, mtls::writeCertFile(dir.file("c.pem"))); + section.set(mtls::keyClientKey, mtls::writeCertFile(dir.file("k.pem"))); + + EXPECT_THAT( + [§ion] { mtls::parseSection(section); }, + ThrowsMessage(AllOf( + HasSubstr(mtls::schemeError), + HasSubstr(mtls::keyMetricsEndpoint), + HasSubstr(mtls::defaultMetricsEndpoint)))); +} + +TEST(TelemetryConfig, one_way_tls_on_a_plain_http_metrics_endpoint_is_accepted) +{ + // The control for the metric guard's scope, matching the trace one below: + // same plain http metrics endpoint and use_tls=1, but no client identity to + // lose. Widen the guard to every use_tls=1 node and this case starts failing. + TempDir const dir; + auto const ca = mtls::writeCertFile(dir.file("ca.pem")); + Section section = mtls::makeSection(true); + section.set("use_tls", "1"); + section.set(mtls::keyMetricsEndpoint, mtls::metricsHttpEndpoint); + section.set("tls_ca_cert", ca); + + telemetry::Telemetry::Setup setup; + ASSERT_NO_THROW(setup = mtls::parseSection(section)); + EXPECT_EQ(setup.metricsEndpoint, mtls::metricsHttpEndpoint); + EXPECT_TRUE(setup.tlsClientCertPath.empty()); +} + TEST(TelemetryConfig, mtls_scheme_check_is_case_sensitive_like_the_exporter) { // The exporter compares the scheme byte for byte, so "HTTPS://" leaves it