From 6bdc9e7b302ab1d3e036db86dc038a562714ccb6 Mon Sep 17 00:00:00 2001 From: Mike Ellery Date: Wed, 31 Oct 2018 08:21:59 -0700 Subject: [PATCH] Use correct manifest cache when loading ValidatorList --- src/ripple/app/misc/impl/ValidatorList.cpp | 2 +- src/ripple/app/misc/impl/ValidatorSite.cpp | 106 +++++++++++---------- src/test/app/ValidatorList_test.cpp | 34 +++++++ src/test/app/ValidatorSite_test.cpp | 13 +++ 4 files changed, 103 insertions(+), 52 deletions(-) diff --git a/src/ripple/app/misc/impl/ValidatorList.cpp b/src/ripple/app/misc/impl/ValidatorList.cpp index afc27f296..b86e06d37 100644 --- a/src/ripple/app/misc/impl/ValidatorList.cpp +++ b/src/ripple/app/misc/impl/ValidatorList.cpp @@ -107,7 +107,7 @@ ValidatorList::load ( auto id = PublicKey(makeSlice(ret.first)); - if (validatorManifests_.revoked (id)) + if (publisherManifests_.revoked (id)) { JLOG (j_.warn()) << "Configured validator list publisher key is revoked: " << key; diff --git a/src/ripple/app/misc/impl/ValidatorSite.cpp b/src/ripple/app/misc/impl/ValidatorSite.cpp index 515f41a21..4d631350a 100644 --- a/src/ripple/app/misc/impl/ValidatorSite.cpp +++ b/src/ripple/app/misc/impl/ValidatorSite.cpp @@ -383,68 +383,72 @@ ValidatorSite::onSiteFetch( detail::response_type&& res, std::size_t siteIdx) { - bool shouldRetry = false; { std::lock_guard lock_sites{sites_mutex_}; - try + auto onError = [&](std::string const& errMsg, bool retry) { - if (ec) - { - JLOG (j_.warn()) << + sites_[siteIdx].lastRefreshStatus.emplace( + Site::Status{clock_type::now(), + ListDisposition::invalid, + errMsg}); + if (retry) + sites_[siteIdx].nextRefresh = + clock_type::now() + ERROR_RETRY_INTERVAL; + }; + if (ec) + { + JLOG (j_.warn()) << "Problem retrieving from " << sites_[siteIdx].activeResource->uri << " " << ec.value() << ":" << ec.message(); - shouldRetry = true; - throw std::runtime_error{"fetch error"}; - } - - using namespace boost::beast::http; - switch (res.result()) - { - case status::ok: - parseJsonResponse(res, siteIdx, lock_sites); - break; - case status::moved_permanently : - case status::permanent_redirect : - case status::found : - case status::temporary_redirect : - { - auto newLocation = processRedirect (res, siteIdx, lock_sites); - assert(newLocation); - // for perm redirects, also update our starting URI - if (res.result() == status::moved_permanently || - res.result() == status::permanent_redirect) - { - sites_[siteIdx].startingResource = newLocation; - } - makeRequest(newLocation, siteIdx, lock_sites); - return; // we are still fetching, so skip - // state update/notify below - } - default: - { - JLOG (j_.warn()) << - "Request for validator list at " << - sites_[siteIdx].activeResource->uri << - " returned bad status: " << - res.result_int(); - shouldRetry = true; - throw std::runtime_error{"bad result code"}; - } - } + onError("fetch error", true); } - catch (std::exception& ex) + else { - sites_[siteIdx].lastRefreshStatus.emplace( - Site::Status{clock_type::now(), - ListDisposition::invalid, - ex.what()}); - if (shouldRetry) - sites_[siteIdx].nextRefresh = - clock_type::now() + ERROR_RETRY_INTERVAL; + try + { + using namespace boost::beast::http; + switch (res.result()) + { + case status::ok: + parseJsonResponse(res, siteIdx, lock_sites); + break; + case status::moved_permanently : + case status::permanent_redirect : + case status::found : + case status::temporary_redirect : + { + auto newLocation = + processRedirect (res, siteIdx, lock_sites); + assert(newLocation); + // for perm redirects, also update our starting URI + if (res.result() == status::moved_permanently || + res.result() == status::permanent_redirect) + { + sites_[siteIdx].startingResource = newLocation; + } + makeRequest(newLocation, siteIdx, lock_sites); + return; // we are still fetching, so skip + // state update/notify below + } + default: + { + JLOG (j_.warn()) << + "Request for validator list at " << + sites_[siteIdx].activeResource->uri << + " returned bad status: " << + res.result_int(); + onError("bad result code", true); + } + } + } + catch (std::exception& ex) + { + onError(ex.what(), false); + } } sites_[siteIdx].activeResource.reset(); } diff --git a/src/test/app/ValidatorList_test.cpp b/src/test/app/ValidatorList_test.cpp index 8168c6589..05a5d253c 100644 --- a/src/test/app/ValidatorList_test.cpp +++ b/src/test/app/ValidatorList_test.cpp @@ -355,6 +355,40 @@ private: for (auto const& key : keys) BEAST_EXPECT(trustedKeys->trustedPublisher (key)); } + { + // Attempt to load a publisher key that has been revoked. + // Should fail + ManifestCache valManifests; + ManifestCache pubManifests; + auto trustedKeys = std::make_unique ( + valManifests, pubManifests, env.timeKeeper(), env.journal); + + auto const pubRevokedSecret = randomSecretKey(); + auto const pubRevokedPublic = + derivePublicKey(KeyType::ed25519, pubRevokedSecret); + auto const pubRevokedSigning = randomKeyPair(KeyType::secp256k1); + // make this manifest revoked (seq num = max) + // -- thus should not be loaded + pubManifests.applyManifest (*Manifest::make_Manifest ( + makeManifestString ( + pubRevokedPublic, + pubRevokedSecret, + pubRevokedSigning.first, + pubRevokedSigning.second, + std::numeric_limits::max ()))); + + // this one is not revoked (and not in manifest cache at all.) + auto legitKey = randomMasterKey(); + + std::vector cfgPublishers = { + strHex(pubRevokedPublic), + strHex(legitKey) }; + BEAST_EXPECT(trustedKeys->load ( + emptyLocalKey, emptyCfgKeys, cfgPublishers)); + + BEAST_EXPECT(!trustedKeys->trustedPublisher (pubRevokedPublic)); + BEAST_EXPECT(trustedKeys->trustedPublisher (legitKey)); + } } void diff --git a/src/test/app/ValidatorSite_test.cpp b/src/test/app/ValidatorSite_test.cpp index 65e5872d6..825fc589b 100644 --- a/src/test/app/ValidatorSite_test.cpp +++ b/src/test/app/ValidatorSite_test.cpp @@ -31,6 +31,7 @@ #include #include #include +#include namespace ripple { namespace test { @@ -165,6 +166,7 @@ private: std::string uri; std::string expectMsg; bool shouldFail; + bool isRetry; }; std::vector servers; @@ -191,6 +193,7 @@ private: servers.push_back({}); auto& item = servers.back(); item.shouldFail = ! cfg.second.empty(); + item.isRetry = cfg.first == "/bad-resource"; item.expectMsg = cfg.second; item.list.reserve (listSize); while (item.list.size () < listSize) @@ -243,11 +246,21 @@ private: != u.shouldFail, to_string(myStatus)); if (u.shouldFail) { + using namespace std::chrono; BEAST_EXPECTS( sink.strm_.str().find(u.expectMsg) != std::string::npos, sink.strm_.str()); log << " -- Msg: " << myStatus[jss::last_refresh_message].asString() << std::endl; + std::stringstream nextRefreshStr + {myStatus[jss::next_refresh_time].asString()}; + system_clock::time_point nextRefresh; + date::from_stream (nextRefreshStr, "%Y-%b-%d %T", nextRefresh); + BEAST_EXPECT(!nextRefreshStr.fail()); + auto now = system_clock::now(); + BEAST_EXPECTS( + nextRefresh <= now + (u.isRetry ? seconds{30} : minutes{5}), + "Now: " + to_string(now) + ", NR: " + nextRefreshStr.str()); } } }