Next

Summary

In order to allow teams to focus on the most critical security items and avoid things being lost due to volume of results, GitLab now allows customers to automatically approve specifically identified merge requests if there are no vulnerabilities introduced of a specified severity. This will enable a workflow that would allow Security teams to be involved in Merge Request Approval workflow without having to approve every single Merge Request.

Problem to solve

Security checks are performed during the pipeline execution, and reports are available in the merge request widget. GitLab is able to determine if new vulnerabilities have been introduced into the code because of the specific changes of the feature branch.

Our security paradigm states that we don't want to make a pipeline fail and block the entire development process if some vulnerability is found. They could be false positive, or just something that is not more important than the development velocity.

On the other side, we're getting a lot of feedback that unsecure code should not be allowed unless previously approved by the security team. This is considered a strict requirement by some customer, and also a must-have by analysts. Security teams don't want to be involved in each and every merge request, but only if security flaws have been found.

GitLab recently added the feature of Merge Request Approval rules (including adding multiple approval rules) that would enable approval being required by specific individuals on a merge request. The pain point is that in practice, a Security Team would have to review every single merge request, and what they are interested in doing is setting a threshold for when a review is required.

We strongly believe that our security paradigm is valid, but we should consider something that allows companies to deal with their compliance requirements and ensure they are able to use GitLab.

Target audience

• Sam, Security Analyst, https://design.gitlab.com/research/personas#persona-sam

Proposal

We recently released multiple approval rules, and we can leverage this feature to build some advanced logic on top of it.

We can use multiple merge request approval rules to create an MVC that auto-approves specifically identified Approvals if there are no vulnerabilities introduced of a certain severity. This will enable a workflow that would allow Security teams to be involved in Merge Request Approval without having to approve every single Merge Request.

Proposed solution

• Default to displaying a Vulnerability-Check Merge Request Approval rule with no Approvers added with a tool-tip to point you to documentation on what the Merge Request Approval rule provides
• Administrators can update the number of Approvals required to activate the rule and add appropriate approvers using the standard Merge Request approval rule UI
• Merge requests themselves utilize existing approval rules UI
• The removing of the required approval is performed by lowering the approvals_required to zero for the Vulnerability-Check approval rule when no Critical, High, or Unknown vulnerabilities are introduced
• Vulnerability Dismissal does not affect whether approvals_required is adjusted to zero
• Usage ping with total number of projects with a Vulnerability-Check project rule that exists with an approval count greater than 0

This avoids the confusing "Double Set of Approvers" or "Special Approvers" that was mentioned in the discovery.

Out of Scope

• Compliance focused reporting on merge approvals
• Selection Vulnerability Severity level
• Adding auto-approval to non-standard (in this case Vulnerability-Check) Approvals

Additional documentation

We should be careful to communicate to users that one Merge request approvals section checkbox might lead to unexpected behavior in this workflow. Allowing users to adjust Approvals in specific Merge Requests could enable un-authorized adjustment to approvers to subvert the workflow.

User Interface

1. Vulnerability-Check approver group rule seen by default in approver menu

2. User opened the Vulnerability-Check group (clicked "edit"), adjusts to 1+ required approvers to activate and selects eligible approvers (name input with Vulnerability-Check disabled)

3. Vulnerability-Check is active, ? on hover displays tooltip explaining the explicit rule

Development Log

Status

• backend refactoring of existing Approval Rules implementation to support new rule types https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/13036
• backend addition of report_type field within approval_merge_request_rules https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/14050
• backend work nearly feature-complete, ongoing discussion on implementation and refactoring https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/13109
• frontend https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/14038
• backend work to fix issue introduced where default rule_type was being overridden https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/30575
• ~Documentation https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/30959
• backend frontend MR to remove feature flag, activating feature by default https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/15087

Decisions

• Add default "security approver rule" row to UI (decision was made but missed in implementation, see below)
• Include unknown-severity in vulnerability checks
• Exclude undefined-severity in vulnerability checks
• "Dismissing feedback" will not be disabled when Security Approvals are enabled. Many edgecases need to be considered still
• Remove MR approval rules when project approval rule is deleted
• Do not create approval rules for open MRs when project approval rule is added (deferred due to complexity)
• The existing validation preventing override rules from being less strict than project rules will be bypassed and not apply to report_approver rules
• We will introduce a rule_type to approval_project_rules to reduce technical debt and better support complex rule types later
• We will not be shipping the placeholder row in the UI as this was missed and won't make the cutoff for %12.1. After research in ux-research#206 (closed) this should be reconsidered

Documentation

• https://docs.gitlab.com/ee/user/application_security/index.html#security-approvals-in-merge-requests