fix upload error when cookbook tarball uid/gid is very large

Fixes #1806

RubyGems' Gem::Package::TarReader has some strict octal checking that is
incompatible with pax format tar files with large (greater than 8^8) uid
or gid entries (see rubygems/rubygems#2213). This can happen
particularly when publishing a cookbook on a Windows workstation that is
a member of a domain; the group IDs there can be very large numbers.

This change swaps out the handling of tar files from RubyGems' library
to Chef's own ffi-libarchive. The previous logic also assumed that
cookbook tarballs must be GZipped which is probably an assumption made
elsewhere in the Chef ecosystem. Because libarchive is pretty forgiving
of the handling of multiple formats of tar files, the requirement for
GZipped tar files is preserved and implemented with FileMagic, already a
dependency of Supermarket via the Fieri engine subcomponent.

Signed-off-by: Robb Kidd <rkidd@chef.io>

src/supermarket/Gemfile

@@ -23,6 +23,7 @@ gem 'foreman'
 gem 'html_truncator'
 gem 'jbuilder'
 gem 'kaminari'
+gem 'ffi-libarchive'
 gem 'mixlib-authentication'
 gem 'newrelic_rpm'
 gem 'nokogiri'
@@ -37,6 +38,7 @@ gem 'redcarpet' # markdown parsing
 gem 'redis-rails'
 gem 'rinku', require: 'rails_rinku'
 gem 'rollout'
+gem 'ruby-filemagic'
 gem 'sass-globbing'
 gem 'sass-rails'
 gem 'sentry-raven', require: false

src/supermarket/Gemfile.lock

@@ -195,6 +195,8 @@ GEM
     faraday (0.11.0)
       multipart-post (>= 1.2, < 3)
     ffi (1.9.25)
+    ffi-libarchive (0.4.6)
+      ffi (~> 1.0)
     ffi-yajl (2.3.1)
       libyajl2 (~> 1.2)
     foodcritic (14.3.0)
@@ -611,6 +613,7 @@ DEPENDENCIES
   dotenv
   factory_bot_rails
   faker
+  ffi-libarchive
   fieri!
   foreman
   guard
@@ -646,6 +649,7 @@ DEPENDENCIES
   rollout
   rspec-rails
   rubocop (>= 0.23.0)
+  ruby-filemagic
   sass-globbing
   sass-rails
   sentry-raven

src/supermarket/app/models/cookbook_upload/archive.rb

@@ -1,4 +1,5 @@
-require 'rubygems/package'
+require 'ffi-libarchive'
+require 'filemagic'
 
 class CookbookUpload
   #
@@ -19,6 +20,8 @@ class NoPath < RuntimeError; end
     # Indicates that the source could not be processed
     #
     class Error < RuntimeError; end
+    class CorruptTarball < Error; end
+    class NotGzipped < Error; end
 
     #
     # Creates a new Archive
@@ -39,10 +42,10 @@ def initialize(source)
     def find(pattern)
       matches = []
 
-      each do |entry|
-        next unless entry.full_name.match(pattern)
+      each do |entry, _|
+        next unless entry.pathname.match(pattern)
 
-        matches << entry.full_name
+        matches << entry.pathname
       end
 
       matches
@@ -59,10 +62,10 @@ def find(pattern)
     def read(path)
       match = nil
 
-      each do |entry|
-        next unless entry.full_name == path
+      each do |entry, contents|
+        next unless entry.pathname == path
 
-        match = entry.read
+        match = contents
 
         break
       end
@@ -78,7 +81,8 @@ def read(path)
     # @raise [NoPath] if the source has no path
     # @raise [Error] if the source is not a compatible archive
     #
-    # @yieldparam [::Gem::Package::TarReader::Entry] entry
+    # @yieldparam [::Archive::Entry] entry
+    # @yieldparam [::String] contents
     #
     # @example
     #   archive = CookbookUpload::Archive.new(tarball)
@@ -88,16 +92,37 @@ def read(path)
     #
     def each
       raise NoPath unless @source.respond_to?(:path)
+      raise NotGzipped unless is_gzipped?(@source.path)
 
       begin
-        Zlib::GzipReader.open(@source.path) do |gzip|
-          Gem::Package::TarReader.new(gzip) do |tar|
-            tar.each { |entry| yield entry }
+        ::Archive::Reader.open_filename(@source.path) do |tar|
+          tar.each_entry_with_data do |entry, data|
+            contents = data.is_a?(String) ? data : ''
+            yield entry, contents
           end
         end
-      rescue Zlib::GzipFile::Error => e
-        raise Error, e.message
+      rescue ::Archive::Error => e
+        case e.message
+        when /Damaged tar archive/
+          raise CorruptTarball, e.message
+        end
       end
     end
+
+    #
+    # Determines whether a file at a path is gzipped or not
+    #
+    #
+    # @return true/false
+    #
+    # @example
+    #   archive = CookbookUpload::Archive.new(tarball)
+    #   archive.each do |entry|
+    #     puts "#{entry.full_name} has the following content:\n#{entry.read}"
+    #   end
+    #
+    def is_gzipped?(file_path)
+      FileMagic.mime.file(file_path) == "application/x-gzip; charset=binary"
+    end
   end
 end

src/supermarket/app/models/cookbook_upload/parameters.rb

@@ -170,11 +170,11 @@ def parse_tarball_metadata(&block)
         errors.add(:base, I18n.t('api.error_messages.metadata_not_json'))
       rescue Virtus::CoercionError
         errors.add(:base, I18n.t('api.error_messages.invalid_metadata'))
-      rescue Archive::Error
+      rescue Archive::NotGzipped
         errors.add(:base, I18n.t('api.error_messages.tarball_not_gzipped'))
       rescue Archive::NoPath
         errors.add(:base, I18n.t('api.error_messages.tarball_has_no_path'))
-      rescue ArgumentError, Gem::Package::TarInvalidError => e
+      rescue Archive::CorruptTarball => e
         errors.add(:base, I18n.t('api.error_messages.tarball_corrupt', error: e))
       end
 

src/supermarket/spec/models/cookbook_upload_spec.rb

@@ -113,6 +113,15 @@
       expect(version).to be_present
     end
 
+    it 'yields the cookbook version when the tarball has uid/gid greater than 8^8' do
+      tarball = File.open('spec/support/cookbook_fixtures/big-gid.tgz')
+
+      upload = CookbookUpload.new(user, cookbook: cookbook, tarball: tarball)
+      version = upload.finish { |_, _, v| v }
+
+      expect(version).to be_present
+    end
+
     it 'yields the cookbook version if the README has no extension' do
       tarball = File.open('spec/support/cookbook_fixtures/readme-no-extension.tgz')
 
@@ -168,7 +177,7 @@
       errors = upload.finish { |e, _| e }
 
       expect(errors.full_messages).
-        to include(I18n.t('api.error_messages.tarball_corrupt', error: '"\x00\x00\x00\x00\x00\x0001" is not an octal string'))
+        to include(I18n.t('api.error_messages.tarball_corrupt', error: 'Damaged tar archive'))
     end
 
     it 'yields an error if the tarball has no metadata.json entry' do