Merge branch 'backup-archive-permissions' into 'master'

CHANGELOG

@@ -16,8 +16,8 @@ v 7.14.0 (unreleased)
   - Set OmniAuth full_host parameter to ensure redirect URIs are correct (Stan Hu)
   - Expire Rails cache entries after two weeks to prevent endless Redis growth
   - Add support for destroying project milestones (Stan Hu)
-  - Add fetch command to the MR page
-  - Fix bug causing Bitbucket importer to crash when OAuth application had been removed.
+  - Add fetch command to the MR page.
+  - Allow custom backup archive permissions
 
 v 7.13.1
   - Fix: Label modifications are not reflected in existing notes and in the issue list

config/gitlab.yml.example

@@ -256,6 +256,7 @@ production: &base
   ## Backup settings
   backup:
     path: "tmp/backups"   # Relative paths are relative to Rails.root (default: tmp/backups/)
+    # archive_permissions: 0640 # Permissions for the resulting backup.tar file (default: 0600)
     # keep_time: 604800   # default: 0 (forever) (in seconds)
     # upload:
     #   # Fog storage connection settings, see http://fog.io/storage/ .
@@ -347,6 +348,8 @@ test:
     # user: YOUR_USERNAME
   satellites:
     path: tmp/tests/gitlab-satellites/
+  backup:
+    path: tmp/tests/backups
   gitlab_shell:
     path: tmp/tests/gitlab-shell/
     repos_path: tmp/tests/repositories/

config/initializers/1_settings.rb

@@ -170,6 +170,7 @@ Settings.gitlab_shell['ssh_path_prefix'] ||= Settings.send(:build_gitlab_shell_s
 Settings['backup'] ||= Settingslogic.new({})
 Settings.backup['keep_time']  ||= 0
 Settings.backup['path']         = File.expand_path(Settings.backup['path'] || "tmp/backups/", Rails.root)
+Settings.backup['archive_permissions']          ||= 0600
 Settings.backup['upload'] ||= Settingslogic.new({ 'remote_directory' => nil, 'connection' => nil })
 # Convert upload connection settings to use symbol keys, to make Fog happy
 if Settings.backup['upload']['connection']

doc/raketasks/backup_restore.md

@@ -148,6 +148,23 @@ with the name of your bucket:
 }
 ```
 
+## Backup archive permissions
+
+The backup archives created by GitLab (123456_gitlab_backup.tar) will have owner/group git:git and 0600 permissions by default.
+This is meant to avoid other system users reading GitLab's data.
+If you need the backup archives to have different permissions you can use the 'archive_permissions' setting.
+
+```
+# In /etc/gitlab/gitlab.rb, for omnibus packages
+gitlab_rails['backup_archive_permissions'] = 0644 # Makes the backup archives world-readable
+```
+
+```
+# In gitlab.yml, for installations from source:
+  backup:
+    archive_permissions: 0644 # Makes the backup archives world-readable
+```
+
 ## Storing configuration files
 
 Please be informed that a backup does not store your configuration

lib/backup/manager.rb

@@ -20,14 +20,14 @@ module Backup
 
         # create archive
         $progress.print "Creating backup archive: #{tar_file} ... "
-        orig_umask = File.umask(0077)
-        if Kernel.system('tar', '-cf', tar_file, *backup_contents)
+        # Set file permissions on open to prevent chmod races.
+        tar_system_options = {out: [tar_file, 'w', Gitlab.config.backup.archive_permissions]}
+        if Kernel.system('tar', '-cf', '-', *backup_contents, tar_system_options)
           $progress.puts "done".green
         else
           puts "creating archive #{tar_file} failed".red
           abort 'Backup failed'
         end
-        File.umask(orig_umask)
 
         upload(tar_file)
       end

spec/support/test_env.rb

@@ -35,6 +35,7 @@ module TestEnv
     clean_test_path
 
     FileUtils.mkdir_p(repos_path)
+    FileUtils.mkdir_p(backup_path)
 
     # Setup GitLab shell for test instance
     setup_gitlab_shell
@@ -127,6 +128,10 @@ module TestEnv
     Gitlab.config.gitlab_shell.repos_path
   end
 
+  def backup_path
+    Gitlab.config.backup.path
+  end
+
   def copy_forked_repo_with_submodules(project)
     base_repo_path = File.expand_path(forked_repo_path_bare)
     target_repo_path = File.expand_path(repos_path + "/#{project.namespace.path}/#{project.path}.git")

spec/tasks/gitlab/backup_rake_spec.rb

@@ -15,6 +15,12 @@ describe 'gitlab:app namespace rake task' do
     Rake.application.invoke_task task_name
   end
 
+  def reenable_backup_sub_tasks
+    %w{db repo uploads}.each do |subtask|
+      Rake::Task["gitlab:backup:#{subtask}:create"].reenable
+    end
+  end
+
   describe 'backup_restore' do
     before do
       # avoid writing task output to spec progress
@@ -60,26 +66,47 @@ describe 'gitlab:app namespace rake task' do
       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
+    def create_backup
+      FileUtils.rm tars_glob
 
       # Redirect STDOUT and run the rake task
       orig_stdout = $stdout
       $stdout = StringIO.new
+      reenable_backup_sub_tasks
       run_rake_task('gitlab:backup:create')
+      reenable_backup_sub_tasks
       $stdout = orig_stdout
 
-      @backup_tar = (tars_glob - existing_tars).first
+      @backup_tar = tars_glob.first
     end
 
-    after :all do
+    before do
+      create_backup
+    end
+
+    after 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')
+    context 'archive file permissions' do
+      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
+
+      context 'with custom archive_permissions' do
+        before do
+          allow(Gitlab.config.backup).to receive(:archive_permissions).and_return(0651)
+          # We created a backup in a before(:all) so it got the default permissions.
+          # We now need to do some work to create a _new_ backup file using our stub.
+          FileUtils.rm(@backup_tar)
+          create_backup
+        end
+
+        it 'uses the custom permissions' do
+          expect(File::Stat.new(@backup_tar).mode.to_s(8)).to eq('100651')
+        end
+      end
     end
 
     it 'should set correct permissions on the tar contents' do
@@ -110,12 +137,9 @@ describe 'gitlab:app namespace rake task' do
     before :all do
       @origin_cd = Dir.pwd
 
-      Rake::Task["gitlab:backup:db:create"].reenable
-      Rake::Task["gitlab:backup:repo:create"].reenable
-      Rake::Task["gitlab:backup:uploads:create"].reenable
+      reenable_backup_sub_tasks
 
-      # Record the existing backup tars so we don't touch them
-      existing_tars = tars_glob
+      FileUtils.rm tars_glob
 
       # Redirect STDOUT and run the rake task
       orig_stdout = $stdout
@@ -124,7 +148,7 @@ describe 'gitlab:app namespace rake task' do
       run_rake_task('gitlab:backup:create')
       $stdout = orig_stdout
 
-      @backup_tar = (tars_glob - existing_tars).first
+      @backup_tar = tars_glob.first
     end
 
     after :all do