Remove HTTP from supported protocols in Downloader (#42)

* Remove HTTP from supported protocols in Downloader

* Fix review findings #1

* Fixed generation of self-signed certificate

* Fixed OCaaS check (disable scan for curl tests)

* Fix workflow (update of ca-certificates.conf)

* Restrict self-signed certificate for testing to localhost

* Fix review findings #2

* Fix OCaaS issues

* Create unavailable directory for self-signed testing certificate

* Fix OCaaS issues

* Interpret backslash in github workflow

* Fix path to crt file in apache2 configuration

* Pass certificates from host to arm64 guest and update ca-certificates internally

* Fix OCaaS issues

* Update ca-certificates in arm64 guest on run

* Fix OCaaS issues

.github/actions/build-native-binary/action.yaml

@@ -102,11 +102,13 @@ runs:
 
   - name: Generate SSL key and certificate
     run: |
+      sudo mkdir -p /usr/share/ca-certificates/extra
       sudo openssl req -x509 -nodes -days 365 -newkey rsa:2048 \
         -keyout /etc/ssl/private/selfupdateagent.key \
-        -out /etc/ssl/certs/selfupdateagent.crt \
-        -config utest/sua-certificate.config
-      sudo tee -a /etc/ssl/certs/ca-certificates.crt < /etc/ssl/certs/selfupdateagent.crt > /dev/null
+        -out /usr/share/ca-certificates/extra/selfupdateagent.crt \
+        -subj '/CN=localhost' -extensions EXT -config utest/sua-certificate.config
+      echo -e "\nextra/selfupdateagent.crt" | sudo tee -a /etc/ca-certificates.conf > /dev/null
+      sudo update-ca-certificates
     shell: bash
 
   - name: Install and configure apache2
@@ -145,13 +147,15 @@ runs:
       dockerRunArgs: |
         --volume "${PWD}:/sua"
         --volume "/data/selfupdates:/data/selfupdates"
-        --volume "/etc/ssl/certs:/etc/ssl/certs"
+        --volume "/usr/share/ca-certificates/extra:/usr/share/ca-certificates/extra"
         --net=host
       shell: /bin/sh
       install: |
         apt-get -y update
-        apt-get -y install mosquitto-clients
+        apt-get -y install mosquitto-clients ca-certificates
       run: |
+        echo "\nextra/selfupdateagent.crt" | tee -a /etc/ca-certificates.conf > /dev/null
+        update-ca-certificates
         cd /sua/dist_arm64/utest
         LD_LIBRARY_PATH=../lib ./TestSelfUpdateAgent > ../../unit_tests_report_arm64.txt
 

.github/workflows/main.yaml

@@ -80,7 +80,7 @@ jobs:
     runs-on: ubuntu-latest
     strategy:
       matrix:
-        arch: [ arm64, amd64 ]
+        arch: [ amd64, arm64 ]
     steps:
       - uses: actions/checkout@v3
         with:

.ort.yml

@@ -1,5 +1,8 @@
 excludes:
   paths:
+  - pattern: "project.spdx.yml"
+    reason: "OTHER"
+    comment: "Configuration for Open Source Scan"
   - pattern: "3rdparty/openssl/tlsfuzzer/**"
     reason: "TEST_OF"
     comment: "Test suite for SSL & TLS, not included in production release."
@@ -36,6 +39,9 @@ excludes:
   - pattern: "3rdparty/curl/docs/**"
     reason: "DOCUMENTATION_OF"
     comment: "Code examples, not included in production release."
+  - pattern: "3rdparty/curl/tests/**"
+    reason: "TEST_OF"
+    comment: "curl tests, not included in production release."
   - pattern: "3rdparty/glib/gio/tests/**"
     reason: "TEST_OF"
     comment: "Tests of glib, not included in production release."

src/Download/Downloader.cpp

@@ -24,6 +24,8 @@
 #include <cstdlib>
 #include <cstring>
 
+#include <tuple>
+
 #include <dirent.h>
 #include <curl/curl.h>
 #include <sys/stat.h>
@@ -82,18 +84,49 @@ namespace {
         return written;
     }
 
-    sua::TechCode download(const std::string & caPath, const std::string & caFile, const char* url)
+    class CurlGlobalGuard {
+    public:
+        CURLcode init() {
+            return curl_global_init(CURL_GLOBAL_ALL);
+        }
+
+        ~CurlGlobalGuard() {
+            curl_global_cleanup();
+        }
+    };
+
+    class CurlEasyHandleGuard {
+    public:
+        CURL * init() {
+            _handle = curl_easy_init();
+            return _handle;
+        }
+
+        ~CurlEasyHandleGuard() {
+            if(_handle) {
+                curl_easy_cleanup(_handle);
+            }
+        }
+
+    private:
+        CURL * _handle = nullptr;
+    };
+
+    sua::DownloadResult download(const std::string & caPath, const std::string & caFile, const char* url)
     {
-        CURLcode gres = curl_global_init(CURL_GLOBAL_ALL);
-        if(gres != 0) {
-            sua::Logger::critical("curl_global_init failed with code = {}", gres);
-            return sua::TechCode::DownloadFailed;
+        CurlGlobalGuard global_guard;
+        auto init_status = global_guard.init();
+        if(init_status != 0) {
+            sua::Logger::critical("curl_global_init failed with code = {}", init_status);
+            return std::make_tuple(sua::TechCode::DownloadFailed, "libcurl init failed");
         }
 
-        CURL* easy_handle = curl_easy_init();
-        if(!easy_handle) {
+        CurlEasyHandleGuard easy_guard;
+        auto h = easy_guard.init();
+
+        if(!h) {
             sua::Logger::critical("curl_easy_init failed");
-            return sua::TechCode::DownloadFailed;
+            return std::make_tuple(sua::TechCode::DownloadFailed, "libcurl init failed");
         }
 
         DIR* dir = opendir(FILE_DIR);
@@ -103,49 +136,50 @@ namespace {
             const int dir_err = mkdir(FILE_DIR, S_IRWXU | S_IRWXG | S_IROTH | S_IXOTH);
             if(dir_err) {
                 sua::Logger::critical("Issue with creating a dir {}, error: {}", FILE_DIR, dir_err);
-                return sua::TechCode::DownloadFailed;
+                return std::make_tuple(sua::TechCode::DownloadFailed, "temporary file folder could not be created");
             }
         }
 
         long response_code;
         FILE* fp = fopen(FILE_PATH, "wb");
         if(!fp) {
             sua::Logger::critical("Failed to open '{}' for writing", FILE_PATH);
-            return sua::TechCode::DownloadFailed;
+            return std::make_tuple(sua::TechCode::DownloadFailed, "temporary file could not be opened for writing");
         }
 
-        curl_easy_setopt(easy_handle, CURLOPT_URL, url);
-        curl_easy_getinfo(easy_handle, CURLINFO_RESPONSE_CODE, &response_code);
+        curl_easy_setopt(h, CURLOPT_URL, url);
+        curl_easy_getinfo(h, CURLINFO_RESPONSE_CODE, &response_code);
         if(!caFile.empty()) {
-            curl_easy_setopt(easy_handle, CURLOPT_CAINFO, caFile.c_str());
+            curl_easy_setopt(h, CURLOPT_CAINFO, caFile.c_str());
         } else {
-            curl_easy_setopt(easy_handle, CURLOPT_CAPATH, caPath.c_str());
+            curl_easy_setopt(h, CURLOPT_CAPATH, caPath.c_str());
         }
-        curl_easy_setopt(easy_handle, CURLOPT_SSL_VERIFYPEER, 1);
-        curl_easy_setopt(easy_handle, CURLOPT_WRITEFUNCTION, write_data);
-        curl_easy_setopt(easy_handle, CURLOPT_WRITEDATA, fp);
-        curl_easy_setopt(easy_handle, CURLOPT_FOLLOWLOCATION, 1L);
-        curl_easy_setopt(easy_handle, CURLOPT_PROGRESSDATA, &data);
-        curl_easy_setopt(easy_handle, CURLOPT_PROGRESSFUNCTION, progress_callback);
-        curl_easy_setopt(easy_handle, CURLOPT_NOPROGRESS, 0);
-        CURLcode res = curl_easy_perform(easy_handle);
+        curl_easy_setopt(h, CURLOPT_SSL_VERIFYPEER, 1L);
+        curl_easy_setopt(h, CURLOPT_SSL_VERIFYHOST, 2L);
+        curl_easy_setopt(h, CURLOPT_WRITEFUNCTION, write_data);
+        curl_easy_setopt(h, CURLOPT_WRITEDATA, fp);
+        curl_easy_setopt(h, CURLOPT_FOLLOWLOCATION, 1L);
+        curl_easy_setopt(h, CURLOPT_PROGRESSDATA, &data);
+        curl_easy_setopt(h, CURLOPT_PROGRESSFUNCTION, progress_callback);
+        curl_easy_setopt(h, CURLOPT_NOPROGRESS, 0);
+        curl_easy_setopt(h, CURLOPT_PROTOCOLS_STR, "https");
+        CURLcode res = curl_easy_perform(h);
 
         sua::Logger::debug("curl_easy_perform ended with code = '{}'", res);
 
         long http_code = 0;
-        curl_easy_getinfo(easy_handle, CURLINFO_RESPONSE_CODE, &http_code);
-        curl_easy_cleanup(easy_handle);
-        curl_global_cleanup();
+        curl_easy_getinfo(h, CURLINFO_RESPONSE_CODE, &http_code);
         fclose(fp);
         progressNotificationLimiter = 0;
 
         sua::Logger::debug("CURLINFO_RESPONSE_CODE = {}", http_code);
         if(http_code != 200) {
-            sua::Logger::error(curl_easy_strerror(res));
-            return sua::TechCode::DownloadFailed;
+            auto e = curl_easy_strerror(res);
+            sua::Logger::error(e);
+            return std::make_tuple(sua::TechCode::DownloadFailed, e);
         }
 
-        return sua::TechCode::OK;
+        return std::make_tuple(sua::TechCode::OK, "");
     }
 
 } // namespace
@@ -162,7 +196,7 @@ namespace sua {
         strncpy(FILE_PATH, filepath.c_str(), FILENAME_MAX - 1);
     }
 
-    TechCode Downloader::start(const std::string & input)
+    DownloadResult Downloader::start(const std::string & input)
     {
         return download(_context.caDirectory, _context.caFilepath, input.c_str());
     }

src/Download/Downloader.h

@@ -28,7 +28,7 @@ namespace sua {
 
         static const std::string EVENT_DOWNLOADING;
 
-        TechCode start(const std::string & input) override;
+        DownloadResult start(const std::string & input) override;
 
     private:
         class Context & _context;

src/Download/IDownloader.h

@@ -20,14 +20,17 @@
 #include "TechCodes.h"
 
 #include <string>
+#include <tuple>
 
 namespace sua {
 
+    using DownloadResult = std::tuple<TechCode, std::string>;
+
     class IDownloader {
     public:
         virtual ~IDownloader() = default;
 
-        virtual TechCode start(const std::string & input) = 0;
+        virtual DownloadResult start(const std::string & input) = 0;
     };
 
 } // namespace sua

src/FSM/FSM.cpp

@@ -41,6 +41,8 @@ namespace sua {
 
     std::string FSM::activeState() const
     {
+        assert(_currentState != nullptr);
+
         return _currentState->name();
     }
 

src/FSM/States/Downloading.cpp

@@ -57,7 +57,7 @@ namespace sua {
             Logger::info("Downloading bundle: '{}'", ctx.desiredState.bundleDownloadUrl);
             const auto result = ctx.downloaderAgent->start(ctx.desiredState.bundleDownloadUrl);
 
-            if(result == TechCode::OK) {
+            if(std::get<0>(result) == TechCode::OK) {
                 Logger::info("Download progress: 100%");
                 send(ctx, IMqttProcessor::TOPIC_FEEDBACK, MqttMessage::Downloaded);
 
@@ -67,9 +67,9 @@ namespace sua {
                 return FotaEvent::DownloadSucceeded;
             } else {
                 Logger::error("Download failed.");
-                send(ctx, IMqttProcessor::TOPIC_FEEDBACK, MqttMessage::DownloadFailed);
                 ctx.desiredState.actionStatus  = "DOWNLOAD_FAILURE";
-                ctx.desiredState.actionMessage = "Download failed.";
+                ctx.desiredState.actionMessage = "Download failed: " + std::get<1>(result);
+                send(ctx, IMqttProcessor::TOPIC_FEEDBACK, MqttMessage::DownloadFailed);
                 return FotaEvent::DownloadFailed;
             }
         } else {

src/Mqtt/MqttMessagingProtocolJSON.cpp

@@ -179,7 +179,7 @@ namespace sua {
         case MqttMessage::DownloadFailed:
             return writeFeedbackWithPayload(ctx.desiredState,
                 "DOWNLOAD_FAILURE", "Download failed.",
-                "DOWNLOAD_FAILURE", "Download failed.",
+                ctx.desiredState.actionStatus, ctx.desiredState.actionMessage,
                 message, ctx.desiredState.downloadProgressPercentage);
         case MqttMessage::VersionChecking:
             return writeFeedbackWithPayload(ctx.desiredState,

utest/CMakeLists.txt

@@ -1,5 +1,6 @@
 add_executable(TestSelfUpdateAgent
     TestDispatcher.cpp
+    TestDownloader.cpp
     TestFSM.cpp
     TestLogger.cpp
     TestMqttMessagingProtocolJSON.cpp

utest/MockDownloader.h

@@ -23,7 +23,7 @@
 
 class MockDownloader : public sua::IDownloader {
 public:
-    MOCK_METHOD(sua::TechCode, start, (const std::string & input), (override));
+    MOCK_METHOD(sua::DownloadResult, start, (const std::string & input), (override));
 };
 
 #endif

utest/TestDownloader.cpp

@@ -0,0 +1,35 @@
+#include "gtest/gtest.h"
+
+#include "Context.h"
+#include "Download/Downloader.h"
+
+#include <spdlog/fmt/fmt.h>
+
+namespace {
+
+    class TestDownloaderP
+        : public ::testing::TestWithParam<std::string>
+    { };
+
+    TEST_P(TestDownloaderP, downloadViaUnsupportedProtocolFails)
+    {
+        sua::Context ctx;
+        sua::Downloader d(ctx);
+
+        auto url = fmt::format("{}://127.0.0.1/bundle", GetParam());
+        auto res = d.start(url);
+
+        EXPECT_EQ(std::get<0>(res), sua::TechCode::DownloadFailed);
+    }
+
+    INSTANTIATE_TEST_CASE_P(
+        TestDownloaderViaUnsupportedProtocols,
+        TestDownloaderP,
+        ::testing::Values(
+            "ftp", "http", "sftp", "smb", "smbs", "ldap", "ldaps"
+        )
+    );
+
+}
+
+

utest/TestMqttMessagingProtocolJSON.cpp

@@ -596,6 +596,9 @@ namespace {
     {
         d.downloadProgressPercentage = 66;
 
+        ctx.desiredState.actionStatus  = "DOWNLOAD_FAILURE";
+        ctx.desiredState.actionMessage = "Download failed: test";
+
         const std::string result = ProtocolJSON().createMessage(ctx, sua::MqttMessage::DownloadFailed);
 
         // clang-format off
@@ -614,7 +617,7 @@ namespace {
                             },
                             "status": "DOWNLOAD_FAILURE",
                             "progress": 66,
-                            "message": "Download failed."
+                            "message": "Download failed: test"
                         }
                     ]
                 }