Commit 07b32287 authored by Rémy Coutable's avatar Rémy Coutable

Merge branch 'git-http-controller' into 'master'

Dismantling Grack::Auth part 1: Git HTTP clients

Part of https://gitlab.com/gitlab-org/gitlab-ce/issues/14501

This does not completely get rid of Grack::Auth yet because Git LFS
support is 'behind' it and I would like to not make this MR bigger
than needed.

- changed tests to make HTTP requests instead of calling Rack apps
- added missing test cases for Git HTTP authentication
- moved Git HTTP requests into a 'normal' Rails controller

See merge request !3361
parents 99ea3271 df62cbd9
...@@ -42,46 +42,8 @@ class JwtController < ApplicationController ...@@ -42,46 +42,8 @@ class JwtController < ApplicationController
end end
def authenticate_user(login, password) def authenticate_user(login, password)
# TODO: this is a copy and paste from grack_auth, user = Gitlab::Auth.find_in_gitlab_or_ldap(login, password)
# it should be refactored in the future Gitlab::Auth.rate_limit!(request.ip, success: user.present?, login: login)
user = Gitlab::Auth.new.find(login, password)
# If the user authenticated successfully, we reset the auth failure count
# from Rack::Attack for that IP. A client may attempt to authenticate
# with a username and blank password first, and only after it receives
# a 401 error does it present a password. Resetting the count prevents
# false positives from occurring.
#
# Otherwise, we let Rack::Attack know there was a failed authentication
# attempt from this IP. This information is stored in the Rails cache
# (Redis) and will be used by the Rack::Attack middleware to decide
# whether to block requests from this IP.
config = Gitlab.config.rack_attack.git_basic_auth
if config.enabled
if user
# A successful login will reset the auth failure count from this IP
Rack::Attack::Allow2Ban.reset(request.ip, config)
else
banned = Rack::Attack::Allow2Ban.filter(request.ip, config) do
# Unless the IP is whitelisted, return true so that Allow2Ban
# increments the counter (stored in Rails.cache) for the IP
if config.ip_whitelist.include?(request.ip)
false
else
true
end
end
if banned
Rails.logger.info "IP #{request.ip} failed to login " \
"as #{login} but has been temporarily banned from Git auth"
return
end
end
end
user user
end end
end end
class Projects::GitHttpController < Projects::ApplicationController
attr_reader :user
skip_before_action :repository
before_action :authenticate_user
before_action :ensure_project_found!
# 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
if upload_pack? && upload_pack_allowed?
render_ok
elsif receive_pack? && receive_pack_allowed?
render_ok
else
render_not_found
end
end
# POST /foo/bar.git/git-upload-pack (git pull)
def git_upload_pack
if upload_pack? && upload_pack_allowed?
render_ok
else
render_not_found
end
end
# POST /foo/bar.git/git-receive-pack" (git push)
def git_receive_pack
if receive_pack? && receive_pack_allowed?
render_ok
else
render_not_found
end
end
private
def authenticate_user
return if project && project.public? && upload_pack?
authenticate_or_request_with_http_basic do |login, password|
auth_result = Gitlab::Auth.find(login, password, project: project, ip: request.ip)
if auth_result.type == :ci && upload_pack?
@ci = true
elsif auth_result.type == :oauth && !upload_pack?
# Not allowed
else
@user = auth_result.user
end
ci? || user
end
end
def ensure_project_found!
render_not_found if project.blank?
end
def project
return @project if defined?(@project)
project_id, _ = project_id_with_suffix
if project_id.blank?
@project = nil
else
@project = Project.find_with_namespace("#{params[:namespace_id]}/#{project_id}")
end
end
# This method returns two values so that we can parse
# params[:project_id] (untrusted input!) in exactly one place.
def project_id_with_suffix
id = params[:project_id] || ''
%w[.wiki.git .git].each do |suffix|
if id.end_with?(suffix)
# Be careful to only remove the suffix from the end of 'id'.
# Accidentally removing it from the middle is how security
# vulnerabilities happen!
return [id.slice(0, id.length - suffix.length), suffix]
end
end
# Something is wrong with params[:project_id]; do not pass it on.
[nil, nil]
end
def upload_pack?
git_command == 'git-upload-pack'
end
def receive_pack?
git_command == 'git-receive-pack'
end
def git_command
if action_name == 'info_refs'
params[:service]
else
action_name.dasherize
end
end
def render_ok
render json: Gitlab::Workhorse.git_http_ok(repository, user)
end
def repository
_, suffix = project_id_with_suffix
if suffix == '.wiki.git'
project.wiki.repository
else
project.repository
end
end
def render_not_found
render text: 'Not Found', status: :not_found
end
def ci?
@ci.present?
end
def upload_pack_allowed?
return false unless Gitlab.config.gitlab_shell.upload_pack
if user
Gitlab::GitAccess.new(user, project).download_access_check.allowed?
else
ci? || project.public?
end
end
def receive_pack_allowed?
return false unless Gitlab.config.gitlab_shell.receive_pack
# Skip user authorization on upload request.
# It will be done by the pre-receive hook in the repository.
user.present?
end
end
...@@ -12,7 +12,7 @@ Doorkeeper.configure do ...@@ -12,7 +12,7 @@ Doorkeeper.configure do
end end
resource_owner_from_credentials do |routes| resource_owner_from_credentials do |routes|
Gitlab::Auth.new.find(params[:username], params[:password]) Gitlab::Auth.find_in_gitlab_or_ldap(params[:username], params[:password])
end end
# If you want to restrict access to the web interface for adding oauth authorized applications, you need to declare the block below. # If you want to restrict access to the web interface for adding oauth authorized applications, you need to declare the block below.
......
...@@ -80,8 +80,8 @@ Rails.application.routes.draw do ...@@ -80,8 +80,8 @@ Rails.application.routes.draw do
# Health check # Health check
get 'health_check(/:checks)' => 'health_check#index', as: :health_check get 'health_check(/:checks)' => 'health_check#index', as: :health_check
# Enable Grack support # Enable Grack support (for LFS only)
mount Grack::AuthSpawner, at: '/', constraints: lambda { |request| /[-\/\w\.]+\.git\//.match(request.path_info) }, via: [:get, :post, :put] mount Grack::AuthSpawner, at: '/', constraints: lambda { |request| /[-\/\w\.]+\.git\/(info\/lfs|gitlab-lfs)/.match(request.path_info) }, via: [:get, :post, :put]
# Help # Help
get 'help' => 'help#index' get 'help' => 'help#index'
...@@ -454,6 +454,13 @@ Rails.application.routes.draw do ...@@ -454,6 +454,13 @@ Rails.application.routes.draw do
end end
scope module: :projects do scope module: :projects do
# Git HTTP clients ('git clone' etc.)
scope constraints: { format: /(git|wiki\.git)/ } do
get '/info/refs', to: 'git_http#info_refs'
post '/git-upload-pack', to: 'git_http#git_upload_pack'
post '/git-receive-pack', to: 'git_http#git_receive_pack'
end
# Blob routes: # Blob routes:
get '/new/*id', to: 'blob#new', constraints: { id: /.+/ }, as: 'new_blob' get '/new/*id', to: 'blob#new', constraints: { id: /.+/ }, as: 'new_blob'
post '/create/*id', to: 'blob#create', constraints: { id: /.+/ }, as: 'create_blob' post '/create/*id', to: 'blob#create', constraints: { id: /.+/ }, as: 'create_blob'
......
...@@ -11,8 +11,7 @@ module API ...@@ -11,8 +11,7 @@ module API
# Example Request: # Example Request:
# POST /session # POST /session
post "/session" do post "/session" do
auth = Gitlab::Auth.new user = Gitlab::Auth.find_in_gitlab_or_ldap(params[:email] || params[:login], params[:password])
user = auth.find(params[:email] || params[:login], params[:password])
return unauthorized! unless user return unauthorized! unless user
present user, with: Entities::UserLogin present user, with: Entities::UserLogin
......
module Gitlab module Gitlab
class Auth module Auth
def find(login, password) Result = Struct.new(:user, :type)
class << self
def find(login, password, project:, ip:)
raise "Must provide an IP for rate limiting" if ip.nil?
result = Result.new
if valid_ci_request?(login, password, project)
result.type = :ci
elsif result.user = find_in_gitlab_or_ldap(login, password)
result.type = :gitlab_or_ldap
elsif result.user = oauth_access_token_check(login, password)
result.type = :oauth
end
rate_limit!(ip, success: !!result.user || (result.type == :ci), login: login)
result
end
def find_in_gitlab_or_ldap(login, password)
user = User.by_login(login) user = User.by_login(login)
# If no user is found, or it's an LDAP server, try LDAP. # If no user is found, or it's an LDAP server, try LDAP.
...@@ -14,5 +34,54 @@ module Gitlab ...@@ -14,5 +34,54 @@ module Gitlab
user if user.valid_password?(password) user if user.valid_password?(password)
end end
end end
def rate_limit!(ip, success:, login:)
rate_limiter = Gitlab::Auth::IpRateLimiter.new(ip)
return unless rate_limiter.enabled?
if success
# Repeated login 'failures' are normal behavior for some Git clients so
# it is important to reset the ban counter once the client has proven
# they are not a 'bad guy'.
rate_limiter.reset!
else
# Register a login failure so that Rack::Attack can block the next
# request from this IP if needed.
rate_limiter.register_fail!
if rate_limiter.banned?
Rails.logger.info "IP #{ip} failed to login " \
"as #{login} but has been temporarily banned from Git auth"
end
end
end
private
def valid_ci_request?(login, password, project)
matched_login = /(?<service>^[a-zA-Z]*-ci)-token$/.match(login)
return false unless project && matched_login.present?
underscored_service = matched_login['service'].underscore
if underscored_service == 'gitlab_ci'
project && project.valid_build_token?(password)
elsif Service.available_services_names.include?(underscored_service)
# We treat underscored_service as a trusted input because it is included
# in the Service.available_services_names whitelist.
service = project.public_send("#{underscored_service}_service")
service && service.activated? && service.valid_token?(password)
end
end
def oauth_access_token_check(login, password)
if login == "oauth2" && password.present?
token = Doorkeeper::AccessToken.by_token(password)
token && token.accessible? && User.find_by(id: token.resource_owner_id)
end
end
end
end end
end end
module Gitlab
module Auth
class IpRateLimiter
attr_reader :ip
def initialize(ip)
@ip = ip
@banned = false
end
def enabled?
config.enabled
end
def reset!
Rack::Attack::Allow2Ban.reset(ip, config)
end
def register_fail!
# Allow2Ban.filter will return false if this IP has not failed too often yet
@banned = Rack::Attack::Allow2Ban.filter(ip, config) do
# If we return false here, the failure for this IP is ignored by Allow2Ban
ip_can_be_banned?
end
end
def banned?
@banned
end
private
def config
Gitlab.config.rack_attack.git_basic_auth
end
def ip_can_be_banned?
config.ip_whitelist.exclude?(ip)
end
end
end
end
...@@ -36,10 +36,7 @@ module Grack ...@@ -36,10 +36,7 @@ module Grack
lfs_response = Gitlab::Lfs::Router.new(project, @user, @request).try_call lfs_response = Gitlab::Lfs::Router.new(project, @user, @request).try_call
return lfs_response unless lfs_response.nil? return lfs_response unless lfs_response.nil?
if project && authorized_request? if @user.nil? && !@ci
# Tell gitlab-workhorse the request is OK, and what the GL_ID is
render_grack_auth_ok
elsif @user.nil? && !@ci
unauthorized unauthorized
else else
render_not_found render_not_found
...@@ -98,7 +95,7 @@ module Grack ...@@ -98,7 +95,7 @@ module Grack
end end
def authenticate_user(login, password) def authenticate_user(login, password)
user = Gitlab::Auth.new.find(login, password) user = Gitlab::Auth.find_in_gitlab_or_ldap(login, password)
unless user unless user
user = oauth_access_token_check(login, password) user = oauth_access_token_check(login, password)
...@@ -141,36 +138,6 @@ module Grack ...@@ -141,36 +138,6 @@ module Grack
user user
end end
def authorized_request?
return true if @ci
case git_cmd
when *Gitlab::GitAccess::DOWNLOAD_COMMANDS
if !Gitlab.config.gitlab_shell.upload_pack
false
elsif user
Gitlab::GitAccess.new(user, project).download_access_check.allowed?
elsif project.public?
# Allow clone/fetch for public projects
true
else
false
end
when *Gitlab::GitAccess::PUSH_COMMANDS
if !Gitlab.config.gitlab_shell.receive_pack
false
elsif user
# Skip user authorization on upload request.
# It will be done by the pre-receive hook in the repository.
true
else
false
end
else
false
end
end
def git_cmd def git_cmd
if @request.get? if @request.get?
@request.params['service'] @request.params['service']
...@@ -197,24 +164,6 @@ module Grack ...@@ -197,24 +164,6 @@ module Grack
end end
end end
def render_grack_auth_ok
repo_path =
if @request.path_info =~ /^([\w\.\/-]+)\.wiki\.git/
ProjectWiki.new(project).repository.path_to_repo
else
project.repository.path_to_repo
end
[
200,
{ "Content-Type" => "application/json" },
[JSON.dump({
'GL_ID' => Gitlab::ShellEnv.gl_id(@user),
'RepoPath' => repo_path,
})]
]
end
def render_not_found def render_not_found
[404, { "Content-Type" => "text/plain" }, ["Not Found"]] [404, { "Content-Type" => "text/plain" }, ["Not Found"]]
end end
......
...@@ -6,6 +6,13 @@ module Gitlab ...@@ -6,6 +6,13 @@ module Gitlab
SEND_DATA_HEADER = 'Gitlab-Workhorse-Send-Data' SEND_DATA_HEADER = 'Gitlab-Workhorse-Send-Data'
class << self class << self
def git_http_ok(repository, user)
{
'GL_ID' => Gitlab::ShellEnv.gl_id(user),
'RepoPath' => repository.path_to_repo,
}
end
def send_git_blob(repository, blob) def send_git_blob(repository, blob)
params = { params = {
'RepoPath' => repository.path_to_repo, 'RepoPath' => repository.path_to_repo,
......
require 'spec_helper' require 'spec_helper'
describe Gitlab::Auth, lib: true do describe Gitlab::Auth, lib: true do
let(:gl_auth) { Gitlab::Auth.new } let(:gl_auth) { described_class }
describe :find do describe 'find' do
it 'recognizes CI' do
token = '123'
project = create(:empty_project)
project.update_attributes(runners_token: token, builds_enabled: true)
ip = 'ip'
expect(gl_auth).to receive(:rate_limit!).with(ip, success: true, login: 'gitlab-ci-token')
expect(gl_auth.find('gitlab-ci-token', token, project: project, ip: ip)).to eq(Gitlab::Auth::Result.new(nil, :ci))
end
it 'recognizes master passwords' do
user = create(:user, password: 'password')
ip = 'ip'
expect(gl_auth).to receive(:rate_limit!).with(ip, success: true, login: user.username)
expect(gl_auth.find(user.username, 'password', project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new(user, :gitlab_or_ldap))
end
it 'recognizes OAuth tokens' do
user = create(:user)
application = Doorkeeper::Application.create!(name: "MyApp", redirect_uri: "https://app.com", owner: user)
token = Doorkeeper::AccessToken.create!(application_id: application.id, resource_owner_id: user.id)
ip = 'ip'
expect(gl_auth).to receive(:rate_limit!).with(ip, success: true, login: 'oauth2')
expect(gl_auth.find("oauth2", token.token, project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new(user, :oauth))
end
it 'returns double nil for invalid credentials' do
login = 'foo'
ip = 'ip'
expect(gl_auth).to receive(:rate_limit!).with(ip, success: false, login: login)
expect(gl_auth.find(login, 'bar', project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new)
end
end
describe 'find_in_gitlab_or_ldap' do
let!(:user) do let!(:user) do
create(:user, create(:user,
username: username, username: username,
...@@ -14,25 +52,25 @@ describe Gitlab::Auth, lib: true do ...@@ -14,25 +52,25 @@ describe Gitlab::Auth, lib: true do
let(:password) { 'my-secret' } let(:password) { 'my-secret' }
it "should find user by valid login/password" do it "should find user by valid login/password" do
expect( gl_auth.find(username, password) ).to eql user expect( gl_auth.find_in_gitlab_or_ldap(username, password) ).to eql user
end end
it 'should find user by valid email/password with case-insensitive email' do it 'should find user by valid email/password with case-insensitive email' do
expect(gl_auth.find(user.email.upcase, password)).to eql user expect(gl_auth.find_in_gitlab_or_ldap(user.email.upcase, password)).to eql user
end end
it 'should find user by valid username/password with case-insensitive username' do it 'should find user by valid username/password with case-insensitive username' do
expect(gl_auth.find(username.upcase, password)).to eql user expect(gl_auth.find_in_gitlab_or_ldap(username.upcase, password)).to eql user
end end
it "should not find user with invalid password" do it "should not find user with invalid password" do
password = 'wrong' password = 'wrong'
expect( gl_auth.find(username, password) ).not_to eql user expect( gl_auth.find_in_gitlab_or_ldap(username, password) ).not_to eql user
end end
it "should not find user with invalid login" do it "should not find user with invalid login" do
user = 'wrong' user = 'wrong'
expect( gl_auth.find(username, password) ).not_to eql user expect( gl_auth.find_in_gitlab_or_ldap(username, password) ).not_to eql user
end end
context "with ldap enabled" do context "with ldap enabled" do
...@@ -43,13 +81,13 @@ describe Gitlab::Auth, lib: true do ...@@ -43,13 +81,13 @@ describe Gitlab::Auth, lib: true do
it "tries to autheticate with db before ldap" do it "tries to autheticate with db before ldap" do
expect(Gitlab::LDAP::Authentication).not_to receive(:login) expect(Gitlab::LDAP::Authentication).not_to receive(:login)
gl_auth.find(username, password) gl_auth.find_in_gitlab_or_ldap(username, password)
end end
it "uses ldap as fallback to for authentication" do it "uses ldap as fallback to for authentication" do
expect(Gitlab::LDAP::Authentication).to receive(:login) expect(Gitlab::LDAP::Authentication).to receive(:login)
gl_auth.find('ldap_user', 'password') gl_auth.find_in_gitlab_or_ldap('ldap_user', 'password')
end end
end end
end end
......
require "spec_helper"
describe Grack::Auth, lib: true do
let(:user) { create(:user) }
let(:project) { create(:project) }
let(:app) { lambda { |env| [200, {}, "Success!"] } }
let!(:auth) { Grack::Auth.new(app) }
let(:env) do
{
'rack.input' => '',
'REQUEST_METHOD' => 'GET',
'QUERY_STRING' => 'service=git-upload-pack'
}
end
let(:status) { auth.call(env).first }
describe "#call" do
context "when the project doesn't exist" do
before do
env["PATH_INFO"] = "doesnt/exist.git"
end
context "when no authentication is provided" do
it "responds with status 401" do
expect(status).to eq(401)
end
end
context "when username and password are provided" do
context "when authentication fails" do
before do
env["HTTP_AUTHORIZATION"] = ActionController::HttpAuthentication::Basic.encode_credentials(user.username, "nope")
end
it "responds with status 401" do
expect(status).to eq(401)
end
end
context "when authentication succeeds" do
before do
env["HTTP_AUTHORIZATION"] = ActionController::HttpAuthentication::Basic.encode_credentials(user.username, user.password)
end
it "responds with status 404" do
expect(status).to eq(404)
end
end
end
end
context "when the Wiki for a project exists" do
before do
@wiki = ProjectWiki.new(project)
env["PATH_INFO"] = "#{@wiki.repository.path_with_namespace}.git/info/refs"
project.update_attribute(:visibility_level, Project::PUBLIC)
end
it "responds with the right project" do
response = auth.call(env)
json_body = ActiveSupport::JSON.decode(response[2][0])
expect(response.first).to eq(200)
expect(json_body['RepoPath']).to include(@wiki.repository.path_with_namespace)
end
end
context "when the project exists" do
before do
env["PATH_INFO"] = project.path_with_namespace + ".git"
end
context "when the project is public" do
before do
project.update_attribute(:visibility_level, Project::PUBLIC)
end
it "responds with status 200" do
expect(status).to eq(200)
end
end
context "when the project is private" do
before do
project.update_attribute(:visibility_level, Project::PRIVATE)
end
context "when no authentication is provided" do
it "responds with status 401" do
expect(status).to eq(401)
end
end
context "when username and password are provided" do
context "when authentication fails" do
before do
env["HTTP_AUTHORIZATION"] = ActionController::HttpAuthentication::Basic.encode_credentials(user.username, "nope")
end
it "responds with status 401" do
expect(status).to eq(401)
end
context "when the user is IP banned" do
before do
expect(Rack::Attack::Allow2Ban).to receive(:filter).and_return(true)
allow_any_instance_of(Rack::Request).to receive(:ip).and_return('1.2.3.4')
end
it "responds with status 401" do
expect(status).to eq(401)
end
end
end
context "when authentication succeeds" do
before do
env["HTTP_AUTHORIZATION"] = ActionController::HttpAuthentication::Basic.encode_credentials(user.username, user.password)
end
context "when the user has access to the project" do
before do
project.team << [user, :master]
end
context "when the user is blocked" do
before do
user.block
project.team << [user, :master]
end
it "responds with status 404" do
expect(status).to eq(404)
end
end
context "when the user isn't blocked" do
before do
expect(Rack::Attack::Allow2Ban).to receive(:reset)
end
it "responds with status 200" do
expect(status).to eq(200)
end
end
context "when blank password attempts follow a valid login" do
let(:options) { Gitlab.config.rack_attack.git_basic_auth }
let(:maxretry) { options[:maxretry] - 1 }
let(:ip) { '1.2.3.4' }
before do
allow_any_instance_of(Rack::Request).to receive(:ip).and_return(ip)
Rack::Attack::Allow2Ban.reset(ip, options)
end
after do
Rack::Attack::Allow2Ban.reset(ip, options)
end
def attempt_login(include_password)
password = include_password ? user.password : ""
env["HTTP_AUTHORIZATION"] = ActionController::HttpAuthentication::Basic.encode_credentials(user.username, password)
Grack::Auth.new(app)
auth.call(env).first
end
it "repeated attempts followed by successful attempt" do
maxretry.times.each do
expect(attempt_login(false)).to eq(401)
end
expect(attempt_login(true)).to eq(200)
expect(Rack::Attack::Allow2Ban.banned?(ip)).to be_falsey
maxretry.times.each do
expect(attempt_login(false)).to eq(401)
end
end
end
end
context "when the user doesn't have access to the project" do
it "responds with status 404" do
expect(status).to eq(404)
end
end
end
end
context "when a gitlab ci token is provided" do
let(:token) { "123" }
let(:project) { FactoryGirl.create :empty_project }
before do
project.update_attributes(runners_token: token, builds_enabled: true)
env["HTTP_AUTHORIZATION"] = ActionController::HttpAuthentication::Basic.encode_credentials("gitlab-ci-token", token)
end
it "responds with status 200" do
expect(status).to eq(200)
end
end
end
end
end
end
This diff is collapsed.
...@@ -44,7 +44,7 @@ describe JwtController do ...@@ -44,7 +44,7 @@ describe JwtController do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:headers) { { authorization: credentials('user', 'password') } } let(:headers) { { authorization: credentials('user', 'password') } }
before { expect_any_instance_of(Gitlab::Auth).to receive(:find).with('user', 'password').and_return(user) } before { expect(Gitlab::Auth).to receive(:find_in_gitlab_or_ldap).with('user', 'password').and_return(user) }
subject! { get '/jwt/auth', parameters, headers } subject! { get '/jwt/auth', parameters, headers }
......
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