Skip to content

Scan Result Policy - `group_approvers` string matches across all namespaces

Summary

If you add compliance to the group_approvers in a Scan Result Policy, the approval groups added will match the group name compliance, however it adds multiple groups from vastly different namespaces.

https://gitlab.com/gitlab-org/gitlab/-/blob/28e56a69561a334186ba15272e6a2af23545f28f/ee/app/services/security/security_orchestration_policies/fetch_policy_approvers_service.rb#L53

This issue was discovered by a customer.

Customer is interested in the outcome of this issue, so please change to non-confidential if this is not required. I made this confidential out of an abundance of caution.

Solution proposal

We have implemented an admin feature that allows instance owners to toggle the search to be open to search across namespaces, or closed to restrict searching to within a namespace.

For GitLab.com, we've enabled this feature to address the bug. For self-managed users, this can be used as a feature to restrict searching across namespaces where it may be appropriate.

The setting security_policy_global_group_approvers_enabled is documented under Application settings API. The page also explains how to Change application settings.

Steps to reproduce

  1. Create a new project
  2. Create a new Scan Result Policy. In YAML view, change group_approvers so it has ["compliance"]
  3. Save the Scan Result Policy, merge via MR.
  4. Edit the policy you've just added in the project. Confirm groups added as approvers.

Example Project

https://gitlab.com/gitlab-gold/tmike/zd323145/zd323145-2/zd323145-2/

What is the current bug behavior?

Groups named compliance are matched but from a lot of different namespaces:

  • anthonybaer/gitlab-evaluation/compliance
  • gaia-x/lab/compliance
  • gitlab-org/govern/compliance
  • pensando/tbd/compliance
  • gitlab-gold/briecarranza/tickets/compliance
  • blee-group/demos/security/compliance

What is the expected correct behavior?

Typically users for self managed instances want to preserve the current functionality; however, for shared instances such as GitLab SaaS, this is viewed as a bug.

This should be an instance-level Application Setting to control whether or not the search extends across all namespaces. The setting should default to the current behavior (searching all namespaces). Once this new setting is introduced, we will change the setting for GitLab SaaS to the non-default value to prevent searching outside of the current namespace.

Relevant logs and/or screenshots

Output of checks

Results of GitLab environment info

Expand for output related to GitLab environment info

(For installations with omnibus-gitlab package run and paste the output of:
`sudo gitlab-rake gitlab:env:info`)

(For installations from source run and paste the output of:
`sudo -u git -H bundle exec rake gitlab:env:info RAILS_ENV=production`)

Results of GitLab application Check

Expand for output related to the GitLab application check

(For installations with omnibus-gitlab package run and paste the output of: sudo gitlab-rake gitlab:check SANITIZE=true)

(For installations from source run and paste the output of: sudo -u git -H bundle exec rake gitlab:check RAILS_ENV=production SANITIZE=true)

(we will only investigate if the tests are passing)

Implementation plan

  • backend Create a new application_setting security_policy_global_group_approvers_enabled and make it disabled by default for gitlab.com
class AddGlobalGroupApproversEnabledToApplicationSettings < Gitlab::Database::Migration[2.0]
  def change
    add_column :application_settings, :security_policy_global_group_approvers_enabled, :boolean, default: false, null: false
  end
end
diff --git a/ee/app/services/security/security_orchestration_policies/fetch_policy_approvers_service.rb b/ee/app/services/security/security_orchestration_policies/fetch_policy_approvers_service.rb
index 7f23f91a070b..3e968eeb3df0 100644
--- a/ee/app/services/security/security_orchestration_policies/fetch_policy_approvers_service.rb
+++ b/ee/app/services/security/security_orchestration_policies/fetch_policy_approvers_service.rb
@@ -50,7 +50,14 @@ def group_approvers(action)
         # Using GroupFinder here would make groups more restrictive than current features related to others approval project rules as in:
         # https://gitlab.com/gitlab-org/gitlab/-/blob/0aa924eaa1a4ca5ed6b226d826f7298ec847ea5f/ee/app/services/concerns/approval_rules/updater.rb#L44
         # Therefore data migrated from Vulnerability-Check into Scan result policies would be inconsistent.
-        Group.public_or_visible_to_user(current_user).by_ids_or_paths(group_ids, group_paths)
+
+        if (Gitlab::CurrentSettings.security_policy_global_group_approvers_enabled)
+          Group.public_or_visible_to_user(current_user).by_ids_or_paths(group_ids, group_paths)
+        else
+          container.root_ancestor.self_and_descendants.select(:id)
+          .public_or_visible_to_user(current_user)
+          .by_ids_or_paths(group_ids, group_paths)
+        end
       end
       # rubocop: enable Cop/GroupPublicOrVisibleToUser
diff --git a/ee/app/helpers/ee/security_orchestration_helper.rb b/ee/app/helpers/ee/security_orchestration_helper.rb
index a4cb77d573f6..b9c37eb86bba 100644
--- a/ee/app/helpers/ee/security_orchestration_helper.rb
+++ b/ee/app/helpers/ee/security_orchestration_helper.rb
@@ -34,7 +34,9 @@ def orchestration_policy_data(container, policy_type = nil, policy = nil, approv
       policy_editor_empty_state_svg_path: image_path('illustrations/monitoring/unable_to_connect.svg'),
       policy_type: policy_type,
       scan_policy_documentation_path: help_page_path('user/application_security/policies/index'),
-      scan_result_approvers: approvers&.to_json
+      scan_result_approvers: approvers&.to_json,
+      global_group_approvers_enabled: Gitlab::CurrentSettings.security_policy_global_group_approvers_enabled,
+      root_namespace_path: container.root_ancestor&.full_path
     }
 
     if container.is_a?(::Project)
query {
  group(fullPath:$rootNamespacePath){
    descendantGroups(search:$searchTerm) {
      nodes{
        name
      }
    }
  }
}
Edited by Grant Hickman