diff --git a/docker/telemetry/otel-collector-config.yaml b/docker/telemetry/otel-collector-config.yaml index 6a38155f71..49b94fd18a 100644 --- a/docker/telemetry/otel-collector-config.yaml +++ b/docker/telemetry/otel-collector-config.yaml @@ -48,15 +48,10 @@ 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 + # No attribute hashing or redaction. Account addresses in span attributes + # (pathfind_source_account, pathfind_dest_account) are public ledger + # identifiers and are stored as emitted so they join against explorers and + # logs. Do not add an attributes/hash processor for them. exporters: debug: @@ -75,5 +70,5 @@ service: pipelines: traces: receivers: [otlp] - processors: [resource/tier, resource/stripsdk, attributes/hash, batch] + processors: [resource/tier, resource/stripsdk, batch] exporters: [debug, otlp_grpc/tempo] diff --git a/include/xrpl/telemetry/Redaction.h b/include/xrpl/telemetry/Redaction.h index 3b1e8f67ef..c3738ddbb7 100644 --- a/include/xrpl/telemetry/Redaction.h +++ b/include/xrpl/telemetry/Redaction.h @@ -1,34 +1,30 @@ #pragma once /** - * Account-address redaction for telemetry span attributes. + * Account-address redaction helper for telemetry span attributes. * - * Path-finding RPC handlers would otherwise emit the caller's raw - * account addresses as span attributes. To keep plaintext 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, obfuscated token. + * A single pure helper that turns a string into a short, stable, + * obfuscated token, for a span attribute whose value should not be stored + * in the clear and is hard to guess. * - * Data flow: - * - * handler -> redactAccount(addr) -> span attribute -> OTLP export + * Not applied to any span today. Account addresses are public ledger + * identifiers, so the path-finding spans emit them raw (see + * PathFindSpanNames.h) and no collector processor hashes them. Use this + * helper only for a value that is genuinely private, and document the + * reason at the attribute constant. * * 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 across nodes and restarts. + * SHA-512Half digest of the input. It is deterministic (same input + * always maps to the same token) so spans for one value still correlate + * across nodes and restarts. * - * The hash is unsalted, so it is obfuscation, not a secrecy guarantee: - * XRP account addresses are a public, enumerable set, so a determined - * observer with the telemetry stream could rebuild the address->token - * mapping. The goal here is to keep plaintext addresses out of traces - * and dashboards, not to defend against a precomputation attack. A salt - * is intentionally omitted because it would break cross-node/restart - * correlation, which is the reason for hashing rather than dropping. - * - * 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. + * The hash is unsalted, so it is obfuscation, not a secrecy guarantee. + * It hides a value only when that value is hard to guess: for an input + * drawn from a small or enumerable set, such as an account address, an + * observer can rebuild the value->token mapping by lookup, which is why + * account addresses are emitted raw instead. A salt is intentionally + * omitted because it would break cross-node/restart correlation, which is + * the reason for hashing rather than dropping. * * @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. @@ -38,8 +34,7 @@ * #include * using namespace xrpl::telemetry; * - * span.setAttribute( - * pathfind_span::attr::sourceAccount, redactAccount(src.asString())); + * auto const token = redactAccount(value); // 16 lowercase hex chars * @endcode * * Edge case (empty input yields empty output): @@ -54,9 +49,10 @@ namespace xrpl::telemetry { /** - * Hash an account address into a short, stable, obfuscated token. + * Hash a value into a short, stable, obfuscated token. * - * @param addr The account address to redact (e.g. an r-address). + * @param addr The value to redact. Named for its original use on account + * addresses; any string can be passed. * @return The first 16 lowercase hex characters of sha512Half(addr), * or an empty string when @p addr is empty. */ diff --git a/src/xrpld/rpc/detail/PathFindSpanAttributes.h b/src/xrpld/rpc/detail/PathFindSpanAttributes.h new file mode 100644 index 0000000000..52d1996439 --- /dev/null +++ b/src/xrpld/rpc/detail/PathFindSpanAttributes.h @@ -0,0 +1,66 @@ +#pragma once + +/** + * Helpers that set path-finding span attributes from RPC request fields. + * + * The path_find and ripple_path_find handlers both record the request's + * source and destination accounts on the pathfind.request span. The value + * comes from client input, so it is emitted only when it parses as an + * r-address; a malformed or mistaken value never reaches the span. The + * address itself is a public ledger identifier and is emitted raw (see the + * attribute docs in PathFindSpanNames.h). + * + * doPathFind() / doRipplePathFind() + * │ params[jss::source_account], read through a const json::Value + * ▼ + * setAccountAttribute(span, key, field) (this header) + * │ parseBase58: nullopt -> nothing emitted + * ▼ + * span.setAttribute(key, toBase58(account)) + * + * @code + * // Primary use, inside the span-live guard of a handler: + * auto const& params = std::as_const(context.params); + * pathfind_span::setAccountAttribute( + * span, pathfind_span::attr::sourceAccount, params[jss::source_account]); + * @endcode + * + * @code + * // Edge cases: a missing field (null), a non-string, or a string that is + * // not an r-address all leave the span untouched. + * pathfind_span::setAccountAttribute(span, key, json::Value{}); + * pathfind_span::setAccountAttribute(span, key, json::Value{"not an address"}); + * @endcode + * + * @note Not a hot path: one Base58 decode per account per RPC call, and only + * while the span is live. Thread-safe; it holds no state. + */ + +#include +#include +#include + +#include + +namespace xrpl::telemetry::pathfind_span { + +/** + * Set an account attribute on a path-finding span when the request field + * holds an r-address. + * + * @param span The live pathfind.request span. + * @param key The attribute key, pathfind_source_account or + * pathfind_dest_account. + * @param field The request parameter. Read it through a const json::Value so + * a missing key is not inserted into the request. + */ +inline void +setAccountAttribute(ScopedSpanGuard& span, std::string_view key, json::Value const& field) +{ + if (!field.isString()) + return; + if (auto const account = parseBase58(field.asString())) + span.setAttribute(key, toBase58(*account)); +} + +} // namespace xrpl::telemetry::pathfind_span diff --git a/src/xrpld/rpc/detail/PathFindSpanNames.h b/src/xrpld/rpc/detail/PathFindSpanNames.h index 47376f7e8b..5e5a71f811 100644 --- a/src/xrpld/rpc/detail/PathFindSpanNames.h +++ b/src/xrpld/rpc/detail/PathFindSpanNames.h @@ -80,10 +80,20 @@ inline constexpr auto discover = makeStr("discover"); namespace attr { /** * "pathfind_source_account" — originating account for path search. + * + * Emitted as the raw r-address, not hashed. An account address is a public + * ledger identifier drawn from an enumerable set, so an unsalted hash of it + * is reversible by lookup and protects nothing; it only breaks the join + * against explorers, RPC responses and logs that show the same address. + * Only a value that parses as an r-address is emitted, so a malformed or + * mistaken request value never reaches the span. Do not add redaction here + * or in a collector processor. */ inline constexpr auto sourceAccount = makeStr("pathfind_source_account"); /** * "pathfind_dest_account" — destination account. + * + * Raw r-address, for the same reason as pathfind_source_account. */ inline constexpr auto destAccount = makeStr("pathfind_dest_account"); /** @@ -109,7 +119,10 @@ inline constexpr auto numRequests = makeStr("pathfind_num_requests"); */ inline constexpr auto ledgerIndex = makeStr("pathfind_ledger_index"); /** - * "pathfind_dest_currency" — destination currency code. + * "pathfind_dest_currency" — destination asset as rendered by to_string(Asset): + * "XRP", "/" for an IOU, or the 48-hex-char + * issuance id for an MPT (its last 20 bytes are the issuer's account id). + * The issuer is a public identifier and is not hashed. */ inline constexpr auto destCurrency = makeStr("pathfind_dest_currency"); /** diff --git a/src/xrpld/rpc/detail/PathRequest.cpp b/src/xrpld/rpc/detail/PathRequest.cpp index 4baf6567d6..a7fb54801c 100644 --- a/src/xrpld/rpc/detail/PathRequest.cpp +++ b/src/xrpld/rpc/detail/PathRequest.cpp @@ -35,7 +35,6 @@ #include #include #include -#include #include #include @@ -780,22 +779,11 @@ PathRequest::doUpdate( if (span) { span.setAttribute(pathfind_span::attr::fast, fast); - // to_string(Issue) renders a non-XRP asset as "/" with - // the issuer as a plaintext Base58 address, so it cannot be emitted - // as-is: every account reaching a span is hashed first. Redact just the - // issuer and keep the currency, which is what this attribute is for. An - // MPT issuance id ends with the issuer's account id, so hash the whole - // id: that still gives one stable token per asset, without publishing - // the issuer. - span.setAttribute( - pathfind_span::attr::destCurrency, - saDstAmount_.asset().visit( - [](Issue const& issue) { - return isXRP(issue.account) - ? to_string(issue.currency) - : redactAccount(toBase58(issue.account)) + "/" + to_string(issue.currency); - }, - [](MPTIssue const& mpt) { return redactAccount(to_string(mpt.getMptID())); })); + // to_string(Asset) renders XRP as "XRP", an IOU as "/" + // with the issuer's Base58 address, and an MPT as its issuance id. The + // issuer is a public ledger identifier, so the asset is emitted as + // rendered (see the attribute docs in PathFindSpanNames.h). + span.setAttribute(pathfind_span::attr::destCurrency, to_string(saDstAmount_.asset())); } JLOG(journal_.debug()) << iIdentifier_ << " update " << (fast ? "fast" : "normal"); diff --git a/src/xrpld/rpc/handlers/orderbook/PathFind.cpp b/src/xrpld/rpc/handlers/orderbook/PathFind.cpp index af15403030..a4323b9e00 100644 --- a/src/xrpld/rpc/handlers/orderbook/PathFind.cpp +++ b/src/xrpld/rpc/handlers/orderbook/PathFind.cpp @@ -1,6 +1,7 @@ #include #include #include +#include #include #include @@ -10,7 +11,6 @@ #include #include #include -#include #include #include @@ -25,26 +25,23 @@ doPathFind(rpc::JsonContext& context) // thread) nest under it. doPathFind does not yield, so scoping is safe. auto span = ScopedSpanGuard( TraceCategory::Rpc, pathfind_span::prefix::pathfind, pathfind_span::op::request); - // Guarded on the span being live because setAttribute's arguments are - // evaluated whatever the build, and neither is free: asString() copies the - // address out of the JSON and redactAccount() takes a SHA-512Half over it. - // That is two copies and two hashes on every path_find call. The - // compiled-out guard's operator bool() is a literal false, so the block - // disappears entirely in that build; with telemetry compiled in it is - // skipped when telemetry is disabled at runtime or the category is off. + // Guarded on the span being live because the account parse below is not + // free and runs on every path_find call otherwise. The compiled-out + // guard's operator bool() is a literal false, so the block disappears + // entirely in that build; with telemetry compiled in it is skipped when + // telemetry is disabled at runtime or the category is off. if (span) { - // Addresses are hashed before emission for privacy. Read through a - // const reference: the non-const json::Value::operator[] inserts a null - // for a missing key, which would make PathRequest::parseJson's - // isMember() checks see an absent field as present and return Malformed - // instead of Missing. Reading for telemetry must not alter what the - // request looks like. + // Read through a const reference: the non-const json::Value::operator[] + // inserts a null for a missing key, which would make + // PathRequest::parseJson's isMember() checks see an absent field as + // present and return Malformed instead of Missing. Reading for + // telemetry must not alter what the request looks like. auto const& params = std::as_const(context.params); - if (auto const& src = params[jss::source_account]; src.isString()) - span.setAttribute(pathfind_span::attr::sourceAccount, redactAccount(src.asString())); - if (auto const& dst = params[jss::destination_account]; dst.isString()) - span.setAttribute(pathfind_span::attr::destAccount, redactAccount(dst.asString())); + pathfind_span::setAccountAttribute( + span, pathfind_span::attr::sourceAccount, params[jss::source_account]); + pathfind_span::setAccountAttribute( + span, pathfind_span::attr::destAccount, params[jss::destination_account]); } // A failed reply carries the rpc error token, so reading the status off the diff --git a/src/xrpld/rpc/handlers/orderbook/RipplePathFind.cpp b/src/xrpld/rpc/handlers/orderbook/RipplePathFind.cpp index 9abe40cdcc..944b442299 100644 --- a/src/xrpld/rpc/handlers/orderbook/RipplePathFind.cpp +++ b/src/xrpld/rpc/handlers/orderbook/RipplePathFind.cpp @@ -2,6 +2,7 @@ #include #include #include +#include #include #include #include @@ -14,7 +15,6 @@ #include #include #include -#include #include #include @@ -34,26 +34,23 @@ doRipplePathFind(rpc::JsonContext& context) // span's log lines stay trace-correlated. auto span = ScopedSpanGuard( TraceCategory::Rpc, pathfind_span::prefix::pathfind, pathfind_span::op::request); - // Guarded on the span being live because setAttribute's arguments are - // evaluated whatever the build, and neither is free: asString() copies the - // address out of the JSON and redactAccount() takes a SHA-512Half over it. - // That is two copies and two hashes on every ripple_path_find call. The - // compiled-out guard's operator bool() is a literal false, so the block - // disappears entirely in that build; with telemetry compiled in it is - // skipped when telemetry is disabled at runtime or the category is off. + // Guarded on the span being live because the account parse below is not + // free and runs on every ripple_path_find call otherwise. The compiled-out + // guard's operator bool() is a literal false, so the block disappears + // entirely in that build; with telemetry compiled in it is skipped when + // telemetry is disabled at runtime or the category is off. if (span) { - // Addresses are hashed before emission for privacy. Read through a - // const reference: the non-const json::Value::operator[] inserts a null - // for a missing key, which would make PathRequest::parseJson's - // isMember() checks see an absent field as present and return Malformed - // instead of Missing. Reading for telemetry must not alter what the - // request looks like. + // Read through a const reference: the non-const json::Value::operator[] + // inserts a null for a missing key, which would make + // PathRequest::parseJson's isMember() checks see an absent field as + // present and return Malformed instead of Missing. Reading for + // telemetry must not alter what the request looks like. auto const& params = std::as_const(context.params); - if (auto const& src = params[jss::source_account]; src.isString()) - span.setAttribute(pathfind_span::attr::sourceAccount, redactAccount(src.asString())); - if (auto const& dst = params[jss::destination_account]; dst.isString()) - span.setAttribute(pathfind_span::attr::destAccount, redactAccount(dst.asString())); + pathfind_span::setAccountAttribute( + span, pathfind_span::attr::sourceAccount, params[jss::source_account]); + pathfind_span::setAccountAttribute( + span, pathfind_span::attr::destAccount, params[jss::destination_account]); } // A failed reply carries the rpc error token, so reading the status off the