Fix/ota 3060/check targets before download

[pattivacek]

This is still a work in progress, as there is additional work I'd like to include before merging. Also, test_aktualizr-lite is failing because of my changes and I'd like to discuss with @doanac how to reach a mutually satisfactory solution.

As to the changes, the biggest part is to recheck the Uptane metadata before downloading or installing a target. uptaneIteration() is now exclusively used during the check for updates, and it actually fetches the necessary metadata. uptaneOfflineIteration() is now exclusively used during downloading and installing to verify the integrity of the stored metadata. If a user then attempts to install a target that doesn't match the stored metadata, it will be rejected.

That stipulation required a lot of reworking of the Uptane tests, because several of them installed fake packages as a shortcut. That no longer works, so I had to expand several of the tests quite a bit. As a result, several of these tests are now rather redundant. We should re-evaluate the test cases in uptane_test and aktualizr_test and determine which are still meaningful and unique.

The other notable change is to only drop Director targets after a failed installation attempt. Otherwise this prevents being able to split updates into multiple individual calls to the Install API call. (A unit test is pending.) This still isn't my favorite fix for this problem, but in the short term, it's an improvement. [That was already merged in https://github.com//pull/1301.]

[pattivacek]

@doanac I'm still working on a few things for this PR, but one of the blockers is the aktualizr-lite test. Can you take a look and see if there's an easy solution for you? I don't want to accidentally break your expectations of how it should work.

[codecov-io]

Codecov Report

Merging #1296 into master will decrease coverage by 0.21%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master    #1296      +/-   ##
==========================================
- Coverage   80.03%   79.81%   -0.22%     
==========================================
  Files         177      177              
  Lines       10392    10475      +83     
==========================================
+ Hits         8317     8361      +44     
- Misses       2075     2114      +39

Impacted Files	Coverage Δ
src/libaktualizr/package_manager/ostreemanager.h	100% <ø> (ø)	⬆️
src/libaktualizr/uptane/iterator.h	100% <ø> (ø)	⬆️
...rc/libaktualizr/package_manager/dockerappmanager.h	80% <ø> (ø)	⬆️
...ktualizr/package_manager/packagemanagerinterface.h	94.73% <ø> (ø)	⬆️
src/aktualizr_lite/main.cc	76.88% <100%> (+0.56%)	⬆️
src/libaktualizr/primary/sotauptaneclient.h	100% <100%> (ø)	⬆️
...tualizr/package_manager/packagemanagerinterface.cc	93.75% <100%> (+1.06%)	⬆️
...c/libaktualizr/package_manager/dockerappmanager.cc	68.91% <29.41%> (-13.34%)	⬇️
src/libaktualizr/package_manager/ostreemanager.cc	79.13% <80%> (+1.16%)	⬆️
src/libaktualizr/uptane/iterator.cc	88.99% <80%> (-0.64%)	⬇️
... and 6 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eae7fdb...f22b669. Read the comment docs.

[doanac]

@patrickvacek - i've fetched your branch and have re-created the failure. I'm traveling to ELC today, so it might be a couple of days before I get real time to debug.

One side comment I'd add to this PR: We could probably add a verifyTarget method to the docker-app-manager code that checks to validate the .dockerapp target is syntactically valid.

[doanac]

@patrickvacek - this looks to be the call that's breaking aktualizr-lite. The problem is that downloadImages was originally a code path that did not have any Uptane dependencies and checkUpdatesOffline is adding one in.

I haven't fully thought through this, but one way we could avoid the Uptane path is by making the downloadImage method public so that aktualizr-lite could call that directly and maybe the new pacman->verifyTarget API as well?

[pattivacek]

this looks to be the call that's breaking aktualizr-lite.

Agreed.

The problem is that downloadImages was originally a code path that did not have any Uptane dependencies and checkUpdatesOffline is adding one in.

Yes, it should've always been there, but we were lazy before.

one way we could avoid the Uptane path is by making the downloadImage method public so that aktualizr-lite could call that directly

Not my favorite solution, but I'm not sure how many options we have, and I think we've already set the precedent that it's okay for tools in the aktualizr repo to access SotaUptaneClient functions directly, but "normal" users should go through the Aktualizr class API.

maybe the new pacman->verifyTarget API as well

That is called automatically during installation. Why would you want to call it after downloading?

We could probably add a verifyTarget method to the docker-app-manager code that checks to validate the .dockerapp target is syntactically valid.

Cool, sounds like a good idea!

I'm traveling to ELC today, so it might be a couple of days before I get real time to debug.

That's okay, I still have some work to do on this PR and it's been slow progress due to competing priorities. Anyway, have fun!

[pattivacek]

maybe the new pacman->verifyTarget API as well

That is called automatically during installation. Why would you want to call it after downloading?

Now I understand; it's because you don't call uptaneInstall directly!

I wish it were possible for you to use the same API but just skip the parts you don't need for your package manager of choice. Is it also possible to mock the DockerApp targets like we do for the fake package manager?

[pattivacek]

This is finally ready for review. The only thing still missing is just getting aktualizr-lite's do_update function to work. I've provided a stub for verifyTarget() but it's currently bypassed anyway.

[doanac]

I'm thinking a kInvalid might be good for this enum as well. eg - a debian package or docker-app target content were invalid.

[doanac]

Here's what I've changed to get things going:

diff --git a/src/aktualizr_lite/main.cc b/src/aktualizr_lite/main.cc
index a469beaf..09417e26 100644
--- a/src/aktualizr_lite/main.cc
+++ b/src/aktualizr_lite/main.cc
@@ -169,14 +169,11 @@ static std::unique_ptr<Uptane::Target> find_target(const std::shared_ptr<SotaUpt
 }
 
 static int do_update(SotaUptaneClient &client, INvStorage &storage, Uptane::Target &target) {
-  std::vector<Uptane::Target> targets{target};
-  auto result = client.downloadImages(targets);
-  if (result.status != result::DownloadStatus::kSuccess &&
-      result.status != result::DownloadStatus::kNothingToDownload) {
-    LOG_ERROR << "Unable to download update: " + result.message;
+  if (!client.downloadImage(target).first) {
     return 1;
   }
 
+  // TODO make pacman->verifyTarget something we can get to via client->
   auto iresult = client.PackageInstall(target);
   if (iresult.result_code.num_code == data::ResultCode::Numeric::kNeedCompletion) {
     LOG_INFO << "Update complete. Please reboot the device to activate";
diff --git a/src/libaktualizr/package_manager/dockerappmanager.cc b/src/libaktualizr/package_manager/dockerappmanager.cc
index da23dcfd..022f8c31 100644
--- a/src/libaktualizr/package_manager/dockerappmanager.cc
+++ b/src/libaktualizr/package_manager/dockerappmanager.cc
@@ -10,7 +10,7 @@ struct DockerApp {
         app_bin(config.docker_app_bin),
         compose_bin(config.docker_compose_bin) {}
 
-  bool render(const std::string &app_content) {
+  bool render(const std::string &app_content, bool persist) {
     auto bin = boost::filesystem::canonical(app_bin).string();
     Utils::writeFile(app_root / (name + ".dockerapp"), app_content);
     std::string cmd("cd " + app_root.string() + " && " + bin + " app render " + name);
@@ -22,7 +22,9 @@ struct DockerApp {
       LOG_ERROR << "Unable to run " << cmd << " output:\n" << yaml;
       return false;
     }
-    Utils::writeFile(app_root / "docker-compose.yml", yaml);
+    if (persist) {
+      Utils::writeFile(app_root / "docker-compose.yml", yaml);
+    }
     return true;
   }
 
@@ -101,7 +103,7 @@ data::InstallationResult DockerAppManager::install(const Uptane::Target &target)
     std::stringstream ss;
     ss << *storage_->openTargetFile(app_target);
     DockerApp dapp(app, config);
-    return dapp.render(ss.str()) && dapp.start();
+    return dapp.render(ss.str(), true) && dapp.start();
   };
   if (!iterate_apps(target, cb)) {
     return data::InstallationResult(data::ResultCode::Numeric::kInstallFailed, "Could not render docker app");
@@ -113,6 +115,16 @@ TargetStatus DockerAppManager::verifyTarget(const Uptane::Target &target) const
   if (target.IsOstree()) {
     return OstreeManager::verifyTarget(target);
   }
-  // TODO: verify DockerApp targets
+
+  auto cb = [this](const std::string &app, const Uptane::Target &app_target) {
+    LOG_INFO << "Verifying " << app << " -> " << app_target;
+    std::stringstream ss;
+    ss << *storage_->openTargetFile(app_target);
+    DockerApp dapp(app, config);
+    return dapp.render(ss.str(), false);
+  };
+  if (!iterate_apps(target, cb)) {
+    return TargetStatus::kInvalid;
+  }
   return TargetStatus::kGood;
 }
diff --git a/src/libaktualizr/package_manager/packagemanagerinterface.h b/src/libaktualizr/package_manager/packagemanagerinterface.h
index cbf4b3c4..4587fb5b 100644
--- a/src/libaktualizr/package_manager/packagemanagerinterface.h
+++ b/src/libaktualizr/package_manager/packagemanagerinterface.h
@@ -29,6 +29,8 @@ enum class TargetStatus {
   kOversized,
   /* Target was found, but hash did not match the metadata. */
   kHashMismatch,
+  /* Target was found and has valid metadata but the content is not suitable for the packagemanager */
+  kInvalid,
 };
 
 class PackageManagerInterface {
diff --git a/src/libaktualizr/primary/sotauptaneclient.h b/src/libaktualizr/primary/sotauptaneclient.h
index 84ea2c7c..28b11e60 100644
--- a/src/libaktualizr/primary/sotauptaneclient.h
+++ b/src/libaktualizr/primary/sotauptaneclient.h
@@ -43,6 +43,8 @@ class SotaUptaneClient {
   void addNewSecondary(const std::shared_ptr<Uptane::SecondaryInterface> &sec);
   result::Download downloadImages(const std::vector<Uptane::Target> &targets,
                                   const api::FlowControlToken *token = nullptr);
+  std::pair<bool, Uptane::Target> downloadImage(const Uptane::Target &target,
+                                                const api::FlowControlToken *token = nullptr);
   void reportPause();
   void reportResume();
   void sendDeviceData();
@@ -118,8 +120,6 @@ class SotaUptaneClient {
 
   bool putManifestSimple(const Json::Value &custom = Json::nullValue);
   bool getNewTargets(std::vector<Uptane::Target> *new_targets, unsigned int *ecus_count = nullptr);
-  std::pair<bool, Uptane::Target> downloadImage(const Uptane::Target &target,
-                                                const api::FlowControlToken *token = nullptr);
   void rotateSecondaryRoot(Uptane::RepositoryType repo, Uptane::SecondaryInterface &secondary);
   bool updateDirectorMeta();
   bool checkDirectorMetaOffline();

[pattivacek]

@doanac Thanks, that did the trick! Your code looks fine for now. We can refine it later.

There are now no blockers on this PR, it just needs review.

[lbonn]

Should we allowed the targets parameter to be null? It could help in niche cases, like tests and should be pretty cheap.

[pattivacek]

Done.