Commit b7e6da5a authored by Jacob Vosmaer (GitLab)'s avatar Jacob Vosmaer (GitLab)

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
parents 483a28a4 7ad0bfac
......@@ -48,3 +48,4 @@
/vendor/bundle/*
/builds/*
/shared/*
/.gitlab_workhorse_secret
......@@ -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
# 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
......
......@@ -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
......
......@@ -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
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
......@@ -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
......
......@@ -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
```
......
......@@ -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
......
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)
......
......@@ -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
......@@ -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
......
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
......
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)
......
......@@ -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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment