diff options
author | Jacob Vosmaer <contact@jacobvosmaer.nl> | 2016-04-29 18:58:55 +0200 |
---|---|---|
committer | Jacob Vosmaer <contact@jacobvosmaer.nl> | 2016-04-29 18:58:55 +0200 |
commit | b1ffc9f0fee16251899e5a2efbc78c4781ef4902 (patch) | |
tree | e5533f2b8a6aba9a69603b040727c2ada5a938c0 | |
parent | 9ef50db6279d722caed1ab1e4576275428e6a94f (diff) | |
download | gitlab-ce-b1ffc9f0fee16251899e5a2efbc78c4781ef4902.tar.gz |
Make CI/Oauth/rate limiting reusable
-rw-r--r-- | app/controllers/projects/git_http_controller.rb | 78 | ||||
-rw-r--r-- | config/initializers/doorkeeper.rb | 2 | ||||
-rw-r--r-- | lib/api/session.rb | 8 | ||||
-rw-r--r-- | lib/gitlab/auth.rb | 107 | ||||
-rw-r--r-- | lib/gitlab/backend/grack_auth.rb | 2 | ||||
-rw-r--r-- | spec/lib/gitlab/auth_spec.rb | 56 |
6 files changed, 158 insertions, 95 deletions
diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb index fafd9e445b5..16a85d6f62b 100644 --- a/app/controllers/projects/git_http_controller.rb +++ b/app/controllers/projects/git_http_controller.rb @@ -41,11 +41,15 @@ class Projects::GitHttpController < Projects::ApplicationController return if project && project.public? && upload_pack? authenticate_or_request_with_http_basic do |login, password| - return @ci = true if valid_ci_request?(login, password) + user, type = Gitlab::Auth.find(login, password, project: project, ip: request.ip) - @user = Gitlab::Auth.new.find(login, password) - @user ||= oauth_access_token_check(login, password) - rate_limit_ip!(login, @user) + if (type == :ci) && upload_pack? + @ci = true + elsif (type == :oauth) && !upload_pack? + @user = nil + else + @user = user + end end end @@ -53,72 +57,6 @@ class Projects::GitHttpController < Projects::ApplicationController render_not_found if project.blank? end - def valid_ci_request?(login, password) - matched_login = /(?<service>^[a-zA-Z]*-ci)-token$/.match(login) - - unless project && matched_login.present? && upload_pack? - return false - end - - 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_method = "#{underscored_service}_service" - service = project.send(service_method) - - service && service.activated? && service.valid_token?(password) - end - end - - def oauth_access_token_check(login, password) - if login == "oauth2" && upload_pack? && password.present? - token = Doorkeeper::AccessToken.by_token(password) - token && token.accessible? && User.find_by(id: token.resource_owner_id) - end - end - - def rate_limit_ip!(login, user) - # 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 - return user unless 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" - end - end - - user - end - def project return @project if defined?(@project) diff --git a/config/initializers/doorkeeper.rb b/config/initializers/doorkeeper.rb index 66ac88e9f4a..0c694e0d37a 100644 --- a/config/initializers/doorkeeper.rb +++ b/config/initializers/doorkeeper.rb @@ -12,7 +12,7 @@ Doorkeeper.configure do end resource_owner_from_credentials do |routes| - Gitlab::Auth.new.find(params[:username], params[:password]) + Gitlab::Auth.find_by_master_or_ldap(params[:username], params[:password]) end # If you want to restrict access to the web interface for adding oauth authorized applications, you need to declare the block below. diff --git a/lib/api/session.rb b/lib/api/session.rb index cc646895914..e308ccc3004 100644 --- a/lib/api/session.rb +++ b/lib/api/session.rb @@ -11,8 +11,12 @@ module API # Example Request: # POST /session post "/session" do - auth = Gitlab::Auth.new - user = auth.find(params[:email] || params[:login], params[:password]) + user, _ = Gitlab::Auth.find( + params[:email] || params[:login], + params[:password], + project: nil, + ip: request.ip + ) return unauthorized! unless user present user, with: Entities::UserLogin diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb index 30509528b8b..32e903905ad 100644 --- a/lib/gitlab/auth.rb +++ b/lib/gitlab/auth.rb @@ -1,17 +1,100 @@ module Gitlab class Auth - def find(login, password) - user = User.by_login(login) - - # If no user is found, or it's an LDAP server, try LDAP. - # LDAP users are only authenticated via LDAP - if user.nil? || user.ldap_user? - # Second chance - try LDAP authentication - return nil unless Gitlab::LDAP::Config.enabled? - - Gitlab::LDAP::Authentication.login(login, password) - else - user if user.valid_password?(password) + class << self + def find(login, password, project:, ip:) + raise "Must provide an IP for rate limiting" if ip.nil? + + user, type = nil, nil + + if valid_ci_request?(login, password, project) + type = :ci + elsif user = find_by_master_or_ldap(login, password) + type = :master_or_ldap + elsif user = oauth_access_token_check(login, password) + type = :oauth + end + + rate_limit!(ip, success: !!user || (type == :ci), login: login) + [user, type] + end + + def find_by_master_or_ldap(login, password) + user = User.by_login(login) + + # If no user is found, or it's an LDAP server, try LDAP. + # LDAP users are only authenticated via LDAP + if user.nil? || user.ldap_user? + # Second chance - try LDAP authentication + return nil unless Gitlab::LDAP::Config.enabled? + + Gitlab::LDAP::Authentication.login(login, password) + else + user if user.valid_password?(password) + 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_method = "#{underscored_service}_service" + service = project.send(service_method) + + 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 + + def rate_limit!(ip, success:, login:) + # 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 + return unless config.enabled + + if success + # A successful login will reset the auth failure count from this IP + Rack::Attack::Allow2Ban.reset(ip, config) + else + banned = Rack::Attack::Allow2Ban.filter(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?(ip) + false + else + true + end + end + + if banned + Rails.logger.info "IP #{ip} failed to login " \ + "as #{login} but has been temporarily banned from Git auth" + end + end end end end diff --git a/lib/gitlab/backend/grack_auth.rb b/lib/gitlab/backend/grack_auth.rb index e2363b91265..b263a27d4d3 100644 --- a/lib/gitlab/backend/grack_auth.rb +++ b/lib/gitlab/backend/grack_auth.rb @@ -95,7 +95,7 @@ module Grack end def authenticate_user(login, password) - user = Gitlab::Auth.new.find(login, password) + user, _ = Gitlab::Auth.new.find_by_master_or_ldap(login, password) unless user user = oauth_access_token_check(login, password) diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb index aad291c03cd..2c2f7ed0665 100644 --- a/spec/lib/gitlab/auth_spec.rb +++ b/spec/lib/gitlab/auth_spec.rb @@ -1,9 +1,47 @@ require 'spec_helper' 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([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([user, :master_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([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 ([nil, nil]) + end + end + + describe 'find_by_master_or_ldap' do let!(:user) do create(:user, username: username, @@ -14,25 +52,25 @@ describe Gitlab::Auth, lib: true do let(:password) { 'my-secret' } it "should find user by valid login/password" do - expect( gl_auth.find(username, password) ).to eql user + expect( gl_auth.find_by_master_or_ldap(username, password) ).to eql user end 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_by_master_or_ldap(user.email.upcase, password)).to eql user end 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_by_master_or_ldap(username.upcase, password)).to eql user end it "should not find user with invalid password" do password = 'wrong' - expect( gl_auth.find(username, password) ).not_to eql user + expect( gl_auth.find_by_master_or_ldap(username, password) ).not_to eql user end it "should not find user with invalid login" do user = 'wrong' - expect( gl_auth.find(username, password) ).not_to eql user + expect( gl_auth.find_by_master_or_ldap(username, password) ).not_to eql user end context "with ldap enabled" do @@ -43,13 +81,13 @@ describe Gitlab::Auth, lib: true do it "tries to autheticate with db before ldap" do expect(Gitlab::LDAP::Authentication).not_to receive(:login) - gl_auth.find(username, password) + gl_auth.find_by_master_or_ldap(username, password) end it "uses ldap as fallback to for authentication" do expect(Gitlab::LDAP::Authentication).to receive(:login) - gl_auth.find('ldap_user', 'password') + gl_auth.find_by_master_or_ldap('ldap_user', 'password') end end end |