Merge branch 'gitlab-workhorse-safeties' into 'master'

Security and safety improvements for gitlab-workhorse integration

Companion to https://gitlab.com/gitlab-org/gitlab-workhorse/merge_requests/60

- Use a custom content type when sending data to gitlab-workhorse
- Verify (using JWT and a shared secret on disk) that internal API requests came from gitlab-workhorse

This will allow us to build features in gitlab-workhorse that require
more trust, and protect us against programming mistakes in the future.

This is designed so that no action is required for installations from
source. For omnibus-gitlab we need to add code that manages the shared
secret.

See merge request !5907

16 files changed, 213 insertions, 19 deletions

diff --git a/.gitignore b/.gitignore
index 1bf9a47aef6..9166512606d 100644
--- a/.gitignore
+++ b/.gitignore
@@ -48,3 +48,4 @@
 /vendor/bundle/*
 /builds/*
 /shared/*
+/.gitlab_workhorse_secret
diff --git a/GITLAB_WORKHORSE_VERSION b/GITLAB_WORKHORSE_VERSION
index b4d6d12101f..a3df0a6959e 100644
--- a/GITLAB_WORKHORSE_VERSION
+++ b/GITLAB_WORKHORSE_VERSION
@@ -1 +1 @@
-0.7.11
+0.8.0
diff --git a/app/controllers/projects/git_http_client_controller.rb b/app/controllers/projects/git_http_client_controller.rb
index a5b4031c30f..f5ce63fdfed 100644
--- a/app/controllers/projects/git_http_client_controller.rb
+++ b/app/controllers/projects/git_http_client_controller.rb
@@ -117,4 +117,8 @@ class Projects::GitHttpClientController < Projects::ApplicationController
   def ci?
     @ci.present?
   end
+
+  def verify_workhorse_api!
+    Gitlab::Workhorse.verify_api_request!(request.headers)
+  end
 end
diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb
index b4373ef89ef..9805705c4e3 100644
--- a/app/controllers/projects/git_http_controller.rb
+++ b/app/controllers/projects/git_http_controller.rb
@@ -1,6 +1,8 @@
 # This file should be identical in GitLab Community Edition and Enterprise Edition
 
 class Projects::GitHttpController < Projects::GitHttpClientController
+  before_action :verify_workhorse_api!
+
   # GET /foo/bar.git/info/refs?service=git-upload-pack (git pull)
   # GET /foo/bar.git/info/refs?service=git-receive-pack (git push)
   def info_refs
@@ -56,6 +58,7 @@ class Projects::GitHttpController < Projects::GitHttpClientController
   end
 
   def render_ok
+    set_workhorse_internal_api_content_type
     render json: Gitlab::Workhorse.git_http_ok(repository, user)
   end
 
diff --git a/app/controllers/projects/lfs_storage_controller.rb b/app/controllers/projects/lfs_storage_controller.rb
index 69066cb40e6..9005b104e90 100644
--- a/app/controllers/projects/lfs_storage_controller.rb
+++ b/app/controllers/projects/lfs_storage_controller.rb
@@ -3,6 +3,7 @@ class Projects::LfsStorageController < Projects::GitHttpClientController
 
   before_action :require_lfs_enabled!
   before_action :lfs_check_access!
+  before_action :verify_workhorse_api!, only: [:upload_authorize]
 
   def download
     lfs_object = LfsObject.find_by_oid(oid)
@@ -15,14 +16,8 @@ class Projects::LfsStorageController < Projects::GitHttpClientController
   end
 
   def upload_authorize
-    render(
-      json: {
-        StoreLFSPath: "#{Gitlab.config.lfs.storage_path}/tmp/upload",
-        LfsOid: oid,
-        LfsSize: size,
-      },
-      content_type: 'application/json; charset=utf-8'
-    )
+    set_workhorse_internal_api_content_type
+    render json: Gitlab::Workhorse.lfs_upload_ok(oid, size)
   end
 
   def upload_finalize
diff --git a/app/helpers/workhorse_helper.rb b/app/helpers/workhorse_helper.rb
index d887cdadc34..88f374be1e5 100644
--- a/app/helpers/workhorse_helper.rb
+++ b/app/helpers/workhorse_helper.rb
@@ -34,4 +34,8 @@ module WorkhorseHelper
     headers.store(*Gitlab::Workhorse.send_artifacts_entry(build, entry))
     head :ok
   end
+
+  def set_workhorse_internal_api_content_type
+    headers['Content-Type'] = Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE
+  end
 end
diff --git a/config/initializers/gitlab_workhorse_secret.rb b/config/initializers/gitlab_workhorse_secret.rb
new file mode 100644
index 00000000000..ed54dc11098
--- /dev/null
+++ b/config/initializers/gitlab_workhorse_secret.rb
@@ -0,0 +1,8 @@
+begin
+  Gitlab::Workhorse.secret
+rescue
+  Gitlab::Workhorse.write_secret
+end
+
+# Try a second time. If it does not work this will raise.
+Gitlab::Workhorse.secret
diff --git a/doc/install/installation.md b/doc/install/installation.md
index 9522c3e7170..c9562e071d9 100644
--- a/doc/install/installation.md
+++ b/doc/install/installation.md
@@ -400,7 +400,7 @@ If you are not using Linux you may have to run `gmake` instead of
     cd /home/git
     sudo -u git -H git clone https://gitlab.com/gitlab-org/gitlab-workhorse.git
     cd gitlab-workhorse
-    sudo -u git -H git checkout v0.7.11
+    sudo -u git -H git checkout v0.8.0
     sudo -u git -H make
 
 ### Initialize Database and Activate Advanced Features
diff --git a/doc/update/8.11-to-8.12.md b/doc/update/8.11-to-8.12.md
index c8ca42f97bc..0615729263f 100644
--- a/doc/update/8.11-to-8.12.md
+++ b/doc/update/8.11-to-8.12.md
@@ -82,7 +82,7 @@ GitLab 8.1.
 ```bash
 cd /home/git/gitlab-workhorse
 sudo -u git -H git fetch --all
-sudo -u git -H git checkout v0.7.11
+sudo -u git -H git checkout v0.8.0
 sudo -u git -H make
 ```
 
diff --git a/lib/ci/api/builds.rb b/lib/ci/api/builds.rb
index eb4947cdbf1..54db63d4628 100644
--- a/lib/ci/api/builds.rb
+++ b/lib/ci/api/builds.rb
@@ -101,6 +101,7 @@ module Ci
         #   POST /builds/:id/artifacts/authorize
         post ":id/artifacts/authorize" do
           require_gitlab_workhorse!
+          Gitlab::Workhorse.verify_api_request!(headers)
           not_allowed! unless Gitlab.config.artifacts.enabled
           build = Ci::Build.find_by_id(params[:id])
           not_found! unless build
@@ -113,7 +114,8 @@ module Ci
           end
 
           status 200
-          { TempPath: ArtifactUploader.artifacts_upload_path }
+          content_type Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE
+          Gitlab::Workhorse.artifact_upload_ok
         end
 
         # Upload artifacts to build - Runners only
diff --git a/lib/gitlab/workhorse.rb b/lib/gitlab/workhorse.rb
index c6826a09bd2..efe4aeb399d 100644
--- a/lib/gitlab/workhorse.rb
+++ b/lib/gitlab/workhorse.rb
@@ -1,19 +1,38 @@
 require 'base64'
 require 'json'
+require 'securerandom'
 
 module Gitlab
   class Workhorse
     SEND_DATA_HEADER = 'Gitlab-Workhorse-Send-Data'
     VERSION_FILE = 'GITLAB_WORKHORSE_VERSION'
+    INTERNAL_API_CONTENT_TYPE = 'application/vnd.gitlab-workhorse+json'
+    INTERNAL_API_REQUEST_HEADER = 'Gitlab-Workhorse-Api-Request'
+
+    # Supposedly the effective key size for HMAC-SHA256 is 256 bits, i.e. 32
+    # bytes https://tools.ietf.org/html/rfc4868#section-2.6
+    SECRET_LENGTH = 32
 
     class << self
       def git_http_ok(repository, user)
         {
-          'GL_ID' => Gitlab::GlId.gl_id(user),
-          'RepoPath' => repository.path_to_repo,
+          GL_ID: Gitlab::GlId.gl_id(user),
+          RepoPath: repository.path_to_repo,
         }
       end
 
+      def lfs_upload_ok(oid, size)
+        {
+          StoreLFSPath: "#{Gitlab.config.lfs.storage_path}/tmp/upload",
+          LfsOid: oid,
+          LfsSize: size,
+        }
+      end
+
+      def artifact_upload_ok
+        { TempPath: ArtifactUploader.artifacts_upload_path }
+      end
+
       def send_git_blob(repository, blob)
         params = {
           'RepoPath' => repository.path_to_repo,
@@ -81,6 +100,35 @@ module Gitlab
         path.readable? ? path.read.chomp : 'unknown'
       end
 
+      def secret
+        @secret ||= begin
+          bytes = Base64.strict_decode64(File.read(secret_path))
+          raise "#{secret_path} does not contain #{SECRET_LENGTH} bytes" if bytes.length != SECRET_LENGTH
+          bytes
+        end
+      end
+      
+      def write_secret
+        bytes = SecureRandom.random_bytes(SECRET_LENGTH)
+        File.open(secret_path, 'w:BINARY', 0600) do |f| 
+          f.chmod(0600)
+          f.write(Base64.strict_encode64(bytes))
+        end
+      end
+      
+      def verify_api_request!(request_headers)
+        JWT.decode(
+          request_headers[INTERNAL_API_REQUEST_HEADER],
+          secret,
+          true,
+          { iss: 'gitlab-workhorse', verify_iss: true, algorithm: 'HS256' },
+        )
+      end
+
+      def secret_path
+        Rails.root.join('.gitlab_workhorse_secret')
+      end
+      
       protected
 
       def encode(hash)
diff --git a/spec/lib/gitlab/workhorse_spec.rb b/spec/lib/gitlab/workhorse_spec.rb
index c5c1402e8fc..395192149a9 100644
--- a/spec/lib/gitlab/workhorse_spec.rb
+++ b/spec/lib/gitlab/workhorse_spec.rb
@@ -4,7 +4,7 @@ describe Gitlab::Workhorse, lib: true do
   let(:project) { create(:project) }
   let(:subject) { Gitlab::Workhorse }
 
-  describe "#send_git_archive" do
+  describe ".send_git_archive" do
     context "when the repository doesn't have an archive file path" do
       before do
         allow(project.repository).to receive(:archive_metadata).and_return(Hash.new)
@@ -15,4 +15,88 @@ describe Gitlab::Workhorse, lib: true do
       end
     end
   end
+
+  describe ".secret" do
+    subject { described_class.secret }
+
+    before do
+      described_class.instance_variable_set(:@secret, nil)
+      described_class.write_secret
+    end
+
+    it 'returns 32 bytes' do
+      expect(subject).to be_a(String)
+      expect(subject.length).to eq(32)
+      expect(subject.encoding).to eq(Encoding::ASCII_8BIT)
+    end
+
+    it 'raises an exception if the secret file cannot be read' do
+      File.delete(described_class.secret_path)
+      expect { subject }.to raise_exception(Errno::ENOENT)
+    end
+
+    it 'raises an exception if the secret file contains the wrong number of bytes' do
+      File.truncate(described_class.secret_path, 0)
+      expect { subject }.to raise_exception(RuntimeError)
+    end
+  end
+
+  describe ".write_secret" do
+    let(:secret_path) { described_class.secret_path }
+    before do
+      begin
+        File.delete(secret_path)
+      rescue Errno::ENOENT
+      end
+
+      described_class.write_secret
+    end
+
+    it 'uses mode 0600' do
+      expect(File.stat(secret_path).mode & 0777).to eq(0600)
+    end
+
+    it 'writes base64 data' do
+      bytes = Base64.strict_decode64(File.read(secret_path))
+      expect(bytes).not_to be_empty
+    end
+  end
+
+  describe '#verify_api_request!' do
+    let(:header_key) { described_class::INTERNAL_API_REQUEST_HEADER }
+    let(:payload) { { 'iss' => 'gitlab-workhorse' } }
+
+    it 'accepts a correct header' do
+      headers = { header_key => JWT.encode(payload, described_class.secret, 'HS256') }
+      expect { call_verify(headers) }.not_to raise_error
+    end
+
+    it 'raises an error when the header is not set' do
+      expect { call_verify({}) }.to raise_jwt_error
+    end
+
+    it 'raises an error when the header is not signed' do
+      headers = { header_key => JWT.encode(payload, nil, 'none') }
+      expect { call_verify(headers) }.to raise_jwt_error
+    end
+
+    it 'raises an error when the header is signed with the wrong key' do
+      headers = { header_key => JWT.encode(payload, 'wrongkey', 'HS256') }
+      expect { call_verify(headers) }.to raise_jwt_error
+    end
+
+    it 'raises an error when the issuer is incorrect' do
+      payload['iss'] = 'somebody else'
+      headers = { header_key => JWT.encode(payload, described_class.secret, 'HS256') }
+      expect { call_verify(headers) }.to raise_jwt_error
+    end
+
+    def raise_jwt_error
+      raise_error(JWT::DecodeError)
+    end
+
+    def call_verify(headers)
+      described_class.verify_api_request!(headers)
+    end
+  end
 end
diff --git a/spec/requests/ci/api/builds_spec.rb b/spec/requests/ci/api/builds_spec.rb
index ca7932dc5da..9e390bea50b 100644
--- a/spec/requests/ci/api/builds_spec.rb
+++ b/spec/requests/ci/api/builds_spec.rb
@@ -230,7 +230,8 @@ describe Ci::API::API do
       let(:post_url) { ci_api("/builds/#{build.id}/artifacts") }
       let(:delete_url) { ci_api("/builds/#{build.id}/artifacts") }
       let(:get_url) { ci_api("/builds/#{build.id}/artifacts") }
-      let(:headers) { { "GitLab-Workhorse" => "1.0" } }
+      let(:jwt_token) { JWT.encode({ 'iss' => 'gitlab-workhorse' }, Gitlab::Workhorse.secret, 'HS256') }
+      let(:headers) { { "GitLab-Workhorse" => "1.0", Gitlab::Workhorse::INTERNAL_API_REQUEST_HEADER => jwt_token } }
       let(:headers_with_token) { headers.merge(Ci::API::Helpers::BUILD_TOKEN_HEADER => build.token) }
 
       before { build.run! }
@@ -240,14 +241,22 @@ describe Ci::API::API do
           it "using token as parameter" do
             post authorize_url, { token: build.token }, headers
             expect(response).to have_http_status(200)
+            expect(response.content_type.to_s).to eq(Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE)
             expect(json_response["TempPath"]).not_to be_nil
           end
 
           it "using token as header" do
             post authorize_url, {}, headers_with_token
             expect(response).to have_http_status(200)
+            expect(response.content_type.to_s).to eq(Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE)
             expect(json_response["TempPath"]).not_to be_nil
           end
+
+          it "reject requests that did not go through gitlab-workhorse" do
+            headers.delete(Gitlab::Workhorse::INTERNAL_API_REQUEST_HEADER)
+            post authorize_url, { token: build.token }, headers
+            expect(response).to have_http_status(500)
+          end
         end
 
         context "should fail to post too large artifact" do
diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb
index 9ca3b021aa2..b7001fede40 100644
--- a/spec/requests/git_http_spec.rb
+++ b/spec/requests/git_http_spec.rb
@@ -1,6 +1,8 @@
 require "spec_helper"
 
 describe 'Git HTTP requests', lib: true do
+  include WorkhorseHelpers
+
   let(:user)    { create(:user) }
   let(:project) { create(:project, path: 'project.git-project') }
 
@@ -48,6 +50,7 @@ describe 'Git HTTP requests', lib: true do
 
         expect(response).to have_http_status(200)
         expect(json_body['RepoPath']).to include(wiki.repository.path_with_namespace)
+        expect(response.content_type.to_s).to eq(Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE)
       end
     end
   end
@@ -63,6 +66,7 @@ describe 'Git HTTP requests', lib: true do
       it "downloads get status 200" do
         download(path, {}) do |response|
           expect(response).to have_http_status(200)
+          expect(response.content_type.to_s).to eq(Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE)
         end
       end
 
@@ -101,6 +105,14 @@ describe 'Git HTTP requests', lib: true do
           end
         end
       end
+      
+      context 'when the request is not from gitlab-workhorse' do
+        it 'raises an exception' do
+          expect do
+            get("/#{project.path_with_namespace}.git/info/refs?service=git-upload-pack")
+          end.to raise_error(JWT::DecodeError)
+        end
+      end
     end
 
     context "when the project is private" do
@@ -170,11 +182,13 @@ describe 'Git HTTP requests', lib: true do
                 clone_get(path, env)
 
                 expect(response).to have_http_status(200)
+                expect(response.content_type.to_s).to eq(Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE)
               end
 
               it "uploads get status 200" do
                 upload(path, env) do |response|
                   expect(response).to have_http_status(200)
+                  expect(response.content_type.to_s).to eq(Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE)
                 end
               end
             end
@@ -189,6 +203,7 @@ describe 'Git HTTP requests', lib: true do
                 clone_get "#{project.path_with_namespace}.git", user: 'oauth2', password: @token.token
 
                 expect(response).to have_http_status(200)
+                expect(response.content_type.to_s).to eq(Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE)
               end
 
               it "uploads get status 401 (no project existence information leak)" do
@@ -297,6 +312,7 @@ describe 'Git HTTP requests', lib: true do
           clone_get "#{project.path_with_namespace}.git", user: 'gitlab-ci-token', password: token
 
           expect(response).to have_http_status(200)
+          expect(response.content_type.to_s).to eq(Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE)
         end
 
         it "uploads get status 401 (no project existence information leak)" do
@@ -426,7 +442,7 @@ describe 'Git HTTP requests', lib: true do
   end
 
   def auth_env(user, password, spnego_request_token)
-    env = {}
+    env = workhorse_internal_api_request_header
     if user && password
       env['HTTP_AUTHORIZATION'] = ActionController::HttpAuthentication::Basic.encode_credentials(user, password)
     elsif spnego_request_token
diff --git a/spec/requests/lfs_http_spec.rb b/spec/requests/lfs_http_spec.rb
index fcd6521317a..6e551bb65fa 100644
--- a/spec/requests/lfs_http_spec.rb
+++ b/spec/requests/lfs_http_spec.rb
@@ -1,6 +1,8 @@
 require 'spec_helper'
 
 describe 'Git LFS API and storage' do
+  include WorkhorseHelpers
+
   let(:user) { create(:user) }
   let!(:lfs_object) { create(:lfs_object, :with_file) }
 
@@ -715,6 +717,12 @@ describe 'Git LFS API and storage' do
             project.team << [user, :developer]
           end
 
+          context 'and the request bypassed workhorse' do
+            it 'raises an exception' do
+              expect { put_authorize(verified: false) }.to raise_error JWT::DecodeError
+            end
+          end
+
           context 'and request is sent by gitlab-workhorse to authorize the request' do
             before do
               put_authorize
@@ -724,6 +732,10 @@ describe 'Git LFS API and storage' do
               expect(response).to have_http_status(200)
             end
 
+            it 'uses the gitlab-workhorse content type' do
+              expect(response.content_type.to_s).to eq(Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE)
+            end
+
             it 'responds with status 200, location of lfs store and object details' do
               expect(json_response['StoreLFSPath']).to eq("#{Gitlab.config.shared.path}/lfs-objects/tmp/upload")
               expect(json_response['LfsOid']).to eq(sample_oid)
@@ -863,8 +875,11 @@ describe 'Git LFS API and storage' do
       end
     end
 
-    def put_authorize
-      put "#{project.http_url_to_repo}/gitlab-lfs/objects/#{sample_oid}/#{sample_size}/authorize", nil, headers
+    def put_authorize(verified: true)
+      authorize_headers = headers
+      authorize_headers.merge!(workhorse_internal_api_request_header) if verified
+
+      put "#{project.http_url_to_repo}/gitlab-lfs/objects/#{sample_oid}/#{sample_size}/authorize", nil, authorize_headers
     end
 
     def put_finalize(lfs_tmp = lfs_tmp_file)
diff --git a/spec/support/workhorse_helpers.rb b/spec/support/workhorse_helpers.rb
index 107b6e30924..47673cd4c3a 100644
--- a/spec/support/workhorse_helpers.rb
+++ b/spec/support/workhorse_helpers.rb
@@ -13,4 +13,9 @@ module WorkhorseHelpers
       ]
     end
   end
+
+  def workhorse_internal_api_request_header
+    jwt_token = JWT.encode({ 'iss' => 'gitlab-workhorse' }, Gitlab::Workhorse.secret, 'HS256')
+    { 'HTTP_' + Gitlab::Workhorse::INTERNAL_API_REQUEST_HEADER.upcase.tr('-', '_') => jwt_token }
+  end
 end