From 625c541ed26775841ac9a85b6ab87de010600e38 Mon Sep 17 00:00:00 2001 From: Phil Wise Date: Fri, 2 Aug 2024 07:24:29 +0100 Subject: [PATCH 1/6] Add missing includes and reduce test flake --- src/aktualizr_info/aktualizr_info_test.cc | 1 + src/aktualizr_secondary/aktualizr_secondary_test.cc | 2 ++ src/cert_provider/cert_provider_test.cc | 1 + src/libaktualizr/config/config_test.cc | 1 + tests/CMakeLists.txt | 3 +++ 5 files changed, 8 insertions(+) diff --git a/src/aktualizr_info/aktualizr_info_test.cc b/src/aktualizr_info/aktualizr_info_test.cc index 6b15c0547..6f4c3753e 100644 --- a/src/aktualizr_info/aktualizr_info_test.cc +++ b/src/aktualizr_info/aktualizr_info_test.cc @@ -1,6 +1,7 @@ #include #include +#include #include "libaktualizr/config.h" #include "storage/sqlstorage.h" diff --git a/src/aktualizr_secondary/aktualizr_secondary_test.cc b/src/aktualizr_secondary/aktualizr_secondary_test.cc index 7094e019f..98705261d 100644 --- a/src/aktualizr_secondary/aktualizr_secondary_test.cc +++ b/src/aktualizr_secondary/aktualizr_secondary_test.cc @@ -2,7 +2,9 @@ #include #include +#include #include +#include #include "aktualizr_secondary_file.h" #include "crypto/keymanager.h" diff --git a/src/cert_provider/cert_provider_test.cc b/src/cert_provider/cert_provider_test.cc index 913ccecab..b1c4336be 100644 --- a/src/cert_provider/cert_provider_test.cc +++ b/src/cert_provider/cert_provider_test.cc @@ -1,6 +1,7 @@ #include #include +#include #include #include "cert_provider_test.h" diff --git a/src/libaktualizr/config/config_test.cc b/src/libaktualizr/config/config_test.cc index 0c1ae8e02..79a8e6e51 100644 --- a/src/libaktualizr/config/config_test.cc +++ b/src/libaktualizr/config/config_test.cc @@ -6,6 +6,7 @@ #include #include #include +#include #include "bootstrap/bootstrap.h" #include "crypto/crypto.h" diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index ffcf09610..4e99310f0 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -197,6 +197,9 @@ add_test(NAME test_ip_secondary COMMAND ${PROJECT_SOURCE_DIR}/tests/ipsecondary_test.py --build-dir ${PROJECT_BINARY_DIR} --src-dir ${PROJECT_SOURCE_DIR}) set_tests_properties(test_ip_secondary PROPERTIES LABELS "noptest") +# This test flakes when run in parallel. Force it to run serially +# TODO Fix this test! +set_tests_properties(test_ip_secondary PROPERTIES RUN_SERIAL ON) add_test(NAME test_ip_secondary_rotation COMMAND ${PROJECT_SOURCE_DIR}/tests/ipsecondary_rotation_test.py From a674dd00904f94e87a9a0389d814f2b718bab890 Mon Sep 17 00:00:00 2001 From: Phil Wise Date: Tue, 9 Jul 2024 07:56:13 +0100 Subject: [PATCH 2/6] Reproducer for TOR-3452 --- src/uptane_generator/repo.cc | 14 ++- src/uptane_generator/repo.h | 2 +- src/uptane_generator/uptane_repo.cc | 6 +- src/uptane_generator/uptane_repo.h | 2 +- src/virtual_secondary/CMakeLists.txt | 18 ++- src/virtual_secondary/bad_rotation_test.cc | 127 +++++++++++++++++++++ 6 files changed, 152 insertions(+), 17 deletions(-) create mode 100644 src/virtual_secondary/bad_rotation_test.cc diff --git a/src/uptane_generator/repo.cc b/src/uptane_generator/repo.cc index 9d21daf8e..faa4d00bc 100644 --- a/src/uptane_generator/repo.cc +++ b/src/uptane_generator/repo.cc @@ -8,6 +8,7 @@ #include "crypto/crypto.h" #include "director_repo.h" #include "image_repo.h" +#include "json/json.h" #include "libaktualizr/campaign.h" Repo::Repo(Uptane::RepositoryType repo_type, boost::filesystem::path path, const std::string &expires, @@ -288,7 +289,7 @@ void Repo::readKeys() { } } -void Repo::refresh(const Uptane::Role &role) { +void Repo::refresh(const Uptane::Role &role, const TimeStamp &expiry) { if (repo_type_ == Uptane::RepositoryType::Director() && (role == Uptane::Role::Timestamp() || role == Uptane::Role::Snapshot())) { throw std::runtime_error("The " + role.ToString() + " in the Director repo is not currently supported."); @@ -307,14 +308,14 @@ void Repo::refresh(const Uptane::Role &role) { throw std::runtime_error("Refreshing custom role " + role.ToString() + " is not currently supported."); } - // The only interesting part here is to increment the version. It could be - // interesting to allow changing the expiry, too. Json::Value meta_raw = Utils::parseJSONFile(meta_path)["signed"]; const unsigned version = meta_raw["version"].asUInt() + 1; auto current_expire_time = TimeStamp(meta_raw["expires"].asString()); - if (current_expire_time.IsExpiredAt(TimeStamp::Now())) { + if (expiry.IsValid()) { + meta_raw["expires"] = expiry.ToString(); + } else if (current_expire_time.IsExpiredAt(TimeStamp::Now())) { time_t new_expiration_time; std::time(&new_expiration_time); new_expiration_time += 60 * 60; // make it valid for the next hour @@ -370,8 +371,9 @@ void Repo::rotate(const Uptane::Role &role, KeyType key_type) { // Sign Root with old and new key auto intermediate_meta = signTuf(role, meta_raw); - const std::string signed_meta = Utils::jsonToCanonicalStr(signTuf(old_key, intermediate_meta)); - Utils::writeFile(meta_path, signed_meta); + auto signed_meta = signTuf(old_key, intermediate_meta); + + Utils::writeFile(meta_path, Utils::jsonToCanonicalStr(signed_meta)); std::stringstream root_name; root_name << version << ".root.json"; diff --git a/src/uptane_generator/repo.h b/src/uptane_generator/repo.h index 4cc4c47f4..e3da013ca 100644 --- a/src/uptane_generator/repo.h +++ b/src/uptane_generator/repo.h @@ -41,7 +41,7 @@ class Repo { Json::Value getTarget(const std::string &target_name); Json::Value signTuf(const Uptane::Role &role, const Json::Value &json); void generateCampaigns() const; - void refresh(const Uptane::Role &role); + void refresh(const Uptane::Role &role, const TimeStamp &expiry); void rotate(const Uptane::Role &role, KeyType key_type = KeyType::kRSA2048); protected: diff --git a/src/uptane_generator/uptane_repo.cc b/src/uptane_generator/uptane_repo.cc index a03bb7724..49d0a1c96 100644 --- a/src/uptane_generator/uptane_repo.cc +++ b/src/uptane_generator/uptane_repo.cc @@ -45,11 +45,11 @@ void UptaneRepo::emptyTargets() { director_repo_.emptyTargets(); } void UptaneRepo::oldTargets() { director_repo_.oldTargets(); } void UptaneRepo::generateCampaigns() { director_repo_.generateCampaigns(); } -void UptaneRepo::refresh(Uptane::RepositoryType repo_type, const Uptane::Role &role) { +void UptaneRepo::refresh(Uptane::RepositoryType repo_type, const Uptane::Role &role, const TimeStamp &expiry) { if (repo_type == Uptane::RepositoryType::Director()) { - director_repo_.refresh(role); + director_repo_.refresh(role, expiry); } else if (repo_type == Uptane::RepositoryType::Image()) { - image_repo_.refresh(role); + image_repo_.refresh(role, expiry); } } diff --git a/src/uptane_generator/uptane_repo.h b/src/uptane_generator/uptane_repo.h index 081c540b5..d46dddffb 100644 --- a/src/uptane_generator/uptane_repo.h +++ b/src/uptane_generator/uptane_repo.h @@ -23,7 +23,7 @@ class UptaneRepo { void emptyTargets(); void oldTargets(); void generateCampaigns(); - void refresh(Uptane::RepositoryType repo_type, const Uptane::Role &role); + void refresh(Uptane::RepositoryType repo_type, const Uptane::Role &role, const TimeStamp &expiry = TimeStamp()); void rotate(Uptane::RepositoryType repo_type, const Uptane::Role &role, KeyType key_type = KeyType::kRSA2048); private: diff --git a/src/virtual_secondary/CMakeLists.txt b/src/virtual_secondary/CMakeLists.txt index 7d75b4ad3..bc442092e 100644 --- a/src/virtual_secondary/CMakeLists.txt +++ b/src/virtual_secondary/CMakeLists.txt @@ -4,13 +4,19 @@ set(HEADERS managedsecondary.h virtualsecondary.h) set(TARGET virtual_secondary) -add_library(${TARGET} STATIC - ${SOURCES} -) +add_library(virtual_secondary STATIC ${SOURCES}) target_include_directories(${TARGET} PUBLIC ${PROJECT_SOURCE_DIR}/src/virtual_secondary) -add_aktualizr_test(NAME virtual_secondary SOURCES virtual_secondary_test.cc PROJECT_WORKING_DIRECTORY LIBRARIES uptane_generator_lib) -target_link_libraries(t_virtual_secondary virtual_secondary) +add_aktualizr_test(NAME virtual_secondary + SOURCES virtual_secondary_test.cc + PROJECT_WORKING_DIRECTORY + LIBRARIES uptane_generator_lib virtual_secondary) -aktualizr_source_file_checks(${HEADERS} ${SOURCES} ${TEST_SOURCES}) +add_aktualizr_test(NAME bad_rotation + SOURCES bad_rotation_test.cc + PROJECT_WORKING_DIRECTORY + LIBRARIES uptane_generator_lib virtual_secondary) + + +aktualizr_source_file_checks(${HEADERS} ${SOURCES} ${TEST_SOURCES} ${FUZZ_SECONDARY_SOURCES}) diff --git a/src/virtual_secondary/bad_rotation_test.cc b/src/virtual_secondary/bad_rotation_test.cc new file mode 100644 index 000000000..2f809f359 --- /dev/null +++ b/src/virtual_secondary/bad_rotation_test.cc @@ -0,0 +1,127 @@ +#include + +#include + +#include +#include "boost/filesystem.hpp" +#include "httpfake.h" +#include "libaktualizr/secondaryinterface.h" +#include "libaktualizr/types.h" +#include "logging/logging.h" +#include "uptane/tuf.h" +#include "uptane_repo.h" +#include "uptane_test_common.h" +#include "utilities/utils.h" +#include "virtualsecondary.h" + +/* + * Reproduction for TOR-3452. Rotate the root metadata to avoid it expiring, then recover + */ +TEST(VirtualSecondary, RootRotationExpires) { // NOLINT + TemporaryDirectory temp_dir; + TemporaryDirectory meta_dir; + auto http = std::make_shared(temp_dir.Path(), "", meta_dir.Path() / "repo"); + Config conf = UptaneTestCommon::makeTestConfig(temp_dir, http->tls_server); + logger_set_threshold(boost::log::trivial::trace); + + UptaneRepo uptane_repo{meta_dir.PathString(), "", "2023-03-04T16:43:12Z"}; + uptane_repo.generateRepo(KeyType::kED25519); + uptane_repo.addImage("tests/test_data/firmware.txt", "firmware.txt", "secondary_hw"); + + const std::string hwid = "primary_hw"; + + Utils::writeFile(meta_dir / "fake_meta/primary_firmware.txt", std::string("asdf")); + uptane_repo.addImage(meta_dir / "fake_meta/primary_firmware.txt", "primary_firmware.txt", hwid); + Utils::writeFile(meta_dir / "fake_meta/primary_firmware2.txt", std::string("asdf")); + uptane_repo.addImage(meta_dir / "fake_meta/primary_firmware2.txt", "primary_firmware2.txt", hwid); + uptane_repo.addImage("tests/test_data/firmware_name.txt", "firmware_name.txt", "secondary_hw"); + uptane_repo.addImage("tests/test_data/firmware.txt", "firmware2.txt", "secondary_hw"); + + time_t new_expiration_time; + std::time(&new_expiration_time); + new_expiration_time += 15; // make it valid for the next 15 seconds + struct tm new_expiration_time_str {}; + gmtime_r(&new_expiration_time, &new_expiration_time_str); + + auto timestamp = TimeStamp(new_expiration_time_str); + uptane_repo.refresh(Uptane::RepositoryType::Image(), Uptane::Role::Root(), timestamp); + uptane_repo.refresh(Uptane::RepositoryType::Director(), Uptane::Role::Root(), timestamp); + + result::UpdateCheck update_result; + result::Download download_result; + result::Install install_result; + + uptane_repo.emptyTargets(); + uptane_repo.addTarget("firmware_name.txt", "secondary_hw", "secondary_ecu_serial"); + uptane_repo.signTargets(); + + { + LOG_INFO << "Starting initial run"; + auto storage = INvStorage::newStorage(conf.storage); + UptaneTestCommon::TestAktualizr aktualizr(conf, storage, http); + aktualizr.Initialize(); + update_result = aktualizr.CheckUpdates().get(); + ASSERT_EQ(update_result.status, result::UpdateStatus::kUpdatesAvailable); + download_result = aktualizr.Download(update_result.updates).get(); + ASSERT_EQ(download_result.status, result::DownloadStatus::kSuccess); + install_result = aktualizr.Install(download_result.updates).get(); + EXPECT_TRUE(install_result.dev_report.success); + } + bool expire = true; + if (expire) { + LOG_INFO << "Sleeping in a warehouse"; + for (int x = 0; x < 25; x++) { + sleep(1); + if (timestamp.IsExpiredAt(TimeStamp::Now())) { + break; + } + } + ASSERT_TRUE(timestamp.IsExpiredAt(TimeStamp::Now())); + } else { + ASSERT_FALSE(timestamp.IsExpiredAt(TimeStamp::Now())); + } + + uptane_repo.refresh(Uptane::RepositoryType::Image(), Uptane::Role::Root(), TimeStamp("2024-01-01T16:43:12Z")); + uptane_repo.refresh(Uptane::RepositoryType::Director(), Uptane::Role::Root(), TimeStamp("2024-01-01T16:43:12Z")); + uptane_repo.refresh(Uptane::RepositoryType::Image(), Uptane::Role::Root(), TimeStamp("2025-01-01T16:43:12Z")); + uptane_repo.refresh(Uptane::RepositoryType::Director(), Uptane::Role::Root(), TimeStamp("2025-01-01T16:43:12Z")); + + { + LOG_INFO << "Starting second run"; + auto storage = INvStorage::newStorage(conf.storage); + UptaneTestCommon::TestAktualizr aktualizr(conf, storage, http); + aktualizr.Initialize(); + + uptane_repo.emptyTargets(); + uptane_repo.addTarget("primary_firmware.txt", hwid, "CA:FE:A6:D2:84:9D"); + uptane_repo.signTargets(); + update_result = aktualizr.CheckUpdates().get(); + ASSERT_EQ(update_result.status, result::UpdateStatus::kUpdatesAvailable); + download_result = aktualizr.Download(update_result.updates).get(); + ASSERT_EQ(download_result.status, result::DownloadStatus::kSuccess); + install_result = aktualizr.Install(download_result.updates).get(); + EXPECT_TRUE(install_result.dev_report.success); + + uptane_repo.emptyTargets(); + uptane_repo.addTarget("firmware2.txt", "secondary_hw", "secondary_ecu_serial"); + uptane_repo.signTargets(); + + update_result = aktualizr.CheckUpdates().get(); + ASSERT_EQ(update_result.status, result::UpdateStatus::kUpdatesAvailable); + download_result = aktualizr.Download(update_result.updates).get(); + ASSERT_EQ(download_result.status, result::DownloadStatus::kSuccess); + install_result = aktualizr.Install(download_result.updates).get(); + EXPECT_TRUE(install_result.dev_report.success); + } +} + +#ifndef __NO_MAIN__ +int main(int argc, char** argv) { + ::testing::InitGoogleTest(&argc, argv); + + logger_init(); + logger_set_threshold(boost::log::trivial::trace); + + return RUN_ALL_TESTS(); +} +#endif From d4f91ffc5b72d07ee1f772bcbd5c4a4de6cbbec3 Mon Sep 17 00:00:00 2001 From: Phil Wise Date: Sun, 21 Jul 2024 15:45:14 +0100 Subject: [PATCH 3/6] Fix for TOR-3452 The problem was that the first call to checkMetaOffline() throws an exception which results in image_repo_->checkMetaOffline() not being called. There are a bunch of root causes that should be fixed at some point: - The *Repository classes are not fully initialized in the ctor - The required initialization is called 'checkMetaOffline()'' - There is no documentation that says this is so - There are no asserts to enforce the above requirement at runtime. --- src/virtual_secondary/managedsecondary.cc | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/virtual_secondary/managedsecondary.cc b/src/virtual_secondary/managedsecondary.cc index 69758d631..65672fbcd 100644 --- a/src/virtual_secondary/managedsecondary.cc +++ b/src/virtual_secondary/managedsecondary.cc @@ -61,9 +61,16 @@ ManagedSecondary::ManagedSecondary(Primary::ManagedSecondaryConfig sconfig_in) : try { director_repo_->checkMetaOffline(*storage_); + } catch (const std::exception &e) { + // This is actually safe. We've done enough initialization to get + // director_repo_ into a valid configuration + LOG_INFO << "No valid Director metadata found in storage: " << e.what(); + } + try { image_repo_->checkMetaOffline(*storage_); } catch (const std::exception &e) { - LOG_INFO << "No valid metadata found in storage."; + // See above ^ image_repo_ is OK + LOG_INFO << "No valid Image metadata found in storage: " << e.what(); } } From ac7a7610e76ed3cde0fca69051c76569efe0f59e Mon Sep 17 00:00:00 2001 From: Phil Wise Date: Sun, 21 Jul 2024 15:55:07 +0100 Subject: [PATCH 4/6] Add documentation for checkMetaOffline --- src/libaktualizr/uptane/directorrepository.h | 2 +- src/libaktualizr/uptane/imagerepository.h | 2 +- src/libaktualizr/uptane/uptanerepository.h | 15 ++++++++++----- 3 files changed, 12 insertions(+), 7 deletions(-) diff --git a/src/libaktualizr/uptane/directorrepository.h b/src/libaktualizr/uptane/directorrepository.h index 1ac0051e7..308468116 100644 --- a/src/libaktualizr/uptane/directorrepository.h +++ b/src/libaktualizr/uptane/directorrepository.h @@ -21,7 +21,7 @@ class DirectorRepository : public RepositoryCommon { return targets.getTargets(ecu_id, hw_id); } Uptane::CorrelationId getCorrelationId() const { return correlation_id_; } - void checkMetaOffline(INvStorage& storage); + void checkMetaOffline(INvStorage& storage) override; void dropTargets(INvStorage& storage); void updateMeta(INvStorage& storage, const IMetadataFetcher& fetcher, diff --git a/src/libaktualizr/uptane/imagerepository.h b/src/libaktualizr/uptane/imagerepository.h index 255cda45a..7b93c5f8b 100644 --- a/src/libaktualizr/uptane/imagerepository.h +++ b/src/libaktualizr/uptane/imagerepository.h @@ -30,7 +30,7 @@ class ImageRepository : public RepositoryCommon { int getRoleVersion(const Uptane::Role& role) const; int64_t getRoleSize(const Uptane::Role& role) const; - void checkMetaOffline(INvStorage& storage); + void checkMetaOffline(INvStorage& storage) override; void updateMeta(INvStorage& storage, const IMetadataFetcher& fetcher, const api::FlowControlToken* flow_control) override; diff --git a/src/libaktualizr/uptane/uptanerepository.h b/src/libaktualizr/uptane/uptanerepository.h index 8dd1762e5..2718950c3 100644 --- a/src/libaktualizr/uptane/uptanerepository.h +++ b/src/libaktualizr/uptane/uptanerepository.h @@ -1,9 +1,8 @@ #ifndef UPTANE_REPOSITORY_H_ #define UPTANE_REPOSITORY_H_ -#include // for int64_t -#include // for string -#include "fetcher.h" +#include // for int64_t +#include // for string #include "libaktualizr/types.h" // for TimeStamp #include "uptane/tuf.h" // for Root, RepositoryType #include "utilities/flow_control.h" @@ -15,8 +14,7 @@ class IMetadataFetcher; class RepositoryCommon { public: - // NOLINTNEXTLINE(google-explicit-constructor, hicpp-explicit-conversions) - RepositoryCommon(RepositoryType type_in) : type{type_in} {} + explicit RepositoryCommon(RepositoryType type_in) : type{type_in} {} virtual ~RepositoryCommon() = default; RepositoryCommon(const RepositoryCommon &guard) = default; RepositoryCommon(RepositoryCommon &&) = default; @@ -26,6 +24,13 @@ class RepositoryCommon { void verifyRoot(const std::string &root_raw); int rootVersion() const { return root.version(); } bool rootExpired() const { return root.isExpired(TimeStamp::Now()); } + + /** + * Load the initial state of the repository from storage. + * Note that this _required_ for correct initialization. + * @throws UptaneException if the local metadata is stale (this is not a failure) + */ + virtual void checkMetaOffline(INvStorage &storage) = 0; virtual void updateMeta(INvStorage &storage, const IMetadataFetcher &fetcher, const api::FlowControlToken *flow_control) = 0; From f476f21600d176ca31f88fe652052a28947622eb Mon Sep 17 00:00:00 2001 From: Phil Wise Date: Sun, 21 Jul 2024 15:57:47 +0100 Subject: [PATCH 5/6] Remove default ctor for Uptane::Root There only one place that uses this default, so remove the default ctor and explicitly express it in RepositoryCommon. This means that no-one has to remember "What is the policy for a default-constructed Root object?"" any more. --- src/libaktualizr/uptane/tuf.h | 2 +- src/libaktualizr/uptane/uptanerepository.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libaktualizr/uptane/tuf.h b/src/libaktualizr/uptane/tuf.h index 0484318cf..ff28c1015 100644 --- a/src/libaktualizr/uptane/tuf.h +++ b/src/libaktualizr/uptane/tuf.h @@ -234,7 +234,7 @@ class Root : public MetaWithKeys { /** * An empty Root, that either accepts or rejects everything */ - explicit Root(Policy policy = Policy::kRejectAll) : policy_(policy) { version_ = 0; } + explicit Root(Policy policy) : policy_(policy) { version_ = 0; } /** * A 'real' Root that implements TUF signature validation * @param repo - Repository type (only used to improve the error messages) diff --git a/src/libaktualizr/uptane/uptanerepository.h b/src/libaktualizr/uptane/uptanerepository.h index 2718950c3..53c44437d 100644 --- a/src/libaktualizr/uptane/uptanerepository.h +++ b/src/libaktualizr/uptane/uptanerepository.h @@ -40,7 +40,7 @@ class RepositoryCommon { static const int64_t kMaxRotations = 1000; - Root root; + Root root{Root::Policy::kRejectAll}; RepositoryType type; }; } // namespace Uptane From bda0573746a86734de7748f6c47ddb243239d379 Mon Sep 17 00:00:00 2001 From: Phil Wise Date: Wed, 24 Jul 2024 17:55:39 +0100 Subject: [PATCH 6/6] Tidy up root rotation logic This wasn't implicated in TOR-3452, but the new version is shorter and has simple control flow. --- src/libaktualizr/primary/sotauptaneclient.cc | 75 +++++++++++--------- 1 file changed, 42 insertions(+), 33 deletions(-) diff --git a/src/libaktualizr/primary/sotauptaneclient.cc b/src/libaktualizr/primary/sotauptaneclient.cc index c45c143e7..7cb0ddb52 100644 --- a/src/libaktualizr/primary/sotauptaneclient.cc +++ b/src/libaktualizr/primary/sotauptaneclient.cc @@ -1292,52 +1292,61 @@ data::InstallationResult SotaUptaneClient::rotateSecondaryRoot(Uptane::Repositor LOG_ERROR << "Error reading Root metadata"; return data::InstallationResult(data::ResultCode::Numeric::kInternalError, "Error reading Root metadata"); } - - data::InstallationResult result{data::ResultCode::Numeric::kOk, ""}; const int last_root_version = Uptane::extractVersionUntrusted(latest_root); const int sec_root_version = secondary.getRootVersion((repo == Uptane::RepositoryType::Director())); - // If sec_root_version is 0, assume either the Secondary doesn't have Root - // metadata or doesn't support the Root version request. Continue on and hope - // for the best. + LOG_DEBUG << "Rotating " << repo << " from " << sec_root_version << " to " << (last_root_version - 1); if (sec_root_version < 0) { LOG_WARNING << "Secondary with serial " << secondary.getSerial() << " reported an invalid " << repo << " repo Root version: " << sec_root_version; - result = - data::InstallationResult(data::ResultCode::Numeric::kInternalError, - "Secondary with serial " + secondary.getSerial().ToString() + " reported an invalid " + - repo.ToString() + " repo Root version: " + std::to_string(sec_root_version)); - } else if (sec_root_version > 0 && last_root_version - sec_root_version > 1) { - // Only send intermediate Roots that would otherwise be skipped. The latest - // will be sent with the complete set of the latest metadata. - for (int v = sec_root_version + 1; v < last_root_version; v++) { - std::string root; - if (!storage->loadRoot(&root, repo, Uptane::Version(v))) { - LOG_WARNING << "Couldn't find Root metadata in the storage, trying remote repo"; - try { - uptane_fetcher->fetchRole(&root, Uptane::kMaxRootSize, repo, Uptane::Role::Root(), Uptane::Version(v), - flow_control_); - } catch (const std::exception &e) { - LOG_ERROR << "Root metadata could not be fetched for Secondary with serial " << secondary.getSerial() - << ", skipping to the next Secondary"; - result = data::InstallationResult(data::ResultCode::Numeric::kInternalError, - "Root metadata could not be fetched for Secondary with serial " + - secondary.getSerial().ToString() + ", skipping to the next Secondary"); - break; - } - } + return {data::ResultCode::Numeric::kInternalError, "Secondary with serial " + secondary.getSerial().ToString() + + " reported an invalid " + repo.ToString() + + " repo Root version: " + std::to_string(sec_root_version)}; + } + + // Only send intermediate Roots that would otherwise be skipped. The latest + // will be sent with the complete set of the latest metadata. + for (int version_to_send = sec_root_version + 1; version_to_send < last_root_version; version_to_send++) { + std::string root; + if (!storage->loadRoot(&root, repo, Uptane::Version(version_to_send))) { + LOG_WARNING << "Couldn't find Root metadata in the storage, trying remote repo"; try { - result = secondary.putRoot(root, repo == Uptane::RepositoryType::Director()); - } catch (const std::exception &ex) { - result = data::InstallationResult(data::ResultCode::Numeric::kInternalError, ex.what()); + uptane_fetcher->fetchRole(&root, Uptane::kMaxRootSize, repo, Uptane::Role::Root(), + Uptane::Version(version_to_send), flow_control_); + } catch (const std::exception &e) { + LOG_ERROR << "Root metadata could not be fetched for Secondary with serial " << secondary.getSerial() + << ", skipping to the next Secondary"; + return {data::ResultCode::Numeric::kInternalError, + "Root metadata could not be fetched for Secondary with serial " + secondary.getSerial().ToString() + + ", skipping to the next Secondary"}; } + } + try { + auto result = secondary.putRoot(root, repo == Uptane::RepositoryType::Director()); if (!result.isSuccess()) { + // Old (pre 2024-07-XX) versions would assume that if sec_root_version + // is 0, either the Secondary doesn't have Root metadata or doesn't + // support the Root version request and skip sending any root metadata. + // Unfortunatately this cause TOR-3452 where an expired root metadata + // would cause updates to fail. Instead assume that '0' could mean 'I + // don't have any root versions yet'. If we send version 1 and it is + // rejected, then assume we are in the case that the code originally was + // defending against: the secondary can't rotate root, and treat this + // as a success. The previous code would have returned success in this + // case anyway. + if (version_to_send == 1) { + LOG_WARNING + << "Sending root.1.json to a secondary failed. Assuming it doesn't allow root rotation and continuing."; + return {data::ResultCode::Numeric::kOk, ""}; + } LOG_ERROR << "Sending Root metadata to Secondary with serial " << secondary.getSerial() << " failed: " << result.result_code << " " << result.description; - break; + return result; } + } catch (const std::exception &ex) { + return {data::ResultCode::Numeric::kInternalError, ex.what()}; } } - return result; + return {data::ResultCode::Numeric::kOk, ""}; } // TODO: the function blocks until it updates all the Secondaries. Consider non-blocking operation.