Merge pull request #115 from uptane/fix/secondary-root-rotation

Fix secondary root rotation on metadata expiry

src/aktualizr_info/aktualizr_info_test.cc

@@ -1,6 +1,7 @@
 #include <gtest/gtest.h>
 
 #include <boost/filesystem.hpp>
+#include <boost/filesystem/fstream.hpp>
 
 #include "libaktualizr/config.h"
 #include "storage/sqlstorage.h"

src/aktualizr_secondary/aktualizr_secondary_test.cc

@@ -2,7 +2,9 @@
 #include <gtest/gtest.h>
 
 #include <boost/filesystem.hpp>
+#include <boost/filesystem/string_file.hpp>
 #include <boost/optional/optional_io.hpp>
+#include <fstream>
 
 #include "aktualizr_secondary_file.h"
 #include "crypto/keymanager.h"

src/cert_provider/cert_provider_test.cc

@@ -1,6 +1,7 @@
 #include <gtest/gtest.h>
 
 #include <boost/filesystem.hpp>
+#include <boost/filesystem/fstream.hpp>
 #include <boost/format.hpp>
 
 #include "cert_provider_test.h"

src/libaktualizr/config/config_test.cc

@@ -6,6 +6,7 @@
 #include <boost/algorithm/hex.hpp>
 #include <boost/filesystem.hpp>
 #include <boost/program_options.hpp>
+#include <fstream>
 
 #include "bootstrap/bootstrap.h"
 #include "crypto/crypto.h"

src/libaktualizr/primary/sotauptaneclient.cc

@@ -1292,52 +1292,61 @@ data::InstallationResult SotaUptaneClient::rotateSecondaryRoot(Uptane::Repositor
     LOG_ERROR << "Error reading Root metadata";
     return data::InstallationResult(data::ResultCode::Numeric::kInternalError, "Error reading Root metadata");
   }
-
-  data::InstallationResult result{data::ResultCode::Numeric::kOk, ""};
   const int last_root_version = Uptane::extractVersionUntrusted(latest_root);
   const int sec_root_version = secondary.getRootVersion((repo == Uptane::RepositoryType::Director()));
-  // If sec_root_version is 0, assume either the Secondary doesn't have Root
-  // metadata or doesn't support the Root version request. Continue on and hope
-  // for the best.
+  LOG_DEBUG << "Rotating " << repo << " from " << sec_root_version << " to " << (last_root_version - 1);
   if (sec_root_version < 0) {
     LOG_WARNING << "Secondary with serial " << secondary.getSerial() << " reported an invalid " << repo
                 << " repo Root version: " << sec_root_version;
-    result =
-        data::InstallationResult(data::ResultCode::Numeric::kInternalError,
-                                 "Secondary with serial " + secondary.getSerial().ToString() + " reported an invalid " +
-                                     repo.ToString() + " repo Root version: " + std::to_string(sec_root_version));
-  } else if (sec_root_version > 0 && last_root_version - sec_root_version > 1) {
-    // Only send intermediate Roots that would otherwise be skipped. The latest
-    // will be sent with the complete set of the latest metadata.
-    for (int v = sec_root_version + 1; v < last_root_version; v++) {
-      std::string root;
-      if (!storage->loadRoot(&root, repo, Uptane::Version(v))) {
-        LOG_WARNING << "Couldn't find Root metadata in the storage, trying remote repo";
-        try {
-          uptane_fetcher->fetchRole(&root, Uptane::kMaxRootSize, repo, Uptane::Role::Root(), Uptane::Version(v),
-                                    flow_control_);
-        } catch (const std::exception &e) {
-          LOG_ERROR << "Root metadata could not be fetched for Secondary with serial " << secondary.getSerial()
-                    << ", skipping to the next Secondary";
-          result = data::InstallationResult(data::ResultCode::Numeric::kInternalError,
-                                            "Root metadata could not be fetched for Secondary with serial " +
-                                                secondary.getSerial().ToString() + ", skipping to the next Secondary");
-          break;
-        }
-      }
+    return {data::ResultCode::Numeric::kInternalError, "Secondary with serial " + secondary.getSerial().ToString() +
+                                                           " reported an invalid " + repo.ToString() +
+                                                           " repo Root version: " + std::to_string(sec_root_version)};
+  }
+
+  // Only send intermediate Roots that would otherwise be skipped. The latest
+  // will be sent with the complete set of the latest metadata.
+  for (int version_to_send = sec_root_version + 1; version_to_send < last_root_version; version_to_send++) {
+    std::string root;
+    if (!storage->loadRoot(&root, repo, Uptane::Version(version_to_send))) {
+      LOG_WARNING << "Couldn't find Root metadata in the storage, trying remote repo";
       try {
-        result = secondary.putRoot(root, repo == Uptane::RepositoryType::Director());
-      } catch (const std::exception &ex) {
-        result = data::InstallationResult(data::ResultCode::Numeric::kInternalError, ex.what());
+        uptane_fetcher->fetchRole(&root, Uptane::kMaxRootSize, repo, Uptane::Role::Root(),
+                                  Uptane::Version(version_to_send), flow_control_);
+      } catch (const std::exception &e) {
+        LOG_ERROR << "Root metadata could not be fetched for Secondary with serial " << secondary.getSerial()
+                  << ", skipping to the next Secondary";
+        return {data::ResultCode::Numeric::kInternalError,
+                "Root metadata could not be fetched for Secondary with serial " + secondary.getSerial().ToString() +
+                    ", skipping to the next Secondary"};
       }
+    }
+    try {
+      auto result = secondary.putRoot(root, repo == Uptane::RepositoryType::Director());
       if (!result.isSuccess()) {
+        // Old (pre 2024-07-XX) versions would assume that if sec_root_version
+        // is 0, either the Secondary doesn't have Root metadata or doesn't
+        // support the Root version request and skip sending any root metadata.
+        // Unfortunatately this cause TOR-3452 where an expired root metadata
+        // would cause updates to fail. Instead assume that '0' could mean 'I
+        // don't have any root versions yet'. If we send  version 1 and it is
+        // rejected, then assume we are in the case that the code originally was
+        // defending against: the secondary can't rotate root, and treat this
+        // as a success. The previous code would have returned success in this
+        // case anyway.
+        if (version_to_send == 1) {
+          LOG_WARNING
+              << "Sending root.1.json to a secondary failed. Assuming it doesn't allow root rotation and continuing.";
+          return {data::ResultCode::Numeric::kOk, ""};
+        }
         LOG_ERROR << "Sending Root metadata to Secondary with serial " << secondary.getSerial()
                   << " failed: " << result.result_code << " " << result.description;
-        break;
+        return result;
       }
+    } catch (const std::exception &ex) {
+      return {data::ResultCode::Numeric::kInternalError, ex.what()};
     }
   }
-  return result;
+  return {data::ResultCode::Numeric::kOk, ""};
 }
 
 // TODO: the function blocks until it updates all the Secondaries. Consider non-blocking operation.

src/libaktualizr/uptane/directorrepository.h

@@ -21,7 +21,7 @@ class DirectorRepository : public RepositoryCommon {
     return targets.getTargets(ecu_id, hw_id);
   }
   Uptane::CorrelationId getCorrelationId() const { return correlation_id_; }
-  void checkMetaOffline(INvStorage& storage);
+  void checkMetaOffline(INvStorage& storage) override;
   void dropTargets(INvStorage& storage);
 
   void updateMeta(INvStorage& storage, const IMetadataFetcher& fetcher,

src/libaktualizr/uptane/imagerepository.h

@@ -30,7 +30,7 @@ class ImageRepository : public RepositoryCommon {
   int getRoleVersion(const Uptane::Role& role) const;
   int64_t getRoleSize(const Uptane::Role& role) const;
 
-  void checkMetaOffline(INvStorage& storage);
+  void checkMetaOffline(INvStorage& storage) override;
   void updateMeta(INvStorage& storage, const IMetadataFetcher& fetcher,
                   const api::FlowControlToken* flow_control) override;
 

src/libaktualizr/uptane/tuf.h

@@ -234,7 +234,7 @@ class Root : public MetaWithKeys {
   /**
    * An empty Root, that either accepts or rejects everything
    */
-  explicit Root(Policy policy = Policy::kRejectAll) : policy_(policy) { version_ = 0; }
+  explicit Root(Policy policy) : policy_(policy) { version_ = 0; }
   /**
    * A 'real' Root that implements TUF signature validation
    * @param repo - Repository type (only used to improve the error messages)

src/libaktualizr/uptane/uptanerepository.h

@@ -1,9 +1,8 @@
 #ifndef UPTANE_REPOSITORY_H_
 #define UPTANE_REPOSITORY_H_
 
-#include <cstdint>  // for int64_t
-#include <string>   // for string
-#include "fetcher.h"
+#include <cstdint>               // for int64_t
+#include <string>                // for string
 #include "libaktualizr/types.h"  // for TimeStamp
 #include "uptane/tuf.h"          // for Root, RepositoryType
 #include "utilities/flow_control.h"
@@ -15,8 +14,7 @@ class IMetadataFetcher;
 
 class RepositoryCommon {
  public:
-  // NOLINTNEXTLINE(google-explicit-constructor, hicpp-explicit-conversions)
-  RepositoryCommon(RepositoryType type_in) : type{type_in} {}
+  explicit RepositoryCommon(RepositoryType type_in) : type{type_in} {}
   virtual ~RepositoryCommon() = default;
   RepositoryCommon(const RepositoryCommon &guard) = default;
   RepositoryCommon(RepositoryCommon &&) = default;
@@ -26,6 +24,13 @@ class RepositoryCommon {
   void verifyRoot(const std::string &root_raw);
   int rootVersion() const { return root.version(); }
   bool rootExpired() const { return root.isExpired(TimeStamp::Now()); }
+
+  /**
+   * Load the initial state of the repository from storage.
+   * Note that this _required_ for correct initialization.
+   * @throws UptaneException if the local metadata is stale (this is not a failure)
+   */
+  virtual void checkMetaOffline(INvStorage &storage) = 0;
   virtual void updateMeta(INvStorage &storage, const IMetadataFetcher &fetcher,
                           const api::FlowControlToken *flow_control) = 0;
 
@@ -35,7 +40,7 @@ class RepositoryCommon {
 
   static const int64_t kMaxRotations = 1000;
 
-  Root root;
+  Root root{Root::Policy::kRejectAll};
   RepositoryType type;
 };
 }  // namespace Uptane

src/uptane_generator/repo.cc

@@ -8,6 +8,7 @@
 #include "crypto/crypto.h"
 #include "director_repo.h"
 #include "image_repo.h"
+#include "json/json.h"
 #include "libaktualizr/campaign.h"
 
 Repo::Repo(Uptane::RepositoryType repo_type, boost::filesystem::path path, const std::string &expires,
@@ -288,7 +289,7 @@ void Repo::readKeys() {
   }
 }
 
-void Repo::refresh(const Uptane::Role &role) {
+void Repo::refresh(const Uptane::Role &role, const TimeStamp &expiry) {
   if (repo_type_ == Uptane::RepositoryType::Director() &&
       (role == Uptane::Role::Timestamp() || role == Uptane::Role::Snapshot())) {
     throw std::runtime_error("The " + role.ToString() + " in the Director repo is not currently supported.");
@@ -307,14 +308,14 @@ void Repo::refresh(const Uptane::Role &role) {
     throw std::runtime_error("Refreshing custom role " + role.ToString() + " is not currently supported.");
   }
 
-  // The only interesting part here is to increment the version. It could be
-  // interesting to allow changing the expiry, too.
   Json::Value meta_raw = Utils::parseJSONFile(meta_path)["signed"];
   const unsigned version = meta_raw["version"].asUInt() + 1;
 
   auto current_expire_time = TimeStamp(meta_raw["expires"].asString());
 
-  if (current_expire_time.IsExpiredAt(TimeStamp::Now())) {
+  if (expiry.IsValid()) {
+    meta_raw["expires"] = expiry.ToString();
+  } else if (current_expire_time.IsExpiredAt(TimeStamp::Now())) {
     time_t new_expiration_time;
     std::time(&new_expiration_time);
     new_expiration_time += 60L * 60;  // make it valid for the next hour
@@ -370,8 +371,9 @@ void Repo::rotate(const Uptane::Role &role, KeyType key_type) {
 
   // Sign Root with old and new key
   auto intermediate_meta = signTuf(role, meta_raw);
-  const std::string signed_meta = Utils::jsonToCanonicalStr(signTuf(old_key, intermediate_meta));
-  Utils::writeFile(meta_path, signed_meta);
+  auto signed_meta = signTuf(old_key, intermediate_meta);
+
+  Utils::writeFile(meta_path, Utils::jsonToCanonicalStr(signed_meta));
 
   std::stringstream root_name;
   root_name << version << ".root.json";

src/uptane_generator/repo.h

@@ -41,7 +41,7 @@ class Repo {
   Json::Value getTarget(const std::string &target_name);
   Json::Value signTuf(const Uptane::Role &role, const Json::Value &json);
   void generateCampaigns() const;
-  void refresh(const Uptane::Role &role);
+  void refresh(const Uptane::Role &role, const TimeStamp &expiry);
   void rotate(const Uptane::Role &role, KeyType key_type = KeyType::kRSA2048);
 
  protected:

src/uptane_generator/uptane_repo.cc

@@ -45,11 +45,11 @@ void UptaneRepo::emptyTargets() { director_repo_.emptyTargets(); }
 void UptaneRepo::oldTargets() { director_repo_.oldTargets(); }
 void UptaneRepo::generateCampaigns() { director_repo_.generateCampaigns(); }
 
-void UptaneRepo::refresh(Uptane::RepositoryType repo_type, const Uptane::Role &role) {
+void UptaneRepo::refresh(Uptane::RepositoryType repo_type, const Uptane::Role &role, const TimeStamp &expiry) {
   if (repo_type == Uptane::RepositoryType::Director()) {
-    director_repo_.refresh(role);
+    director_repo_.refresh(role, expiry);
   } else if (repo_type == Uptane::RepositoryType::Image()) {
-    image_repo_.refresh(role);
+    image_repo_.refresh(role, expiry);
   }
 }
 

src/uptane_generator/uptane_repo.h

@@ -23,7 +23,7 @@ class UptaneRepo {
   void emptyTargets();
   void oldTargets();
   void generateCampaigns();
-  void refresh(Uptane::RepositoryType repo_type, const Uptane::Role &role);
+  void refresh(Uptane::RepositoryType repo_type, const Uptane::Role &role, const TimeStamp &expiry = TimeStamp());
   void rotate(Uptane::RepositoryType repo_type, const Uptane::Role &role, KeyType key_type = KeyType::kRSA2048);
 
  private:

src/virtual_secondary/CMakeLists.txt

@@ -4,13 +4,19 @@ set(HEADERS managedsecondary.h virtualsecondary.h)
 
 set(TARGET virtual_secondary)
 
-add_library(${TARGET} STATIC
-  ${SOURCES}
-)
+add_library(virtual_secondary STATIC ${SOURCES})
 
 target_include_directories(${TARGET} PUBLIC ${PROJECT_SOURCE_DIR}/src/virtual_secondary)
 
-add_aktualizr_test(NAME virtual_secondary SOURCES virtual_secondary_test.cc PROJECT_WORKING_DIRECTORY LIBRARIES uptane_generator_lib)
-target_link_libraries(t_virtual_secondary virtual_secondary)
+add_aktualizr_test(NAME virtual_secondary
+                   SOURCES virtual_secondary_test.cc
+                   PROJECT_WORKING_DIRECTORY 
+                   LIBRARIES uptane_generator_lib virtual_secondary)
 
-aktualizr_source_file_checks(${HEADERS} ${SOURCES} ${TEST_SOURCES})
+add_aktualizr_test(NAME bad_rotation
+                   SOURCES bad_rotation_test.cc
+                   PROJECT_WORKING_DIRECTORY
+                   LIBRARIES uptane_generator_lib virtual_secondary)
+
+
+aktualizr_source_file_checks(${HEADERS} ${SOURCES} ${TEST_SOURCES} ${FUZZ_SECONDARY_SOURCES})