From 7a6ff619e5a5b66fa34418736d301bb100611664 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Mon, 6 Jul 2026 20:37:08 +0100 Subject: [PATCH] code review changes Signed-off-by: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> --- .../scripts/levelization/results/ordering.txt | 1 + OpenTelemetryPlan/02-design-decisions.md | 44 ++++++++++----- OpenTelemetryPlan/Phase2_taskList.md | 5 +- cmake/XrplCore.cmake | 3 + docker/telemetry/otel-collector-config.yaml | 11 +++- include/xrpl/telemetry/Redaction.h | 56 +++++++++++++++++++ src/libxrpl/telemetry/Redaction.cpp | 32 +++++++++++ src/tests/libxrpl/telemetry/Redaction.cpp | 44 +++++++++++++++ src/xrpld/rpc/detail/PathFindSpanNames.h | 2 - src/xrpld/rpc/detail/PathRequest.cpp | 1 - src/xrpld/rpc/detail/PathRequestManager.cpp | 17 +++--- src/xrpld/rpc/handlers/orderbook/PathFind.cpp | 6 +- .../rpc/handlers/orderbook/RipplePathFind.cpp | 6 +- 13 files changed, 195 insertions(+), 33 deletions(-) create mode 100644 include/xrpl/telemetry/Redaction.h create mode 100644 src/libxrpl/telemetry/Redaction.cpp create mode 100644 src/tests/libxrpl/telemetry/Redaction.cpp diff --git a/.github/scripts/levelization/results/ordering.txt b/.github/scripts/levelization/results/ordering.txt index 94d2ef76d5..934fbc67a9 100644 --- a/.github/scripts/levelization/results/ordering.txt +++ b/.github/scripts/levelization/results/ordering.txt @@ -47,6 +47,7 @@ libxrpl.shamap > xrpl.protocol libxrpl.shamap > xrpl.shamap libxrpl.telemetry > xrpl.basics libxrpl.telemetry > xrpl.config +libxrpl.telemetry > xrpl.protocol libxrpl.telemetry > xrpl.telemetry libxrpl.tx > xrpl.basics libxrpl.tx > xrpl.conditions diff --git a/OpenTelemetryPlan/02-design-decisions.md b/OpenTelemetryPlan/02-design-decisions.md index 7beb89d096..aef9705223 100644 --- a/OpenTelemetryPlan/02-design-decisions.md +++ b/OpenTelemetryPlan/02-design-decisions.md @@ -378,32 +378,46 @@ The following data is explicitly **excluded** from telemetry collection: #### Privacy Protection Mechanisms -| Mechanism | Description | -| ----------------------------- | ------------------------------------------------------------------------- | -| **Account Hashing** | `tx_account` is hashed at collector level before storage | -| **Configurable Redaction** | Sensitive fields can be excluded via `[telemetry]` config section | -| **Sampling** | Only 10% of traces recorded by default, reducing data exposure | -| **Local Control** | Node operators have full control over what gets exported | -| **No Raw Payloads** | Transaction content is never recorded, only metadata (hash, type, result) | -| **Collector-Level Filtering** | Additional redaction/hashing can be configured at OTel Collector | +| Mechanism | Description | +| ----------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| **Account Hashing** | Account addresses are hashed both SDK-side (`pathfind_source_account`, `pathfind_dest_account` — always hashed before emission) and again at the collector level, so raw addresses never reach storage | +| **Sampling** | Only 10% of traces recorded by default, reducing data exposure | +| **Local Control** | Node operators have full control over what gets exported | +| **No Raw Payloads** | Transaction content is never recorded, only metadata (hash, type, result) | +| **Collector-Level Filtering** | Additional redaction/hashing can be configured at OTel Collector | + +#### Account Address Hashing + +Account addresses are **always** hashed before they reach the telemetry +backend — there is no opt-out flag and therefore no insecure-by-default +failure mode. Protection is applied in two independent layers: + +1. **SDK-side** (this node): the path-finding RPC handlers call + `redactAccount()` (`xrpl::telemetry`, `Redaction.h`) before setting the + `pathfind_source_account` / `pathfind_dest_account` span attributes. The + helper emits the first 16 characters of `sha512Half(address)` as + lowercase hex — deterministic (spans for one account still correlate) + but non-reversible. +2. **Collector-side** (defense-in-depth): an `attributes/hash` processor in + the OpenTelemetry Collector re-hashes those same attributes, so any node + that emitted a raw value is still redacted before storage. #### Collector-Level Data Protection The OpenTelemetry Collector can be configured (via an `attributes` processor) to hash or redact sensitive attributes before export — for example, hashing -`tx_account`, deleting `peer_address` to drop IP addresses, and deleting -`params` to redact request parameters. +`pathfind_source_account` / `pathfind_dest_account`, deleting `peer_address` +to drop IP addresses, and deleting `params` to redact request parameters. #### Configuration Options for Privacy In `xrpld.cfg`, operators control data collection granularity through the `[telemetry]` section. Besides `enabled`, per-component toggles (`trace_transactions`, `trace_consensus`, `trace_rpc`, `trace_peer` — the last -often disabled due to high volume) select which spans are emitted, and -redaction flags (`redact_account` to hash account addresses, `redact_peer_address` -to remove peer IP addresses) control SDK-level redaction before export. - -> **Note**: The `redact_account` configuration in `xrpld.cfg` controls SDK-level redaction before export, while collector-level filtering (see [Collector-Level Data Protection](#collector-level-data-protection) above) provides an additional defense-in-depth layer. Both can operate independently. +often disabled due to high volume) select which spans are emitted. Account +address hashing is not configurable: addresses are hashed unconditionally by +the SDK helper described above, with collector-level hashing as a second +layer. > **Key Principle**: Telemetry collects **operational metadata** (timing, counts, hashes) — never **sensitive content** (keys, balances, amounts, raw payloads). diff --git a/OpenTelemetryPlan/Phase2_taskList.md b/OpenTelemetryPlan/Phase2_taskList.md index d3afd8633b..0e19be9e9c 100644 --- a/OpenTelemetryPlan/Phase2_taskList.md +++ b/OpenTelemetryPlan/Phase2_taskList.md @@ -169,11 +169,12 @@ Node health (`amendment_blocked`, `server_state`) is not part of the telemetry s | `rpc.process` | `batch_size` | int64 | `params.size()` (only when batch) | | `rpc.ws_message` | `command` | string | `jv[command]` or `jv[method]` | | `rpc.command.*` | `load_type` | string | `context.loadType.label()` | -| `pathfind.compute` | `pathfind_dest_amount` | string | `saDstAmount_.getFullText()` | | `pathfind.compute` | `pathfind_dest_currency` | string | `to_string(saDstAmount_.asset())` | | `pathfind.discover` | `pathfind_num_source_assets` | int64 | `sourceAssets.size()` | -**New attr keys**: `RpcSpanNames.h` (`isBatch`, `batchSize`, `loadType`), `PathFindSpanNames.h` (`destAmount`, `destCurrency`, `numSourceAssets`). +_Note: `pathfind_dest_amount` was removed — the destination amount is a financial value excluded by the privacy policy (design §2.4.4)._ + +**New attr keys**: `RpcSpanNames.h` (`isBatch`, `batchSize`, `loadType`), `PathFindSpanNames.h` (`destCurrency`, `numSourceAssets`). **Modified files**: diff --git a/cmake/XrplCore.cmake b/cmake/XrplCore.cmake index b061a0b82a..99322d7fd0 100644 --- a/cmake/XrplCore.cmake +++ b/cmake/XrplCore.cmake @@ -206,10 +206,13 @@ target_link_libraries(xrpl.libxrpl.tx PUBLIC xrpl.libxrpl.ledger) # When telemetry=ON, links the Conan-provided umbrella target # opentelemetry-cpp::opentelemetry-cpp (individual component targets like # ::api, ::sdk are not available in the Conan package). +# +# Links xrpl.libxrpl.protocol PRIVATELY for sha512Half (digest.h) add_module(xrpl telemetry) target_link_libraries( xrpl.libxrpl.telemetry PUBLIC xrpl.libxrpl.basics xrpl.libxrpl.beast xrpl.libxrpl.config + PRIVATE xrpl.libxrpl.protocol ) if(telemetry) target_link_libraries( diff --git a/docker/telemetry/otel-collector-config.yaml b/docker/telemetry/otel-collector-config.yaml index 957adf036e..4f6f4f1ae4 100644 --- a/docker/telemetry/otel-collector-config.yaml +++ b/docker/telemetry/otel-collector-config.yaml @@ -48,6 +48,15 @@ processors: action: delete - key: telemetry.sdk.version action: delete + # Defense-in-depth: hash path-finding account attributes. The xrpld SDK + # already hashes these before export, but a node that emitted raw values + # is caught here so raw addresses never reach the backend. + attributes/hash: + actions: + - key: pathfind_source_account + action: hash + - key: pathfind_dest_account + action: hash exporters: debug: @@ -66,5 +75,5 @@ service: pipelines: traces: receivers: [otlp] - processors: [resource/tier, resource/stripsdk, batch] + processors: [resource/tier, resource/stripsdk, attributes/hash, batch] exporters: [debug, otlp/tempo] diff --git a/include/xrpl/telemetry/Redaction.h b/include/xrpl/telemetry/Redaction.h new file mode 100644 index 0000000000..50ab17efe2 --- /dev/null +++ b/include/xrpl/telemetry/Redaction.h @@ -0,0 +1,56 @@ +#pragma once + +/** Account-address redaction for telemetry span attributes. + + Path-finding RPC handlers would otherwise emit the caller's raw + account addresses as span attributes. To keep addresses out of the + telemetry backend, they are hashed at the point of emission. This + header exposes a single pure helper that turns an address into a + short, stable, non-reversible token. + + Data flow: + + handler -> redactAccount(addr) -> span attribute -> OTLP export + + The returned token is the first 16 hex characters (lowercase) of the + SHA-512Half digest of the address. It is deterministic (same address + always maps to the same token) so operators can still correlate spans + for a given account, but the original address cannot be recovered. + + A second, independent hashing layer runs in the OpenTelemetry + Collector (an `attributes/hash` processor) as defense-in-depth for + any node that emits a raw value. + + @note This function is pure and reentrant: it holds no global state, + performs no I/O, and is safe to call concurrently from any thread. + + Usage example: + @code + #include + using namespace xrpl::telemetry; + + span.setAttribute( + pathfind_span::attr::sourceAccount, redactAccount(src.asString())); + @endcode + + Edge case (empty input yields empty output): + @code + assert(redactAccount("") == ""); + @endcode +*/ + +#include +#include + +namespace xrpl::telemetry { + +/** Hash an account address into a short, stable, non-reversible token. + + @param addr The account address to redact (e.g. an r-address). + @return The first 16 lowercase hex characters of sha512Half(addr), + or an empty string when @p addr is empty. +*/ +[[nodiscard]] std::string +redactAccount(std::string_view addr); + +} // namespace xrpl::telemetry diff --git a/src/libxrpl/telemetry/Redaction.cpp b/src/libxrpl/telemetry/Redaction.cpp new file mode 100644 index 0000000000..ee15b8812e --- /dev/null +++ b/src/libxrpl/telemetry/Redaction.cpp @@ -0,0 +1,32 @@ +#include + +#include +#include +#include + +#include +#include +#include +#include + +namespace xrpl::telemetry { + +std::string +redactAccount(std::string_view addr) +{ + // Empty in, empty out: nothing to hash and no token to emit. + if (addr.empty()) + return {}; + + // sha512Half yields a uint256; to_string renders it as uppercase hex. + // Keep the first 16 chars (64 bits) — enough to correlate spans while + // staying non-reversible — and lowercase them for a stable token. + auto const digest = sha512Half(Slice(addr.data(), addr.size())); + std::string token = to_string(digest).substr(0, 16); + std::transform(token.begin(), token.end(), token.begin(), [](unsigned char c) { + return static_cast(std::tolower(c)); + }); + return token; +} + +} // namespace xrpl::telemetry diff --git a/src/tests/libxrpl/telemetry/Redaction.cpp b/src/tests/libxrpl/telemetry/Redaction.cpp new file mode 100644 index 0000000000..7f16c0f1dc --- /dev/null +++ b/src/tests/libxrpl/telemetry/Redaction.cpp @@ -0,0 +1,44 @@ +#include + +#include + +#include +#include +#include + +using namespace xrpl; +using namespace xrpl::telemetry; + +// Empty input must produce empty output (edge / negative path). +TEST(Redaction, empty_input_returns_empty) +{ + EXPECT_EQ(redactAccount(""), ""); +} + +// Success path: assert the EXACT expected token. The value is the first +// 16 chars of sha512Half(addr) rendered as lowercase hex, computed once +// and hard-coded here so any change to the hashing recipe is caught. +TEST(Redaction, known_account_exact_hash) +{ + EXPECT_EQ(redactAccount("rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh"), "a513895f49311f54"); +} + +// Structural invariants: length, lowercase-hex alphabet, and stability. +TEST(Redaction, token_is_16_lowercase_hex_and_stable) +{ + auto const token = redactAccount("rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh"); + EXPECT_EQ(token.size(), 16u); + EXPECT_TRUE(std::all_of(token.begin(), token.end(), [](unsigned char c) { + return std::isdigit(c) || (c >= 'a' && c <= 'f'); + })); + // Deterministic: hashing the same input twice yields the same token. + EXPECT_EQ(token, redactAccount("rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh")); +} + +// Distinct inputs must produce distinct tokens (no accidental constant). +TEST(Redaction, distinct_inputs_distinct_tokens) +{ + EXPECT_NE( + redactAccount("rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh"), + redactAccount("rDifferentAccount1234567890abcdefgh")); +} diff --git a/src/xrpld/rpc/detail/PathFindSpanNames.h b/src/xrpld/rpc/detail/PathFindSpanNames.h index 3a12937fbc..71eb805ea3 100644 --- a/src/xrpld/rpc/detail/PathFindSpanNames.h +++ b/src/xrpld/rpc/detail/PathFindSpanNames.h @@ -84,8 +84,6 @@ inline constexpr auto numPaths = makeStr("pathfind_num_paths"); inline constexpr auto numRequests = makeStr("pathfind_num_requests"); /// "pathfind_ledger_index" — pathfind target ledger index. inline constexpr auto ledgerIndex = makeStr("pathfind_ledger_index"); -/// "pathfind_dest_amount" — requested destination amount as string. -inline constexpr auto destAmount = makeStr("pathfind_dest_amount"); /// "pathfind_dest_currency" — destination currency code. inline constexpr auto destCurrency = makeStr("pathfind_dest_currency"); /// "pathfind_num_source_assets" — candidate source assets count. diff --git a/src/xrpld/rpc/detail/PathRequest.cpp b/src/xrpld/rpc/detail/PathRequest.cpp index 324036d39b..4fb11ab10c 100644 --- a/src/xrpld/rpc/detail/PathRequest.cpp +++ b/src/xrpld/rpc/detail/PathRequest.cpp @@ -742,7 +742,6 @@ PathRequest::doUpdate( auto span = SpanGuard::span( TraceCategory::Rpc, pathfind_span::prefix::pathfind, pathfind_span::op::compute); span.setAttribute(pathfind_span::attr::fast, fast); - span.setAttribute(pathfind_span::attr::destAmount, saDstAmount_.getFullText().c_str()); span.setAttribute(pathfind_span::attr::destCurrency, to_string(saDstAmount_.asset()).c_str()); JLOG(journal_.debug()) << iIdentifier_ << " update " << (fast ? "fast" : "normal"); diff --git a/src/xrpld/rpc/detail/PathRequestManager.cpp b/src/xrpld/rpc/detail/PathRequestManager.cpp index b2831c5518..01df901ce3 100644 --- a/src/xrpld/rpc/detail/PathRequestManager.cpp +++ b/src/xrpld/rpc/detail/PathRequestManager.cpp @@ -73,15 +73,16 @@ PathRequestManager::updateAll(std::shared_ptr const& inLedger) cache = getAssetCache(inLedger, true); } - // updateAll runs on every ledger close; skip span emission entirely when - // there are no active path subscriptions to avoid a steady stream of empty - // spans at mainnet close cadence. - if (requests.empty()) - return; - using namespace telemetry; - auto span = SpanGuard::span( - TraceCategory::Rpc, pathfind_span::prefix::pathfind, pathfind_span::op::updateAll); + // updateAll runs on every ledger close. Skip span emission when there are + // no active path subscriptions, to avoid a steady stream of empty spans at + // mainnet close cadence. A null guard is used in that case; all other work + // still runs unchanged (notably the isNewPathRequest() flag reset below), + // so behaviour matches the pre-span code path. + auto span = requests.empty() + ? SpanGuard{} + : SpanGuard::span( + TraceCategory::Rpc, pathfind_span::prefix::pathfind, pathfind_span::op::updateAll); span.setAttribute(pathfind_span::attr::ledgerIndex, static_cast(inLedger->seq())); span.setAttribute(pathfind_span::attr::numRequests, static_cast(requests.size())); diff --git a/src/xrpld/rpc/handlers/orderbook/PathFind.cpp b/src/xrpld/rpc/handlers/orderbook/PathFind.cpp index cecd1083c9..f04756be15 100644 --- a/src/xrpld/rpc/handlers/orderbook/PathFind.cpp +++ b/src/xrpld/rpc/handlers/orderbook/PathFind.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include namespace xrpl { @@ -20,10 +21,11 @@ doPathFind(RPC::JsonContext& context) using namespace telemetry; auto span = SpanGuard::span( TraceCategory::Rpc, pathfind_span::prefix::pathfind, pathfind_span::op::request); + // Addresses are hashed before emission for privacy. if (auto const& src = context.params[jss::source_account]; src.isString()) - span.setAttribute(pathfind_span::attr::sourceAccount, src.asString()); + span.setAttribute(pathfind_span::attr::sourceAccount, redactAccount(src.asString())); if (auto const& dst = context.params[jss::destination_account]; dst.isString()) - span.setAttribute(pathfind_span::attr::destAccount, dst.asString()); + span.setAttribute(pathfind_span::attr::destAccount, redactAccount(dst.asString())); if (context.app.config().pathSearchMax == 0) return rpcError(RpcNotSupported); diff --git a/src/xrpld/rpc/handlers/orderbook/RipplePathFind.cpp b/src/xrpld/rpc/handlers/orderbook/RipplePathFind.cpp index b55ba2515f..e08ef5445c 100644 --- a/src/xrpld/rpc/handlers/orderbook/RipplePathFind.cpp +++ b/src/xrpld/rpc/handlers/orderbook/RipplePathFind.cpp @@ -14,6 +14,7 @@ #include #include #include +#include #include #include @@ -28,10 +29,11 @@ doRipplePathFind(RPC::JsonContext& context) using namespace telemetry; auto span = SpanGuard::span( TraceCategory::Rpc, pathfind_span::prefix::pathfind, pathfind_span::op::request); + // Addresses are hashed before emission for privacy. if (auto const& src = context.params[jss::source_account]; src.isString()) - span.setAttribute(pathfind_span::attr::sourceAccount, src.asString()); + span.setAttribute(pathfind_span::attr::sourceAccount, redactAccount(src.asString())); if (auto const& dst = context.params[jss::destination_account]; dst.isString()) - span.setAttribute(pathfind_span::attr::destAccount, dst.asString()); + span.setAttribute(pathfind_span::attr::destAccount, redactAccount(dst.asString())); if (context.app.config().pathSearchMax == 0) return rpcError(RpcNotSupported);