Merge branch 'backup-permissions' into 'master'

Change permissions on backup files - #2

Use more restrictive permissions for backup tar files and for the db, uploads, and repositories directories inside the tar files.  See #1894.  Now the backup task recursively `chmod`s the `db/`, `uploads/`, and `repositories/` folders with 0700 permissions, and the tar file is created as 0600.

This is a followup to !1703, which was reverted because it broke Rspec tests.  The test failures were due to the rake task changing directories and not changing back, which I fixed with this commit.

cc @sytse

See merge request !1716

CHANGELOG

@@ -20,6 +20,7 @@ v 7.10.0 (unreleased)
   - Fix print view for markdown files and wiki pages
   - Improve GitLab performance when working with git repositories
   - Add tag message and last commit to tag hook (Kamil Trzciński)
+  - Restrict permissions on backup files
 
 v 7.9.0 (unreleased)
   - Add HipChat integration documentation (Stan Hu)

lib/backup/manager.rb

@@ -11,22 +11,27 @@ module Backup
       s[:tar_version]        = tar_version
       tar_file = "#{s[:backup_created_at].to_i}_gitlab_backup.tar"
 
-      Dir.chdir(Gitlab.config.backup.path)
+      Dir.chdir(Gitlab.config.backup.path) do
+        File.open("#{Gitlab.config.backup.path}/backup_information.yml",
+                  "w+") do |file|
+          file << s.to_yaml.gsub(/^---\n/,'')
+        end
 
-      File.open("#{Gitlab.config.backup.path}/backup_information.yml", "w+") do |file|
-        file << s.to_yaml.gsub(/^---\n/,'')
-      end
+        FileUtils.chmod_R(0700, %w{db uploads repositories})
 
-      # create archive
-      $progress.print "Creating backup archive: #{tar_file} ... "
-      if Kernel.system('tar', '-cf', tar_file, *BACKUP_CONTENTS)
-        $progress.puts "done".green
-      else
-        puts "creating archive #{tar_file} failed".red
-        abort 'Backup failed'
-      end
+        # create archive
+        $progress.print "Creating backup archive: #{tar_file} ... "
+        orig_umask = File.umask(0077)
+        if Kernel.system('tar', '-cf', tar_file, *BACKUP_CONTENTS)
+          $progress.puts "done".green
+        else
+          puts "creating archive #{tar_file} failed".red
+          abort 'Backup failed'
+        end
+        File.umask(orig_umask)
 
-      upload(tar_file)
+        upload(tar_file)
+      end
     end
 
     def upload(tar_file)
@@ -51,11 +56,13 @@ module Backup
 
     def cleanup
       $progress.print "Deleting tmp directories ... "
-      if Kernel.system('rm', '-rf', *BACKUP_CONTENTS)
-        $progress.puts "done".green
-      else
-        puts "deleting tmp directory failed".red
-        abort 'Backup failed'
+      BACKUP_CONTENTS.each do |dir|
+        if FileUtils.rm_rf(File.join(Gitlab.config.backup.path, dir))
+          $progress.puts "done".green
+        else
+          puts "deleting tmp directory '#{dir}' failed".red
+          abort 'Backup failed'
+        end
       end
     end
 

spec/tasks/gitlab/backup_rake_spec.rb

@@ -10,17 +10,17 @@ describe 'gitlab:app namespace rake task' do
     Rake::Task.define_task :environment
   end
 
+  def run_rake_task(task_name)
+    Rake::Task[task_name].reenable
+    Rake.application.invoke_task task_name
+  end
+
   describe 'backup_restore' do
     before do
       # avoid writing task output to spec progress
       allow($stdout).to receive :write
     end
 
-    let :run_rake_task do
-      Rake::Task["gitlab:backup:restore"].reenable
-      Rake.application.invoke_task "gitlab:backup:restore"
-    end
-
     context 'gitlab version' do
       before do
         Dir.stub glob: []
@@ -36,7 +36,9 @@ describe 'gitlab:app namespace rake task' do
 
       it 'should fail on mismatch' do
         YAML.stub load_file: {gitlab_version: "not #{gitlab_version}" }
-        expect { run_rake_task }.to raise_error SystemExit
+        expect { run_rake_task('gitlab:backup:restore') }.to(
+          raise_error SystemExit
+        )
       end
 
       it 'should invoke restoration on mach' do
@@ -44,9 +46,56 @@ describe 'gitlab:app namespace rake task' do
         expect(Rake::Task["gitlab:backup:db:restore"]).to receive :invoke
         expect(Rake::Task["gitlab:backup:repo:restore"]).to receive :invoke
         expect(Rake::Task["gitlab:shell:setup"]).to receive :invoke
-        expect { run_rake_task }.to_not raise_error
+        expect { run_rake_task('gitlab:backup:restore') }.to_not raise_error
       end
     end
 
   end # backup_restore task
+
+  describe 'backup_create' do
+    def tars_glob
+      Dir.glob(File.join(Gitlab.config.backup.path, '*_gitlab_backup.tar'))
+    end
+
+    before :all do
+      # Record the existing backup tars so we don't touch them
+      existing_tars = tars_glob
+
+      # Redirect STDOUT and run the rake task
+      orig_stdout = $stdout
+      $stdout = StringIO.new
+      run_rake_task('gitlab:backup:create')
+      $stdout = orig_stdout
+
+      @backup_tar = (tars_glob - existing_tars).first
+    end
+
+    after :all do
+      FileUtils.rm(@backup_tar)
+    end
+
+    it 'should set correct permissions on the tar file' do
+      expect(File.exist?(@backup_tar)).to be_truthy
+      expect(File::Stat.new(@backup_tar).mode.to_s(8)).to eq('100600')
+    end
+
+    it 'should set correct permissions on the tar contents' do
+      tar_contents, exit_status = Gitlab::Popen.popen(
+        %W{tar -tvf #{@backup_tar} db uploads repositories}
+      )
+      expect(exit_status).to eq(0)
+      expect(tar_contents).to match('db/')
+      expect(tar_contents).to match('uploads/')
+      expect(tar_contents).to match('repositories/')
+      expect(tar_contents).not_to match(/^.{4,9}[rwx]/)
+    end
+
+    it 'should delete temp directories' do
+      temp_dirs = Dir.glob(
+        File.join(Gitlab.config.backup.path, '{db,repositories,uploads}')
+      )
+
+      expect(temp_dirs).to be_empty
+    end
+  end # backup_create task
 end # gitlab:app namespace