From 66b2c853399be6570e6fa202c67fcf0189c4320f Mon Sep 17 00:00:00 2001 From: Patrick Vacek Date: Wed, 3 Jun 2020 16:34:41 +0200 Subject: [PATCH 01/10] Allow ECU re-registration to support changing Secondaries. Works for Virtual Secondaries, but still a work in progress for IP Secondaries. Signed-off-by: Patrick Vacek --- src/aktualizr_info/aktualizr_info_test.cc | 5 +- .../primary_secondary_registration_test.cc | 22 +++-- .../aktualizr_secondary.cc | 2 +- src/libaktualizr/primary/aktualizr_test.cc | 4 +- src/libaktualizr/primary/initializer.cc | 80 ++++++++++++++----- src/libaktualizr/primary/initializer.h | 33 +++++--- src/libaktualizr/primary/sotauptaneclient.cc | 65 +-------------- src/libaktualizr/primary/sotauptaneclient.h | 15 +--- src/libaktualizr/primary/uptane_key_test.cc | 6 +- src/libaktualizr/storage/invstorage.cc | 4 +- src/libaktualizr/storage/invstorage.h | 6 +- src/libaktualizr/storage/sqlstorage.cc | 32 ++------ src/libaktualizr/storage/sqlstorage.h | 2 +- .../storage/storage_common_test.cc | 11 +-- src/libaktualizr/uptane/uptane_test.cc | 4 +- 15 files changed, 130 insertions(+), 161 deletions(-) diff --git a/src/aktualizr_info/aktualizr_info_test.cc b/src/aktualizr_info/aktualizr_info_test.cc index 09e8b9ac4c..1c2817c263 100644 --- a/src/aktualizr_info/aktualizr_info_test.cc +++ b/src/aktualizr_info/aktualizr_info_test.cc @@ -166,9 +166,8 @@ TEST_F(AktualizrInfoTest, PrintSecondaryNotRegisteredOrRemoved) { db_storage_->storeEcuSerials({{primary_ecu_serial, primary_hw_id}, {secondary_ecu_serial, secondary_hw_id}}); db_storage_->storeEcuRegistered(); - db_storage_->storeMisconfiguredEcus( - {{secondary_ecu_serial_not_reg, secondary_hw_id_not_reg, EcuState::kNotRegistered}, - {secondary_ecu_serial_old, secondary_hw_id_old, EcuState::kOld}}); + db_storage_->saveMisconfiguredEcu({secondary_ecu_serial_not_reg, secondary_hw_id_not_reg, EcuState::kUnused}); + db_storage_->saveMisconfiguredEcu({secondary_ecu_serial_old, secondary_hw_id_old, EcuState::kOld}); aktualizr_info_process_.run(); ASSERT_FALSE(aktualizr_info_output.empty()); diff --git a/src/aktualizr_primary/primary_secondary_registration_test.cc b/src/aktualizr_primary/primary_secondary_registration_test.cc index 44fefce65d..f30c3268ea 100644 --- a/src/aktualizr_primary/primary_secondary_registration_test.cc +++ b/src/aktualizr_primary/primary_secondary_registration_test.cc @@ -9,20 +9,24 @@ boost::filesystem::path uptane_repos_dir; boost::filesystem::path fake_meta_dir; +/* This tests that a device that had an IP Secondary will still find it after + * recent changes, even if it does not connect when the device starts. */ TEST(PrimarySecondaryReg, SecondariesMigration) { - // This tests that a device that had an ip secondary will still find it after - // recent changes, even if it does not connect when the device starts + const Uptane::EcuSerial primary_serial{"p_serial"}; + const Uptane::EcuSerial secondary_serial{"s_serial"}; + const Uptane::HardwareIdentifier primary_hwid{"p_hwid"}; + const Uptane::HardwareIdentifier secondary_hwid{"s_hwid"}; TemporaryDirectory temp_dir; auto http = std::make_shared(temp_dir.Path(), "noupdates", fake_meta_dir); - const auto& url = http->tls_server; + Config conf("tests/config/basic.toml"); conf.uptane.director_server = url + "/director"; conf.uptane.repo_server = url + "/repo"; conf.provision.server = url; - conf.provision.primary_ecu_serial = "CA:FE:A6:D2:84:9D"; - conf.provision.primary_ecu_hardware_id = "primary_hw"; + conf.provision.primary_ecu_serial = primary_serial.ToString(); + conf.provision.primary_ecu_hardware_id = primary_hwid.ToString(); conf.storage.path = temp_dir.Path(); conf.import.base_path = temp_dir.Path() / "import"; conf.tls.server = url; @@ -30,14 +34,8 @@ TEST(PrimarySecondaryReg, SecondariesMigration) { const boost::filesystem::path sec_conf_path = temp_dir / "s_config.json"; conf.uptane.secondary_config_file = sec_conf_path; - Json::Value sec_conf; - auto storage = INvStorage::newStorage(conf.storage); - - const Uptane::EcuSerial primary_serial{"p_serial"}; - const Uptane::EcuSerial secondary_serial{"s_serial"}; - const Uptane::HardwareIdentifier primary_hwid{"p_hwid"}; - const Uptane::HardwareIdentifier secondary_hwid{"s_hwid"}; + Json::Value sec_conf; { // prepare storage the "old" way: diff --git a/src/aktualizr_secondary/aktualizr_secondary.cc b/src/aktualizr_secondary/aktualizr_secondary.cc index 67a8145b37..7b454f2cb7 100644 --- a/src/aktualizr_secondary/aktualizr_secondary.cc +++ b/src/aktualizr_secondary/aktualizr_secondary.cc @@ -199,7 +199,7 @@ bool AktualizrSecondary::doFullVerification(const Metadata& metadata) { void AktualizrSecondary::uptaneInitialize() { if (keys_->generateUptaneKeyPair().size() == 0) { - throw std::runtime_error("Failed to generate uptane key pair"); + throw std::runtime_error("Failed to generate Uptane key pair"); } // from uptane/initialize.cc but we only take care of our own serial/hwid diff --git a/src/libaktualizr/primary/aktualizr_test.cc b/src/libaktualizr/primary/aktualizr_test.cc index 1b8b5384d6..db86688a3c 100644 --- a/src/libaktualizr/primary/aktualizr_test.cc +++ b/src/libaktualizr/primary/aktualizr_test.cc @@ -158,9 +158,11 @@ TEST(Aktualizr, AddSecondary) { } EXPECT_EQ(expected_ecus.size(), 0); + // Historically, adding Secondaries after provisioning was not supported. Now + // it is. ecu_config.ecu_serial = "ecuserial4"; auto sec4 = std::make_shared(ecu_config); - EXPECT_THROW(aktualizr.AddSecondary(sec4), std::logic_error); + EXPECT_NO_THROW(aktualizr.AddSecondary(sec4)); } /* diff --git a/src/libaktualizr/primary/initializer.cc b/src/libaktualizr/primary/initializer.cc index 2baa2f59ed..dff7a46c7d 100644 --- a/src/libaktualizr/primary/initializer.cc +++ b/src/libaktualizr/primary/initializer.cc @@ -36,13 +36,8 @@ void Initializer::resetDeviceId() { storage_->clearDeviceId(); } // Postcondition [(serial, hw_id)] is in the storage void Initializer::initEcuSerials() { - EcuSerials ecu_serials; - - // TODO: the assumption now is that the set of connected ECUs doesn't change, but it might obviously - // not be the case. ECU discovery seems to be a big story and should be worked on accordingly. - if (storage_->loadEcuSerials(&ecu_serials)) { - return; - } + EcuSerials stored_ecu_serials; + storage_->loadEcuSerials(&stored_ecu_serials); std::string primary_ecu_serial_local = config_.primary_ecu_serial; if (primary_ecu_serial_local.empty()) { @@ -53,23 +48,67 @@ void Initializer::initEcuSerials() { if (primary_ecu_hardware_id.empty()) { primary_ecu_hardware_id = Utils::getHostname(); if (primary_ecu_hardware_id == "") { - throw Error("Could not get current host name, please configure an hardware ID explicitely"); + throw Error("Could not get current host name, please configure an hardware ID explicitly"); } } - ecu_serials.emplace_back(Uptane::EcuSerial(primary_ecu_serial_local), - Uptane::HardwareIdentifier(primary_ecu_hardware_id)); - + EcuSerials new_ecu_serials; + new_ecu_serials.emplace_back(Uptane::EcuSerial(primary_ecu_serial_local), + Uptane::HardwareIdentifier(primary_ecu_hardware_id)); for (const auto& s : secondaries_) { - ecu_serials.emplace_back(s.first, s.second->getHwId()); + new_ecu_serials.emplace_back(s.first, s.second->getHwId()); } - storage_->storeEcuSerials(ecu_serials); -} + register_ecus = stored_ecu_serials.empty(); + if (!stored_ecu_serials.empty()) { + // We should probably clear the misconfigured_ecus table once we have + // consent working. + std::vector found(stored_ecu_serials.size(), false); + + EcuCompare primary_comp(new_ecu_serials[0]); + EcuSerials::const_iterator store_it; + store_it = std::find_if(stored_ecu_serials.cbegin(), stored_ecu_serials.cend(), primary_comp); + if (store_it == stored_ecu_serials.cend()) { + LOG_INFO << "Configured Primary ECU serial " << new_ecu_serials[0].first << " with hardware ID " + << new_ecu_serials[0].second << " not found in storage."; + register_ecus = true; + } else { + found[static_cast(store_it - stored_ecu_serials.cbegin())] = true; + } + + // Check all configured Secondaries to see if any are new. + for (auto it = secondaries_.cbegin(); it != secondaries_.cend(); ++it) { + EcuCompare secondary_comp(std::make_pair(it->second->getSerial(), it->second->getHwId())); + store_it = std::find_if(stored_ecu_serials.cbegin(), stored_ecu_serials.cend(), secondary_comp); + if (store_it == stored_ecu_serials.cend()) { + LOG_INFO << "Configured Secondary ECU serial " << it->second->getSerial() << " with hardware ID " + << it->second->getHwId() << " not found in storage."; + register_ecus = true; + } else { + found[static_cast(store_it - stored_ecu_serials.cbegin())] = true; + } + } -void Initializer::resetEcuSerials() { storage_->clearEcuSerials(); } + // Check all stored Secondaries not already matched to see if any have been + // removed. Store them in a separate table to keep track of them. + std::vector::iterator found_it; + for (found_it = found.begin(); found_it != found.end(); ++found_it) { + if (!*found_it) { + auto not_registered = stored_ecu_serials[static_cast(found_it - found.begin())]; + LOG_INFO << "ECU serial " << not_registered.first << " with hardware ID " << not_registered.second + << " in storage was not found in Secondary configuration."; + register_ecus = true; + storage_->saveMisconfiguredEcu({not_registered.first, not_registered.second, EcuState::kOld}); + } + } + } -// Postcondition: (public, private) is in the storage. It should not be stored until secondaries are provisioned + if (register_ecus) { + storage_->storeEcuSerials(new_ecu_serials); + } +} + +// Postcondition: (public, private) is in the storage. It should not be stored until Secondaries are provisioned void Initializer::initPrimaryEcuKeys() { std::string key_pair; try { @@ -83,8 +122,6 @@ void Initializer::initPrimaryEcuKeys() { } } -void Initializer::resetEcuKeys() { storage_->clearPrimaryKeys(); } - bool Initializer::loadSetTlsCreds() { keys_.copyCertsToCurl(*http_client_); return keys_.isOk(); @@ -153,7 +190,9 @@ void Initializer::resetTlsCreds() { // Postcondition: "ECUs registered" flag set in the storage void Initializer::initEcuRegister() { - if (storage_->loadEcuRegistered()) { + // Allow re-registration if the ECUs have changed. + if (storage_->loadEcuRegistered() && !register_ecus) { + LOG_DEBUG << "All ECUs are already registered with the server."; return; } @@ -215,7 +254,8 @@ void Initializer::initSecondaryInfo() { // won't be there in case of an upgrade from an older version. // On the first initialization ECU serials should be already // initialized by this point. - if (!storage_->loadSecondaryInfo(serial, &info) || info.type == "" || info.pub_key.Type() == KeyType::kUnknown) { + if (register_ecus || !storage_->loadSecondaryInfo(serial, &info) || info.type == "" || + info.pub_key.Type() == KeyType::kUnknown) { info.serial = serial; info.hw_id = sec.getHwId(); info.type = sec.Type(); diff --git a/src/libaktualizr/primary/initializer.h b/src/libaktualizr/primary/initializer.h index a336ef85be..6f6bb72971 100644 --- a/src/libaktualizr/primary/initializer.h +++ b/src/libaktualizr/primary/initializer.h @@ -6,6 +6,7 @@ #include "http/httpinterface.h" #include "storage/invstorage.h" #include "uptane/secondaryinterface.h" +#include "uptane/tuf.h" const int MaxInitializationAttempts = 3; @@ -21,7 +22,7 @@ class Initializer { }; class KeyGenerationError : public Error { public: - KeyGenerationError(const std::string& what) : Error(std::string("Could not generate uptane key pair: ") + what) {} + KeyGenerationError(const std::string& what) : Error(std::string("Could not generate Uptane key pair: ") + what) {} }; class StorageError : public Error { public: @@ -37,25 +38,37 @@ class Initializer { }; private: - const ProvisionConfig& config_; - std::shared_ptr storage_; - std::shared_ptr http_client_; - KeyManager& keys_; - const std::map& secondaries_; - std::vector sec_info_; - void initDeviceId(); void resetDeviceId(); void initEcuSerials(); - void resetEcuSerials(); void initPrimaryEcuKeys(); - void resetEcuKeys(); void initTlsCreds(); void resetTlsCreds(); void initSecondaryInfo(); void initEcuRegister(); bool loadSetTlsCreds(); void initEcuReportCounter(); + + const ProvisionConfig& config_; + std::shared_ptr storage_; + std::shared_ptr http_client_; + KeyManager& keys_; + const std::map& secondaries_; + std::vector sec_info_; + bool register_ecus{false}; +}; + +class EcuCompare { + public: + explicit EcuCompare(std::pair ecu_in) + : serial(std::move(ecu_in.first)), hardware_id(std::move(ecu_in.second)) {} + bool operator()(const std::pair& in) const { + return (in.first == serial && in.second == hardware_id); + } + + private: + const Uptane::EcuSerial serial; + const Uptane::HardwareIdentifier hardware_id; }; #endif // INITIALIZER_H_ diff --git a/src/libaktualizr/primary/sotauptaneclient.cc b/src/libaktualizr/primary/sotauptaneclient.cc index e0e1534ce9..5e883f0c63 100644 --- a/src/libaktualizr/primary/sotauptaneclient.cc +++ b/src/libaktualizr/primary/sotauptaneclient.cc @@ -27,15 +27,6 @@ static void report_progress_cb(event::Channel *channel, const Uptane::Target &ta void SotaUptaneClient::addSecondary(const std::shared_ptr &sec) { Uptane::EcuSerial serial = sec->getSerial(); - if (storage->loadEcuRegistered()) { - EcuSerials serials; - storage->loadEcuSerials(&serials); - SerialCompare secondary_comp(serial); - if (std::find_if(serials.cbegin(), serials.cend(), secondary_comp) == serials.cend()) { - throw std::logic_error("Adding new Secondaries to a provisioned device is not implemented yet"); - } - } - const auto map_it = secondaries.find(serial); if (map_it != secondaries.end()) { throw std::runtime_error(std::string("Multiple Secondaries found with the same serial: ") + serial.ToString()); @@ -348,8 +339,6 @@ void SotaUptaneClient::initialize() { primary_ecu_hw_id_ = serials[0].second; LOG_INFO << "Primary ECU serial: " << primary_ecu_serial_ << " with hardware ID: " << primary_ecu_hw_id_; - verifySecondaries(); - std::string device_id; if (!storage->loadDeviceId(&device_id)) { throw std::runtime_error("Unable to load device ID after device registration."); @@ -429,7 +418,7 @@ void SotaUptaneClient::computeDeviceInstallationResult(data::InstallationResult auto ecu_serial = r.first; auto installation_res = r.second; - auto hw_id = ecuHwId(ecu_serial); + auto hw_id = getEcuHwId(ecu_serial); if (!hw_id) { // couldn't find any ECU with the given serial/ID @@ -496,7 +485,7 @@ void SotaUptaneClient::getNewTargets(std::vector *new_targets, u // and the ECU performing the verification is the Primary ECU, check that // all listed ECU identifiers correspond to ECUs that are actually present // in the vehicle. - const auto hw_id_known = ecuHwId(ecu_serial); + const auto hw_id_known = getEcuHwId(ecu_serial); if (!hw_id_known) { LOG_ERROR << "Unknown ECU ID in Director Targets metadata: " << ecu_serial.ToString(); throw Uptane::BadEcuId(target.filename()); @@ -1137,54 +1126,6 @@ bool SotaUptaneClient::putManifest(const Json::Value &custom) { return success; } -// Check stored Secondaries list against Secondaries known to aktualizr. -void SotaUptaneClient::verifySecondaries() { - storage->clearMisconfiguredEcus(); - EcuSerials serials; - if (!storage->loadEcuSerials(&serials) || serials.empty()) { - LOG_ERROR << "No ECU serials found in storage!"; - return; - } - - std::vector misconfigured_ecus; - std::vector found(serials.size(), false); - SerialCompare primary_comp(primaryEcuSerial()); - EcuSerials::const_iterator store_it; - store_it = std::find_if(serials.cbegin(), serials.cend(), primary_comp); - if (store_it == serials.cend()) { - LOG_ERROR << "Primary ECU serial " << primaryEcuSerial() << " not found in storage!"; - misconfigured_ecus.emplace_back(primaryEcuSerial(), Uptane::HardwareIdentifier(""), EcuState::kOld); - } else { - found[static_cast(std::distance(serials.cbegin(), store_it))] = true; - } - - for (auto it = secondaries.cbegin(); it != secondaries.cend(); ++it) { - SerialCompare secondary_comp(it->second->getSerial()); - store_it = std::find_if(serials.cbegin(), serials.cend(), secondary_comp); - if (store_it == serials.cend()) { - LOG_ERROR << "Secondary ECU serial " << it->second->getSerial() << " (hardware ID " << it->second->getHwId() - << ") not found in storage!"; - misconfigured_ecus.emplace_back(it->second->getSerial(), it->second->getHwId(), EcuState::kNotRegistered); - } else if (found[static_cast(std::distance(serials.cbegin(), store_it))]) { - LOG_ERROR << "Secondary ECU serial " << it->second->getSerial() << " (hardware ID " << it->second->getHwId() - << ") has a duplicate entry in storage!"; - } else { - found[static_cast(std::distance(serials.cbegin(), store_it))] = true; - } - } - - std::vector::iterator found_it; - for (found_it = found.begin(); found_it != found.end(); ++found_it) { - if (!*found_it) { - auto not_registered = serials[static_cast(std::distance(found.begin(), found_it))]; - LOG_WARNING << "ECU serial " << not_registered.first << " in storage was not reported to aktualizr!"; - misconfigured_ecus.emplace_back(not_registered.first, not_registered.second, EcuState::kOld); - } - } - - storage->storeMisconfiguredEcus(misconfigured_ecus); -} - bool SotaUptaneClient::waitSecondariesReachable(const std::vector &updates) { std::map targeted_secondaries; const Uptane::EcuSerial &primary_ecu_serial = primaryEcuSerial(); @@ -1505,7 +1446,7 @@ void SotaUptaneClient::checkAndUpdatePendingSecondaries() { } } -boost::optional SotaUptaneClient::ecuHwId(const Uptane::EcuSerial &serial) const { +boost::optional SotaUptaneClient::getEcuHwId(const Uptane::EcuSerial &serial) const { if (serial == primary_ecu_serial_ || serial.ToString().empty()) { if (primary_ecu_hw_id_ == Uptane::HardwareIdentifier::Unknown()) { return boost::none; diff --git a/src/libaktualizr/primary/sotauptaneclient.h b/src/libaktualizr/primary/sotauptaneclient.h index b06c32161f..ffcdeb8b95 100644 --- a/src/libaktualizr/primary/sotauptaneclient.h +++ b/src/libaktualizr/primary/sotauptaneclient.h @@ -27,6 +27,7 @@ #include "uptane/imagerepository.h" #include "uptane/iterator.h" #include "uptane/secondaryinterface.h" +#include "uptane/tuf.h" class SotaUptaneClient { public: @@ -137,7 +138,6 @@ class SotaUptaneClient { void reportInstalledPackages(); void reportNetworkInfo(); void reportAktualizrConfiguration(); - void verifySecondaries(); bool waitSecondariesReachable(const std::vector &updates); bool sendMetadataToEcus(const std::vector &targets); std::future sendFirmwareAsync(Uptane::SecondaryInterface &secondary, @@ -157,7 +157,7 @@ class SotaUptaneClient { bool offline); void checkAndUpdatePendingSecondaries(); const Uptane::EcuSerial &primaryEcuSerial() const { return primary_ecu_serial_; } - boost::optional ecuHwId(const Uptane::EcuSerial &serial) const; + boost::optional getEcuHwId(const Uptane::EcuSerial &serial) const; template void sendEvent(Args &&... args) { @@ -197,15 +197,4 @@ class TargetCompare { const Uptane::Target ⌖ }; -class SerialCompare { - public: - explicit SerialCompare(Uptane::EcuSerial serial_in) : serial(std::move(serial_in)) {} - bool operator()(const std::pair &in) const { - return (in.first == serial); - } - - private: - const Uptane::EcuSerial serial; -}; - #endif // SOTA_UPTANE_CLIENT_H_ diff --git a/src/libaktualizr/primary/uptane_key_test.cc b/src/libaktualizr/primary/uptane_key_test.cc index a72cb8a0d7..5267e336a7 100644 --- a/src/libaktualizr/primary/uptane_key_test.cc +++ b/src/libaktualizr/primary/uptane_key_test.cc @@ -151,7 +151,8 @@ TEST(UptaneKey, RecoverWithoutKeys) { { auto storage = INvStorage::newStorage(config.storage); auto sota_client = std_::make_unique(config, storage, http); - + sota_client->addSecondary(std::make_shared(ecu_config1)); + sota_client->addSecondary(std::make_shared(ecu_config2)); EXPECT_NO_THROW(sota_client->initialize()); UptaneKey_Check_Test::checkKeyTests(storage, *sota_client); @@ -167,7 +168,8 @@ TEST(UptaneKey, RecoverWithoutKeys) { { auto storage = INvStorage::newStorage(config.storage); auto sota_client = std_::make_unique(config, storage, http); - + sota_client->addSecondary(std::make_shared(ecu_config1)); + sota_client->addSecondary(std::make_shared(ecu_config2)); EXPECT_NO_THROW(sota_client->initialize()); UptaneKey_Check_Test::checkKeyTests(storage, *sota_client); } diff --git a/src/libaktualizr/storage/invstorage.cc b/src/libaktualizr/storage/invstorage.cc index 40fb9ab4ce..6ad3fb5d09 100644 --- a/src/libaktualizr/storage/invstorage.cc +++ b/src/libaktualizr/storage/invstorage.cc @@ -172,7 +172,9 @@ void INvStorage::FSSToSQLS(FSStorageRead& fs_storage, SQLStorage& sql_storage) { std::vector ecus; if (fs_storage.loadMisconfiguredEcus(&ecus)) { - sql_storage.storeMisconfiguredEcus(ecus); + for (auto& ecu : ecus) { + sql_storage.saveMisconfiguredEcu(ecu); + } } std::vector installed_versions; diff --git a/src/libaktualizr/storage/invstorage.h b/src/libaktualizr/storage/invstorage.h index 251b8804dd..004cee46b6 100644 --- a/src/libaktualizr/storage/invstorage.h +++ b/src/libaktualizr/storage/invstorage.h @@ -24,7 +24,9 @@ using load_data_t = bool (INvStorage::*)(std::string*) const; typedef std::vector> EcuSerials; -enum class EcuState { kOld = 0, kNotRegistered }; +// kUnused was previously kNotRegistered, but re-registration is now possible so +// that is no longer a misconfiguration. +enum class EcuState { kOld = 0, kUnused }; struct MisconfiguredEcu { MisconfiguredEcu(Uptane::EcuSerial serial_in, Uptane::HardwareIdentifier hardware_id_in, EcuState state_in) @@ -109,7 +111,7 @@ class INvStorage { virtual void storeCachedEcuManifest(const Uptane::EcuSerial& ecu_serial, const std::string& manifest) = 0; virtual bool loadCachedEcuManifest(const Uptane::EcuSerial& ecu_serial, std::string* manifest) const = 0; - virtual void storeMisconfiguredEcus(const std::vector& ecus) = 0; + virtual void saveMisconfiguredEcu(const MisconfiguredEcu& ecu) = 0; virtual bool loadMisconfiguredEcus(std::vector* ecus) const = 0; virtual void clearMisconfiguredEcus() = 0; diff --git a/src/libaktualizr/storage/sqlstorage.cc b/src/libaktualizr/storage/sqlstorage.cc index e207cccb6a..560bdaf6b4 100644 --- a/src/libaktualizr/storage/sqlstorage.cc +++ b/src/libaktualizr/storage/sqlstorage.cc @@ -975,7 +975,7 @@ void SQLStorage::storeCachedEcuManifest(const Uptane::EcuSerial& ecu_serial, con auto statement = db.prepareStatement( "UPDATE secondary_ecus SET manifest = ? WHERE (serial = ?);", manifest, ecu_serial.ToString()); if (statement.step() != SQLITE_DONE || sqlite3_changes(db.get()) != 1) { - LOG_ERROR << "Can't save Secondary manifest " << db.errmsg(); + LOG_ERROR << "Can't save Secondary manifest: " << db.errmsg(); return; } } @@ -1006,30 +1006,14 @@ bool SQLStorage::loadCachedEcuManifest(const Uptane::EcuSerial& ecu_serial, std: return !empty; } -void SQLStorage::storeMisconfiguredEcus(const std::vector& ecus) { - if (ecus.size() >= 1) { - SQLite3Guard db = dbConnection(); - - db.beginTransaction(); - - if (db.exec("DELETE FROM misconfigured_ecus;", nullptr, nullptr) != SQLITE_OK) { - LOG_ERROR << "Can't clear misconfigured_ecus: " << db.errmsg(); - return; - } - - std::vector::const_iterator it; - for (it = ecus.begin(); it != ecus.end(); it++) { - auto statement = db.prepareStatement( - "INSERT INTO misconfigured_ecus VALUES (?,?,?);", it->serial.ToString(), it->hardware_id.ToString(), - static_cast(it->state)); - - if (statement.step() != SQLITE_DONE) { - LOG_ERROR << "Can't set misconfigured_ecus: " << db.errmsg(); - return; - } - } +void SQLStorage::saveMisconfiguredEcu(const MisconfiguredEcu& ecu) { + SQLite3Guard db = dbConnection(); - db.commitTransaction(); + auto statement = db.prepareStatement( + "INSERT OR REPLACE INTO misconfigured_ecus VALUES (?,?,?);", ecu.serial.ToString(), ecu.hardware_id.ToString(), + static_cast(ecu.state)); + if (statement.step() != SQLITE_DONE) { + throw std::runtime_error(db.errmsg().insert(0, "Can't save misconfigured_ecus: ")); } } diff --git a/src/libaktualizr/storage/sqlstorage.h b/src/libaktualizr/storage/sqlstorage.h index e649cd39e3..820f9ebcff 100644 --- a/src/libaktualizr/storage/sqlstorage.h +++ b/src/libaktualizr/storage/sqlstorage.h @@ -63,7 +63,7 @@ class SQLStorage : public SQLStorageBase, public INvStorage { void clearEcuSerials() override; void storeCachedEcuManifest(const Uptane::EcuSerial& ecu_serial, const std::string& manifest) override; bool loadCachedEcuManifest(const Uptane::EcuSerial& ecu_serial, std::string* manifest) const override; - void storeMisconfiguredEcus(const std::vector& ecus) override; + void saveMisconfiguredEcu(const MisconfiguredEcu& ecu) override; bool loadMisconfiguredEcus(std::vector* ecus) const override; void clearMisconfiguredEcus() override; void storeEcuRegistered() override; diff --git a/src/libaktualizr/storage/storage_common_test.cc b/src/libaktualizr/storage/storage_common_test.cc index 33dcf9d3e4..9e6566a989 100644 --- a/src/libaktualizr/storage/storage_common_test.cc +++ b/src/libaktualizr/storage/storage_common_test.cc @@ -253,20 +253,17 @@ TEST(storage, load_store_misconfigured_ecus) { TemporaryDirectory temp_dir; std::unique_ptr storage = Storage(temp_dir.Path()); - std::vector ecus; - ecus.push_back(MisconfiguredEcu(Uptane::EcuSerial("primary"), Uptane::HardwareIdentifier("primary_hw"), - EcuState::kNotRegistered)); - - storage->storeMisconfiguredEcus(ecus); + storage->saveMisconfiguredEcu( + {Uptane::EcuSerial("primary"), Uptane::HardwareIdentifier("primary_hw"), EcuState::kOld}); std::vector ecus_out; EXPECT_TRUE(storage->loadMisconfiguredEcus(&ecus_out)); - EXPECT_EQ(ecus_out.size(), ecus.size()); + EXPECT_EQ(ecus_out.size(), 1); EXPECT_EQ(ecus_out[0].serial, Uptane::EcuSerial("primary")); EXPECT_EQ(ecus_out[0].hardware_id, Uptane::HardwareIdentifier("primary_hw")); - EXPECT_EQ(ecus_out[0].state, EcuState::kNotRegistered); + EXPECT_EQ(ecus_out[0].state, EcuState::kOld); storage->clearMisconfiguredEcus(); ecus_out.clear(); diff --git a/src/libaktualizr/uptane/uptane_test.cc b/src/libaktualizr/uptane/uptane_test.cc index a7a58279a2..c858d68a44 100644 --- a/src/libaktualizr/uptane/uptane_test.cc +++ b/src/libaktualizr/uptane/uptane_test.cc @@ -740,13 +740,13 @@ TEST(Uptane, UptaneSecondaryMisconfigured) { storage->loadMisconfiguredEcus(&ecus); EXPECT_EQ(ecus.size(), 2); if (ecus[0].serial.ToString() == "new_secondary_ecu_serial") { - EXPECT_EQ(ecus[0].state, EcuState::kNotRegistered); + EXPECT_EQ(ecus[0].state, EcuState::kUnused); EXPECT_EQ(ecus[1].serial.ToString(), "secondary_ecu_serial"); EXPECT_EQ(ecus[1].state, EcuState::kOld); } else if (ecus[0].serial.ToString() == "secondary_ecu_serial") { EXPECT_EQ(ecus[0].state, EcuState::kOld); EXPECT_EQ(ecus[1].serial.ToString(), "new_secondary_ecu_serial"); - EXPECT_EQ(ecus[1].state, EcuState::kNotRegistered); + EXPECT_EQ(ecus[1].state, EcuState::kUnused); } else { FAIL() << "Unexpected Secondary serial in storage: " << ecus[0].serial.ToString(); } From 1ed4d38399c2a7b36b0aa9cad70f04f1ed9d6542 Mon Sep 17 00:00:00 2001 From: Patrick Vacek Date: Wed, 3 Jun 2020 16:35:43 +0200 Subject: [PATCH 02/10] Fix IP Secondary re-registration and some broken tests. Signed-off-by: Patrick Vacek --- src/aktualizr_primary/CMakeLists.txt | 4 +- src/aktualizr_primary/main.cc | 2 +- .../primary_secondary_registration_test.cc | 53 +------ src/aktualizr_primary/secondary.cc | 134 +++++++++--------- src/libaktualizr-posix/ipuptanesecondary.cc | 34 ++--- src/libaktualizr/primary/aktualizr.cc | 2 - src/libaktualizr/primary/aktualizr.h | 5 - src/libaktualizr/primary/aktualizr_test.cc | 10 +- src/libaktualizr/primary/sotauptaneclient.cc | 6 +- 9 files changed, 97 insertions(+), 153 deletions(-) diff --git a/src/aktualizr_primary/CMakeLists.txt b/src/aktualizr_primary/CMakeLists.txt index ec72a9aa12..c83f085738 100644 --- a/src/aktualizr_primary/CMakeLists.txt +++ b/src/aktualizr_primary/CMakeLists.txt @@ -7,7 +7,9 @@ target_include_directories(aktualizr PUBLIC ${PROJECT_SOURCE_DIR}/third_party) install(TARGETS aktualizr RUNTIME DESTINATION bin COMPONENT aktualizr) -add_aktualizr_test(NAME primary_secondary_registration SOURCES primary_secondary_registration_test.cc secondary.cc secondary_config.cc PROJECT_WORKING_DIRECTORY ARGS ${PROJECT_BINARY_DIR}/uptane_repos LIBRARIES PUBLIC aktualizr-posix virtual_secondary uptane_generator_lib) +add_aktualizr_test(NAME primary_secondary_registration + SOURCES primary_secondary_registration_test.cc secondary.cc secondary_config.cc + PROJECT_WORKING_DIRECTORY LIBRARIES PUBLIC aktualizr-posix virtual_secondary uptane_generator_lib) target_include_directories(t_primary_secondary_registration PUBLIC ${PROJECT_SOURCE_DIR}/src/libaktualizr-posix) aktualizr_source_file_checks(${SOURCES} ${HEADERS} ${TEST_SOURCES}) diff --git a/src/aktualizr_primary/main.cc b/src/aktualizr_primary/main.cc index bdd781c7e0..51775f9a22 100644 --- a/src/aktualizr_primary/main.cc +++ b/src/aktualizr_primary/main.cc @@ -131,7 +131,7 @@ int main(int argc, char *argv[]) { try { Primary::initSecondaries(aktualizr, config.uptane.secondary_config_file); } catch (const std::exception &e) { - LOG_ERROR << "Failed to initialize Secondaries :" << e.what(); + LOG_ERROR << "Failed to initialize Secondaries: " << e.what(); LOG_ERROR << "Exiting..."; return EXIT_FAILURE; } diff --git a/src/aktualizr_primary/primary_secondary_registration_test.cc b/src/aktualizr_primary/primary_secondary_registration_test.cc index f30c3268ea..ceb013aec1 100644 --- a/src/aktualizr_primary/primary_secondary_registration_test.cc +++ b/src/aktualizr_primary/primary_secondary_registration_test.cc @@ -6,11 +6,11 @@ #include "uptane_test_common.h" #include "utilities/utils.h" -boost::filesystem::path uptane_repos_dir; boost::filesystem::path fake_meta_dir; /* This tests that a device that had an IP Secondary will still find it after - * recent changes, even if it does not connect when the device starts. */ + * recent changes, even if it does not connect when the device starts. Note that + * this is only supported for a single IP Secondary. */ TEST(PrimarySecondaryReg, SecondariesMigration) { const Uptane::EcuSerial primary_serial{"p_serial"}; const Uptane::EcuSerial secondary_serial{"s_serial"}; @@ -38,7 +38,7 @@ TEST(PrimarySecondaryReg, SecondariesMigration) { Json::Value sec_conf; { - // prepare storage the "old" way: + // Prepare storage the "old" way (without the secondary_ecus table): storage->storeDeviceId("device"); storage->storeEcuSerials({{primary_serial, primary_hwid}, {secondary_serial, secondary_hwid}}); storage->storeEcuRegistered(); @@ -51,9 +51,8 @@ TEST(PrimarySecondaryReg, SecondariesMigration) { } { - // verifies that aktualizr can still start if it can't connect to its - // secondary - + // Verify that aktualizr can still start if it can't connect to its + // Secondary: UptaneTestCommon::TestAktualizr aktualizr(conf, storage, http); Primary::initSecondaries(aktualizr, sec_conf_path); aktualizr.Initialize(); @@ -65,53 +64,11 @@ TEST(PrimarySecondaryReg, SecondariesMigration) { EXPECT_EQ(secs_info[0].type, "IP"); EXPECT_EQ(secs_info[0].extra, R"({"ip":"127.0.0.1","port":9061})"); } - - const Uptane::EcuSerial secondary_serial2{"s_serial2"}; - const Uptane::HardwareIdentifier secondary_hwid2{"s_hwid2"}; - { - // prepare the storage the "old" way with two secondaries - storage->clearEcuSerials(); - storage->storeEcuSerials({ - {primary_serial, primary_hwid}, - {secondary_serial, secondary_hwid}, - {secondary_serial2, secondary_hwid2}, - }); - - sec_conf["IP"]["secondaries"][1]["addr"] = "127.0.0.1:9062"; - Utils::writeFile(sec_conf_path, sec_conf); - } - - { - UptaneTestCommon::TestAktualizr aktualizr(conf, storage, http); - - // this will fail to actually register the secondaries as there is no way to - // tell them apart (since they haven't connected) - // however, we still allow aktualizr to go through in case we would like to - // update the primary, to maybe fix this situation - Primary::initSecondaries(aktualizr, sec_conf_path); - aktualizr.Initialize(); - aktualizr.CheckUpdates().get(); - - // there was no way to correlate info here - std::vector secs_info; - storage->loadSecondariesInfo(&secs_info); - EXPECT_EQ(secs_info[0].serial.ToString(), secondary_serial.ToString()); - EXPECT_EQ(secs_info[0].type, ""); - EXPECT_EQ(secs_info[0].extra, ""); - EXPECT_EQ(secs_info[1].serial.ToString(), secondary_serial2.ToString()); - EXPECT_EQ(secs_info[1].type, ""); - EXPECT_EQ(secs_info[1].extra, ""); - } } #ifndef __NO_MAIN__ int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv); - if (argc != 2) { - std::cerr << "Error: " << argv[0] << " requires the path to the base directory of Uptane repos.\n"; - return EXIT_FAILURE; - } - uptane_repos_dir = argv[1]; logger_init(); logger_set_threshold(boost::log::trivial::trace); diff --git a/src/aktualizr_primary/secondary.cc b/src/aktualizr_primary/secondary.cc index 60f3378ed1..b68fc1d713 100644 --- a/src/aktualizr_primary/secondary.cc +++ b/src/aktualizr_primary/secondary.cc @@ -49,7 +49,7 @@ void initSecondaries(Aktualizr& aktualizr, const boost::filesystem::path& config for (auto& config : secondary_configs) { try { - LOG_INFO << "Registering " << config->type() << " Secondaries..."; + LOG_INFO << "Initializing " << config->type() << " Secondaries..."; Secondaries secondaries = createSecondaries(*config, aktualizr); for (const auto& secondary : secondaries) { @@ -66,8 +66,9 @@ void initSecondaries(Aktualizr& aktualizr, const boost::filesystem::path& config class SecondaryWaiter { public: - SecondaryWaiter(uint16_t wait_port, int timeout_s, Secondaries& secondaries) - : endpoint_{boost::asio::ip::tcp::v4(), wait_port}, + SecondaryWaiter(Aktualizr& aktualizr, uint16_t wait_port, int timeout_s, Secondaries& secondaries) + : aktualizr_(aktualizr), + endpoint_{boost::asio::ip::tcp::v4(), wait_port}, timeout_{static_cast(timeout_s)}, timer_{io_context_}, connected_secondaries_(secondaries) {} @@ -83,10 +84,10 @@ class SecondaryWaiter { timer_.async_wait([&](const boost::system::error_code& error_code) { if (!!error_code) { LOG_ERROR << "Wait for Secondaries has failed: " << error_code; - throw std::runtime_error("Error while waiting for Secondaries"); + throw std::runtime_error("Error while waiting for IP Secondaries"); } else { LOG_ERROR << "Timeout while waiting for Secondaries: " << error_code; - throw std::runtime_error("Timeout while waiting for Secondaries"); + throw std::runtime_error("Timeout while waiting for IP Secondaries"); } io_context_.stop(); }); @@ -111,6 +112,11 @@ class SecondaryWaiter { auto secondary = Uptane::IpUptaneSecondary::create(sec_ip, sec_port, con_socket_.native_handle()); if (secondary) { connected_secondaries_.push_back(secondary); + // set ip/port in the db so that we can match everything later + Json::Value d; + d["ip"] = sec_ip; + d["port"] = sec_port; + aktualizr_.SetSecondaryData(secondary->getSerial(), Utils::jsonToCanonicalStr(d)); } } catch (const std::exception& exc) { LOG_ERROR << "Failed to initialize a Secondary: " << exc.what(); @@ -132,6 +138,8 @@ class SecondaryWaiter { static std::string key(const std::string& ip, uint16_t port) { return (ip + std::to_string(port)); } private: + Aktualizr& aktualizr_; + boost::asio::io_service io_context_; boost::asio::ip::tcp::endpoint endpoint_; boost::asio::ip::tcp::acceptor acceptor_{io_context_, endpoint_}; @@ -143,86 +151,74 @@ class SecondaryWaiter { std::unordered_set secondaries_to_wait_for_; }; +// Four options for each Secondary: +// 1. Secondary is configured and stored: nothing to do. +// 2. Secondary is configured but not stored: it must be new. Try to connect to get information and store it. This will +// cause re-registration. +// 3. Same as 2 but cannot connect: abort. +// 4. Secondary is stored but not configured: it must have been removed. Skip it. This will cause re-registration. static Secondaries createIPSecondaries(const IPSecondariesConfig& config, Aktualizr& aktualizr) { Secondaries result; - const bool provision = !aktualizr.IsRegistered(); - - if (provision) { - SecondaryWaiter sec_waiter{config.secondaries_wait_port, config.secondaries_timeout_s, result}; - - for (const auto& ip_sec_cfg : config.secondaries_cfg) { - auto secondary = Uptane::IpUptaneSecondary::connectAndCreate(ip_sec_cfg.ip, ip_sec_cfg.port); - if (secondary) { - result.push_back(secondary); - } else { - sec_waiter.addSecondary(ip_sec_cfg.ip, ip_sec_cfg.port); - } - } - - sec_waiter.wait(); + Secondaries new_secondaries; + SecondaryWaiter sec_waiter{aktualizr, config.secondaries_wait_port, config.secondaries_timeout_s, result}; + auto secondaries_info = aktualizr.GetSecondaries(); + + for (const auto& cfg : config.secondaries_cfg) { + Uptane::SecondaryInterface::Ptr secondary; + const SecondaryInfo* info = nullptr; + + // Try to match the configured Secondaries to stored Secondaries. + auto f = std::find_if(secondaries_info.cbegin(), secondaries_info.cend(), [&cfg](const SecondaryInfo& i) { + Json::Value d = Utils::parseJSON(i.extra); + return d["ip"] == cfg.ip && d["port"] == cfg.port; + }); - // set ip/port in the db so that we can match everything later - for (size_t k = 0; k < config.secondaries_cfg.size(); k++) { - const auto cfg = config.secondaries_cfg[k]; - const auto sec = result[k]; + if (f == secondaries_info.cend() && config.secondaries_cfg.size() == 1 && secondaries_info.size() == 1 && + secondaries_info[0].extra.empty()) { + // /!\ backward compatibility: if we have just one Secondary in the old + // storage format (before we had the secondary_ecus table) and the + // configuration, migrate it to the new format. + info = &secondaries_info[0]; Json::Value d; d["ip"] = cfg.ip; d["port"] = cfg.port; - aktualizr.SetSecondaryData(sec->getSerial(), Utils::jsonToCanonicalStr(d)); - } - } else { - auto secondaries_info = aktualizr.GetSecondaries(); - - for (const auto& cfg : config.secondaries_cfg) { - Uptane::SecondaryInterface::Ptr secondary; - const SecondaryInfo* info = nullptr; - - auto f = std::find_if(secondaries_info.cbegin(), secondaries_info.cend(), [&cfg](const SecondaryInfo& i) { - Json::Value d = Utils::parseJSON(i.extra); - return d["ip"] == cfg.ip && d["port"] == cfg.port; - }); - - if (f == secondaries_info.cend() && config.secondaries_cfg.size() == 1 && secondaries_info.size() == 1) { - // /!\ backward compatibility: handle the case with one secondary, but - // store the info for later anyway - info = &secondaries_info[0]; + aktualizr.SetSecondaryData(info->serial, Utils::jsonToCanonicalStr(d)); + LOG_INFO << "Migrated a single IP Secondary to new storage format."; + } else if (f == secondaries_info.cend()) { + // Secondary was not found in storage; it must be new. + secondary = Uptane::IpUptaneSecondary::connectAndCreate(cfg.ip, cfg.port); + if (secondary == nullptr) { + LOG_DEBUG << "Could not connect to IP Secondary at " << cfg.ip << ":" << cfg.port + << "; now trying to wait for it."; + sec_waiter.addSecondary(cfg.ip, cfg.port); + } else { + result.push_back(secondary); + // set ip/port in the db so that we can match everything later Json::Value d; d["ip"] = cfg.ip; d["port"] = cfg.port; - aktualizr.SetSecondaryData(info->serial, Utils::jsonToCanonicalStr(d)); - LOG_INFO << "Migrated single IP Secondary to new storage format"; - } else if (f == secondaries_info.cend()) { - // Match the other way if we can - secondary = Uptane::IpUptaneSecondary::connectAndCreate(cfg.ip, cfg.port); - if (secondary == nullptr) { - LOG_ERROR << "Could not instantiate Secondary " << cfg.ip << ":" << cfg.port; - continue; - } - auto f_serial = - std::find_if(secondaries_info.cbegin(), secondaries_info.cend(), - [&secondary](const SecondaryInfo& i) { return i.serial == secondary->getSerial(); }); - if (f_serial == secondaries_info.cend()) { - LOG_ERROR << "Could not instantiate Secondary " << cfg.ip << ":" << cfg.port; - continue; - } - info = &(*f_serial); - } else { - info = &(*f); + aktualizr.SetSecondaryData(secondary->getSerial(), Utils::jsonToCanonicalStr(d)); } + continue; + } else { + // The configured Secondary was found in storage. + info = &(*f); + } + if (secondary == nullptr) { + secondary = + Uptane::IpUptaneSecondary::connectAndCheck(cfg.ip, cfg.port, info->serial, info->hw_id, info->pub_key); if (secondary == nullptr) { - secondary = - Uptane::IpUptaneSecondary::connectAndCheck(cfg.ip, cfg.port, info->serial, info->hw_id, info->pub_key); - } - - if (secondary != nullptr) { - result.push_back(secondary); - } else { - LOG_ERROR << "Could not instantiate Secondary " << info->serial; + throw std::runtime_error("Unable to connect to or verify IP Secondary at " + cfg.ip + ":" + + std::to_string(cfg.port)); } } + + result.push_back(secondary); } + sec_waiter.wait(); + return result; } diff --git a/src/libaktualizr-posix/ipuptanesecondary.cc b/src/libaktualizr-posix/ipuptanesecondary.cc index d5a45d7f23..72e275e8c1 100644 --- a/src/libaktualizr-posix/ipuptanesecondary.cc +++ b/src/libaktualizr-posix/ipuptanesecondary.cc @@ -31,12 +31,12 @@ Uptane::SecondaryInterface::Ptr IpUptaneSecondary::create(const std::string& add req->present(AKIpUptaneMes_PR_getInfoReq); auto m = req->getInfoReq(); - auto resp = Asn1Rpc(req, con_fd); if (resp->present() != AKIpUptaneMes_PR_getInfoResp) { - LOG_ERROR << "Failed to get info response message from Secondary"; - throw std::runtime_error("Failed to obtain information about a Secondary: " + address + std::to_string(port)); + LOG_ERROR << "IP Secondary failed to respond to information request at " << address << ":" << port; + return std::make_shared(address, port, EcuSerial::Unknown(), HardwareIdentifier::Unknown(), + PublicKey("", KeyType::kUnknown)); } auto r = resp->getInfoResp(); @@ -46,7 +46,7 @@ Uptane::SecondaryInterface::Ptr IpUptaneSecondary::create(const std::string& add auto type = static_cast(r->keyType); PublicKey pub_key = PublicKey(key, type); - LOG_INFO << "Got info from IP Secondary: " + LOG_INFO << "Got ECU information from IP Secondary: " << "hardware ID: " << hw_id << " serial: " << serial; return std::make_shared(address, port, serial, hw_id, pub_key); @@ -62,28 +62,28 @@ SecondaryInterface::Ptr IpUptaneSecondary::connectAndCheck(const std::string& ad auto sec = IpUptaneSecondary::connectAndCreate(address, port); if (sec != nullptr) { auto s = sec->getSerial(); - if (s != serial) { - LOG_ERROR << "Mismatch between Secondary serials " << s << " and " << serial; - return nullptr; + if (s != serial && serial != EcuSerial::Unknown()) { + LOG_WARNING << "Expected IP Secondary at " << address << ":" << port << " with serial " << serial + << " but found " << s; } auto h = sec->getHwId(); - if (h != hw_id) { - LOG_ERROR << "Mismatch between hardware IDs " << h << " and " << hw_id; - return nullptr; + if (h != hw_id && hw_id != HardwareIdentifier::Unknown()) { + LOG_WARNING << "Expected IP Secondary at " << address << ":" << port << " with hardware ID " << hw_id + << " but found " << h; } auto p = sec->getPublicKey(); - if (pub_key.Type() == KeyType::kUnknown) { - LOG_INFO << "Secondary " << s << " do not have a known public key"; - } else if (p != pub_key) { - LOG_ERROR << "Mismatch between public keys " << p.Value() << " and " << pub_key.Value() << " for Secondary " - << serial; + if (p.Type() == KeyType::kUnknown) { + LOG_ERROR << "IP Secondary at " << address << ":" << port << " has an unknown key type!"; return nullptr; + } else if (p != pub_key && pub_key.Type() != KeyType::kUnknown) { + LOG_WARNING << "Expected IP Secondary at " << address << ":" << port << " with public key:\n" + << pub_key.Value() << "... but found:\n" + << p.Value(); } return sec; } } catch (std::exception& e) { - LOG_WARNING << "Could not connect to Secondary " << serial << " at " << address << ":" << port - << " using previously known registration data"; + LOG_WARNING << "Could not connect to IP Secondary at " << address << ":" << port << " with serial " << serial; } return std::make_shared(address, port, std::move(serial), std::move(hw_id), std::move(pub_key)); diff --git a/src/libaktualizr/primary/aktualizr.cc b/src/libaktualizr/primary/aktualizr.cc index c445c40b9d..d03e91aeab 100644 --- a/src/libaktualizr/primary/aktualizr.cc +++ b/src/libaktualizr/primary/aktualizr.cc @@ -31,8 +31,6 @@ void Aktualizr::Initialize() { api_queue_.run(); } -bool Aktualizr::IsRegistered() const { return storage_->loadEcuRegistered(); } - bool Aktualizr::UptaneCycle() { result::UpdateCheck update_result = CheckUpdates().get(); if (update_result.updates.empty()) { diff --git a/src/libaktualizr/primary/aktualizr.h b/src/libaktualizr/primary/aktualizr.h index d17b50e529..d4e18ea98b 100644 --- a/src/libaktualizr/primary/aktualizr.h +++ b/src/libaktualizr/primary/aktualizr.h @@ -32,11 +32,6 @@ class Aktualizr { */ void Initialize(); - /** - * Returns true if the device has been registered to the backend succesffully. - */ - bool IsRegistered() const; - /** * Asynchronously run aktualizr indefinitely until Shutdown is called. * @param custom_hwinfo if not empty will be sent to the backend instead of `lshw` output diff --git a/src/libaktualizr/primary/aktualizr_test.cc b/src/libaktualizr/primary/aktualizr_test.cc index db86688a3c..392fd4d7ca 100644 --- a/src/libaktualizr/primary/aktualizr_test.cc +++ b/src/libaktualizr/primary/aktualizr_test.cc @@ -174,25 +174,21 @@ TEST(Aktualizr, DeviceInstallationResult) { Config conf = UptaneTestCommon::makeTestConfig(temp_dir, http->tls_server); auto storage = INvStorage::newStorage(conf.storage); - EcuSerials serials{ - {Uptane::EcuSerial("primary"), Uptane::HardwareIdentifier("primary_hw")}, + {Uptane::EcuSerial("CA:FE:A6:D2:84:9D"), Uptane::HardwareIdentifier("primary_hw")}, {Uptane::EcuSerial("secondary_ecu_serial"), Uptane::HardwareIdentifier("secondary_hw")}, {Uptane::EcuSerial("ecuserial3"), Uptane::HardwareIdentifier("hw_id3")}, }; storage->storeEcuSerials(serials); UptaneTestCommon::TestAktualizr aktualizr(conf, storage, http); - Primary::VirtualSecondaryConfig ecu_config = virtual_configuration(temp_dir.Path()); - aktualizr.AddSecondary(std::make_shared(ecu_config)); - aktualizr.Initialize(); storage->saveEcuInstallationResult(Uptane::EcuSerial("ecuserial3"), data::InstallationResult()); - storage->saveEcuInstallationResult(Uptane::EcuSerial("primary"), data::InstallationResult()); - storage->saveEcuInstallationResult(Uptane::EcuSerial("primary"), + storage->saveEcuInstallationResult(Uptane::EcuSerial("CA:FE:A6:D2:84:9D"), data::InstallationResult()); + storage->saveEcuInstallationResult(Uptane::EcuSerial("CA:FE:A6:D2:84:9D"), data::InstallationResult(data::ResultCode::Numeric::kInstallFailed, "")); data::InstallationResult result; diff --git a/src/libaktualizr/primary/sotauptaneclient.cc b/src/libaktualizr/primary/sotauptaneclient.cc index 5e883f0c63..2b5dd61a49 100644 --- a/src/libaktualizr/primary/sotauptaneclient.cc +++ b/src/libaktualizr/primary/sotauptaneclient.cc @@ -406,8 +406,8 @@ void SotaUptaneClient::computeDeviceInstallationResult(data::InstallationResult if (!storage->loadEcuInstallationResults(&ecu_results)) { // failed to load ECUs' installation result device_installation_result = data::InstallationResult(data::ResultCode::Numeric::kInternalError, - "Unable to get installation results from ecus"); - raw_ir = "Failed to load ECUs' installation result"; + "Unable to get installation results from ECUs"); + raw_ir = "Failed to load ECU installation results"; break; } @@ -425,7 +425,7 @@ void SotaUptaneClient::computeDeviceInstallationResult(data::InstallationResult device_installation_result = data::InstallationResult(data::ResultCode::Numeric::kInternalError, "Unable to get installation results from ECUs"); - raw_ir = "Couldn't find any ECU with the given serial: " + ecu_serial.ToString(); + raw_ir = "Failed to find an ECU with the given serial: " + ecu_serial.ToString(); break; } From 57673c873df333e7b7185c72c6529aaa88f7c7bf Mon Sep 17 00:00:00 2001 From: Patrick Vacek Date: Thu, 4 Jun 2020 15:42:21 +0200 Subject: [PATCH 03/10] Expand Secondary registration tests. Now they cover adding, remove, and replacing Secondary ECUs. Signed-off-by: Patrick Vacek --- src/libaktualizr/primary/CMakeLists.txt | 3 + src/libaktualizr/primary/aktualizr_test.cc | 57 +------ .../primary/reregistration_test.cc | 151 ++++++++++++++++++ tests/uptane_test_common.h | 16 ++ 4 files changed, 171 insertions(+), 56 deletions(-) create mode 100644 src/libaktualizr/primary/reregistration_test.cc diff --git a/src/libaktualizr/primary/CMakeLists.txt b/src/libaktualizr/primary/CMakeLists.txt index a59540155a..be56e06602 100644 --- a/src/libaktualizr/primary/CMakeLists.txt +++ b/src/libaktualizr/primary/CMakeLists.txt @@ -20,6 +20,9 @@ add_aktualizr_test(NAME aktualizr SOURCES aktualizr_test.cc PROJECT_WORKING_DIRE add_dependencies(t_aktualizr uptane_repo_full_no_correlation_id) target_link_libraries(t_aktualizr virtual_secondary) +add_aktualizr_test(NAME reregistration SOURCES reregistration_test.cc PROJECT_WORKING_DIRECTORY LIBRARIES uptane_generator_lib) +target_link_libraries(t_reregistration virtual_secondary) + if (BUILD_OSTREE) add_aktualizr_test(NAME aktualizr_fullostree SOURCES aktualizr_fullostree_test.cc PROJECT_WORKING_DIRECTORY ARGS $ ${PROJECT_BINARY_DIR}/ostree_repo) set_target_properties(t_aktualizr_fullostree PROPERTIES LINK_FLAGS -Wl,--export-dynamic) diff --git a/src/libaktualizr/primary/aktualizr_test.cc b/src/libaktualizr/primary/aktualizr_test.cc index 392fd4d7ca..0f63214b50 100644 --- a/src/libaktualizr/primary/aktualizr_test.cc +++ b/src/libaktualizr/primary/aktualizr_test.cc @@ -45,22 +45,6 @@ void verifyNothingInstalled(const Json::Value& manifest) { "noimage"); } -static Primary::VirtualSecondaryConfig virtual_configuration(const boost::filesystem::path& client_dir) { - Primary::VirtualSecondaryConfig ecu_config; - - ecu_config.partial_verifying = false; - ecu_config.full_client_dir = client_dir; - ecu_config.ecu_serial = "ecuserial3"; - ecu_config.ecu_hardware_id = "hw_id3"; - ecu_config.ecu_private_key = "sec.priv"; - ecu_config.ecu_public_key = "sec.pub"; - ecu_config.firmware_path = client_dir / "firmware.txt"; - ecu_config.target_name_path = client_dir / "firmware_name.txt"; - ecu_config.metadata_path = client_dir / "secondary_metadata"; - - return ecu_config; -} - /* * Initialize -> UptaneCycle -> no updates -> no further action or events. */ @@ -126,45 +110,6 @@ TEST(Aktualizr, FullNoUpdates) { verifyNothingInstalled(aktualizr.uptane_client()->AssembleManifest()); } -/* - * Add Secondaries via API - */ -TEST(Aktualizr, AddSecondary) { - TemporaryDirectory temp_dir; - auto http = std::make_shared(temp_dir.Path(), "noupdates", fake_meta_dir); - Config conf = UptaneTestCommon::makeTestConfig(temp_dir, http->tls_server); - - auto storage = INvStorage::newStorage(conf.storage); - UptaneTestCommon::TestAktualizr aktualizr(conf, storage, http); - - Primary::VirtualSecondaryConfig ecu_config = virtual_configuration(temp_dir.Path()); - - aktualizr.AddSecondary(std::make_shared(ecu_config)); - - aktualizr.Initialize(); - - EcuSerials serials; - storage->loadEcuSerials(&serials); - - std::vector expected_ecus = {"CA:FE:A6:D2:84:9D", "ecuserial3", "secondary_ecu_serial"}; - EXPECT_EQ(serials.size(), 3); - for (const auto& ecu : serials) { - auto found = std::find(expected_ecus.begin(), expected_ecus.end(), ecu.first.ToString()); - if (found != expected_ecus.end()) { - expected_ecus.erase(found); - } else { - FAIL() << "Unknown ecu: " << ecu.first.ToString(); - } - } - EXPECT_EQ(expected_ecus.size(), 0); - - // Historically, adding Secondaries after provisioning was not supported. Now - // it is. - ecu_config.ecu_serial = "ecuserial4"; - auto sec4 = std::make_shared(ecu_config); - EXPECT_NO_THROW(aktualizr.AddSecondary(sec4)); -} - /* * Compute device installation failure code as concatenation of ECU failure codes. */ @@ -182,7 +127,7 @@ TEST(Aktualizr, DeviceInstallationResult) { storage->storeEcuSerials(serials); UptaneTestCommon::TestAktualizr aktualizr(conf, storage, http); - Primary::VirtualSecondaryConfig ecu_config = virtual_configuration(temp_dir.Path()); + Primary::VirtualSecondaryConfig ecu_config = UptaneTestCommon::altVirtualConfiguration(temp_dir.Path()); aktualizr.AddSecondary(std::make_shared(ecu_config)); aktualizr.Initialize(); diff --git a/src/libaktualizr/primary/reregistration_test.cc b/src/libaktualizr/primary/reregistration_test.cc new file mode 100644 index 0000000000..220c052e33 --- /dev/null +++ b/src/libaktualizr/primary/reregistration_test.cc @@ -0,0 +1,151 @@ +#include + +#include +#include + +#include "config/config.h" +#include "httpfake.h" +#include "primary/aktualizr.h" +#include "uptane_test_common.h" +#include "virtualsecondary.h" + +boost::filesystem::path fake_meta_dir; + +void verifyEcus(TemporaryDirectory& temp_dir, std::vector expected_ecus) { + const Json::Value ecu_data = Utils::parseJSONFile(temp_dir / "post.json"); + EXPECT_EQ(ecu_data["ecus"].size(), expected_ecus.size()); + for (const Json::Value& ecu : ecu_data["ecus"]) { + auto found = std::find(expected_ecus.begin(), expected_ecus.end(), ecu["ecu_serial"].asString()); + if (found != expected_ecus.end()) { + expected_ecus.erase(found); + } else { + FAIL() << "Unknown ECU in registration data: " << ecu["ecu_serial"].asString(); + } + } + EXPECT_EQ(expected_ecus.size(), 0); +} + +class HttpFakeRegistration : public HttpFake { + public: + HttpFakeRegistration(const boost::filesystem::path& test_dir_in, const boost::filesystem::path& meta_dir_in) + : HttpFake(test_dir_in, "noupdates", meta_dir_in) {} + + HttpResponse post(const std::string& url, const Json::Value& data) override { + if (url.find("/director/ecus") != std::string::npos) { + registration_count += 1; + EXPECT_EQ(data["primary_ecu_serial"].asString(), "CA:FE:A6:D2:84:9D"); + EXPECT_EQ(data["ecus"][0]["ecu_serial"].asString(), "CA:FE:A6:D2:84:9D"); + EXPECT_EQ(data["ecus"][0]["hardware_identifier"].asString(), "primary_hw"); + } + return HttpFake::post(url, data); + } + + unsigned int registration_count{0}; +}; + +/* + * Add a Secondary via API, register the ECUs, then add another, and re-register. + */ +TEST(Aktualizr, AddSecondary) { + TemporaryDirectory temp_dir; + auto http = std::make_shared(temp_dir.Path(), fake_meta_dir); + Config conf = UptaneTestCommon::makeTestConfig(temp_dir, http->tls_server); + auto storage = INvStorage::newStorage(conf.storage); + + UptaneTestCommon::TestAktualizr aktualizr(conf, storage, http); + Primary::VirtualSecondaryConfig ecu_config = UptaneTestCommon::altVirtualConfiguration(temp_dir.Path()); + aktualizr.AddSecondary(std::make_shared(ecu_config)); + aktualizr.Initialize(); + + std::vector expected_ecus = {"CA:FE:A6:D2:84:9D", "ecuserial3", "secondary_ecu_serial"}; + verifyEcus(temp_dir, expected_ecus); + EXPECT_EQ(http->registration_count, 1); + + ecu_config.ecu_serial = "ecuserial4"; + auto sec4 = std::make_shared(ecu_config); + aktualizr.AddSecondary(sec4); + aktualizr.Initialize(); + expected_ecus.push_back(ecu_config.ecu_serial); + verifyEcus(temp_dir, expected_ecus); + EXPECT_EQ(http->registration_count, 2); +} + +/* + * Add a Secondary via API, register the ECUs, remove one, and re-register. + */ +TEST(Aktualizr, RemoveSecondary) { + TemporaryDirectory temp_dir; + auto http = std::make_shared(temp_dir.Path(), fake_meta_dir); + Config conf = UptaneTestCommon::makeTestConfig(temp_dir, http->tls_server); + auto storage = INvStorage::newStorage(conf.storage); + + { + UptaneTestCommon::TestAktualizr aktualizr(conf, storage, http); + Primary::VirtualSecondaryConfig ecu_config = UptaneTestCommon::altVirtualConfiguration(temp_dir.Path()); + aktualizr.AddSecondary(std::make_shared(ecu_config)); + aktualizr.Initialize(); + + std::vector expected_ecus = {"CA:FE:A6:D2:84:9D", "ecuserial3", "secondary_ecu_serial"}; + verifyEcus(temp_dir, expected_ecus); + EXPECT_EQ(http->registration_count, 1); + } + + { + UptaneTestCommon::TestAktualizr aktualizr(conf, storage, http); + aktualizr.Initialize(); + + std::vector expected_ecus = {"CA:FE:A6:D2:84:9D", "secondary_ecu_serial"}; + verifyEcus(temp_dir, expected_ecus); + EXPECT_EQ(http->registration_count, 2); + } +} + +/* + * Add a Secondary via API, register the ECUs, replace one, and re-register. + */ +TEST(Aktualizr, ReplaceSecondary) { + TemporaryDirectory temp_dir; + auto http = std::make_shared(temp_dir.Path(), fake_meta_dir); + Config conf = UptaneTestCommon::makeTestConfig(temp_dir, http->tls_server); + auto storage = INvStorage::newStorage(conf.storage); + + { + UptaneTestCommon::TestAktualizr aktualizr(conf, storage, http); + Primary::VirtualSecondaryConfig ecu_config = UptaneTestCommon::altVirtualConfiguration(temp_dir.Path()); + aktualizr.AddSecondary(std::make_shared(ecu_config)); + aktualizr.Initialize(); + + std::vector expected_ecus = {"CA:FE:A6:D2:84:9D", "ecuserial3", "secondary_ecu_serial"}; + verifyEcus(temp_dir, expected_ecus); + EXPECT_EQ(http->registration_count, 1); + } + + { + UptaneTestCommon::TestAktualizr aktualizr(conf, storage, http); + Primary::VirtualSecondaryConfig ecu_config = UptaneTestCommon::altVirtualConfiguration(temp_dir.Path()); + ecu_config.ecu_serial = "ecuserial4"; + aktualizr.AddSecondary(std::make_shared(ecu_config)); + aktualizr.Initialize(); + + std::vector expected_ecus = {"CA:FE:A6:D2:84:9D", "ecuserial4", "secondary_ecu_serial"}; + verifyEcus(temp_dir, expected_ecus); + EXPECT_EQ(http->registration_count, 2); + } +} + +#ifndef __NO_MAIN__ +int main(int argc, char** argv) { + ::testing::InitGoogleTest(&argc, argv); + + logger_init(); + logger_set_threshold(boost::log::trivial::trace); + + TemporaryDirectory tmp_dir; + fake_meta_dir = tmp_dir.Path(); + MetaFake meta_fake(fake_meta_dir); + + return RUN_ALL_TESTS(); +} +#endif + +// vim: set tabstop=2 shiftwidth=2 expandtab: diff --git a/tests/uptane_test_common.h b/tests/uptane_test_common.h index fcc217c7c2..fb28abdb44 100644 --- a/tests/uptane_test_common.h +++ b/tests/uptane_test_common.h @@ -87,6 +87,22 @@ struct UptaneTestCommon { return ecu_config; } + static Primary::VirtualSecondaryConfig altVirtualConfiguration(const boost::filesystem::path& client_dir) { + Primary::VirtualSecondaryConfig ecu_config; + + ecu_config.partial_verifying = false; + ecu_config.full_client_dir = client_dir; + ecu_config.ecu_serial = "ecuserial3"; + ecu_config.ecu_hardware_id = "hw_id3"; + ecu_config.ecu_private_key = "sec.priv"; + ecu_config.ecu_public_key = "sec.pub"; + ecu_config.firmware_path = client_dir / "firmware.txt"; + ecu_config.target_name_path = client_dir / "firmware_name.txt"; + ecu_config.metadata_path = client_dir / "secondary_metadata"; + + return ecu_config; + } + static Config makeTestConfig(const TemporaryDirectory& temp_dir, const std::string& url) { Config conf("tests/config/basic.toml"); conf.uptane.director_server = url + "/director"; From a00ef7c5caa6344b7ff7535ea3a086d65f11c1a8 Mon Sep 17 00:00:00 2001 From: Patrick Vacek Date: Mon, 8 Jun 2020 14:18:04 +0200 Subject: [PATCH 04/10] Test adding, removing, and replacing IP Secondaries. The logic for matching addresses to ECUs is not trivial, so it's worth being extra careful. Also test replacing an ECU that reuses the same address and port and that we correctly track an ECU that changes its address or port. Also note some the timeouts (and some ordering issues) that were necessary to get some of these tests to pass. These are more reasons why a proper connection manager would be nice to have; things like this really shouldn't be necessary. Signed-off-by: Patrick Vacek --- tests/ipsecondary_test.py | 179 ++++++++++++++++++++++++++++++++++++-- tests/test_fixtures.py | 10 ++- 2 files changed, 182 insertions(+), 7 deletions(-) diff --git a/tests/ipsecondary_test.py b/tests/ipsecondary_test.py index a4a37152b8..a57f227f2d 100755 --- a/tests/ipsecondary_test.py +++ b/tests/ipsecondary_test.py @@ -7,7 +7,7 @@ from os import getcwd, chdir, path from test_fixtures import with_aktualizr, with_uptane_backend, KeyStore, with_secondary, with_treehub,\ - with_sysroot, with_director, TestRunner + with_sysroot, with_director, TestRunner, IPSecondary logger = logging.getLogger("IPSecondaryTest") @@ -63,7 +63,6 @@ def test_secondary_update_if_primary_starts_first(uptane_repo, secondary, aktual def test_secondary_update(uptane_repo, secondary, aktualizr, director, **kwargs): '''Test Secondary update if a boot order of Secondary and Primary is undefined''' - test_result = True # add a new image to the repo in order to update the Secondary with it secondary_image_filename = "secondary_image_filename.img" secondary_image_hash = uptane_repo.add_image(id=secondary.id, image_filename=secondary_image_filename) @@ -97,6 +96,168 @@ def test_secondary_update(uptane_repo, secondary, aktualizr, director, **kwargs) return True +@with_uptane_backend() +@with_secondary(start=True, id=('hwid1', 'serial1'), output_logs=False) +@with_aktualizr(start=False, output_logs=True) +def test_add_secondary(uptane_repo, secondary, aktualizr, **kwargs): + '''Test adding a Secondary after registration''' + + with aktualizr: + aktualizr.wait_for_completion() + + if not aktualizr.is_ecu_registered(secondary.id): + logger.error("Secondary ECU is not registered.") + return False + + with IPSecondary(output_logs=False, id=('hwid1', 'serial2')) as secondary2: + # Why is this necessary? The Primary waiting works outside of this test. + time.sleep(5) + aktualizr.add_secondary(secondary2) + with aktualizr: + aktualizr.wait_for_completion() + + if not aktualizr.is_ecu_registered(secondary.id) or not aktualizr.is_ecu_registered(secondary2.id): + logger.error("Secondary ECU is not registered.") + return False + + return True + + +@with_uptane_backend() +@with_secondary(start=True, id=('hwid1', 'serial1'), output_logs=False) +@with_secondary(start=True, id=('hwid1', 'serial2'), output_logs=False, arg_name='secondary2') +@with_aktualizr(start=False, output_logs=True) +def test_remove_secondary(uptane_repo, secondary, secondary2, aktualizr, **kwargs): + '''Test removing a Secondary after registration''' + + with aktualizr: + aktualizr.wait_for_completion() + + if not aktualizr.is_ecu_registered(secondary.id) or not aktualizr.is_ecu_registered(secondary2.id): + logger.error("Secondary ECU is not registered.") + return False + + aktualizr.remove_secondary(secondary2) + with aktualizr: + aktualizr.wait_for_completion() + + if not aktualizr.is_ecu_registered(secondary.id): + logger.error("Secondary ECU is not registered.") + return False + if aktualizr.is_ecu_registered(secondary2.id): + logger.error("Secondary ECU is unexpectedly still registered.") + return False + + return True + + +@with_uptane_backend() +@with_secondary(start=True, id=('hwid1', 'serial1'), output_logs=False) +@with_secondary(start=True, id=('hwid1', 'serial2'), output_logs=False, arg_name='secondary2') +@with_aktualizr(start=False, output_logs=True) +def test_replace_secondary(uptane_repo, secondary, secondary2, aktualizr, **kwargs): + '''Test replacing a Secondary after registration''' + + with aktualizr: + aktualizr.wait_for_completion() + + if not aktualizr.is_ecu_registered(secondary.id) or not aktualizr.is_ecu_registered(secondary2.id): + logger.error("Secondary ECU is not registered.") + return False + + aktualizr.remove_secondary(secondary2) + + with IPSecondary(output_logs=False, id=('hwid1', 'serial3')) as secondary3: + # Why is this necessary? The Primary waiting works outside of this test. + time.sleep(5) + aktualizr.add_secondary(secondary3) + with aktualizr: + aktualizr.wait_for_completion() + + if not aktualizr.is_ecu_registered(secondary.id) or not aktualizr.is_ecu_registered(secondary3.id): + logger.error("Secondary ECU is not registered.") + return False + if aktualizr.is_ecu_registered(secondary2.id): + logger.error("Secondary ECU is unexpectedly still registered.") + return False + + return True + + +@with_uptane_backend() +@with_secondary(start=True, id=('hwid1', 'serial1'), output_logs=False) +@with_aktualizr(start=False, output_logs=True) +def test_replace_secondary_same_port(uptane_repo, secondary, aktualizr, **kwargs): + '''Test replacing a Secondary that reuses the same port''' + + port = IPSecondary.get_free_port() + with IPSecondary(output_logs=False, id=('hwid1', 'serial2'), port=port) as secondary2: + # Why is this necessary? The Primary waiting works outside of this test. + time.sleep(5) + aktualizr.add_secondary(secondary2) + with aktualizr: + aktualizr.wait_for_completion() + + if not aktualizr.is_ecu_registered(secondary.id) or not aktualizr.is_ecu_registered(secondary2.id): + logger.error("Secondary ECU is not registered.") + return False + + aktualizr.remove_secondary(secondary2) + + with IPSecondary(output_logs=False, id=('hwid1', 'serial3'), port=port) as secondary3: + # Why is this necessary? The Primary waiting works outside of this test. + time.sleep(5) + aktualizr.add_secondary(secondary3) + with aktualizr: + aktualizr.wait_for_completion() + + if not aktualizr.is_ecu_registered(secondary.id) or not aktualizr.is_ecu_registered(secondary3.id): + logger.error("Secondary ECU is not registered.") + return False + if aktualizr.is_ecu_registered(secondary2.id): + logger.error("Secondary ECU is unexpectedly still registered.") + return False + + return True + + +@with_uptane_backend() +@with_secondary(start=True, id=('hwid1', 'serial1'), output_logs=False) +@with_aktualizr(start=False, output_logs=True) +def test_change_secondary_port(uptane_repo, secondary, aktualizr, **kwargs): + '''Test changing a Secondary's port but not the ECU serial''' + + with IPSecondary(output_logs=False, id=('hwid1', 'serial2')) as secondary2: + # Why is this necessary? The Primary waiting works outside of this test. + time.sleep(5) + aktualizr.add_secondary(secondary2) + with aktualizr: + aktualizr.wait_for_completion() + + if not aktualizr.is_ecu_registered(secondary.id) or not aktualizr.is_ecu_registered(secondary2.id): + logger.error("Secondary ECU is not registered.") + return False + + aktualizr.remove_secondary(secondary2) + + with IPSecondary(output_logs=False, id=('hwid1', 'serial2')) as secondary3: + # Why is this necessary? The Primary waiting works outside of this test. + time.sleep(5) + aktualizr.add_secondary(secondary3) + with aktualizr: + aktualizr.wait_for_completion() + + if secondary2.port == secondary3.port: + logger.error("Secondary ECU port unexpectedly did not change!") + return False + + if not aktualizr.is_ecu_registered(secondary.id) or not aktualizr.is_ecu_registered(secondary3.id): + logger.error("Secondary ECU is not registered.") + return False + + return True + + @with_treehub() @with_uptane_backend() @with_director() @@ -170,8 +331,10 @@ def test_secondary_ostree_reboot(uptane_repo, secondary, aktualizr, treehub, sys sysroot.update_revision(pending_rev) aktualizr.set_mode('full') - with aktualizr: - with secondary: + with secondary: + # Why is this necessary? The Primary waiting works outside of this test. + time.sleep(5) + with aktualizr: director.wait_for_install() if not director.get_install_result(): @@ -223,7 +386,6 @@ def test_secondary_install_timeout(uptane_repo, secondary, aktualizr, director, return not director.get_install_result() - @with_uptane_backend() @with_secondary(start=False) @with_aktualizr(start=False, output_logs=False, wait_timeout=0.1) @@ -315,7 +477,7 @@ def test_primary_timeout_after_device_is_registered(uptane_repo, secondary, aktu @with_secondary(start=False, arg_name='secondary2') @with_aktualizr(start=False, output_logs=True) def test_primary_multiple_secondaries(uptane_repo, secondary, secondary2, aktualizr, **kwargs): - '''Test Aktualizr with multiple ip secondaries''' + '''Test Aktualizr with multiple IP secondaries''' with aktualizr, secondary, secondary2: aktualizr.wait_for_completion() @@ -359,6 +521,11 @@ def test_primary_multiple_secondaries(uptane_repo, secondary, secondary2, aktual test_secondary_update, test_secondary_update_if_secondary_starts_first, test_secondary_update_if_primary_starts_first, + test_add_secondary, + test_remove_secondary, + test_replace_secondary, + test_replace_secondary_same_port, + test_change_secondary_port, test_secondary_install_timeout, test_primary_timeout_during_first_run, test_primary_timeout_after_device_is_registered, diff --git a/tests/test_fixtures.py b/tests/test_fixtures.py index e9f6baeaad..f88f389b3d 100644 --- a/tests/test_fixtures.py +++ b/tests/test_fixtures.py @@ -139,6 +139,14 @@ def add_secondary(self, secondary): config_file.seek(0) json.dump(sec_cfg, config_file) + def remove_secondary(self, secondary): + with open(self._secondary_config_file, "r+") as config_file: + sec_cfg = json.load(config_file) + sec_cfg["IP"]["secondaries"].remove({"addr": "127.0.0.1:{}".format(secondary.port)}) + config_file.seek(0) + json.dump(sec_cfg, config_file) + config_file.truncate() + def update_wait_timeout(self, timeout): with open(self._secondary_config_file, "r+") as config_file: sec_cfg = json.load(config_file) @@ -293,7 +301,7 @@ def cert(): class IPSecondary: - def __init__(self, aktualizr_secondary_exe, id, port=None, primary_port=None, + def __init__(self, id, aktualizr_secondary_exe='src/aktualizr_secondary/aktualizr-secondary', port=None, primary_port=None, sysroot=None, treehub=None, output_logs=True, force_reboot=False, ostree_mock_path=None, **kwargs): self.id = id From b11a78538853ca03a699d7cc8788cbcb4bd87db3 Mon Sep 17 00:00:00 2001 From: Patrick Vacek Date: Mon, 8 Jun 2020 17:06:22 +0200 Subject: [PATCH 05/10] Fix a bogus message about unregistered ECUs. We no longer track unregistered ECUs, since that is no longer a problem. Still dump anything in the database with that flag, just in case, but it shouldn't happen. Signed-off-by: Patrick Vacek --- src/aktualizr_info/main.cc | 2 +- tests/test_fixtures.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/aktualizr_info/main.cc b/src/aktualizr_info/main.cc index 7922dfc57f..3747027bf1 100644 --- a/src/aktualizr_info/main.cc +++ b/src/aktualizr_info/main.cc @@ -357,7 +357,7 @@ int main(int argc, char **argv) { std::vector misconfigured_ecus; storage->loadMisconfiguredEcus(&misconfigured_ecus); if (misconfigured_ecus.size() != 0U) { - std::cout << "Removed or not registered ECUs:" << std::endl; + std::cout << "Removed or unregistered ECUs (deprecated):" << std::endl; std::vector::const_iterator it; for (it = misconfigured_ecus.begin(); it != misconfigured_ecus.end(); ++it) { std::cout << " '" << it->serial << "' with hardware_id '" << it->hardware_id << "' " diff --git a/tests/test_fixtures.py b/tests/test_fixtures.py index f88f389b3d..b35dec6b09 100644 --- a/tests/test_fixtures.py +++ b/tests/test_fixtures.py @@ -183,7 +183,7 @@ def is_ecu_registered(self, ecu_id): device_status = self.get_info() if not ((device_status.find(ecu_id[0]) != -1) and (device_status.find(ecu_id[1]) != -1)): return False - not_registered_field = "Removed or not registered ECUs:" + not_registered_field = "Removed or unregistered ECUs (deprecated):" not_reg_start = device_status.find(not_registered_field) return not_reg_start == -1 or (device_status.find(ecu_id[1], not_reg_start) == -1) From e473c86902aa38c51de68ae4acc68ed47b9e69f4 Mon Sep 17 00:00:00 2001 From: Patrick Vacek Date: Mon, 8 Jun 2020 17:10:25 +0200 Subject: [PATCH 06/10] Add a comment about a semi-obscure almost-bug. There's no real problem, just an ugly log message that will go away eventually on its own. Signed-off-by: Patrick Vacek --- src/libaktualizr/primary/sotauptaneclient.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/libaktualizr/primary/sotauptaneclient.cc b/src/libaktualizr/primary/sotauptaneclient.cc index 2b5dd61a49..ecb17afc8e 100644 --- a/src/libaktualizr/primary/sotauptaneclient.cc +++ b/src/libaktualizr/primary/sotauptaneclient.cc @@ -487,6 +487,9 @@ void SotaUptaneClient::getNewTargets(std::vector *new_targets, u // in the vehicle. const auto hw_id_known = getEcuHwId(ecu_serial); if (!hw_id_known) { + // This is triggered if a Secondary is removed after an update was + // installed on it because of the empty targets optimization. + // Thankfully if the Director issues new Targets, it fixes itself. LOG_ERROR << "Unknown ECU ID in Director Targets metadata: " << ecu_serial.ToString(); throw Uptane::BadEcuId(target.filename()); } From 9548b887c2fb0c461154fc23f15e7048383b73fb Mon Sep 17 00:00:00 2001 From: Patrick Vacek Date: Tue, 9 Jun 2020 13:25:54 +0200 Subject: [PATCH 07/10] Always read the device ID from the cert if available. For device credential provisioning, it is still a fatal error if the cert is unavailable. For shared cred prov, we revert to the old behavior of generating a random pet name if the cert is unavailable. This is generally only observable if you attempt to replace a Primary ECU, which requires reusing the same device certificate. Signed-off-by: Patrick Vacek --- src/libaktualizr/primary/initializer.cc | 21 ++++++--- src/libaktualizr/primary/initializer_test.cc | 45 ++++++++++++++++++-- 2 files changed, 56 insertions(+), 10 deletions(-) diff --git a/src/libaktualizr/primary/initializer.cc b/src/libaktualizr/primary/initializer.cc index dff7a46c7d..0b2a66fac5 100644 --- a/src/libaktualizr/primary/initializer.cc +++ b/src/libaktualizr/primary/initializer.cc @@ -11,21 +11,28 @@ // Postcondition: device_id is in the storage void Initializer::initDeviceId() { - // if device_id is already stored, just return + // If device_id is already stored, just return. std::string device_id; if (storage_->loadDeviceId(&device_id)) { return; } - // if device_id is specified in config, just use it, otherwise generate a random one + // If device_id is specified in the config, use that. device_id = config_.device_id; if (device_id.empty()) { - if (config_.mode == ProvisionMode::kSharedCred) { - device_id = Utils::genPrettyName(); - } else if (config_.mode == ProvisionMode::kDeviceCred) { + // Otherwise, try to read the device certificate if it is available. + try { device_id = keys_.getCN(); - } else { - throw Error("Unknown provisioning method"); + } catch (const std::exception& e) { + // No certificate: for device credential provisioning, abort. For shared + // credential provisioning, generate a random name. + if (config_.mode == ProvisionMode::kSharedCred) { + device_id = Utils::genPrettyName(); + } else if (config_.mode == ProvisionMode::kDeviceCred) { + throw e; + } else { + throw Error("Unknown provisioning method"); + } } } diff --git a/src/libaktualizr/primary/initializer_test.cc b/src/libaktualizr/primary/initializer_test.cc index 850b99d3c5..86458f8496 100644 --- a/src/libaktualizr/primary/initializer_test.cc +++ b/src/libaktualizr/primary/initializer_test.cc @@ -116,7 +116,7 @@ TEST(Initializer, InitializeTwice) { * Check that aktualizr does not generate a pet name when device ID is * specified. */ -TEST(Initializer, PetNameProvided) { +TEST(Initializer, PetNameConfiguration) { RecordProperty("zephyr_key", "OTA-985,TST-146"); TemporaryDirectory temp_dir; const std::string test_name = "test-name-123"; @@ -148,6 +148,45 @@ TEST(Initializer, PetNameProvided) { } } +/** + * Check that aktualizr does not generate a pet name when device ID is + * already present in the device certificate's common name. This is the expected + * behavior required to support replacing a Primary ECU. + */ +TEST(Initializer, PetNameDeviceCert) { + std::string test_name; + Config conf("tests/config/basic.toml"); + + { + TemporaryDirectory temp_dir; + auto http = std::make_shared(temp_dir.Path()); + conf.storage.path = temp_dir.Path(); + auto storage = INvStorage::newStorage(conf.storage); + KeyManager keys(storage, conf.keymanagerConfig()); + storage->storeTlsCert(Utils::readFile("tests/test_data/prov/client.pem")); + test_name = keys.getCN(); + EXPECT_NO_THROW(Initializer(conf.provision, storage, http, keys, {})); + std::string devid; + EXPECT_TRUE(storage->loadDeviceId(&devid)); + EXPECT_EQ(devid, test_name); + } + + { + /* Make sure name is unchanged after re-initializing with the same device + * certificate but otherwise a completely fresh storage. */ + TemporaryDirectory temp_dir; + auto http = std::make_shared(temp_dir.Path()); + conf.storage.path = temp_dir.Path(); + auto storage = INvStorage::newStorage(conf.storage); + KeyManager keys(storage, conf.keymanagerConfig()); + storage->storeTlsCert(Utils::readFile("tests/test_data/prov/client.pem")); + EXPECT_NO_THROW(Initializer(conf.provision, storage, http, keys, {})); + std::string devid; + EXPECT_TRUE(storage->loadDeviceId(&devid)); + EXPECT_EQ(devid, test_name); + } +} + /** * Check that aktualizr generates a pet name if no device ID is specified. */ @@ -173,8 +212,8 @@ TEST(Initializer, PetNameCreation) { EXPECT_NE(test_name1, ""); } - // Make sure a new name is generated if the config does not specify a name and - // there is no device_id file. + // Make sure a new name is generated if using a new database and the config + // does not specify a device ID. TemporaryDirectory temp_dir2; { conf.storage.path = temp_dir2.Path(); From 64aac5523a9cc3ac3b8b12ed49d9396ff828e7be Mon Sep 17 00:00:00 2001 From: Patrick Vacek Date: Tue, 9 Jun 2020 16:19:40 +0200 Subject: [PATCH 08/10] Fall back to "unknown" image name instead of an empty string. The backend does not accept empty filenames, but we have to say something. This comes up if you wipe a Secondary's database but keep the image file. Signed-off-by: Patrick Vacek --- src/aktualizr_secondary/aktualizr_secondary_factory.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/aktualizr_secondary/aktualizr_secondary_factory.cc b/src/aktualizr_secondary/aktualizr_secondary_factory.cc index eb34d21ec2..9ac11ceff9 100644 --- a/src/aktualizr_secondary/aktualizr_secondary_factory.cc +++ b/src/aktualizr_secondary/aktualizr_secondary_factory.cc @@ -30,6 +30,8 @@ AktualizrSecondary::Ptr AktualizrSecondaryFactory::create(const AktualizrSeconda if (installed_version_res && !!current_version) { current_target_name = current_version->filename(); + } else { + current_target_name = "unknown"; } update_agent = From 3ca4e32072567eac5873aabc3996b55f8e5beca4 Mon Sep 17 00:00:00 2001 From: Patrick Vacek Date: Wed, 17 Jun 2020 09:59:35 +0200 Subject: [PATCH 09/10] uptane_serial_test: Get rid of unused input parameter. Signed-off-by: Patrick Vacek --- src/libaktualizr/uptane/CMakeLists.txt | 2 +- src/libaktualizr/uptane/uptane_serial_test.cc | 7 ------- 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/src/libaktualizr/uptane/CMakeLists.txt b/src/libaktualizr/uptane/CMakeLists.txt index 544fa37a30..82d416f843 100644 --- a/src/libaktualizr/uptane/CMakeLists.txt +++ b/src/libaktualizr/uptane/CMakeLists.txt @@ -51,7 +51,7 @@ add_aktualizr_test(NAME uptane_network SOURCES uptane_network_test.cc PROJECT_WO set_tests_properties(test_uptane_network PROPERTIES LABELS "crypto") target_link_libraries(t_uptane_network virtual_secondary) -add_aktualizr_test(NAME uptane_serial SOURCES uptane_serial_test.cc ARGS ${PROJECT_BINARY_DIR} +add_aktualizr_test(NAME uptane_serial SOURCES uptane_serial_test.cc PROJECT_WORKING_DIRECTORY LIBRARIES uptane_generator_lib) target_link_libraries(t_uptane_serial virtual_secondary) diff --git a/src/libaktualizr/uptane/uptane_serial_test.cc b/src/libaktualizr/uptane/uptane_serial_test.cc index a5d1efb56a..018a6be4da 100644 --- a/src/libaktualizr/uptane/uptane_serial_test.cc +++ b/src/libaktualizr/uptane/uptane_serial_test.cc @@ -21,8 +21,6 @@ namespace bpo = boost::program_options; -boost::filesystem::path build_dir; - /** * Check that aktualizr generates random ecu_serial for Primary and all * Secondaries. @@ -133,11 +131,6 @@ int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv); logger_set_threshold(boost::log::trivial::trace); - if (argc != 2) { - std::cerr << "Error: " << argv[0] << " requires the path to the build directory as an input argument.\n"; - return EXIT_FAILURE; - } - build_dir = argv[1]; return RUN_ALL_TESTS(); } #endif From d943cd95476ad5caf76379e99632675dd169100d Mon Sep 17 00:00:00 2001 From: Patrick Vacek Date: Wed, 17 Jun 2020 10:01:06 +0200 Subject: [PATCH 10/10] Only store ECU changes if we successfully registered the ECUs. If registration fails (due to the server complaining about an update in progress, for example), we want to make it easier to undo the changes and prevent re-registration. Signed-off-by: Patrick Vacek --- src/libaktualizr/primary/initializer.cc | 65 +++++++++---------- src/libaktualizr/primary/initializer.h | 3 +- .../uptane/uptane_network_test.cc | 9 --- 3 files changed, 33 insertions(+), 44 deletions(-) diff --git a/src/libaktualizr/primary/initializer.cc b/src/libaktualizr/primary/initializer.cc index 0b2a66fac5..57607999c6 100644 --- a/src/libaktualizr/primary/initializer.cc +++ b/src/libaktualizr/primary/initializer.cc @@ -59,26 +59,25 @@ void Initializer::initEcuSerials() { } } - EcuSerials new_ecu_serials; - new_ecu_serials.emplace_back(Uptane::EcuSerial(primary_ecu_serial_local), - Uptane::HardwareIdentifier(primary_ecu_hardware_id)); + new_ecu_serials_.emplace_back(Uptane::EcuSerial(primary_ecu_serial_local), + Uptane::HardwareIdentifier(primary_ecu_hardware_id)); for (const auto& s : secondaries_) { - new_ecu_serials.emplace_back(s.first, s.second->getHwId()); + new_ecu_serials_.emplace_back(s.first, s.second->getHwId()); } - register_ecus = stored_ecu_serials.empty(); + register_ecus_ = stored_ecu_serials.empty(); if (!stored_ecu_serials.empty()) { // We should probably clear the misconfigured_ecus table once we have // consent working. std::vector found(stored_ecu_serials.size(), false); - EcuCompare primary_comp(new_ecu_serials[0]); + EcuCompare primary_comp(new_ecu_serials_[0]); EcuSerials::const_iterator store_it; store_it = std::find_if(stored_ecu_serials.cbegin(), stored_ecu_serials.cend(), primary_comp); if (store_it == stored_ecu_serials.cend()) { - LOG_INFO << "Configured Primary ECU serial " << new_ecu_serials[0].first << " with hardware ID " - << new_ecu_serials[0].second << " not found in storage."; - register_ecus = true; + LOG_INFO << "Configured Primary ECU serial " << new_ecu_serials_[0].first << " with hardware ID " + << new_ecu_serials_[0].second << " not found in storage."; + register_ecus_ = true; } else { found[static_cast(store_it - stored_ecu_serials.cbegin())] = true; } @@ -90,7 +89,7 @@ void Initializer::initEcuSerials() { if (store_it == stored_ecu_serials.cend()) { LOG_INFO << "Configured Secondary ECU serial " << it->second->getSerial() << " with hardware ID " << it->second->getHwId() << " not found in storage."; - register_ecus = true; + register_ecus_ = true; } else { found[static_cast(store_it - stored_ecu_serials.cbegin())] = true; } @@ -104,15 +103,11 @@ void Initializer::initEcuSerials() { auto not_registered = stored_ecu_serials[static_cast(found_it - found.begin())]; LOG_INFO << "ECU serial " << not_registered.first << " with hardware ID " << not_registered.second << " in storage was not found in Secondary configuration."; - register_ecus = true; + register_ecus_ = true; storage_->saveMisconfiguredEcu({not_registered.first, not_registered.second, EcuState::kOld}); } } } - - if (register_ecus) { - storage_->storeEcuSerials(new_ecu_serials); - } } // Postcondition: (public, private) is in the storage. It should not be stored until Secondaries are provisioned @@ -198,7 +193,7 @@ void Initializer::resetTlsCreds() { // Postcondition: "ECUs registered" flag set in the storage void Initializer::initEcuRegister() { // Allow re-registration if the ECUs have changed. - if (storage_->loadEcuRegistered() && !register_ecus) { + if (!register_ecus_) { LOG_DEBUG << "All ECUs are already registered with the server."; return; } @@ -209,19 +204,13 @@ void Initializer::initEcuRegister() { throw StorageError("Invalid key in storage"); } - EcuSerials ecu_serials; - // initEcuSerials should have been called by this point - if (!storage_->loadEcuSerials(&ecu_serials) || ecu_serials.size() < 1) { - throw StorageError("Could not load ECUs from storage"); - } - Json::Value all_ecus; - all_ecus["primary_ecu_serial"] = ecu_serials[0].first.ToString(); + all_ecus["primary_ecu_serial"] = new_ecu_serials_[0].first.ToString(); all_ecus["ecus"] = Json::arrayValue; { Json::Value primary_ecu; - primary_ecu["hardware_identifier"] = ecu_serials[0].second.ToString(); - primary_ecu["ecu_serial"] = ecu_serials[0].first.ToString(); + primary_ecu["hardware_identifier"] = new_ecu_serials_[0].second.ToString(); + primary_ecu["ecu_serial"] = new_ecu_serials_[0].first.ToString(); primary_ecu["clientKey"] = keys_.UptanePublicKey().ToUptane(); all_ecus["ecus"].append(primary_ecu); } @@ -246,6 +235,11 @@ void Initializer::initEcuRegister() { throw ServerError(err); } + // Only store the changes if we successfully registered the ECUs. + storage_->storeEcuSerials(new_ecu_serials_); + for (const auto& info : sec_info_) { + storage_->saveSecondaryInfo(info.serial, info.type, info.pub_key); + } storage_->storeEcuRegistered(); LOG_INFO << "ECUs have been successfully registered with the server."; @@ -257,12 +251,11 @@ void Initializer::initSecondaryInfo() { Uptane::SecondaryInterface& sec = *s.second; SecondaryInfo info; - // We must check this on every initialization, as this info - // won't be there in case of an upgrade from an older version. - // On the first initialization ECU serials should be already - // initialized by this point. - if (register_ecus || !storage_->loadSecondaryInfo(serial, &info) || info.type == "" || - info.pub_key.Type() == KeyType::kUnknown) { + // If upgrading from the older version of the storage without the + // secondary_ecus table, we need to migrate the data. This should be done + // regardless of whether we need to (re-)register the ECUs. + // The ECU serials should be already initialized by this point. + if (!storage_->loadSecondaryInfo(serial, &info) || info.type == "" || info.pub_key.Type() == KeyType::kUnknown) { info.serial = serial; info.hw_id = sec.getHwId(); info.type = sec.Type(); @@ -270,7 +263,11 @@ void Initializer::initSecondaryInfo() { if (p.Type() != KeyType::kUnknown) { info.pub_key = p; } - storage_->saveSecondaryInfo(info.serial, info.type, info.pub_key); + // If we don't need to register the ECUs, we still need to store this info + // to complete the migration. + if (!register_ecus_) { + storage_->saveSecondaryInfo(info.serial, info.type, info.pub_key); + } } // We will need this info later if the device is not yet provisioned sec_info_.push_back(std::move(info)); @@ -319,12 +316,12 @@ Initializer::Initializer(const ProvisionConfig& config_in, std::shared_ptr& secondaries_; std::vector sec_info_; - bool register_ecus{false}; + EcuSerials new_ecu_serials_; + bool register_ecus_{false}; }; class EcuCompare { diff --git a/src/libaktualizr/uptane/uptane_network_test.cc b/src/libaktualizr/uptane/uptane_network_test.cc index 969af81e11..92f0d72b07 100644 --- a/src/libaktualizr/uptane/uptane_network_test.cc +++ b/src/libaktualizr/uptane/uptane_network_test.cc @@ -60,15 +60,6 @@ bool doTestInit(StorageType storage_type, const std::string &device_register_sta conf.provision.expiry_days = "noerrors"; conf.provision.primary_ecu_serial = "noerrors"; - if (device_register_state == "noerrors" && ecu_register_state != "noerrors") { - // restore a "good" ECU serial in the ECU register fault injection case - // (the bad value has been cached in storage) - EcuSerials serials; - store->loadEcuSerials(&serials); - serials[0].first = Uptane::EcuSerial(conf.provision.primary_ecu_serial); - store->storeEcuSerials(serials); - } - KeyManager keys(store, conf.keymanagerConfig()); try { Initializer initializer(conf.provision, store, http, keys, {});