From 0fb652239b152c758c97acf33ce573b825da3daa Mon Sep 17 00:00:00 2001 From: JohnMcPMS Date: Sat, 23 Sep 2023 21:26:45 -0700 Subject: [PATCH] Improve packaged source updating (#3657) This change refactors the index package updating to enable an optimization; using an HTTP header to determine the available version rather than reading the package contents remotely. If the header is present, containing a valid package version string, it will be used as the available version. If not, the existing package content examination will be used. The refactoring was required to do all package inspection steps after downloading the package, rather than before. This enables the single version value to be sufficient until we decide to update, and then in the very unlikely event of the package not meeting criteria we will delete it after having downloaded it. --- .github/actions/spelling/expect.txt | 1 + src/AppInstallerCommonCore/Downloader.cpp | 35 +- .../Public/AppInstallerDownloader.h | 4 + .../PreIndexedPackageSourceFactory.cpp | 301 ++++++++++++------ .../Public/AppInstallerVersions.h | 2 + src/AppInstallerSharedLib/Versions.cpp | 20 +- 6 files changed, 256 insertions(+), 107 deletions(-) diff --git a/.github/actions/spelling/expect.txt b/.github/actions/spelling/expect.txt index a04de5e310..748e0345d1 100644 --- a/.github/actions/spelling/expect.txt +++ b/.github/actions/spelling/expect.txt @@ -429,6 +429,7 @@ SMTO sortof sourceforge SOURCESDIRECTORY +sourceversion spamming SPAPI Srinivasan diff --git a/src/AppInstallerCommonCore/Downloader.cpp b/src/AppInstallerCommonCore/Downloader.cpp index cb22039a53..d69ee1d28e 100644 --- a/src/AppInstallerCommonCore/Downloader.cpp +++ b/src/AppInstallerCommonCore/Downloader.cpp @@ -1,6 +1,5 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. - #include "pch.h" #include "Public/AppInstallerErrors.h" #include "Public/AppInstallerRuntime.h" @@ -17,6 +16,9 @@ using namespace AppInstaller::Runtime; using namespace AppInstaller::Settings; using namespace AppInstaller::Filesystem; +using namespace winrt::Windows::Web::Http; +using namespace winrt::Windows::Web::Http::Headers; +using namespace winrt::Windows::Web::Http::Filters; namespace AppInstaller::Utility { @@ -141,6 +143,37 @@ namespace AppInstaller::Utility return result; } + std::map GetHeaders(std::string_view url) + { + AICLI_LOG(Core, Verbose, << "Retrieving headers from url: " << url); + + HttpBaseProtocolFilter filter; + filter.CacheControl().ReadBehavior(HttpCacheReadBehavior::MostRecent); + + HttpClient client(filter); + client.DefaultRequestHeaders().Connection().Clear(); + client.DefaultRequestHeaders().Append(L"Connection", L"close"); + client.DefaultRequestHeaders().UserAgent().ParseAdd(Utility::ConvertToUTF16(Runtime::GetDefaultUserAgent().get())); + + winrt::Windows::Foundation::Uri uri{ Utility::ConvertToUTF16(url) }; + HttpRequestMessage request(HttpMethod::Head(), uri); + + HttpResponseMessage response = client.SendRequestAsync(request, HttpCompletionOption::ResponseHeadersRead).get(); + + THROW_HR_IF( + MAKE_HRESULT(SEVERITY_ERROR, FACILITY_HTTP, response.StatusCode()), + response.StatusCode() != HttpStatusCode::Ok); + + std::map result; + + for (const auto& header : response.Headers()) + { + result.emplace(Utility::ConvertToUTF8(header.Key()), Utility::ConvertToUTF8(header.Value())); + } + + return result; + } + std::optional> DownloadToStream( const std::string& url, std::ostream& dest, diff --git a/src/AppInstallerCommonCore/Public/AppInstallerDownloader.h b/src/AppInstallerCommonCore/Public/AppInstallerDownloader.h index 9b96d18504..8434c6e981 100644 --- a/src/AppInstallerCommonCore/Public/AppInstallerDownloader.h +++ b/src/AppInstallerCommonCore/Public/AppInstallerDownloader.h @@ -7,6 +7,7 @@ #include #include +#include #include #include #include @@ -59,6 +60,9 @@ namespace AppInstaller::Utility bool computeHash = false, std::optional info = {}); + // Gets the headers for the given URL. + std::map GetHeaders(std::string_view url); + // Determines if the given url is a remote location. bool IsUrlRemote(std::string_view url); diff --git a/src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp b/src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp index b7952d7ea6..56af8f95fa 100644 --- a/src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp +++ b/src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp @@ -6,6 +6,7 @@ #include "Microsoft/SQLiteIndexSource.h" #include +#include #include #include @@ -17,20 +18,48 @@ namespace AppInstaller::Repository::Microsoft namespace { static constexpr std::string_view s_PreIndexedPackageSourceFactory_PackageFileName = "source.msix"sv; + static constexpr std::string_view s_PreIndexedPackageSourceFactory_PackageVersionHeader = "x-ms-meta-sourceversion"sv; static constexpr std::string_view s_PreIndexedPackageSourceFactory_IndexFileName = "index.db"sv; // TODO: This being hard coded to force using the Public directory name is not ideal. static constexpr std::string_view s_PreIndexedPackageSourceFactory_IndexFilePath = "Public\\index.db"sv; + // Construct the package location from the given details. + // Currently expects that the arg is an https uri pointing to the root of the data. + std::string GetPackageLocation(const std::string& basePath, std::string_view fileName) + { + std::string result = basePath; + if (result.back() != '/') + { + result += '/'; + } + result += fileName; + return result; + } + + std::string GetPrimaryPackageLocation(const SourceDetails& details, std::string_view fileName) + { + THROW_HR_IF(E_INVALIDARG, details.Arg.empty()); + return GetPackageLocation(details.Arg, fileName); + } + + std::string GetAlternatePackageLocation(const SourceDetails& details, std::string_view fileName) + { + return (details.AlternateArg.empty() ? + std::string{} : + GetPackageLocation(details.AlternateArg, fileName)); + } + + // Abstracts the fallback for package location when the MsixInfo is needed. struct PreIndexedPackageInfo { template PreIndexedPackageInfo(const SourceDetails& details, LocationCheck&& locationCheck) { // Get both locations to force the alternate location check - m_packageLocation = GetPrimaryPackageLocation(details); + m_packageLocation = GetPrimaryPackageLocation(details, s_PreIndexedPackageSourceFactory_PackageFileName); locationCheck(m_packageLocation); - std::string alternateLocation = GetAlternatePackageLocation(details); + std::string alternateLocation = GetAlternatePackageLocation(details, s_PreIndexedPackageSourceFactory_PackageFileName); if (!alternateLocation.empty()) { locationCheck(alternateLocation); @@ -72,29 +101,78 @@ namespace AppInstaller::Repository::Microsoft private: std::string m_packageLocation; std::unique_ptr m_msixInfo; + }; - std::string GetPrimaryPackageLocation(const SourceDetails& details) + // Abstracts the fallback for package location when an update is being done. + struct PreIndexedPackageUpdateCheck + { + PreIndexedPackageUpdateCheck(const SourceDetails& details) { - THROW_HR_IF(E_INVALIDARG, details.Arg.empty()); - return GetPackageLocation(details.Arg); - } + m_packageLocation = GetPrimaryPackageLocation(details, s_PreIndexedPackageSourceFactory_PackageFileName); + std::string alternateLocation = GetAlternatePackageLocation(details, s_PreIndexedPackageSourceFactory_PackageFileName); - std::string GetAlternatePackageLocation(const SourceDetails& details) - { - return (details.AlternateArg.empty() ? std::string{} : GetPackageLocation(details.AlternateArg)); + // Try getting the primary location's info + HRESULT primaryHR = S_OK; + + try + { + m_availableVersion = GetAvailableVersionFrom(m_packageLocation); + return; + } + catch (...) + { + if (alternateLocation.empty()) + { + throw; + } + primaryHR = LOG_CAUGHT_EXCEPTION_MSG("PreIndexedPackageUpdateCheck failed on primary location"); + } + + // Try alternate location + m_packageLocation = std::move(alternateLocation); + + try + { + m_availableVersion = GetAvailableVersionFrom(m_packageLocation); + return; + } + CATCH_LOG_MSG("PreIndexedPackageUpdateCheck failed on alternate location"); + + THROW_HR(primaryHR); } - // Construct the package location from the given details. - // Currently expects that the arg is an https uri pointing to the root of the data. - std::string GetPackageLocation(const std::string& basePath) + const std::string& PackageLocation() const { return m_packageLocation; } + const Msix::PackageVersion& AvailableVersion() const { return m_availableVersion; } + + private: + std::string m_packageLocation; + Msix::PackageVersion m_availableVersion; + + Msix::PackageVersion GetAvailableVersionFrom(const std::string& packageLocation) { - std::string result = basePath; - if (result.back() != '/') + if (Utility::IsUrlRemote(packageLocation)) { - result += '/'; + try + { + std::map headers = Utility::GetHeaders(packageLocation); + auto itr = headers.find(std::string{ s_PreIndexedPackageSourceFactory_PackageVersionHeader }); + if (itr != headers.end()) + { + AICLI_LOG(Repo, Verbose, << "Header indicates version is: " << itr->second); + return { itr->second }; + } + } + CATCH_LOG(); } - result += s_PreIndexedPackageSourceFactory_PackageFileName; - return result; + + AICLI_LOG(Repo, Verbose, << "No version header, falling back to reading the package data"); + Msix::MsixInfo info{ packageLocation }; + auto manifest = info.GetAppPackageManifests(); + + THROW_HR_IF(APPINSTALLER_CLI_ERROR_PACKAGE_IS_BUNDLE, manifest.size() > 1); + THROW_HR_IF(E_UNEXPECTED, manifest.size() == 0); + + return manifest[0].GetIdentity().GetVersion(); } }; @@ -164,7 +242,7 @@ namespace AppInstaller::Repository::Microsoft return false; } - return UpdateInternal(packageInfo.PackageLocation(), packageInfo.MsixInfo(), details, progress); + return UpdateInternal(packageInfo.PackageLocation(), details, progress); } bool Update(const SourceDetails& details, IProgressCallback& progress) override final @@ -177,7 +255,10 @@ namespace AppInstaller::Repository::Microsoft return UpdateBase(details, true, progress); } - virtual bool UpdateInternal(const std::string& packageLocation, Msix::MsixInfo& packageInfo, const SourceDetails& details, IProgressCallback& progress) = 0; + // Retrieves the currently cached version of the package. + virtual std::optional GetCurrentVersion(const SourceDetails& details) = 0; + + virtual bool UpdateInternal(const std::string& packageLocation, const SourceDetails& details, IProgressCallback& progress) = 0; bool Remove(const SourceDetails& details, IProgressCallback& progress) override final { @@ -215,14 +296,23 @@ namespace AppInstaller::Repository::Microsoft { THROW_HR_IF(E_INVALIDARG, details.Type != PreIndexedPackageSourceFactory::Type()); - PreIndexedPackageInfo packageInfo(details, [](const std::string&){}); + std::optional currentVersion = GetCurrentVersion(details); + PreIndexedPackageUpdateCheck updateCheck(details); - // The package should not be a bundle - THROW_HR_IF(APPINSTALLER_CLI_ERROR_PACKAGE_IS_BUNDLE, packageInfo.MsixInfo().GetIsBundle()); - - // Ensure that family name has not changed - THROW_HR_IF(APPINSTALLER_CLI_ERROR_SOURCE_DATA_INTEGRITY_FAILURE, - GetPackageFamilyNameFromDetails(details) != Msix::GetPackageFamilyNameFromFullName(packageInfo.MsixInfo().GetPackageFullName())); + if (currentVersion) + { + if (currentVersion.value() >= updateCheck.AvailableVersion()) + { + AICLI_LOG(Repo, Verbose, << "Remote source data (" << updateCheck.AvailableVersion().ToString() << + ") was not newer than existing (" << currentVersion.value().ToString() << "), no update needed"); + return true; + } + else + { + AICLI_LOG(Repo, Verbose, << "Remote source data (" << updateCheck.AvailableVersion().ToString() << + ") was newer than existing (" << currentVersion.value().ToString() << "), updating"); + } + } if (progress.IsCancelledBy(CancelReason::Any)) { @@ -236,7 +326,7 @@ namespace AppInstaller::Repository::Microsoft return false; } - return UpdateInternal(packageInfo.PackageLocation(), packageInfo.MsixInfo(), details, progress); + return UpdateInternal(updateCheck.PackageLocation(), details, progress); } }; @@ -305,46 +395,58 @@ namespace AppInstaller::Repository::Microsoft return std::make_shared(details); } - bool UpdateInternal(const std::string& packageLocation, Msix::MsixInfo& packageInfo, const SourceDetails& details, IProgressCallback& progress) override + std::optional GetCurrentVersion(const SourceDetails& details) override { - // Check if the package is newer before calling into deployment. - // This can save us a lot of time over letting deployment detect same version. auto extension = GetExtensionFromDetails(details); + if (extension) { - if (!packageInfo.IsNewerThan(extension->GetPackageVersion())) - { - AICLI_LOG(Repo, Info, << "Remote source data was not newer than existing, no update needed"); - return true; - } + auto version = extension->GetPackageVersion(); + return Msix::PackageVersion{ version.Major, version.Minor, version.Build, version.Revision }; } - - if (progress.IsCancelledBy(CancelReason::Any)) + else { - AICLI_LOG(Repo, Info, << "Cancelling update upon request"); - return false; + return std::nullopt; } + } + bool UpdateInternal(const std::string& packageLocation, const SourceDetails& details, IProgressCallback& progress) override + { // Due to complications with deployment, download the file and deploy from // a local source while we investigate further. bool download = Utility::IsUrlRemote(packageLocation); - std::filesystem::path tempFile; - winrt::Windows::Foundation::Uri uri = nullptr; + std::filesystem::path localFile; if (download) { - tempFile = Runtime::GetPathTo(Runtime::PathName::Temp); - tempFile /= GetPackageFamilyNameFromDetails(details) + ".msix"; - - Utility::Download(packageLocation, tempFile, Utility::DownloadType::Index, progress); + localFile = Runtime::GetPathTo(Runtime::PathName::Temp); + localFile /= GetPackageFamilyNameFromDetails(details) + ".msix"; - uri = winrt::Windows::Foundation::Uri(tempFile.c_str()); + Utility::Download(packageLocation, localFile, Utility::DownloadType::Index, progress); } else { - uri = winrt::Windows::Foundation::Uri(Utility::ConvertToUTF16(packageLocation)); + localFile = Utility::ConvertToUTF16(packageLocation); } + // Verify the local file + Msix::WriteLockedMsixFile fileLock{ localFile }; + Msix::MsixInfo localMsixInfo{ localFile }; + + // The package should not be a bundle + THROW_HR_IF(APPINSTALLER_CLI_ERROR_PACKAGE_IS_BUNDLE, localMsixInfo.GetIsBundle()); + + // Ensure that family name has not changed + THROW_HR_IF(APPINSTALLER_CLI_ERROR_SOURCE_DATA_INTEGRITY_FAILURE, + GetPackageFamilyNameFromDetails(details) != Msix::GetPackageFamilyNameFromFullName(localMsixInfo.GetPackageFullName())); + + if (!fileLock.ValidateTrustInfo(WI_IsFlagSet(details.TrustLevel, SourceTrustLevel::StoreOrigin))) + { + AICLI_LOG(Repo, Error, << "Source update failed. Source package failed trust validation."); + THROW_HR(APPINSTALLER_CLI_ERROR_SOURCE_DATA_INTEGRITY_FAILURE); + } + + winrt::Windows::Foundation::Uri uri = winrt::Windows::Foundation::Uri(localFile.c_str()); Deployment::AddPackage( uri, winrt::Windows::Management::Deployment::DeploymentOptions::None, @@ -356,29 +458,11 @@ namespace AppInstaller::Repository::Microsoft try { // If successful, delete the file - std::filesystem::remove(tempFile); + std::filesystem::remove(localFile); } CATCH_LOG(); } - // Ensure origin if necessary - // TODO: Move to checking this before deploying it. That requires significant code to be written though - // as there is no public API to check the origin directly. - if (WI_IsFlagSet(details.TrustLevel, SourceTrustLevel::StoreOrigin)) - { - std::wstring pfn = packageInfo.GetPackageFullNameWide(); - - PackageOrigin origin = PackageOrigin::PackageOrigin_Unknown; - if (SUCCEEDED_WIN32_LOG(GetStagedPackageOrigin(pfn.c_str(), &origin))) - { - if (origin != PackageOrigin::PackageOrigin_Store) - { - Deployment::RemovePackage(Utility::ConvertToUTF8(pfn), winrt::Windows::Management::Deployment::RemovalOptions::None, progress); - THROW_HR(APPINSTALLER_CLI_ERROR_SOURCE_DATA_INTEGRITY_FAILURE); - } - } - } - return true; } @@ -480,27 +564,51 @@ namespace AppInstaller::Repository::Microsoft return std::make_shared(details); } - bool UpdateInternal(const std::string& packageLocation, Msix::MsixInfo& packageInfo, const SourceDetails& details, IProgressCallback& progress) override + std::optional GetCurrentVersion(const SourceDetails& details) override { - // We will extract the manifest and index files directly to this location std::filesystem::path packageState = GetStatePathFromDetails(details); - std::filesystem::create_directories(packageState); - std::filesystem::path packagePath = packageState / s_PreIndexedPackageSourceFactory_PackageFileName; if (std::filesystem::exists(packagePath)) { // If we already have a trusted index package, use it to determine if we need to update or not. Msix::WriteLockedMsixFile indexPackage{ packagePath }; - if (indexPackage.ValidateTrustInfo(WI_IsFlagSet(details.TrustLevel, SourceTrustLevel::StoreOrigin)) && - !packageInfo.IsNewerThan(packagePath)) + if (indexPackage.ValidateTrustInfo(WI_IsFlagSet(details.TrustLevel, SourceTrustLevel::StoreOrigin))) { - AICLI_LOG(Repo, Info, << "Remote source data was not newer than existing, no update needed"); - return true; + Msix::MsixInfo msixInfo{ packagePath }; + auto manifest = msixInfo.GetAppPackageManifests(); + + if (manifest.size() == 1) + { + return manifest[0].GetIdentity().GetVersion(); + } } } + return std::nullopt; + } + + bool UpdateInternal(const std::string& packageLocation, const SourceDetails& details, IProgressCallback& progress) override + { + // We will extract the manifest and index files directly to this location + std::filesystem::path packageState = GetStatePathFromDetails(details); + std::filesystem::create_directories(packageState); + + std::filesystem::path packagePath = packageState / s_PreIndexedPackageSourceFactory_PackageFileName; + std::filesystem::path tempPackagePath = packagePath.u8string() + ".dnld.msix"; + auto removeTempFileOnExit = wil::scope_exit([&]() + { + try + { + std::filesystem::remove(tempPackagePath); + } + catch (...) + { + AICLI_LOG(Repo, Info, << "Failed to remove temp index file at: " << tempPackagePath); + } + }); + if (Utility::IsUrlRemote(packageLocation)) { AppInstaller::Utility::Download(packageLocation, tempPackagePath, AppInstaller::Utility::DownloadType::Index, progress); @@ -511,46 +619,37 @@ namespace AppInstaller::Repository::Microsoft progress.OnProgress(100, 100, ProgressType::Percent); } - bool updateSuccess = false; if (progress.IsCancelledBy(CancelReason::Any)) { AICLI_LOG(Repo, Info, << "Cancelling update upon request"); + return false; } - else + { - bool tempIndexPackageTrusted = false; + // Extra scope to release the file lock right after trust validation. + Msix::WriteLockedMsixFile tempIndexPackage{ tempPackagePath }; + Msix::MsixInfo tempMsixInfo{ tempPackagePath }; - { - // Extra scope to release the file lock right after trust validation. - Msix::WriteLockedMsixFile tempIndexPackage{ tempPackagePath }; - tempIndexPackageTrusted = tempIndexPackage.ValidateTrustInfo(WI_IsFlagSet(details.TrustLevel, SourceTrustLevel::StoreOrigin)); - } + // The package should not be a bundle + THROW_HR_IF(APPINSTALLER_CLI_ERROR_PACKAGE_IS_BUNDLE, tempMsixInfo.GetIsBundle()); - if (tempIndexPackageTrusted) - { - std::filesystem::rename(tempPackagePath, packagePath); - AICLI_LOG(Repo, Info, << "Source update success."); - updateSuccess = true; - } - else + // Ensure that family name has not changed + THROW_HR_IF(APPINSTALLER_CLI_ERROR_SOURCE_DATA_INTEGRITY_FAILURE, + GetPackageFamilyNameFromDetails(details) != Msix::GetPackageFamilyNameFromFullName(tempMsixInfo.GetPackageFullName())); + + if (!tempIndexPackage.ValidateTrustInfo(WI_IsFlagSet(details.TrustLevel, SourceTrustLevel::StoreOrigin))) { AICLI_LOG(Repo, Error, << "Source update failed. Source package failed trust validation."); + THROW_HR(APPINSTALLER_CLI_ERROR_SOURCE_DATA_INTEGRITY_FAILURE); } } - if (!updateSuccess) - { - try - { - std::filesystem::remove(tempPackagePath); - } - catch (...) - { - AICLI_LOG(Repo, Info, << "Failed to remove temp index file at: " << tempPackagePath); - } - } + std::filesystem::rename(tempPackagePath, packagePath); + AICLI_LOG(Repo, Info, << "Source update success."); - return updateSuccess; + removeTempFileOnExit.release(); + + return true; } bool RemoveInternal(const SourceDetails& details, IProgressCallback&) override diff --git a/src/AppInstallerSharedLib/Public/AppInstallerVersions.h b/src/AppInstallerSharedLib/Public/AppInstallerVersions.h index 13f8e57a8f..aa98c64c3f 100644 --- a/src/AppInstallerSharedLib/Public/AppInstallerVersions.h +++ b/src/AppInstallerSharedLib/Public/AppInstallerVersions.h @@ -127,12 +127,14 @@ namespace AppInstaller::Utility { UInt64Version() = default; UInt64Version(UINT64 version); + UInt64Version(uint16_t major, uint16_t minor, uint16_t build, uint16_t revision); UInt64Version(std::string&& version, std::string_view splitChars = DefaultSplitChars); UInt64Version(const std::string& version, std::string_view splitChars = DefaultSplitChars) : UInt64Version(std::string(version), splitChars) {} void Assign(std::string version, std::string_view splitChars = DefaultSplitChars) override; void Assign(UINT64 version); + void Assign(uint16_t major, uint16_t minor, uint16_t build, uint16_t revision); UINT64 Major() const { return m_parts.size() > 0 ? m_parts[0].Integer : 0; } UINT64 Minor() const { return m_parts.size() > 1 ? m_parts[1].Integer : 0; } diff --git a/src/AppInstallerSharedLib/Versions.cpp b/src/AppInstallerSharedLib/Versions.cpp index 86139ac6ba..abb097b724 100644 --- a/src/AppInstallerSharedLib/Versions.cpp +++ b/src/AppInstallerSharedLib/Versions.cpp @@ -396,14 +396,24 @@ namespace AppInstaller::Utility Assign(version); } + UInt64Version::UInt64Version(uint16_t major, uint16_t minor, uint16_t build, uint16_t revision) + { + Assign(major, minor, build, revision); + } + void UInt64Version::Assign(UINT64 version) { - const UINT64 mask16 = (1 << 16) - 1; - UINT64 revision = version & mask16; - UINT64 build = (version >> 0x10) & mask16; - UINT64 minor = (version >> 0x20) & mask16; - UINT64 major = (version >> 0x30) & mask16; + constexpr UINT64 mask16 = (1 << 16) - 1; + uint16_t revision = version & mask16; + uint16_t build = (version >> 0x10) & mask16; + uint16_t minor = (version >> 0x20) & mask16; + uint16_t major = (version >> 0x30) & mask16; + Assign(major, minor, build, revision); + } + + void UInt64Version::Assign(uint16_t major, uint16_t minor, uint16_t build, uint16_t revision) + { // Construct a string representation of the provided version std::stringstream ssVersion; ssVersion << major