diff --git a/src/tests/tools/validator-keys/Commands.cpp b/src/tests/tools/validator-keys/Commands.cpp index 48f9ba88cf..97b220d827 100644 --- a/src/tests/tools/validator-keys/Commands.cpp +++ b/src/tests/tools/validator-keys/Commands.cpp @@ -184,7 +184,15 @@ TEST_F(CommandsTest, create_token) unwritable.outFile = file("missing/token.txt"); EXPECT_EQ( commandError("create_token", {}, unwritable), - "Cannot open output file: " + unwritable.outFile->string()); + "Cannot write output file: " + unwritable.outFile->string()); + EXPECT_EQ(keys(options_).sequence(), 2u); + + // The output may not replace the key file + ToolOptions aliased = options_; + aliased.outFile = options_.keyFile; + EXPECT_EQ( + commandError("create_token", {}, aliased), + "--out names an input file: " + options_.keyFile.string()); EXPECT_EQ(keys(options_).sequence(), 2u); { @@ -410,7 +418,22 @@ TEST_F(CommandsTest, list_commands) bad.outFile = file("missing/vl.json"); EXPECT_EQ( commandError("sign_list", {unsignedList.string()}, bad), - "Cannot open output file: " + bad.outFile->string()); + "Cannot write output file: " + bad.outFile->string()); + + // The output may not replace the list it signs + bad.outFile = unsignedList; + EXPECT_EQ( + commandError("sign_list", {unsignedList.string()}, bad), + "--out names an input file: " + unsignedList.string()); + + // A failed command leaves an existing output as it was + bad.outFile = file("kept.json"); + writeFile(*bad.outFile, "previous"); + EXPECT_EQ( + commandError("sign_list", {file("missing.json").string()}, bad), + "Failed to open file: " + file("missing.json").string()); + EXPECT_EQ(readFile(*bad.outFile), "previous"); + EXPECT_FALSE(std::filesystem::exists(file("kept.json.tmp"))); } EXPECT_EQ( commandError("verify_list", {file("missing.json").string()}, verifier), diff --git a/src/tests/tools/validator-keys/SigningKeys.cpp b/src/tests/tools/validator-keys/SigningKeys.cpp index cf24f0c42e..e229c98e25 100644 --- a/src/tests/tools/validator-keys/SigningKeys.cpp +++ b/src/tests/tools/validator-keys/SigningKeys.cpp @@ -145,6 +145,10 @@ TEST_F(SigningKeysTest, write_to_file_errors) auto const nested = dir_.file("a/b/c/validator_keys.json"); keys.writeToFile(nested); EXPECT_TRUE(keys == SigningKeys::makeSigningKeys(nested)); + EXPECT_EQ( + std::filesystem::status(std::filesystem::path(nested).parent_path()).permissions() & + (std::filesystem::perms::group_all | std::filesystem::perms::others_all), + std::filesystem::perms::none); // The parent path is a file auto const blocked = std::filesystem::path(keyFile_.string() + "/keys.json"); diff --git a/src/tools/validator-keys/CMakeLists.txt b/src/tools/validator-keys/CMakeLists.txt index d3f52260d7..f92848d15b 100644 --- a/src/tools/validator-keys/CMakeLists.txt +++ b/src/tools/validator-keys/CMakeLists.txt @@ -6,7 +6,7 @@ include(GNUInstallDirs) add_library(xrpl.validator-keys STATIC) target_sources( xrpl.validator-keys - PRIVATE Commands.cpp ListSigning.cpp SigningKeys.cpp + PRIVATE Commands.cpp ListSigning.cpp OwnerOnlyFile.cpp SigningKeys.cpp ) target_include_directories( xrpl.validator-keys diff --git a/src/tools/validator-keys/Commands.cpp b/src/tools/validator-keys/Commands.cpp index 6397eaa8b7..5fca3540b7 100644 --- a/src/tools/validator-keys/Commands.cpp +++ b/src/tools/validator-keys/Commands.cpp @@ -19,6 +19,7 @@ #include #include +#include #include #include @@ -26,8 +27,7 @@ #include #include #include -#include -#include +#include #include #include #include @@ -72,58 +72,50 @@ struct Context }; /** - * Where a command's result goes: the output file named by `--out`, opened - * before the command changes any state so an unwritable path fails first, or - * the output stream. A file that receives nothing is removed again. + * Where a command's result goes: the file named by `--out`, replaced whole + * once the command has succeeded, or the output stream. The file is prepared + * before the command changes any state so an unwritable path fails first. */ class Output { - // Empty when the result goes to the output stream. - std::filesystem::path file_; - std::ofstream stream_; + // Null when the result goes to the output stream. + std::unique_ptr file_; std::ostream& out_; - bool written_ = false; public: - Output(std::optional const& file, std::ostream& out) - : file_(file.value_or(std::filesystem::path{})), out_(out) + /** + * @param inputs Files the command reads besides those in the options; + * `--out` may not name any input, since the output replaces it. + */ + // A result that only goes to the output stream. + explicit Output(std::ostream& out) : out_(out) { - if (!file_.empty()) - { - if (std::filesystem::is_symlink(file_)) - throw std::runtime_error("Refusing to write through a symlink: " + file_.string()); - stream_.open(file_, std::ios_base::trunc); - if (stream_.fail()) - throw std::runtime_error("Cannot open output file: " + file_.string()); - // A token holds a secret: restrict the file before anything is written. - std::error_code ec; - std::filesystem::permissions( - file_, - std::filesystem::perms::owner_read | std::filesystem::perms::owner_write, - ec); - if (ec) - { - stream_.close(); // LCOV_EXCL_LINE - std::filesystem::remove(file_, ec); // LCOV_EXCL_LINE - throw std::runtime_error( // LCOV_EXCL_LINE - "Cannot restrict output file: " + file_.string()); // LCOV_EXCL_LINE - } - } } - ~Output() + Output(Context const& ctx, std::vector const& inputs = {}) + : out_(ctx.out) { - if (!file_.empty() && !written_) - { - stream_.close(); - std::error_code ec; - std::filesystem::remove(file_, ec); - } - } + auto const& options = ctx.options; + if (!options.outFile) + return; - Output(Output const&) = delete; - Output& - operator=(Output const&) = delete; + std::vector read(inputs); + read.push_back(options.keyFile); + for (auto const& file : + {options.tokenFile, options.manifestFile, options.appendFile, options.validatorsFile}) + { + if (file) + read.push_back(*file); + } + for (auto const& file : read) + { + std::error_code ec; + if (std::filesystem::equivalent(file, *options.outFile, ec)) + throw std::runtime_error("--out names an input file: " + options.outFile->string()); + } + + file_ = std::make_unique(*options.outFile, "output file"); + } // A config block in 72-character lines. void @@ -133,7 +125,7 @@ public: for (std::size_t i = 0; i < body.size(); i += kBlockLineLength) text.append(body, i, kBlockLineLength).push_back('\n'); - if (file_.empty()) + if (!file_) { out_ << "Update xrpld.cfg file with these values and restart xrpld:\n\n" << text << std::endl; @@ -145,7 +137,7 @@ public: void json(json::Value const& jv) { - if (file_.empty()) + if (!file_) { out_ << jv.toStyledString() << std::endl; return; @@ -157,15 +149,9 @@ private: void write(std::string const& text, std::string const& what) { - stream_ << text; - stream_.close(); - if (stream_.fail()) - { - throw std::runtime_error( // LCOV_EXCL_LINE - "Cannot write output file: " + file_.string()); // LCOV_EXCL_LINE - } - written_ = true; - out_ << what << " written to " << file_.string() << "\n"; + file_->write(text); + file_->commit(); + out_ << what << " written to " << file_->target().string() << "\n"; } }; @@ -325,7 +311,7 @@ cmdCreateExternal(Args const& args, Context& ctx) int cmdCreateToken(Args const&, Context& ctx) { - Output output(ctx.options.outFile, ctx.out); + Output output(ctx); auto keys = SigningKeys::makeSigningKeys(ctx.options.keyFile); auto const token = keys.createToken(ctx.options.tokenKeyType); keys.writeToFile(ctx.options.keyFile); @@ -346,7 +332,7 @@ cmdStartToken(Args const&, Context& ctx) int cmdFinishToken(Args const& args, Context& ctx) { - Output output(ctx.options.outFile, ctx.out); + Output output(ctx); auto keys = SigningKeys::makeSigningKeys(ctx.options.keyFile); std::optional signingSig; if (args.size() == 2) @@ -364,7 +350,7 @@ cmdRevokeKeys(Args const&, Context& ctx) warnRevocation(keys, ctx.err); auto const revocation = keys.revoke(); keys.writeToFile(ctx.options.keyFile); - Output(std::nullopt, ctx.out).block("validator_key_revocation", nodePublic(keys), revocation); + Output(ctx.out).block("validator_key_revocation", nodePublic(keys), revocation); return EXIT_SUCCESS; } @@ -384,14 +370,14 @@ cmdFinishRevokeKeys(Args const& args, Context& ctx) warnRevocation(keys, ctx.err); auto const revocation = keys.finishRevoke(decodeSignature(args[0])); keys.writeToFile(ctx.options.keyFile); - Output(std::nullopt, ctx.out).block("validator_key_revocation", nodePublic(keys), revocation); + Output(ctx.out).block("validator_key_revocation", nodePublic(keys), revocation); return EXIT_SUCCESS; } int setDomain(std::string const& domain, Context& ctx) { - Output output(ctx.options.outFile, ctx.out); + Output output(ctx); auto keys = loadUnrevoked(ctx.options.keyFile); if (domain == keys.domain()) @@ -529,7 +515,7 @@ cmdSignList(Args const& args, Context& ctx) { if (!ctx.options.tokenFile) throw std::runtime_error("sign_list needs --token-file"); - Output output(ctx.options.outFile, ctx.out); + Output output(ctx, {args[0]}); auto const token = loadTokenFile(*ctx.options.tokenFile); auto const manifest = deserializeManifest(base64Decode(token.manifest)); @@ -563,7 +549,7 @@ int cmdFinishSignList(Args const& args, Context& ctx) { auto const [manifest, signingKey] = loadSigningManifest(ctx, "finish_sign_list"); - Output output(ctx.options.outFile, ctx.out); + Output output(ctx, {args[1]}); auto const list = loadUnsignedList(args[1]); auto const sig = decodeSignature(args[0]); diff --git a/src/tools/validator-keys/OwnerOnlyFile.cpp b/src/tools/validator-keys/OwnerOnlyFile.cpp new file mode 100644 index 0000000000..77800a40bb --- /dev/null +++ b/src/tools/validator-keys/OwnerOnlyFile.cpp @@ -0,0 +1,70 @@ +#include + +#include +#include +#include +#include +#include +#include +#include + +namespace xrpl { + +namespace fs = std::filesystem; + +OwnerOnlyFile::OwnerOnlyFile(fs::path target, std::string what) + : target_(std::move(target)), temp_(target_.string() + ".tmp"), what_(std::move(what)) +{ + for (auto const& path : {target_, temp_}) + { + if (fs::is_symlink(path)) + throw std::runtime_error("Refusing to write through a symlink: " + path.string()); + } + + stream_.open(temp_, std::ios_base::trunc); + if (stream_.fail()) + throw std::runtime_error("Cannot write " + what_ + ": " + target_.string()); + + std::error_code ec; + fs::permissions(temp_, fs::perms::owner_read | fs::perms::owner_write, ec); + if (ec) + { + stream_.close(); // LCOV_EXCL_LINE + fs::remove(temp_, ec); // LCOV_EXCL_LINE + throw std::runtime_error( // LCOV_EXCL_LINE + "Cannot write " + what_ + ": " + target_.string()); // LCOV_EXCL_LINE + } +} + +OwnerOnlyFile::~OwnerOnlyFile() +{ + if (!committed_) + { + stream_.close(); + std::error_code ec; + fs::remove(temp_, ec); + } +} + +void +OwnerOnlyFile::write(std::string const& text) +{ + stream_ << text; +} + +void +OwnerOnlyFile::commit() +{ + stream_.close(); + std::error_code ec; + if (!stream_.fail()) + fs::rename(temp_, target_, ec); + if (stream_.fail() || ec) + { + fs::remove(temp_, ec); + throw std::runtime_error("Cannot write " + what_ + ": " + target_.string()); + } + committed_ = true; +} + +} // namespace xrpl diff --git a/src/tools/validator-keys/OwnerOnlyFile.h b/src/tools/validator-keys/OwnerOnlyFile.h new file mode 100644 index 0000000000..9f24fcaa93 --- /dev/null +++ b/src/tools/validator-keys/OwnerOnlyFile.h @@ -0,0 +1,59 @@ +#pragma once + +#include +#include +#include + +namespace xrpl { + +/** + * A file written for its owner only and replaced whole. + * + * The content goes to a temporary beside the target, restricted to the owner + * before the first byte, and the target is replaced in one step by `commit`. + * Without `commit` the temporary is removed, so a failed command leaves the + * previous target as it was. + */ +class OwnerOnlyFile +{ + std::filesystem::path target_; + std::filesystem::path temp_; + // Names the file in errors: "key file", "output file". + std::string what_; + std::ofstream stream_; + bool committed_ = false; + +public: + /** + * Opens the temporary. + * + * @throws std::runtime_error if the target or the temporary is a symlink, + * or the temporary cannot be opened or restricted + */ + OwnerOnlyFile(std::filesystem::path target, std::string what); + ~OwnerOnlyFile(); + + OwnerOnlyFile(OwnerOnlyFile const&) = delete; + OwnerOnlyFile& + operator=(OwnerOnlyFile const&) = delete; + + void + write(std::string const& text); + + /** + * Replaces the target with what was written. + * + * @throws std::runtime_error if the content could not be written or the + * target could not be replaced + */ + void + commit(); + + [[nodiscard]] std::filesystem::path const& + target() const + { + return target_; + } +}; + +} // namespace xrpl diff --git a/src/tools/validator-keys/SigningKeys.cpp b/src/tools/validator-keys/SigningKeys.cpp index 7057a79fb8..99cc8a080c 100644 --- a/src/tools/validator-keys/SigningKeys.cpp +++ b/src/tools/validator-keys/SigningKeys.cpp @@ -24,13 +24,13 @@ #include +#include + #include #include #include #include #include -#include -#include #include #include #include @@ -290,35 +290,16 @@ SigningKeys::writeToFile(std::filesystem::path const& keyFile) const std::error_code ec; if (auto const parent = keyFile.parent_path(); !parent.empty()) { - fs::create_directories(parent, ec); + // A directory made here is the owner's alone. + if (fs::create_directories(parent, ec) && !ec) + fs::permissions(parent, fs::perms::owner_all, ec); if (ec || !fs::is_directory(parent)) throw std::runtime_error("Cannot create directory: " + parent.string()); } - // Write beside the key file, restrict it to the owner, then replace the - // key file in one step. - auto const temp = fs::path(keyFile.string() + ".tmp"); - if (fs::is_symlink(temp)) - throw std::runtime_error("Refusing to write through a symlink: " + temp.string()); - { - std::ofstream o(temp, std::ios_base::trunc); - if (!o.fail()) - fs::permissions(temp, fs::perms::owner_read | fs::perms::owner_write, ec); - if (!ec) - o << jv.toStyledString(); - o.close(); - if (ec || o.fail()) - { - fs::remove(temp, ec); - throw std::runtime_error("Cannot write key file: " + keyFile.string()); - } - } - fs::rename(temp, keyFile, ec); - if (ec) - { - fs::remove(temp, ec); - throw std::runtime_error("Cannot write key file: " + keyFile.string()); - } + OwnerOnlyFile file(keyFile, "key file"); + file.write(jv.toStyledString()); + file.commit(); } STObject