From ef5df5c366ce022df7dcdfbe2622a3f6dbb06d25 Mon Sep 17 00:00:00 2001 From: Seth Boyles Date: Fri, 8 Nov 2024 16:20:31 -0800 Subject: [PATCH 1/2] Validate checksums when fetching cached files for app packaging * This also deletes any resources with bad/mismatched checksums --- errors/v2.yml | 5 ++++ .../packager/shared_bits_packer.rb | 10 ++++++++ spec/api/documentation/app_bits_api_spec.rb | 4 ++-- spec/request/v2/apps_spec.rb | 2 +- .../app_bits_upload_controller_spec.rb | 4 ++-- .../packager/local_bits_packer_spec.rb | 23 ++++++++++++++++++- 6 files changed, 42 insertions(+), 6 deletions(-) diff --git a/errors/v2.yml b/errors/v2.yml index 67dade40300..e35df8aa263 100644 --- a/errors/v2.yml +++ b/errors/v2.yml @@ -613,6 +613,11 @@ http_code: 502 message: "Deletion of app %s failed because one or more associated resources could not be deleted.\n\n%s" +150010: + name: ResourceChecksumMismatch + http_code: 500 + message: "One or more cached resources did not match the given checksum. They have been deleted; please retry package upload." + 160001: name: AppBitsUploadInvalid http_code: 400 diff --git a/lib/cloud_controller/packager/shared_bits_packer.rb b/lib/cloud_controller/packager/shared_bits_packer.rb index 8d3201635d2..174d943bc86 100644 --- a/lib/cloud_controller/packager/shared_bits_packer.rb +++ b/lib/cloud_controller/packager/shared_bits_packer.rb @@ -42,9 +42,19 @@ def append_matched_resources(app_packager, cached_files_fingerprints, root_path) cached_resources_dir = File.join(root_path, 'cached_resources_dir') FileUtils.mkdir(cached_resources_dir) + checksum_mismatch = false matched_resources.each do |local_destination, file_sha, mode| + file_path = File.join(cached_resources_dir, local_destination) global_app_bits_cache.download_from_blobstore(file_sha, File.join(cached_resources_dir, local_destination), mode:) + + if Digester.new.digest_path(file_path) != file_sha + checksum_mismatch = true + global_app_bits_cache.delete(file_sha) + end end + + raise CloudController::Errors::ApiError.new_from_details('ResourceChecksumMismatch') if checksum_mismatch + app_packager.append_dir_contents(cached_resources_dir) end diff --git a/spec/api/documentation/app_bits_api_spec.rb b/spec/api/documentation/app_bits_api_spec.rb index 3a4605e24fd..87caa7e5c67 100644 --- a/spec/api/documentation/app_bits_api_spec.rb +++ b/spec/api/documentation/app_bits_api_spec.rb @@ -35,8 +35,8 @@ let(:fingerprints) do [ - { fn: 'path/to/content.txt', size: 123, sha1: 'b907173290db6a155949ab4dc9b2d019dea0c901' }, - { fn: 'path/to/code.jar', size: 123, sha1: 'ff84f89760317996b9dd180ab996b079f418396f' } + { fn: 'path/to/content.txt', size: 123, sha1: 'da39a3ee5e6b4b0d3255bfef95601890afd80709' }, + { fn: 'path/to/code.jar', size: 123, sha1: 'da39a3ee5e6b4b0d3255bfef95601890afd80709' } ] end diff --git a/spec/request/v2/apps_spec.rb b/spec/request/v2/apps_spec.rb index a12307794d4..36a1536a3a3 100644 --- a/spec/request/v2/apps_spec.rb +++ b/spec/request/v2/apps_spec.rb @@ -1649,7 +1649,7 @@ def make_actual_lrp(instance_guid:, index:, state:, error:, since:) TestConfig.config[:directories][:tmpdir] = File.dirname(valid_zip.path) upload_params = { application: valid_zip, - resources: [{ fn: 'a/b/c', size: 1, sha1: 'sha' }].to_json + resources: [{ fn: 'a/b/c', size: 1, sha1: 'da39a3ee5e6b4b0d3255bfef95601890afd80709' }].to_json } put "/v2/apps/#{process.guid}/bits", upload_params, headers_for(user) end diff --git a/spec/unit/controllers/runtime/app_bits_upload_controller_spec.rb b/spec/unit/controllers/runtime/app_bits_upload_controller_spec.rb index 1c843fe937b..00bd764a89c 100644 --- a/spec/unit/controllers/runtime/app_bits_upload_controller_spec.rb +++ b/spec/unit/controllers/runtime/app_bits_upload_controller_spec.rb @@ -117,7 +117,7 @@ def make_request end context 'with at least one resource and no application' do - let(:req_body) { { resources: Oj.dump([{ 'fn' => 'lol', 'sha1' => 'abc', 'size' => 2048 }]) } } + let(:req_body) { { resources: Oj.dump([{ 'fn' => 'lol', 'sha1' => 'da39a3ee5e6b4b0d3255bfef95601890afd80709', 'size' => 2048 }]) } } before do # rack_test overrides 'CONTENT_TYPE' header with 'boundary' which causes errors when the request does not contain an application @@ -132,7 +132,7 @@ def make_request end context 'with at least one resource and an application' do - let(:req_body) { { resources: Oj.dump([{ 'fn' => 'lol', 'sha1' => 'abc', 'size' => 2048 }]), application: valid_zip } } + let(:req_body) { { resources: Oj.dump([{ 'fn' => 'lol', 'sha1' => 'da39a3ee5e6b4b0d3255bfef95601890afd80709', 'size' => 2048 }]), application: valid_zip } } it 'succeeds to upload' do make_request diff --git a/spec/unit/lib/cloud_controller/packager/local_bits_packer_spec.rb b/spec/unit/lib/cloud_controller/packager/local_bits_packer_spec.rb index 0a938051d37..1b0846a6e77 100644 --- a/spec/unit/lib/cloud_controller/packager/local_bits_packer_spec.rb +++ b/spec/unit/lib/cloud_controller/packager/local_bits_packer_spec.rb @@ -26,7 +26,7 @@ module CloudController::Packager ) end - let(:fingerprints) do + let(:corrupted_fingerprints) do path = File.join(local_tmp_dir, 'content') sha = 'some_fake_sha' File.write(path, 'content') @@ -35,6 +35,15 @@ module CloudController::Packager [{ 'fn' => 'path/to/content.txt', 'size' => 123, 'sha1' => sha }] end + let(:fingerprints) do + path = File.join(local_tmp_dir, 'content') + File.write(path, 'content') + sha = Digester.new.digest_path(path) + global_app_bits_cache.cp_to_blobstore(path, sha) + + [{ 'fn' => 'path/to/content.txt', 'size' => 123, 'sha1' => sha }] + end + before do TestConfig.override(directories: { tmpdir: local_tmp_dir }) @@ -136,6 +145,18 @@ module CloudController::Packager expect(package_blobstore.exists?(blobstore_key)).to be true end end + + context 'and there are corrupted cached files' do + let(:cached_files_fingerprints) { corrupted_fingerprints } + + it 'deletes the offending files from the blobstore and errors with a ChecksumMismatch error' do + expect(global_app_bits_cache).to receive(:delete).with('some_fake_sha') + expect do + packer.send_package_to_blobstore(blobstore_key, uploaded_files_path, cached_files_fingerprints) + end. + to raise_error(CloudController::Errors::ApiError, /One or more cached resources/) + end + end end context 'when the zip file is invalid' do From bcdcc3eaf619dc205370bea128e1081b48aeed46 Mon Sep 17 00:00:00 2001 From: Seth Boyles Date: Mon, 11 Nov 2024 10:19:52 -0800 Subject: [PATCH 2/2] Remove SharedBitsPacker mixin * This module was a holdover from the 'RegistryBitsPacker' for cf-4-k8s. Since it's only used by the LocalBitsPacker, we can just merge the two files again. --- .../packager/local_bits_packer.rb | 82 ++++++++++++++++- .../packager/shared_bits_packer.rb | 87 ------------------- 2 files changed, 79 insertions(+), 90 deletions(-) delete mode 100644 lib/cloud_controller/packager/shared_bits_packer.rb diff --git a/lib/cloud_controller/packager/local_bits_packer.rb b/lib/cloud_controller/packager/local_bits_packer.rb index 62688d2275b..835382662a1 100644 --- a/lib/cloud_controller/packager/local_bits_packer.rb +++ b/lib/cloud_controller/packager/local_bits_packer.rb @@ -1,6 +1,6 @@ require 'cloud_controller/blobstore/fingerprints_collection' require 'cloud_controller/app_packager' -require 'cloud_controller/packager/shared_bits_packer' +require 'shellwords' module CloudController module Packager @@ -8,8 +8,6 @@ class ConflictError < StandardError end class LocalBitsPacker - include Packager::SharedBitsPacker - def send_package_to_blobstore(blobstore_key, uploaded_package_zip, cached_files_fingerprints) tmp_dir = VCAP::CloudController::Config.config.get(:directories, :tmpdir) local_bits_packer_path = File.join(tmp_dir, "local_bits_packer-#{blobstore_key}") @@ -39,6 +37,84 @@ def send_package_to_blobstore(blobstore_key, uploaded_package_zip, cached_files_ def package_blobstore CloudController::DependencyLocator.instance.package_blobstore end + + def package_zip_exists?(package_zip) + package_zip && File.exist?(package_zip) + end + + def validate_size!(app_packager) + return unless max_package_size + + return unless app_packager.size > max_package_size + + raise CloudController::Errors::ApiError.new_from_details('AppPackageInvalid', "Package may not be larger than #{max_package_size} bytes") + end + + def copy_uploaded_package(uploaded_package_zip, app_packager) + FileUtils.chmod('u+w', uploaded_package_zip) + FileUtils.cp(uploaded_package_zip, app_packager.path) + end + + def populate_resource_cache(app_packager, app_contents_path) + FileUtils.mkdir(app_contents_path) + app_packager.unzip(app_contents_path) + + remove_symlinks(app_contents_path) + + global_app_bits_cache.cp_r_to_blobstore(app_contents_path) + end + + def remove_symlinks(app_contents_path) + Find.find(app_contents_path) do |path| + File.delete(path) if File.symlink?(path) + end + end + + def append_matched_resources(app_packager, cached_files_fingerprints, root_path) + matched_resources = CloudController::Blobstore::FingerprintsCollection.new(cached_files_fingerprints, root_path) + cached_resources_dir = File.join(root_path, 'cached_resources_dir') + + FileUtils.mkdir(cached_resources_dir) + checksum_mismatch = false + matched_resources.each do |local_destination, file_sha, mode| + file_path = File.join(cached_resources_dir, local_destination) + global_app_bits_cache.download_from_blobstore(file_sha, File.join(cached_resources_dir, local_destination), mode:) + + if Digester.new.digest_path(file_path) != file_sha + checksum_mismatch = true + global_app_bits_cache.delete(file_sha) + end + end + + raise CloudController::Errors::ApiError.new_from_details('ResourceChecksumMismatch') if checksum_mismatch + + app_packager.append_dir_contents(cached_resources_dir) + end + + def match_resources_and_validate_package(root_path, uploaded_package_zip, cached_files_fingerprints) + app_packager = AppPackager.new(File.join(root_path, 'copied_app_package.zip')) + app_contents_path = File.join(root_path, 'application_contents') + + if package_zip_exists?(uploaded_package_zip) + copy_uploaded_package(uploaded_package_zip, app_packager) + validate_size!(app_packager) + populate_resource_cache(app_packager, app_contents_path) unless VCAP::CloudController::FeatureFlag.disabled?(:resource_matching) + end + + append_matched_resources(app_packager, cached_files_fingerprints, root_path) + + validate_size!(app_packager) + app_packager.fix_subdir_permissions(root_path, app_contents_path) + app_packager.path + end + + def max_package_size + VCAP::CloudController::Config.config.get(:packages, :max_package_size) + end + + def global_app_bits_cache + CloudController::DependencyLocator.instance.global_app_bits_cache + end end end end diff --git a/lib/cloud_controller/packager/shared_bits_packer.rb b/lib/cloud_controller/packager/shared_bits_packer.rb deleted file mode 100644 index 174d943bc86..00000000000 --- a/lib/cloud_controller/packager/shared_bits_packer.rb +++ /dev/null @@ -1,87 +0,0 @@ -require 'shellwords' - -module CloudController - module Packager - module SharedBitsPacker - private - - def package_zip_exists?(package_zip) - package_zip && File.exist?(package_zip) - end - - def validate_size!(app_packager) - return unless max_package_size - - return unless app_packager.size > max_package_size - - raise CloudController::Errors::ApiError.new_from_details('AppPackageInvalid', "Package may not be larger than #{max_package_size} bytes") - end - - def copy_uploaded_package(uploaded_package_zip, app_packager) - FileUtils.chmod('u+w', uploaded_package_zip) - FileUtils.cp(uploaded_package_zip, app_packager.path) - end - - def populate_resource_cache(app_packager, app_contents_path) - FileUtils.mkdir(app_contents_path) - app_packager.unzip(app_contents_path) - - remove_symlinks(app_contents_path) - - global_app_bits_cache.cp_r_to_blobstore(app_contents_path) - end - - def remove_symlinks(app_contents_path) - Find.find(app_contents_path) do |path| - File.delete(path) if File.symlink?(path) - end - end - - def append_matched_resources(app_packager, cached_files_fingerprints, root_path) - matched_resources = CloudController::Blobstore::FingerprintsCollection.new(cached_files_fingerprints, root_path) - cached_resources_dir = File.join(root_path, 'cached_resources_dir') - - FileUtils.mkdir(cached_resources_dir) - checksum_mismatch = false - matched_resources.each do |local_destination, file_sha, mode| - file_path = File.join(cached_resources_dir, local_destination) - global_app_bits_cache.download_from_blobstore(file_sha, File.join(cached_resources_dir, local_destination), mode:) - - if Digester.new.digest_path(file_path) != file_sha - checksum_mismatch = true - global_app_bits_cache.delete(file_sha) - end - end - - raise CloudController::Errors::ApiError.new_from_details('ResourceChecksumMismatch') if checksum_mismatch - - app_packager.append_dir_contents(cached_resources_dir) - end - - def match_resources_and_validate_package(root_path, uploaded_package_zip, cached_files_fingerprints) - app_packager = AppPackager.new(File.join(root_path, 'copied_app_package.zip')) - app_contents_path = File.join(root_path, 'application_contents') - - if package_zip_exists?(uploaded_package_zip) - copy_uploaded_package(uploaded_package_zip, app_packager) - validate_size!(app_packager) - populate_resource_cache(app_packager, app_contents_path) unless VCAP::CloudController::FeatureFlag.disabled?(:resource_matching) - end - - append_matched_resources(app_packager, cached_files_fingerprints, root_path) - - validate_size!(app_packager) - app_packager.fix_subdir_permissions(root_path, app_contents_path) - app_packager.path - end - - def max_package_size - VCAP::CloudController::Config.config.get(:packages, :max_package_size) - end - - def global_app_bits_cache - CloudController::DependencyLocator.instance.global_app_bits_cache - end - end - end -end