Merge pull request #1579 from advancedtelematic/fix/abort-if-already-…

…registered

Fix/abort if already registered

src/libaktualizr/primary/CMakeLists.txt

@@ -34,6 +34,8 @@ else (BUILD_OSTREE)
     aktualizr_source_file_checks(aktualizr_fullostree_test.cc download_nonostree_test.cc)
 endif (BUILD_OSTREE)
 
+add_aktualizr_test(NAME initializer SOURCES initializer_test.cc PROJECT_WORKING_DIRECTORY LIBRARIES PUBLIC uptane_generator_lib)
+
 add_aktualizr_test(NAME reportqueue SOURCES reportqueue_test.cc PROJECT_WORKING_DIRECTORY LIBRARIES PUBLIC uptane_generator_lib)
 add_aktualizr_test(NAME empty_targets SOURCES empty_targets_test.cc PROJECT_WORKING_DIRECTORY
                    ARGS "$<TARGET_FILE:uptane-generator>" LIBRARIES uptane_generator_lib)

src/libaktualizr/primary/initializer.cc

@@ -26,14 +26,15 @@ bool Initializer::initDeviceId() {
     } else if (config_.mode == ProvisionMode::kDeviceCred) {
       device_id = keys_.getCN();
     } else {
-      LOG_ERROR << "Unknown provisioning method";
+      LOG_ERROR << "Unknown provisioning method.";
       return false;
     }
   }
 
   storage_->storeDeviceId(device_id);
   return true;
 }
+
 void Initializer::resetDeviceId() { storage_->clearDeviceId(); }
 
 // Postcondition [(serial, hw_id)] is in the storage
@@ -89,7 +90,7 @@ InitRetCode Initializer::initTlsCreds() {
   }
 
   if (config_.mode != ProvisionMode::kSharedCred) {
-    LOG_ERROR << "Credentials not found";
+    LOG_ERROR << "Shared credentials expected but not found.";
     return InitRetCode::kStorageFailure;
   }
 
@@ -104,7 +105,7 @@ InitRetCode Initializer::initTlsCreds() {
   Json::Value data;
   std::string device_id;
   if (!storage_->loadDeviceId(&device_id)) {
-    LOG_ERROR << "Unknown device_id during shared credential provisioning.";
+    LOG_ERROR << "Unable to load device_id during shared credential provisioning.";
     return InitRetCode::kStorageFailure;
   }
   data["deviceId"] = device_id;
@@ -113,10 +114,10 @@ InitRetCode Initializer::initTlsCreds() {
   if (!response.isOk()) {
     Json::Value resp_code = response.getJson()["code"];
     if (resp_code.isString() && resp_code.asString() == "device_already_registered") {
-      LOG_ERROR << "Device id" << device_id << "is occupied";
+      LOG_ERROR << "Device ID " << device_id << " is already registered.";
       return InitRetCode::kOccupied;
     }
-    LOG_ERROR << "Shared credential provisioning failed, response: " << response.body;
+    LOG_ERROR << "Shared credential provisioning failed: " << response.http_status_code << " " << response.body;
     return InitRetCode::kServerFailure;
   }
 
@@ -126,18 +127,18 @@ InitRetCode Initializer::initTlsCreds() {
   StructGuard<BIO> device_p12(BIO_new_mem_buf(response.body.c_str(), static_cast<int>(response.body.size())),
                               BIO_vfree);
   if (!Crypto::parseP12(device_p12.get(), "", &pkey, &cert, &ca)) {
-    LOG_ERROR << "Received a malformed P12 package from the server";
+    LOG_ERROR << "Received malformed device credentials from the server,.";
     return InitRetCode::kBadP12;
   }
   storage_->storeTlsCreds(ca, cert, pkey);
 
   // set provisioned credentials
   if (!loadSetTlsCreds()) {
-    LOG_ERROR << "Failed to set provisioned credentials";
+    LOG_ERROR << "Failed to set provisioned device credentials.";
     return InitRetCode::kStorageFailure;
   }
 
-  LOG_INFO << "Provisioned successfully on Device Gateway";
+  LOG_INFO << "Provisioned successfully on Device Gateway.";
   return InitRetCode::kOk;
 }
 
@@ -199,12 +200,12 @@ InitRetCode Initializer::initEcuRegister() {
       LOG_ERROR << "One or more ECUs are unexpectedly already registered.";
       return InitRetCode::kOccupied;
     }
-    LOG_ERROR << "Error registering device on Uptane, response: " << response.body;
+    LOG_ERROR << "Error registering device: " << response.http_status_code << " " << response.body;
     return InitRetCode::kServerFailure;
   }
   // do not call storage_->storeEcuRegistered(), it will be called from the top-level Init function after the
   // acknowledgement
-  LOG_INFO << "ECUs have been successfully registered to the server.";
+  LOG_INFO << "ECUs have been successfully registered with the server.";
   return InitRetCode::kOk;
 }
 
@@ -246,7 +247,7 @@ Initializer::Initializer(const ProvisionConfig& config_in, std::shared_ptr<INvSt
     // generate a new one
     if (ret_code == InitRetCode::kOccupied) {
       resetDeviceId();
-      LOG_INFO << "Device name is already registered. Restarting.";
+      LOG_ERROR << "Device name is already registered. Retrying.";
       continue;
     } else if (ret_code == InitRetCode::kStorageFailure) {
       LOG_ERROR << "Error reading existing provisioning data from storage.";
@@ -269,17 +270,11 @@ Initializer::Initializer(const ProvisionConfig& config_in, std::shared_ptr<INvSt
       return;
     }
 
-    ret_code = initEcuRegister();
-    // if ECUs with same ID have been registered to the server, we don't have a
-    // clear remediation path right now, just ignore the error
-    if (ret_code == InitRetCode::kOccupied) {
-      LOG_INFO << "ECU serial is already registered.";
-    } else if (ret_code != InitRetCode::kOk) {
+    if (initEcuRegister() != InitRetCode::kOk) {
       LOG_ERROR << "ECU registration failed. Aborting initialization.";
       return;
     }
 
-    // TODO: acknowledge on server _before_ setting the flag
     storage_->storeEcuRegistered();
     success_ = true;
     return;

src/libaktualizr/uptane/uptane_init_test.cc → src/libaktualizr/primary/initializer_test.cc

@@ -13,7 +13,7 @@
 /*
  * Check that aktualizr creates provisioning data if they don't exist already.
  */
-TEST(Uptane, Initialize) {
+TEST(Initializer, Success) {
   RecordProperty("zephyr_key", "OTA-983,TST-153");
   TemporaryDirectory temp_dir;
   auto http = std::make_shared<HttpFake>(temp_dir.Path());
@@ -60,7 +60,7 @@ TEST(Uptane, Initialize) {
  * Check that aktualizr does NOT change provisioning data if they DO exist
  * already.
  */
-TEST(Uptane, InitializeTwice) {
+TEST(Initializer, InitializeTwice) {
   RecordProperty("zephyr_key", "OTA-983,TST-154");
   TemporaryDirectory temp_dir;
   auto http = std::make_shared<HttpFake>(temp_dir.Path());
@@ -119,7 +119,7 @@ TEST(Uptane, InitializeTwice) {
  * Check that aktualizr does not generate a pet name when device ID is
  * specified.
  */
-TEST(Uptane, PetNameProvided) {
+TEST(Initializer, PetNameProvided) {
   RecordProperty("zephyr_key", "OTA-985,TST-146");
   TemporaryDirectory temp_dir;
   const std::string test_name = "test-name-123";
@@ -155,7 +155,7 @@ TEST(Uptane, PetNameProvided) {
 /**
  * Check that aktualizr generates a pet name if no device ID is specified.
  */
-TEST(Uptane, PetNameCreation) {
+TEST(Initializer, PetNameCreation) {
   RecordProperty("zephyr_key", "OTA-985,TST-145");
   TemporaryDirectory temp_dir;
 
@@ -232,10 +232,90 @@ TEST(Uptane, PetNameCreation) {
   }
 }
 
-/* Detect and recover from failed provisioning. */
-TEST(Uptane, InitializeFail) {
+class HttpFakeDeviceRegistration : public HttpFake {
+ public:
+  HttpFakeDeviceRegistration(const boost::filesystem::path& test_dir_in) : HttpFake(test_dir_in) {}
+
+  HttpResponse post(const std::string& url, const Json::Value& data) override {
+    if (url.find("/devices") != std::string::npos) {
+      if (retcode == InitRetCode::kOk) {
+        return HttpResponse(Utils::readFile("tests/test_data/cred.p12"), 200, CURLE_OK, "");
+      } else if (retcode == InitRetCode::kOccupied) {
+        Json::Value response;
+        response["code"] = "device_already_registered";
+        return HttpResponse(Utils::jsonToStr(response), 400, CURLE_OK, "");
+      } else {
+        return HttpResponse("", 400, CURLE_OK, "");
+      }
+    }
+    return HttpFake::post(url, data);
+  }
+
+  InitRetCode retcode{InitRetCode::kOk};
+};
+
+/* Detect and recover from failed device provisioning. */
+TEST(Initializer, DeviceRegistration) {
   TemporaryDirectory temp_dir;
-  auto http = std::make_shared<HttpFake>(temp_dir.Path());
+  auto http = std::make_shared<HttpFakeDeviceRegistration>(temp_dir.Path());
+  Config conf("tests/config/basic.toml");
+  conf.uptane.director_server = http->tls_server + "/director";
+  conf.uptane.repo_server = http->tls_server + "/repo";
+  conf.tls.server = http->tls_server;
+  conf.storage.path = temp_dir.Path();
+  conf.provision.primary_ecu_serial = "testecuserial";
+
+  auto storage = INvStorage::newStorage(conf.storage);
+  KeyManager keys(storage, conf.keymanagerConfig());
+
+  // Force a failure from the fake server due to device already registered.
+  {
+    http->retcode = InitRetCode::kOccupied;
+    Initializer initializer(conf.provision, storage, http, keys, {});
+    EXPECT_FALSE(initializer.isSuccessful());
+  }
+
+  // Force an arbitrary failure from the fake server.
+  {
+    http->retcode = InitRetCode::kServerFailure;
+    Initializer initializer(conf.provision, storage, http, keys, {});
+    EXPECT_FALSE(initializer.isSuccessful());
+  }
+
+  // Don't force a failure and make sure it actually works this time.
+  {
+    http->retcode = InitRetCode::kOk;
+    Initializer initializer(conf.provision, storage, http, keys, {});
+    EXPECT_TRUE(initializer.isSuccessful());
+  }
+}
+
+class HttpFakeEcuRegistration : public HttpFake {
+ public:
+  HttpFakeEcuRegistration(const boost::filesystem::path& test_dir_in) : HttpFake(test_dir_in) {}
+
+  HttpResponse post(const std::string& url, const Json::Value& data) override {
+    if (url.find("/director/ecus") != std::string::npos) {
+      if (retcode == InitRetCode::kOk) {
+        return HttpResponse("", 200, CURLE_OK, "");
+      } else if (retcode == InitRetCode::kOccupied) {
+        Json::Value response;
+        response["code"] = "ecu_already_registered";
+        return HttpResponse(Utils::jsonToStr(response), 400, CURLE_OK, "");
+      } else {
+        return HttpResponse("", 400, CURLE_OK, "");
+      }
+    }
+    return HttpFake::post(url, data);
+  }
+
+  InitRetCode retcode{InitRetCode::kOk};
+};
+
+/* Detect and recover from failed ECU registration. */
+TEST(Initializer, EcuRegisteration) {
+  TemporaryDirectory temp_dir;
+  auto http = std::make_shared<HttpFakeEcuRegistration>(temp_dir.Path());
   Config conf("tests/config/basic.toml");
   conf.uptane.director_server = http->tls_server + "/director";
   conf.uptane.repo_server = http->tls_server + "/repo";
@@ -246,16 +326,23 @@ TEST(Uptane, InitializeFail) {
   auto storage = INvStorage::newStorage(conf.storage);
   KeyManager keys(storage, conf.keymanagerConfig());
 
-  // Force a failure from the fake server.
+  // Force a failure from the fake server due to ECUs already registered.
+  {
+    http->retcode = InitRetCode::kOccupied;
+    Initializer initializer(conf.provision, storage, http, keys, {});
+    EXPECT_FALSE(initializer.isSuccessful());
+  }
+
+  // Force an arbitary failure from the fake server.
   {
-    http->provisioningResponse = ProvisioningResult::kFailure;
+    http->retcode = InitRetCode::kServerFailure;
     Initializer initializer(conf.provision, storage, http, keys, {});
     EXPECT_FALSE(initializer.isSuccessful());
   }
 
   // Don't force a failure and make sure it actually works this time.
   {
-    http->provisioningResponse = ProvisioningResult::kOK;
+    http->retcode = InitRetCode::kOk;
     Initializer initializer(conf.provision, storage, http, keys, {});
     EXPECT_TRUE(initializer.isSuccessful());
   }
@@ -269,7 +356,7 @@ TEST(Uptane, InitializeFail) {
  *
  * - [x] Use the system hostname as hardware ID if one is not provided
  */
-TEST(Uptane, HostnameAsHardwareID) {
+TEST(Initializer, HostnameAsHardwareID) {
   TemporaryDirectory temp_dir;
   Config conf("tests/config/basic.toml");
   conf.storage.path = temp_dir.Path();

src/libaktualizr/uptane/CMakeLists.txt

@@ -61,8 +61,6 @@ add_aktualizr_test(NAME uptane_serial SOURCES uptane_serial_test.cc ARGS ${PROJE
                    PROJECT_WORKING_DIRECTORY LIBRARIES uptane_generator_lib)
 target_link_libraries(t_uptane_serial virtual_secondary)
 
-add_aktualizr_test(NAME uptane_init SOURCES uptane_init_test.cc PROJECT_WORKING_DIRECTORY LIBRARIES PUBLIC uptane_generator_lib)
-
 add_aktualizr_test(NAME director SOURCES director_test.cc PROJECT_WORKING_DIRECTORY
                    ARGS "$<TARGET_FILE:uptane-generator>")
 

tests/httpfake.h

@@ -17,8 +17,6 @@
 #include "metafake.h"
 #include "utilities/utils.h"
 
-enum class ProvisioningResult { kOK, kFailure };
-
 class HttpFake : public HttpInterface {
  public:
   // old style HttpFake with centralized multi repo and url rewriting
@@ -102,17 +100,11 @@ class HttpFake : public HttpInterface {
 
   HttpResponse post(const std::string &url, const Json::Value &data) override {
     if (url.find("/devices") != std::string::npos || url.find("/director/ecus") != std::string::npos || url.empty()) {
-      LOG_ERROR << "OK create device";
       Utils::writeFile((test_dir / "post.json").string(), data);
-      if (provisioningResponse == ProvisioningResult::kOK) {
-        return HttpResponse(Utils::readFile("tests/test_data/cred.p12"), 200, CURLE_OK, "");
-      } else {
-        return HttpResponse("", 400, CURLE_OK, "");
-      }
+      return HttpResponse(Utils::readFile("tests/test_data/cred.p12"), 200, CURLE_OK, "");
     } else if (url.find("/events") != std::string::npos) {
       return handle_event(url, data);
     }
-
     return HttpResponse("", 400, CURLE_OK, "");
   }
 
@@ -166,7 +158,6 @@ class HttpFake : public HttpInterface {
   }
 
   const std::string tls_server = "https://tlsserver.com";
-  ProvisioningResult provisioningResponse{ProvisioningResult::kOK};
   Json::Value last_manifest;
 
  protected: