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:
- https://gitlab.com/gitlab-silver/tpoffenbarger/separation-of-duties
- https://gitlab.com/gitlab-silver/tpoffenbarger/separation-of-duties-deploy
Screenshots:
Here's an example of a Operator ("allowed to deploy" but without access to the protected branch - not set to merge/push)
OR
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`
in
# 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
# ...
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.