Merge branch 'per-build-token-without-lfs' into 'master'

Make CI to use the permission of the user who is trigger the build

This is continuation of https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5735, but with removed all LFS code that is added by: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6043.

This also incorporates most of LFS code added in !6043 to simplify further merge.

See merge request !6409

CHANGELOG

@@ -22,6 +22,7 @@ v 8.12.0 (unreleased)
   - Instructions for enabling Git packfile bitmaps !6104
   - Use Search::GlobalService.new in the `GET /projects/search/:query` endpoint
   - Fix pagination on user snippets page
+  - Run CI builds with the permissions of users !5735
   - Fix sorting of issues in API
   - Sort project variables by key. !6275 (Diego Souza)
   - Ensure specs on sorting of issues in API are deterministic on MySQL

app/controllers/jwt_controller.rb

@@ -11,7 +11,10 @@ class JwtController < ApplicationController
     service = SERVICES[params[:service]]
     return head :not_found unless service
-    result = service.new(@project, @user, auth_params).execute
+    @authentication_result ||= Gitlab::Auth::Result.new
+
+    result = service.new(@authentication_result.project, @authentication_result.actor, auth_params).
+      execute(authentication_abilities: @authentication_result.authentication_abilities)
     render json: result, status: result[:http_status]
   end
@@ -20,30 +23,23 @@ class JwtController < ApplicationController
   def authenticate_project_or_user
     authenticate_with_http_basic do |login, password|
-      # if it's possible we first try to authenticate project with login and password
-      @project = authenticate_project(login, password)
-      return if @project
-
-      @user = authenticate_user(login, password)
-      return if @user
-      render_403
+      @authentication_result = Gitlab::Auth.find_for_git_client(login, password, project: nil, ip: request.ip)
+      render_403 unless @authentication_result.success? &&
+        (@authentication_result.actor.nil? || @authentication_result.actor.is_a?(User))
     end
+  rescue Gitlab::Auth::MissingPersonalTokenError
+    render_missing_personal_token
   end
-  def auth_params
-    params.permit(:service, :scope, :account, :client_id)
+  def render_missing_personal_token
+    render plain: "HTTP Basic: Access denied\n" \
+                  "You have 2FA enabled, please use a personal access token for Git over HTTP.\n" \
+                  "You can generate one at #{profile_personal_access_tokens_url}",
+           status: 401
   end
-  def authenticate_project(login, password)
-    if login == 'gitlab-ci-token'
-      Project.with_builds_enabled.find_by(runners_token: password)
-    end
-  end
-
-  def authenticate_user(login, password)
-    user = Gitlab::Auth.find_with_user_password(login, password)
-    Gitlab::Auth.rate_limit!(request.ip, success: user.present?, login: login)
-    user
+  def auth_params
+    params.permit(:service, :scope, :account, :client_id)
   end
 end

app/controllers/projects/builds_controller.rb

@@ -35,7 +35,11 @@ class Projects::BuildsController < Projects::ApplicationController
     respond_to do |format|
       format.html
       format.json do
-        render json: @build.to_json(methods: :trace_html)
+        render json: {
+          id: @build.id,
+          status: @build.status,
+          trace_html: @build.trace_html
+        }
       end
     end
   end

app/controllers/projects/git_http_client_controller.rb

@@ -4,7 +4,11 @@ class Projects::GitHttpClientController < Projects::ApplicationController
   include ActionController::HttpAuthentication::Basic
   include KerberosSpnegoHelper
-  attr_reader :user
+  attr_reader :authentication_result
+
+  delegate :actor, :authentication_abilities, to: :authentication_result, allow_nil: true
+
+  alias_method :user, :actor
   # Git clients will not know what authenticity token to send along
   skip_before_action :verify_authenticity_token
@@ -15,32 +19,25 @@ class Projects::GitHttpClientController < Projects::ApplicationController
   private
   def authenticate_user
+    @authentication_result = Gitlab::Auth::Result.new
+
     if project && project.public? && download_request?
       return # Allow access
     end
     if allow_basic_auth? && basic_auth_provided?
       login, password = user_name_and_password(request)
-      auth_result = Gitlab::Auth.find_for_git_client(login, password, project: project, ip: request.ip)
-
-      if auth_result.type == :ci && download_request?
-        @ci = true
-      elsif auth_result.type == :oauth && !download_request?
-        # Not allowed
-      elsif auth_result.type == :missing_personal_token
-        render_missing_personal_token
-        return # Render above denied access, nothing left to do
-      else
-        @user = auth_result.user
-      end
-      if ci? || user
+      if handle_basic_authentication(login, password)
         return # Allow access
       end
     elsif allow_kerberos_spnego_auth? && spnego_provided?
-      @user = find_kerberos_user
+      user = find_kerberos_user
       if user
+        @authentication_result = Gitlab::Auth::Result.new(
+          user, nil, :kerberos, Gitlab::Auth.full_authentication_abilities)
+
         send_final_spnego_response
         return # Allow access
       end
@@ -48,6 +45,8 @@ class Projects::GitHttpClientController < Projects::ApplicationController
     send_challenges
     render plain: "HTTP Basic: Access denied\n", status: 401
+  rescue Gitlab::Auth::MissingPersonalTokenError
+    render_missing_personal_token
   end
   def basic_auth_provided?
@@ -114,8 +113,39 @@ class Projects::GitHttpClientController < Projects::ApplicationController
     render plain: 'Not Found', status: :not_found
   end
+  def handle_basic_authentication(login, password)
+    @authentication_result = Gitlab::Auth.find_for_git_client(
+      login, password, project: project, ip: request.ip)
+
+    return false unless @authentication_result.success?
+
+    if download_request?
+      authentication_has_download_access?
+    else
+      authentication_has_upload_access?
+    end
+  end
+
   def ci?
-    @ci.present?
+    authentication_result.ci? &&
+      authentication_project &&
+      authentication_project == project
+  end
+
+  def authentication_has_download_access?
+    has_authentication_ability?(:download_code) || has_authentication_ability?(:build_download_code)
+  end
+
+  def authentication_has_upload_access?
+    has_authentication_ability?(:push_code)
+  end
+
+  def has_authentication_ability?(capability)
+    (authentication_abilities || []).include?(capability)
+  end
+
+  def authentication_project
+    authentication_result.project
   end
   def verify_workhorse_api!

app/controllers/projects/git_http_controller.rb

@@ -86,7 +86,7 @@ class Projects::GitHttpController < Projects::GitHttpClientController
   end
   def access
-    @access ||= Gitlab::GitAccess.new(user, project, 'http')
+    @access ||= Gitlab::GitAccess.new(user, project, 'http', authentication_abilities: authentication_abilities)
   end
   def access_check

app/helpers/lfs_helper.rb

@@ -25,13 +25,21 @@ module LfsHelper
   def lfs_download_access?
     return false unless project.lfs_enabled?
-    project.public? || ci? || (user && user.can?(:download_code, project))
+    project.public? || ci? || user_can_download_code? || build_can_download_code?
+  end
+
+  def user_can_download_code?
+    has_authentication_ability?(:download_code) && can?(user, :download_code, project)
+  end
+
+  def build_can_download_code?
+    has_authentication_ability?(:build_download_code) && can?(user, :build_download_code, project)
   end
   def lfs_upload_access?
     return false unless project.lfs_enabled?
-    user && user.can?(:push_code, project)
+    has_authentication_ability?(:push_code) && can?(user, :push_code, project)
   end
   def render_lfs_forbidden

app/models/ci/build.rb

 module Ci
   class Build < CommitStatus
+    include TokenAuthenticatable
+
     belongs_to :runner, class_name: 'Ci::Runner'
     belongs_to :trigger_request, class_name: 'Ci::TriggerRequest'
     belongs_to :erased_by, class_name: 'User'
@@ -23,7 +25,10 @@ module Ci
     acts_as_taggable
+    add_authentication_token_field :token
+
     before_save :update_artifacts_size, if: :artifacts_file_changed?
+    before_save :ensure_token
     before_destroy { project }
     after_create :execute_hooks
@@ -38,6 +43,7 @@ module Ci
         new_build.status = 'pending'
         new_build.runner_id = nil
         new_build.trigger_request_id = nil
+        new_build.token = nil
         new_build.save
       end
@@ -176,7 +182,7 @@ module Ci
     end
     def repo_url
-      auth = "gitlab-ci-token:#{token}@"
+      auth = "gitlab-ci-token:#{ensure_token!}@"
       project.http_url_to_repo.sub(/^https?:\/\//) do |prefix|
         prefix + auth
       end
@@ -238,12 +244,7 @@ module Ci
     end
     def trace
-      trace = raw_trace
-      if project && trace.present? && project.runners_token.present?
-        trace.gsub(project.runners_token, 'xxxxxx')
-      else
-        trace
-      end
+      hide_secrets(raw_trace)
     end
     def trace_length
@@ -256,6 +257,7 @@ module Ci
     def trace=(trace)
       recreate_trace_dir
+      trace = hide_secrets(trace)
       File.write(path_to_trace, trace)
     end
@@ -269,6 +271,8 @@ module Ci
     def append_trace(trace_part, offset)
       recreate_trace_dir
+      trace_part = hide_secrets(trace_part)
+
       File.truncate(path_to_trace, offset) if File.exist?(path_to_trace)
       File.open(path_to_trace, 'ab') do |f|
         f.write(trace_part)
@@ -344,12 +348,8 @@ module Ci
       )
     end
-    def token
-      project.runners_token
-    end
-
     def valid_token?(token)
-      project.valid_runners_token?(token)
+      self.token && ActiveSupport::SecurityUtils.variable_size_secure_compare(token, self.token)
     end
     def has_tags?
@@ -491,5 +491,11 @@ module Ci
       pipeline.config_processor.build_attributes(name)
     end
+
+    def hide_secrets(trace)
+      trace = Ci::MaskSecret.mask(trace, project.runners_token) if project
+      trace = Ci::MaskSecret.mask(trace, token)
+      trace
+    end
   end
 end

app/models/project.rb

@@ -1137,12 +1137,6 @@ class Project < ActiveRecord::Base
     self.runners_token && ActiveSupport::SecurityUtils.variable_size_secure_compare(token, self.runners_token)
   end
-  # TODO (ayufan): For now we use runners_token (backward compatibility)
-  # In 8.4 every build will have its own individual token valid for time of build
-  def valid_build_token?(token)
-    self.builds_enabled? && self.runners_token && ActiveSupport::SecurityUtils.variable_size_secure_compare(token, self.runners_token)
-  end
-
   def build_coverage_enabled?
     build_coverage_regex.present?
   end

app/policies/project_policy.rb

@@ -64,6 +64,12 @@ class ProjectPolicy < BasePolicy
     can! :read_deployment
   end
+  # Permissions given when an user is team member of a project
+  def team_member_reporter_access!
+    can! :build_download_code
+    can! :build_read_container_image
+  end
+
   def developer_access!
     can! :admin_merge_request
     can! :update_merge_request
@@ -109,6 +115,8 @@ class ProjectPolicy < BasePolicy
     can! :read_commit_status
     can! :read_pipeline
     can! :read_container_image
+    can! :build_download_code
+    can! :build_read_container_image
   end
   def owner_access!
@@ -130,10 +138,11 @@ class ProjectPolicy < BasePolicy
   def team_access!(user)
     access = project.team.max_member_access(user.id)
-    guest_access!     if access >= Gitlab::Access::GUEST
-    reporter_access!  if access >= Gitlab::Access::REPORTER
-    developer_access! if access >= Gitlab::Access::DEVELOPER
-    master_access!    if access >= Gitlab::Access::MASTER
+    guest_access!                if access >= Gitlab::Access::GUEST
+    reporter_access!             if access >= Gitlab::Access::REPORTER
+    team_member_reporter_access! if access >= Gitlab::Access::REPORTER
+    developer_access!            if access >= Gitlab::Access::DEVELOPER
+    master_access!               if access >= Gitlab::Access::MASTER
   end
   def archived_access!

app/services/auth/container_registry_authentication_service.rb

@@ -4,7 +4,9 @@ module Auth
     AUDIENCE = 'container_registry'
-    def execute
+    def execute(authentication_abilities:)
+      @authentication_abilities = authentication_abilities || []
+
       return error('not found', 404) unless registry.enabled
       unless current_user || project
@@ -74,9 +76,9 @@ module Auth
       case requested_action
       when 'pull'
-        requested_project == project || can?(current_user, :read_container_image, requested_project)
+        requested_project.public? || build_can_pull?(requested_project) || user_can_pull?(requested_project)
       when 'push'
-        requested_project == project || can?(current_user, :create_container_image, requested_project)
+        build_can_push?(requested_project) || user_can_push?(requested_project)
       else
         false
       end
@@ -85,5 +87,29 @@ module Auth
     def registry
       Gitlab.config.registry
     end
+
+    def build_can_pull?(requested_project)
+      # Build can:
+      # 1. pull from its own project (for ex. a build)
+      # 2. read images from dependent projects if creator of build is a team member
+      @authentication_abilities.include?(:build_read_container_image) &&
+        (requested_project == project || can?(current_user, :build_read_container_image, requested_project))
+    end
+
+    def user_can_pull?(requested_project)
+      @authentication_abilities.include?(:read_container_image) &&
+        can?(current_user, :read_container_image, requested_project)
+    end
+
+    def build_can_push?(requested_project)
+      # Build can push only to the project from which it originates
+      @authentication_abilities.include?(:build_create_container_image) &&
+        requested_project == project
+    end
+
+    def user_can_push?(requested_project)
+      @authentication_abilities.include?(:create_container_image) &&
+        can?(current_user, :create_container_image, requested_project)
+    end
   end
 end

db/migrate/20160808085531_add_token_to_build.rb

+class AddTokenToBuild < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  def change
+    add_column :ci_builds, :token, :string
+  end
+end

db/migrate/20160808085602_add_index_for_build_token.rb

+class AddIndexForBuildToken < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def change
+    add_concurrent_index :ci_builds, :token, unique: true
+  end
+end

db/schema.rb

@@ -181,6 +181,7 @@ ActiveRecord::Schema.define(version: 20160913212128) do
     t.string   "when"
     t.text     "yaml_variables"
     t.datetime "queued_at"
+    t.string   "token"
   end
   add_index "ci_builds", ["commit_id", "stage_idx", "created_at"], name: "index_ci_builds_on_commit_id_and_stage_idx_and_created_at", using: :btree
@@ -192,6 +193,7 @@ ActiveRecord::Schema.define(version: 20160913212128) do
   add_index "ci_builds", ["project_id"], name: "index_ci_builds_on_project_id", using: :btree
   add_index "ci_builds", ["runner_id"], name: "index_ci_builds_on_runner_id", using: :btree
   add_index "ci_builds", ["status"], name: "index_ci_builds_on_status", using: :btree
+  add_index "ci_builds", ["token"], name: "index_ci_builds_on_token", unique: true, using: :btree
   create_table "ci_commits", force: :cascade do |t|
     t.integer  "project_id"

lib/api/internal.rb

@@ -35,6 +35,14 @@ module API
             Project.find_with_namespace(project_path)
           end
         end
+
+        def ssh_authentication_abilities
+          [
+            :read_project,
+            :download_code,
+            :push_code
+          ]
+        end
       end
       post "/allowed" do
@@ -51,9 +59,9 @@ module API
         access =
           if wiki?
-            Gitlab::GitAccessWiki.new(actor, project, protocol)
+            Gitlab::GitAccessWiki.new(actor, project, protocol, authentication_abilities: ssh_authentication_abilities)
           else
-            Gitlab::GitAccess.new(actor, project, protocol)
+            Gitlab::GitAccess.new(actor, project, protocol, authentication_abilities: ssh_authentication_abilities)
           end
         access_status = access.check(params[:action], params[:changes])

lib/ci/api/helpers.rb

@@ -14,12 +14,20 @@ module Ci
       end
       def authenticate_build_token!(build)
-        token = (params[BUILD_TOKEN_PARAM] || env[BUILD_TOKEN_HEADER]).to_s
-        forbidden! unless token && build.valid_token?(token)
+        forbidden! unless build_token_valid?(build)
       end
       def runner_registration_token_valid?
-        params[:token] == current_application_settings.runners_registration_token
+        ActiveSupport::SecurityUtils.variable_size_secure_compare(
+          params[:token],
+          current_application_settings.runners_registration_token)
+      end
+
+      def build_token_valid?(build)
+        token = (params[BUILD_TOKEN_PARAM] || env[BUILD_TOKEN_HEADER]).to_s
+
+        # We require to also check `runners_token` to maintain compatibility with old version of runners
+        token && (build.valid_token?(token) || build.project.valid_runners_token?(token))
       end
       def update_runner_last_contact(save: true)

lib/ci/mask_secret.rb

+module Ci::MaskSecret
+  class << self
+    def mask(value, token)
+      return value unless value.present? && token.present?
+
+      value.gsub(token, 'x' * token.length)
+    end
+  end
+end

lib/gitlab/auth.rb

 module Gitlab
   module Auth
-    Result = Struct.new(:user, :type)
+    class MissingPersonalTokenError < StandardError; end
     class << self
       def find_for_git_client(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
-        else
-          result = populate_result(login, password)
-        end
-        success = result.user.present? || [:ci, :missing_personal_token].include?(result.type)
-        rate_limit!(ip, success: success, login: login)
+        result =
+          service_request_check(login, password, project) ||
+          build_access_token_check(login, password) ||
+          user_with_password_for_git(login, password) ||
+          oauth_access_token_check(login, password) ||
+          personal_access_token_check(login, password) ||
+          Gitlab::Auth::Result.new
+        rate_limit!(ip, success: result.success?, login: login)
         result
       end
@@ -57,44 +57,31 @@ module Gitlab
       private
-      def valid_ci_request?(login, password, project)
+      def service_request_check(login, password, project)
         matched_login = /(?<service>^[a-zA-Z]*-ci)-token$/.match(login)
-        return false unless project && matched_login.present?
+        return 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)
+        if 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 populate_result(login, password)
-        result =
-          user_with_password_for_git(login, password) ||
-          oauth_access_token_check(login, password) ||
-          personal_access_token_check(login, password)
-
-        if result
-          result.type = nil unless result.user
-
-          if result.user && result.user.two_factor_enabled? && result.type == :gitlab_or_ldap
-            result.type = :missing_personal_token
+          if service && service.activated? && service.valid_token?(password)
+            Gitlab::Auth::Result.new(nil, project, :ci, build_authentication_abilities)
           end
         end
-
-        result || Result.new
       end
       def user_with_password_for_git(login, password)
         user = find_with_user_password(login, password)
-        Result.new(user, :gitlab_or_ldap) if user
+        return unless user
+
+        raise Gitlab::Auth::MissingPersonalTokenError if user.two_factor_enabled?
+
+        Gitlab::Auth::Result.new(user, nil, :gitlab_or_ldap, full_authentication_abilities)
       end
       def oauth_access_token_check(login, password)
@@ -102,7 +89,7 @@ module Gitlab
           token = Doorkeeper::AccessToken.by_token(password)
           if token && token.accessible?
             user = User.find_by(id: token.resource_owner_id)
-            Result.new(user, :oauth)
+            Gitlab::Auth::Result.new(user, nil, :oauth, read_authentication_abilities)
           end
         end
       end
@@ -111,9 +98,52 @@ module Gitlab
         if login && password
           user = User.find_by_personal_access_token(password)
           validation = User.by_login(login)
-          Result.new(user, :personal_token) if user == validation
+          Gitlab::Auth::Result.new(user, nil, :personal_token, full_authentication_abilities) if user.present? && user == validation
+        end
+      end
+
+      def build_access_token_check(login, password)
+        return unless login == 'gitlab-ci-token'
+        return unless password
+
+        build = ::Ci::Build.running.find_by_token(password)
+        return unless build
+        return unless build.project.builds_enabled?
+
+        if build.user
+          # If user is assigned to build, use restricted credentials of user
+          Gitlab::Auth::Result.new(build.user, build.project, :build, build_authentication_abilities)
+        else
+          # Otherwise use generic CI credentials (backward compatibility)
+          Gitlab::Auth::Result.new(nil, build.project, :ci, build_authentication_abilities)
         end
       end
+
+      public
+
+      def build_authentication_abilities
+        [
+          :read_project,
+          :build_download_code,
+          :build_read_container_image,
+          :build_create_container_image
+        ]
+      end
+
+      def read_authentication_abilities
+        [
+          :read_project,
+          :download_code,
+          :read_container_image
+        ]
+      end
+
+      def full_authentication_abilities
+        read_authentication_abilities + [
+          :push_code,
+          :create_container_image
+        ]
+      end
     end
   end
 end

lib/gitlab/auth/result.rb

+module Gitlab
+  module Auth
+    Result = Struct.new(:actor, :project, :type, :authentication_abilities) do
+      def ci?
+        type == :ci
+      end
+
+      def success?
+        actor.present? || type == :ci
+      end
+    end
+  end
+end

lib/gitlab/git_access.rb

@@ -5,12 +5,13 @@ module Gitlab
     DOWNLOAD_COMMANDS = %w{ git-upload-pack git-upload-archive }
     PUSH_COMMANDS = %w{ git-receive-pack }
-    attr_reader :actor, :project, :protocol, :user_access
-    def initialize(actor, project, protocol)
+    attr_reader :actor, :project, :protocol, :user_access, :authentication_abilities
+    def initialize(actor, project, protocol, authentication_abilities:)
       @actor    = actor
       @project  = project
       @protocol = protocol
+      @authentication_abilities = authentication_abilities
       @user_access = UserAccess.new(user, project: project)
     end
@@ -60,14 +61,26 @@ module Gitlab
     end
     def user_download_access_check
-      unless user_access.can_do_action?(:download_code)
+      unless user_can_download_code? || build_can_download_code?
         return build_status_object(false, "You are not allowed to download code from this project.")
       end
       build_status_object(true)
     end
+    def user_can_download_code?
+      authentication_abilities.include?(:download_code) && user_access.can_do_action?(:download_code)
+    end
+
+    def build_can_download_code?
+      authentication_abilities.include?(:build_download_code) && user_access.can_do_action?(:build_download_code)
+    end
+
     def user_push_access_check(changes)
+      unless authentication_abilities.include?(:push_code)
+        return build_status_object(false, "You are not allowed to upload code for this project.")
+      end
+
       if changes.blank?
         return build_status_object(true)
       end

spec/lib/ci/mask_secret_spec.rb

+require 'spec_helper'
+
+describe Ci::MaskSecret, lib: true do
+  subject { described_class }
+
+  describe '#mask' do
+    it 'masks exact number of characters' do
+      expect(subject.mask('token', 'oke')).to eq('txxxn')
+    end
+
+    it 'masks multiple occurrences' do
+      expect(subject.mask('token token token', 'oke')).to eq('txxxn txxxn txxxn')
+    end
+
+    it 'does not mask if not found' do
+      expect(subject.mask('token', 'not')).to eq('token')
+    end
+  end
+end