Improve Segregation of Duties for Operators

Closing in Favor of Two Smaller Issues

Problem to solve

Users that have no access to modify the code (Reporters) ought to still be able to Approve MRs and Deploy to Protected Environments when they are designated as approvers or deployers

When an operator is “allowed to deploy” to production, they need developer access and the ability to push/merge into the branch (if it's protected) in order to deploy into protected environments. This permits operators to commit code, causing auditing concerns.

A similar issue exists when a non-code-contributing Approver for merge requests is required. They must be developers to approve Merge Requests.

Intended users

Further details

We will need to add a configuration option for groups to select maintainers cannot deploy, and only this deployer role can deploy.

Scenario:

Developers (with developer access) to a project ought to be allowed to develop, push/merge into protected branches but NOT deploy to production. This is accomplished by pairing Protected Branches and Protected Environments with an externalized CI YAML.

Operators (with reporter access) to a project ought not allowed to push/merge any code to any branches but CAN deploy to production. When Developers merge to the protected branch, currently, the Operator cannot deploy to production unless they have the ability to 'modify' the protected branch.

Sample Projects:

Screenshots:

Here's an example of a Operator ("allowed to deploy" but without access to the protected branch - not set to merge/push)

a Operator ("allowed to deploy" and with access to the protected branch - set to merge/push - but with reporter/guest access)

Here's an example of an Operator ("allowed to deploy" and with access to the protected branch - this is not preferred)

Merge Request Approvers

Here's an example of being logged in as an approver - Tim Poffenbarger Tester - (allowed to approve but cannot because user is reporter/guest)

Here's an example of being logged in as an approver - Tim Poffenbarger Tester - (allowed to approve because is a developer - this is not preferred)

Proposal

The Gist: Approval/Deployment Rule Designations Start with Reporters, not Developers

The Protected Environments "allowed to deploy" list of people ought to be allowed to deploy in any scenario as long as they are a reporter.
The Merge Requests Approvals (including for CODEOWNER rules) user/group list of people ought to be allowed to approve in any scenario as long as they are a reporter.

Suggested Change for Approval Rule

ee/app/policies/ee/merge_request_policy.rb:

added

      rule { can?(:read_merge_request) }.policy do
        enable :approve_merge_request
      end`

# frozen_string_literal: true

module EE
  module MergeRequestPolicy
    extend ActiveSupport::Concern

    prepended do
      with_scope :subject
      condition(:can_override_approvers, score: 0) do
        @subject.target_project&.can_override_approvers?
      end

      rule { ~can_override_approvers }.prevent :update_approvers
      rule { can?(:update_merge_request) }.policy do
        enable :update_approvers
        enable :approve_merge_request
      end

      rule { can?(:read_merge_request) }.policy do
        enable :approve_merge_request
      end
    end
  end
end

Suggested Change for Permitting reporters to deploy

ee/app/assets/javascripts/projects/settings/access_dropdown.js:

  getUsers(query) {
    return axios.get(this.buildUrl(gon.relative_url_root, this.usersPath), {
      params: {
        search: query,
        per_page: 20,
        active: true,
        project_id: gon.current_project_id,
        read_build: true,
      },
    });
  }

app/policies/ci/build_policy.rb:

    rule { can?(:update_build) & (protected_ref | archived) }.policy do
      prevent :update_build
      prevent :update_commit_status
      prevent :erase_build
    end

ee/app/policies/ee/ci/build_policy.rb:

        # ...

        condition(:deployable_by_reporter) { deployable_by_reporter? }

        rule { deployable_by_reporter }.policy do
          enable :update_build
          enable :update_commit_status
        end

        # ...

        def deployable_by_reporter?
          # We need to check if Protected Environments feature is available,
          # as evaluating `build.expanded_environment_name` is expensive.
          return true unless build.project.protected_environments_feature_available?

          build.project.protected_environment_enforced?(build.persisted_environment&.name, user)
        end

        # ...

ee/app/models/ee/project.rb:

    def protected_environment_enforced?(environment_name, user)
      protected_environment = protected_environment_by_name(environment_name)

      protected_environment && protected_environment.accessible_to?(user)
    end

ee/app/models/protected_environment/deploy_access_level.rb:

  ALLOWED_ACCESS_LEVELS = [
    Gitlab::Access::MAINTAINER,
    Gitlab::Access::DEVELOPER,
    Gitlab::Access::REPORTER,
    Gitlab::Access::ADMIN
  ].freeze

Permissions and Security

The proposed solution does introduce an inconsistency given that it's an outlier that "allowed to deploy" users now have an elevated level of access specific to deployments. However, without it, I do not see a way to provide true segregation of duties.

The permissions are paired within the Core Policies and the EE Policies.

Documentation

Availability & Testing

What does success look like, and how can we measure that?

Operators will no longer need access to modify source code.

What is the type of buyer?

Silver/Premium and above given that this requires the Protected Environments feature.

Links / references

Edited Jul 01, 2020 by Tim Poffenbarger