From c87540ed46ba8756154f767be99f80be75c27a43 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <jacob@gitlab.com>
Date: Fri, 19 Aug 2016 19:10:41 +0200
Subject: [PATCH] Verify JWT messages from gitlab-workhorse
---
.gitignore | 1 +
.../projects/git_http_client_controller.rb | 4 +
.../projects/git_http_controller.rb | 3 +
.../projects/lfs_storage_controller.rb | 11 +--
app/helpers/workhorse_helper.rb | 4 +
.../initializers/gitlab_workhorse_secret.rb | 8 ++
lib/ci/api/builds.rb | 4 +-
lib/gitlab/workhorse.rb | 52 ++++++++++-
spec/lib/gitlab/workhorse_spec.rb | 86 ++++++++++++++++++-
spec/requests/ci/api/builds_spec.rb | 11 ++-
spec/requests/git_http_spec.rb | 18 +++-
spec/requests/lfs_http_spec.rb | 19 +++-
spec/support/workhorse_helpers.rb | 5 ++
13 files changed, 210 insertions(+), 16 deletions(-)
create mode 100644 config/initializers/gitlab_workhorse_secret.rb
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/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/lib/ci/api/builds.rb b/lib/ci/api/builds.rb
index 9f3b582a263..2b18ecef8ba 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..63032d43027 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.const_get('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..29a194b31f6 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-Api-Request')
+ 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
--
GitLab