Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved correctness, security, and failure-handling findings remain.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Moves validator-keys into the repository, adding external signing and validator-list signing/verification.
Changes:
- Adds in-tree key, token, manifest, revocation, and signing implementations.
- Adds validator-list signing, verification, and version 2 support.
- Updates tests, documentation, CMake, packaging, and CI.
File summaries
| File | Reviewed change |
|---|---|
src/tools/validator-keys/ValidatorKeysTool.h |
Tool APIs and command options |
src/tools/validator-keys/ValidatorKeysTool.cpp |
CLI commands and workflows |
src/tools/validator-keys/test/ValidatorKeysTool_test.cpp |
Command-layer tests |
src/tools/validator-keys/test/SigningKeys_test.cpp |
Signing and key-management tests |
src/tools/validator-keys/test/ListSigning_test.cpp |
Validator-list signing and verification tests |
src/tools/validator-keys/test/KeyFileGuard.h |
Key-file test helper |
src/tools/validator-keys/SigningKeys.h |
Signing key interface |
src/tools/validator-keys/SigningKeys.cpp |
Key, token, and manifest implementation |
src/tools/validator-keys/README.md |
Tool build documentation |
src/tools/validator-keys/ListSigning.h |
Validator-list APIs |
src/tools/validator-keys/ListSigning.cpp |
List parsing, signing, and verification |
src/tools/validator-keys/LICENSE |
Bundled licensing notice |
src/tools/validator-keys/doc/validator-keys-tool-guide.md |
Operator guide |
src/tools/validator-keys/CMakeLists.txt |
Tool target definition |
package/README.md |
Packaging documentation |
package/build_pkg.py |
Package validation updates |
cmake/XrplValidatorKeys.cmake |
In-tree build and installation |
.github/workflows/reusable-clang-tidy.yml |
Clang-tidy configuration |
.github/scripts/rename/cmake.sh |
Removes obsolete repository renaming |
.cspell.config.yaml |
Dictionary additions |
.clang-tidy |
Includes tool headers in analysis |
Review details
Suppressed comments (8)
src/tools/validator-keys/SigningKeys.cpp:494
- This comment says "that 128 characters"; the intended comparison is "than 128 characters".
// that 128 characters.
src/tools/validator-keys/SigningKeys.cpp:239
- The key file is overwritten in place with
trunc. A crash or write error after truncation can leave the only copy of the master key, manifest, and pending signing state partially written or empty; this path is exercised on every token operation. Write a fully validated temporary file in the same directory and atomically replace the key file (while preserving owner-only permissions).
std::ofstream o(keyFile.string(), std::ios_base::trunc);
if (o.fail())
throw std::runtime_error("Cannot open key file: " + keyFile.string());
o << jv.toStyledString();
src/tools/validator-keys/SigningKeys.cpp:123
isIntegral()includesBoolean, andasUInt()convertsfalse/trueto 0/1. A malformed key file with"token_sequence": falseis therefore accepted and can generate a manifest from a fabricated sequence instead of being rejected as invalid key-file data; require signed/unsigned integer types explicitly.
if (!jKeys["token_sequence"].isIntegral())
throw std::runtime_error("");
tokenSequence = jKeys["token_sequence"].asUInt();
src/tools/validator-keys/ValidatorKeysTool.cpp:284
- The one-signature
finish_tokenpath has the same failure mode: it commits the pending token and clears its pending secret beforeemitBlockhas successfully produced the token. A failed/open or silently short output leaves no way to reconstruct the validation secret for the manifest now stored in the key file; make output and key-file state commit failure-safe together.
keys.writeToFile(options.keyFile);
src/tools/validator-keys/ValidatorKeysTool.cpp:159
- After opening the signed-list output, this writes the JSON but never checks the stream or close result, then reports success. A disk-full or other write error can therefore leave a truncated
vl.jsonwhile the command exits successfully; check the write/close status before printing the success message.
o << jv.toStyledString();
std::cout << "Written to " << outFile->string() << "\n";
src/tools/validator-keys/ValidatorKeysTool.cpp:142
- The token writer also ignores failures from
operator<<andclose(), so a disk-full or short write can leave a truncated secret-bearing token file while the key-file update has already succeeded. Check the stream before changing permissions or reporting success.
o << block.str();
o.close();
src/tools/validator-keys/doc/validator-keys-tool-guide.md:72
- The implementation allows the final non-revocation sequence (
UINT32_MAX - 1): starting at sequence 0, the guard atSigningKeys.cpp:307still permits the increment fromUINT32_MAX - 2to that value, while onlyUINT32_MAXis reserved for revocation. Therefore the documented limit is one token too low.
There is a hard limit of 4,294,967,293 tokens that can be generated for a given
validator key pair.
src/tools/validator-keys/doc/validator-keys-tool-guide.md:94
- The documented revocation value also contains a literal
|, which is not valid base64. Copying this example produces an invalid[validator_key_revocation]; remove the bar from the sample.
JP////9xIe0hvssbqmgzFH4/NDp1z|3ShkmCtFXuC5A0IUocppHopnASQN2MuMD1Puoyjvnr
- Files reviewed: 21/21 changed files
- Comments generated: 13
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…uites for validator-keys
…ut files before writing
There was a problem hiding this comment.
This slice of the diff covers two new gtest suites (ListSigning.cpp, SigningKeys.cpp) exercising the new validator-keys tool's list-signing and key-management logic, plus the CMakeLists.txt wiring the xrpl.validator-keys static library and the validator-keys executable. The production implementation files (SigningKeys.h/.cpp, ListSigning.h/.cpp, Commands.cpp) were not present with actual content in this diff slice (Commands.cpp diff was empty), so the real logic under test could not be reviewed here. The test code itself is thorough (covers file I/O, permission checks, external signing, revocation, list append/rotation) and I did not find concrete correctness or security bugs in the added lines. The CMakeLists.txt additions look standard and consistent with other tool targets in the repo.
There was a problem hiding this comment.
This diff adds only test infrastructure (Fixtures.h) and a comprehensive gtest suite (ListSigning.cpp) for the new validator-keys tool's list-signing functionality; no production code is included in this diff to review. The test code is well-structured (RAII TempDir, clear helper functions, thorough coverage of error paths for canonical JSON, unsigned-list parsing, versioned list signing/verification, and key-rotation append scenarios) and I found no correctness bugs, resource leaks, or test-pollution issues in it. Per review guidance, test-only style/naming/organization concerns are not flagged.
There was a problem hiding this comment.
This diff excerpt covers the new gtest suite for SigningKeys (src/tests/tools/validator-keys/SigningKeys.cpp) and the CMake wiring for the new validator-keys tool target. The test coverage is thorough (key-file round-trips, error paths for malformed key files, external master/signing key flows, token exhaustion/revocation, and manifest signature checks), and the CMakeLists.txt correctly builds the static xrpl.validator-keys library, links it into the validator-keys executable, and installs the binary. I did not find correctness, security, or resource-leak issues in the added lines. The actual implementation files (SigningKeys.h/.cpp, Commands.h/.cpp, ListSigning.h/.cpp, Main.cpp) referenced by these tests are not included in this diff excerpt, so the underlying signing/token/manifest logic itself could not be reviewed here.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical output-data-loss risk and additional correctness issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (12)
src/tests/tools/validator-keys/CMakeLists.txt:24
- This suite is configured while
cmake/XrplValidatorKeys.cmakeis processed before the top-levelinclude(CTest), sogtest_discover_tests()runs before CTest is enabled and can leave the suite absent fromctest(the direct CI executable invocation does not catch that). Enable CTest before adding this subdirectory in the top-level configuration, rather than relying only on the binary being built.
gtest_discover_tests(validator_keys_tests)
src/tools/validator-keys/Commands.cpp:185
base64Decodereturns partial data for invalid input, so a valid public-key base64 value followed by trailing junk is silently accepted here and the junk is ignored. This makescreate_externalaccept a key different from the supplied argument; require a base64 round-trip before checkingpublicKeyType, asdecodeSignaturedoes above.
if (auto const bytes = base64Decode(data); publicKeyType(makeSlice(bytes)))
return PublicKey(makeSlice(bytes));
src/tools/validator-keys/Commands.cpp:331
createTokenadvances the sequence and generates the secret before this state is persisted, but the key file is committed beforeoutput.blockactually writes the token. If the already-open output fails on write/close (for example, disk-full), the destructor removes the incomplete output while the key file has advanced, so the generated secret is lost and the next token skips a sequence. Stage the token/output and commit the key-file state only after the output succeeds, or otherwise preserve a retryable pending token.
keys.writeToFile(ctx.options.keyFile);
src/tools/validator-keys/Commands.cpp:355
- The same commit ordering loses an externally signed token if
output.blockfails after this write:finishTokenhas already cleared the pending state and advanced the sequence, while the manifest/secret is only emitted afterward. A failed write therefore leaves no recoverable token but consumes the sequence. Commit the key-file transition only after the output is successfully written, or retain the finished token as retryable state.
keys.writeToFile(ctx.options.keyFile);
src/tools/validator-keys/Commands.cpp:416
- Changing a domain for an external-master key creates and advances a token state here before
output.blockemits the manifest/token instructions. A write/close failure after the key-file update leaves the new manifest unavailable while the key file has already changed, so the external signing workflow cannot recover the intended token cleanly. Defer the key-file commit until output succeeds or retain the generated manifest as pending state.
keys.writeToFile(ctx.options.keyFile);
src/tools/validator-keys/ListSigning.cpp:325
- When appending a v2 list whose top-level manifest differs, this branch always re-signs with the supplied manifest, even if its sequence is older or equal to the existing one. A server that has already cached the existing newer manifest treats the supplied manifest as stale and verifies with the cached signing key, so the generated list is rejected. Compare the incoming and existing/per-entry manifest sequences and reject stale or same-sequence rotations.
if (existing[jss::manifest].asString() != manifestBase64)
{
if (!resign)
{
src/tools/validator-keys/ListSigning.cpp:323
- If the appended document has the same manifest,
blobs_v2is copied without validating its entries; the validation below only runs when the manifest changes. Thus a malformed existing v2 file (for example, an entry without a signature) is accepted and the output remains unusable byValidatorSite::parseBlobs. Validate the existing entries before copying them in either branch.
jv[jss::blobs_v2] = existing[jss::blobs_v2];
if (existing[jss::manifest].asString() != manifestBase64)
{
src/tools/validator-keys/ListSigning.cpp:150
- Duplicate detection scans the entire accumulated vector for every validator, making
parseUnsignedListO(n²) in the list size. The parser accepts documents up to 4 MiB, so a large or untrusted list can makeverify_listspend disproportionate time comparing keys; track seen keys in astd::set/hash set while building the vector instead.
if (std::ranges::find(list.validators, *key) != list.validators.end())
src/tools/validator-keys/ListSigning.cpp:123
ValidatorList::verifyaccepts an emptyvalidatorsarray andupdatePublisherListdeliberately handles it by removing the publisher's previous entries (it only logs a warning atsrc/xrpld/app/misc/detail/ValidatorList.cpp:985-988). Requiring a non-empty array here makessign_listunable to produce a valid empty list for clearing a publisher's roster, and makesverify_listreject such server-accepted lists; allow an empty array while still validating each entry.
if (!jv.isMember(jss::validators) || !jv[jss::validators].isArray() ||
jv[jss::validators].size() == 0)
throw std::runtime_error("\"validators\" must be a non-empty array");
src/tools/validator-keys/Main.cpp:43
- The help text makes the encoding argument look optional (
[hex|base64]), but the command table requires exactly one argument andcmdShowManifestindexesargs[0].validator-keys show_manifesttherefore fails despite the advertised syntax; either implement a default encoding or show the argument as required.
" show_manifest [hex|base64] Displays the last generated "
"manifest\n"
src/tools/validator-keys/SigningKeys.cpp:431
startPendingsigns a manifest built with the currentdomain_, butfinishPendingrebuilds it from the mutabledomain_at finish time. For an external key,set_domain/clear_domaincan change the persisted domain betweenstart_tokenandfinish_token; the returned bytes were signed without that change, so finishing with valid signatures fails (andstartTokenno longer fixes the manifest contents as documented). Persist the domain snapshot inPendingor retain/reuse the exact serialized partial manifest.
STObject st = partialManifest(tokenSequence_ + 1, pending.signingKey);
src/tools/validator-keys/doc/validator-keys-tool-guide.md:208
- This documents the version-2 append limit as four blobs, but the implementation and server accept five (
kMaxBlobs/kMaxSupportedBlobsare 5). Operators following this guide may unnecessarily reject a valid fifth blob or misunderstand the replacement window; update the guide to say five.
`blobs_v2` instead, and `--append <existing.json>` adds it to a version 2
document that already holds up to four blobs, so a list can be published
alongside the one it will replace.
- Files reviewed: 28/28 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
This portion of the diff refactors TrustedPublisherServer.h to reuse the shared makeManifest() helper from xrpl/server/Manifest.h instead of a local, duplicated implementation, and adds the new gtest suite for the validator-keys tool (CMakeLists.txt, Commands.cpp, Fixtures.h). The refactor looks correct and consistent with the change's stated goal of de-duplicating manifest/list signing logic. I didn't find any clear correctness, security, or resource-leak issues in the code shown; the new test files are well-scoped test helpers and cases, and per review guidance I'm not flagging test-only style/naming/assertion-pattern nitpicks. Without visibility into the corresponding production sources (src/tools/validator-keys/Commands.h, SigningKeys.h, ListSigning.h), I can't independently verify some of the exact error-string/sequence-number expectations asserted in the tests, so I'm not flagging those as bugs.
| throw std::runtime_error("Refusing to write through a symlink: " + path.string()); | ||
| } | ||
|
|
||
| stream_.open(temp_, std::ios_base::trunc); |
There was a problem hiding this comment.
Symlink TOCTOU between is_symlink check and stream_.open. Use O_NOFOLLOW in atomic creation:
| stream_.open(temp_, std::ios_base::trunc); | |
| int fd = ::open(temp_.c_str(), O_WRONLY | O_CREAT | O_EXCL | O_NOFOLLOW, 0600); | |
| if (fd < 0) | |
| throw std::runtime_error("Cannot safely create file"); | |
| stream_.rdbuf()->open(fd, std::ios::out); |
| throw std::runtime_error("Refusing to write through a symlink: " + path.string()); | ||
| } | ||
|
|
||
| stream_.open(temp_, std::ios_base::trunc); |
There was a problem hiding this comment.
⚪ Severity: LOW
TOCTOU race between the is_symlink() check (line 20) and stream_.open(). An attacker with write access to the parent directory can replace the path with a symlink between the check and the open, causing secret key material (master keys, token secrets) to be written to an attacker-controlled location. ofstream::open follows symlinks; use open() with O_CREAT|O_EXCL|O_NOFOLLOW for atomic creation.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: Replace the is_symlink() check + stream_.open() pattern with a POSIX open() call using O_CREAT | O_WRONLY | O_TRUNC | O_NOFOLLOW (and optionally O_EXCL if the file should not already exist). This atomically refuses to open a symlink, eliminating the TOCTOU race window.
Concretely:
- In
OwnerOnlyFile.h, add#include <fcntl.h>and#include <unistd.h>(or use<cstdio>with platform guards for Windows). - In the constructor, replace the symlink check loop and
stream_.open(temp_, ...)with:int fd = ::open(temp_.c_str(), O_CREAT | O_WRONLY | O_TRUNC | O_NOFOLLOW, 0600);- If
fd < 0anderrno == ELOOP, throw the symlink error. - If
fd < 0otherwise, throw the cannot-write error. - Use a platform-specific mechanism to attach
fdtostream_(e.g., GCC's__gnu_cxx::stdio_filebuf, orfdopen()+ a customstd::streambuf, or switchstream_fromstd::ofstreamto a POSIX file-descriptor–based wrapper).
- Keep the symlink check for
target_(which is only used later incommit()viafs::rename), or apply a similarO_NOFOLLOWstrategy iftarget_is also opened elsewhere. - On Windows,
O_NOFOLLOWis not available; guard with#ifdef _WIN32or useCreateFilewith appropriate flags.
The fs::permissions() call at line 29 can also be replaced since open() with mode 0600 sets owner-only permissions atomically at creation time.
There was a problem hiding this comment.
🟡 Changes recommended
Critical and moderate review findings remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (9)
Previously missed (2) — in code that hasn't changed since the last review.
src/tools/validator-keys/Commands.cpp:171
base64Decodestops at the first invalid character, so a value such as the canonical base64 public key followed by!decodes to the valid 33-byte key and is accepted here even though the supplied encoding is malformed. Require a canonical round trip (base64Encode(bytes) == data) asdecodeSignaturedoes before constructing the key.
src/tools/validator-keys/ListSigning.cpp:322- If the existing manifest is unchanged,
blobs_v2is copied without validating any entry; the shape checks below only run when the manifest changes. A malformed append (for example an entry without a stringsignature) can therefore produce a signed-list document thatverify_listand xrpld reject. Validate every existing blob entry before copying it.
src/tools/validator-keys/Commands.cpp:318
- The key file is committed before the output file is written and renamed.
Outputonly pre-opens<out>.tmp; a later failure such as--outnaming an existing directory makescommit()fail after this line, socreate_tokenloses the generated validation secret (the same ordering is used by token/domain commands). Stage both updates or otherwise make output failure recoverable before consuming the key state.
keys.writeToFile(ctx.options.keyFile);
output.block("validator_token", nodePublic(keys), tokenToBase64(token));
src/tools/validator-keys/ListSigning.cpp:322
- When the supplied manifest differs from the existing v2 document, this path re-signs all existing blobs with the supplied key without checking its sequence. Passing an older token therefore downgrades the top-level manifest; a server that has already cached the existing newer manifest will treat the new one as stale and reject blobs signed by the older key. Compare manifest sequences and reject a downgrade before appending.
if (existing[jss::manifest].asString() != manifestBase64)
src/tools/validator-keys/ListSigning.cpp:359
- Appending this blob does not validate its
sequenceagainst the existing v2 blobs.--appendcan therefore emit a document with a lower or duplicate sequence;ValidatorList::applyListtreats that blob as stale/same and ignores it, whileverifyListcan report the document as valid. Compare the parsed new sequence with the existing blobs and reject non-increasing values.
json::Value entry(json::ValueType::Object);
entry[jss::blob] = blob;
entry[jss::signature] = signatureHex;
jv[jss::blobs_v2].append(entry);
src/tools/validator-keys/OwnerOnlyFile.cpp:16
- The deterministic temporary name can be an input file that
Outputdid not reject: for example,--out foo --keyfile foo.tmpopensfoo.tmpwith truncation before the key is loaded, destroying the key file. Use a non-colliding temporary name or reject inputs equivalent totarget + ".tmp"before opening it.
: target_(std::move(target)), temp_(target_.string() + ".tmp"), what_(std::move(what))
src/tools/validator-keys/SigningKeys.cpp:213
- A stored regular manifest is only checked for signatures and master/revocation state; its sequence is not compared with
tokenSequence_. If a key file is migrated or corrupted withmanifest.sequence > token_sequence, the nextcreate_tokensignstoken_sequence + 1, producing a stale manifest that xrpld will ignore. Reject regular stored manifests whose sequence exceeds the persisted token sequence (while allowing a higher token sequence for migration).
src/tools/validator-keys/SigningKeys.cpp:393 startTokensigns bytes containing the currentdomain_, butPendingdoes not retain that domain;finishPendingreconstructs the manifest with whatever domain is set later. Runningset_domain/domain()betweenstart_tokenandfinish_tokentherefore makes the previously returned signature fail. Snapshot the domain in the pending state or reject domain changes while a token is pending.
src/tools/validator-keys/SigningKeys.cpp:468revoke()reports that an external key file cannot be used to "sign tokens", even though this command is creating a key revocation. This is misleading for the documented external-signing flow; use the existing generic signing error (or a revocation-specific message) here.
- Files reviewed: 35/35 changed files
- Comments generated: 2
- Review effort level: Lite
|
|
||
| namespace xrpl { | ||
|
|
||
| class STObject; |
| throw std::runtime_error("Refusing to write through a symlink: " + path.string()); | ||
| } | ||
|
|
||
| stream_.open(temp_, std::ios_base::trunc); |
| throw std::runtime_error("Refusing to write through a symlink: " + path.string()); | ||
| } | ||
|
|
||
| stream_.open(temp_, std::ios_base::trunc); |
There was a problem hiding this comment.
TOCTOU: File permissions set after creation; use umask(0177) or open(O_CREAT|O_EXCL, 0600):
| stream_.open(temp_, std::ios_base::trunc); | |
| auto const old_umask = umask(0177); | |
| stream_.open(temp_, std::ios_base::trunc); | |
| umask(old_umask); |
High Level Overview of Change
Moves
validator-keysfrom ripple/validator-keys-tool into the tree assrc/tools/validator-keys. Same-Dvalidator_keys=ONoption, same binary, same packages.Also in this PR:
create_external,start_token,finish_token,start_revoke_keys,finish_revoke_keys,sign_hex. The master key can stay in a hardware signer; the tool prints the bytes to sign and takes the signature back.start_token --signing-keyand a two-signaturefinish_tokencover a signing key that is external as well.sign_list,start_sign_list,finish_sign_list,verify_list. A publisher's signing key is a validator token fromcreate_token --token-key-type ed25519, the input is the unsigned list JSON thatValidatorListalready reads, andverify_listmakes the checks a server makes before trusting a list.create_token --out, and Renamerippled.cfgtoxrpld.cfgripple/validator-keys-tool#60.ripple/validator-keys-tool#61 asked where the tool should live; a separate target in this repository was the option nobody objected to. Same shape as #7555 without the Conan test package change.
Context of Change
#7905 put
validator-keysin the packages but left its source in another repository, pinned by commit. The XRPLF publishes its list with a separate signer that reimplements manifest and list signing in three languages, all copying what already exists here:Manifest.handValidatorListfor verification, the helpers inManifest_test.cppandValidatorList_test.cppfor signing. With the signing half in the tool that ships with xrpld there is one implementation to review, and the publisher tooling stops handling keys.Manifest and revocation signing moves into
libxrplasmakeManifest,makeRevocationand the unsigned-fields functions inManifest.h, next todeserializeManifest. The tool uses them, and the copies inManifest_test.cpp,ValidatorList_test.cppandTrustedPublisherServer.hare deleted.The tool's
ValidatorKeysclass is renamedSigningKeysbecausesrc/xrpld/app/misc/ValidatorKeys.halready declares one. Tests are gtest undersrc/tests/tools/validator-keys, built asvalidator_keys_tests; CI runs that where it ranvalidator-keys --unittest. The tool linksxrpl.libxrplonly.API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)Manifest.hgains signing functions; nothing existing changes. No RPC or peer protocol change.Test Plan
validator_keys_tests: 27 cases, 0 failures.xrpld --unittest=xrpl.app.Manifest,xrpl.app.ValidatorList,xrpl.app.ValidatorSite,xrpl.app.ValidatorKeys: 8943 tests, 0 failures. A list signed by the tool was served to a standalone xrpld through[validator_list_sites]and thevalidatorsRPC reported the publisher available with both validators.Future Tasks
Archive ripple/validator-keys-tool with a pointer here.