Fix/ota 4941/installed version matching

src/libaktualizr/package_manager/packagemanagerfake.cc

@@ -110,6 +110,8 @@ bool PackageManagerFake::fetchTarget(const Uptane::Target& target, Uptane::Fetch
     return false;
   }
 
+  // TODO(OTA-4939): Unify this with the check in
+  // SotaUptaneClient::getNewTargets() and make it more generic.
   if (target.IsOstree()) {
     LOG_ERROR << "Cannot download OSTree target " << target.filename() << " with the fake package manager!";
     return false;

src/libaktualizr/primary/sotauptaneclient.cc

@@ -452,22 +452,22 @@ void SotaUptaneClient::computeDeviceInstallationResult(data::InstallationResult
 }
 
 void SotaUptaneClient::getNewTargets(std::vector<Uptane::Target> *new_targets, unsigned int *ecus_count) {
-  std::vector<Uptane::Target> targets = director_repo.getTargets().targets;
-  Uptane::EcuSerial primary_ecu_serial = primaryEcuSerial();
+  const std::vector<Uptane::Target> targets = director_repo.getTargets().targets;
+  const Uptane::EcuSerial primary_ecu_serial = primaryEcuSerial();
   if (ecus_count != nullptr) {
     *ecus_count = 0;
   }
   for (const Uptane::Target &target : targets) {
     bool is_new = false;
     for (const auto &ecu : target.ecus()) {
-      Uptane::EcuSerial ecu_serial = ecu.first;
-      Uptane::HardwareIdentifier hw_id = ecu.second;
+      const Uptane::EcuSerial ecu_serial = ecu.first;
+      const Uptane::HardwareIdentifier hw_id = ecu.second;
 
       // 5.4.4.6.8. If checking Targets metadata from the Director repository,
       // 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.
-      auto hw_id_known = ecuHwId(ecu_serial);
+      const auto hw_id_known = ecuHwId(ecu_serial);
       if (!hw_id_known) {
         LOG_ERROR << "Unknown ECU ID in Director Targets metadata: " << ecu_serial.ToString();
         throw Uptane::BadEcuId(target.filename());
@@ -487,10 +487,18 @@ void SotaUptaneClient::getNewTargets(std::vector<Uptane::Target> *new_targets, u
       if (!current_version) {
         LOG_WARNING << "Current version for ECU ID: " << ecu_serial.ToString() << " is unknown";
         is_new = true;
-      } else if (current_version->filename() != target.filename()) {
+      } else if (current_version->MatchTarget(target)) {
+        // Do nothing; target is already installed.
+      } else if (current_version->filename() == target.filename()) {
+        LOG_ERROR << "Director Target filename matches currently installed version, but content differs!";
+        throw Uptane::TargetContentMismatch(target.filename());
+      } else {
         is_new = true;
       }
 
+      // Reject non-OSTree updates for the Primary if using OSTree.
+      // TODO(OTA-4939): Unify this with the check in
+      // SotaUptaneClient::getNewTargets() and make it more generic.
       if (primary_ecu_serial == ecu_serial) {
         if (!target.IsOstree() &&
             (config.pacman.type == PACKAGE_MANAGER_OSTREE || config.pacman.type == PACKAGE_MANAGER_OSTREEDOCKERAPP)) {
@@ -711,24 +719,15 @@ void SotaUptaneClient::uptaneIteration(std::vector<Uptane::Target> *targets, uns
   try {
     getNewTargets(&tmp_targets, &ecus);
   } catch (const std::exception &e) {
-    LOG_ERROR << "Inconsistency between Director metadata and discovered ECUs";
+    LOG_ERROR << "Inconsistency between Director metadata and available ECUs: " << e.what();
     throw;
   }
 
-  if (tmp_targets.empty()) {
-    if (targets != nullptr) {
-      *targets = std::move(tmp_targets);
-    }
-    if (ecus_count != nullptr) {
-      *ecus_count = ecus;
-    }
-    return;
+  if (!tmp_targets.empty()) {
+    LOG_INFO << "New updates found in Director metadata. Checking Image repo metadata...";
+    updateImageMeta();
   }
 
-  LOG_INFO << "New updates found in Director metadata. Checking Image repo metadata...";
-
-  updateImageMeta();
-
   if (targets != nullptr) {
     *targets = std::move(tmp_targets);
   }
@@ -745,21 +744,18 @@ void SotaUptaneClient::uptaneOfflineIteration(std::vector<Uptane::Target> *targe
   try {
     getNewTargets(&tmp_targets, &ecus);
   } catch (const std::exception &e) {
-    LOG_ERROR << "Inconsistency between Director metadata and existing ECUs: " << e.what();
+    LOG_ERROR << "Inconsistency between Director metadata and available ECUs: " << e.what();
     throw;
   }
 
-  if (tmp_targets.empty()) {
-    *targets = std::move(tmp_targets);
-    if (ecus_count != nullptr) {
-      *ecus_count = ecus;
-    }
-    return;
+  if (!tmp_targets.empty()) {
+    LOG_DEBUG << "New updates found in stored Director metadata. Checking stored Image repo metadata...";
+    checkImageMetaOffline();
   }
 
-  checkImageMetaOffline();
-
-  *targets = std::move(tmp_targets);
+  if (targets != nullptr) {
+    *targets = std::move(tmp_targets);
+  }
   if (ecus_count != nullptr) {
     *ecus_count = ecus;
   }

src/libaktualizr/uptane/directorrepository.cc

@@ -122,6 +122,9 @@ void DirectorRepository::updateMeta(INvStorage& storage, const IMetadataFetcher&
 
     verifyTargets(director_targets);
 
+    // TODO(OTA-4940): check if versions are equal but content is different. In
+    // that case, the member variable targets is updated, but it isn't stored in
+    // the database, which can cause some minor confusion.
     if (local_version > remote_version) {
       throw Uptane::SecurityException(RepositoryType::DIRECTOR, "Rollback attempt");
     } else if (local_version < remote_version && !usePreviousTargets()) {

src/libaktualizr/uptane/exceptions.h

@@ -31,6 +31,13 @@ class SecurityException : public Exception {
   ~SecurityException() noexcept override = default;
 };
 
+class TargetContentMismatch : public Exception {
+ public:
+  explicit TargetContentMismatch(const std::string& targetname)
+      : Exception(targetname, "Director Target filename matches currently installed version, but content differs.") {}
+  ~TargetContentMismatch() noexcept override = default;
+};
+
 class TargetHashMismatch : public Exception {
  public:
   explicit TargetHashMismatch(const std::string& targetname)

src/libaktualizr/uptane/imagerepository.cc

@@ -115,7 +115,7 @@ void ImageRepository::fetchTargets(INvStorage& storage, const IMetadataFetcher&
   verifyTargets(image_targets, false);
 
   if (local_version > remote_version) {
-    throw Uptane::SecurityException(RepositoryType::IMAGE, "Mismatched target versions");
+    throw Uptane::SecurityException(RepositoryType::IMAGE, "Rollback attempt");
   } else if (local_version < remote_version) {
     storage.storeNonRoot(image_targets, RepositoryType::Image(), targets_role);
   }

src/libaktualizr/uptane/uptane_test.cc

@@ -323,9 +323,12 @@ void process_events_Install(const std::shared_ptr<event::BaseEvent> &event) {
     auto concrete_event = std::static_pointer_cast<event::AllInstallsComplete>(event);
     if (num_events_AllInstalls == 0) {
       EXPECT_TRUE(concrete_event->result.dev_report.isSuccess());
-    } else {
+    } else if (num_events_AllInstalls == 1) {
       EXPECT_FALSE(concrete_event->result.dev_report.isSuccess());
       EXPECT_EQ(concrete_event->result.dev_report.result_code, data::ResultCode::Numeric::kAlreadyProcessed);
+    } else {
+      EXPECT_FALSE(concrete_event->result.dev_report.isSuccess());
+      EXPECT_EQ(concrete_event->result.dev_report.result_code, data::ResultCode::Numeric::kInternalError);
     }
     num_events_AllInstalls++;
   }
@@ -340,6 +343,8 @@ void process_events_Install(const std::shared_ptr<event::BaseEvent> &event) {
  * Store installation result for Primary.
  * Store installation result for device.
  * Check if an update is already installed.
+ * Reject an update that matches the currently installed version's filename but
+ * not the length and/or hashes.
  */
 TEST(Uptane, InstallFakeGood) {
   Config conf("tests/config/basic.toml");
@@ -390,7 +395,8 @@ TEST(Uptane, InstallFakeGood) {
   EXPECT_EQ(installation_report["items"][1]["result"]["success"].asBool(), true);
   EXPECT_EQ(installation_report["items"][1]["result"]["code"].asString(), "OK");
 
-  // second install
+  // Second install to verify that we detect already installed updates
+  // correctly.
   result::Install install_result2 = up->uptaneInstall(download_result.updates);
   EXPECT_FALSE(install_result2.dev_report.isSuccess());
   EXPECT_EQ(install_result2.dev_report.result_code, data::ResultCode::Numeric::kAlreadyProcessed);
@@ -399,6 +405,28 @@ TEST(Uptane, InstallFakeGood) {
   manifest = up->AssembleManifest();
   installation_report = manifest["installation_report"]["report"];
   EXPECT_EQ(installation_report["result"]["success"].asBool(), false);
+
+  // Recheck updates in order to repopulate Director Targets metadata in the
+  // database (it gets dropped after failure).
+  result::UpdateCheck update_result2 = up->fetchMeta();
+  EXPECT_EQ(update_result2.status, result::UpdateStatus::kNoUpdatesAvailable);
+
+  // Remove the hashes from the current Target version stored in the database
+  // for the Primary.
+  boost::optional<Uptane::Target> current_version;
+  EXPECT_TRUE(storage->loadInstalledVersions("CA:FE:A6:D2:84:9D", &current_version, nullptr));
+  const auto bad_target = Uptane::Target(current_version->filename(), current_version->ecus(), std::vector<Hash>{},
+                                         current_version->length());
+  storage->saveInstalledVersion("CA:FE:A6:D2:84:9D", bad_target, InstalledVersionUpdateMode::kCurrent);
+
+  // Third install to verify that we reject updates with the same filename but
+  // different contents.
+  result::Install install_result3 = up->uptaneInstall(download_result.updates);
+  EXPECT_FALSE(install_result3.dev_report.isSuccess());
+  EXPECT_EQ(install_result3.dev_report.result_code, data::ResultCode::Numeric::kInternalError);
+  EXPECT_THROW(std::rethrow_exception(up->getLastException()), Uptane::TargetContentMismatch);
+  EXPECT_EQ(num_events_InstallTarget, 2);
+  EXPECT_EQ(num_events_AllInstalls, 3);
 }
 
 /*