From b6b4e8f54c1bb5c4ecb14a7ee4682f4b1754be2f Mon Sep 17 00:00:00 2001 From: Serena Fang Date: Mon, 25 Mar 2024 20:00:40 -0500 Subject: [PATCH 01/12] Add skip secret detection audit event WIP, need MR id Changelog: added EE: true --- ee/lib/gitlab/checks/secrets_check.rb | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/ee/lib/gitlab/checks/secrets_check.rb b/ee/lib/gitlab/checks/secrets_check.rb index aca17149d6164e..3f12ed81e10e85 100644 --- a/ee/lib/gitlab/checks/secrets_check.rb +++ b/ee/lib/gitlab/checks/secrets_check.rb @@ -40,15 +40,18 @@ def validate! # 1. unless application setting is enabled (regardless of whether it's a gitlab dedicated instance or not) # 2. feature flag is disabled for this project (when instance type is not gitlab dedicated) # 3. license is not ultimate - return unless ::Gitlab::CurrentSettings.pre_receive_secret_detection_enabled + # return unless ::Gitlab::CurrentSettings.pre_receive_secret_detection_enabled - return unless ::Gitlab::CurrentSettings.gitlab_dedicated_instance || - ::Feature.enabled?(:pre_receive_secret_detection_push_check, project) + # return unless ::Gitlab::CurrentSettings.gitlab_dedicated_instance || + # ::Feature.enabled?(:pre_receive_secret_detection_push_check, project) - return unless project.licensed_feature_available?(:pre_receive_secret_detection) + # return unless project.licensed_feature_available?(:pre_receive_secret_detection) # Skip if any commit has the special bypass flag `[skip secret detection]` - return if skip_secret_detection? + if skip_secret_detection? + log_audit_event + return + end logger.log_timed(LOG_MESSAGES[:secrets_check]) do blobs = ::Gitlab::Checks::ChangedBlobs.new( @@ -83,6 +86,10 @@ def secret_detection_logger @secret_detection_logger ||= ::Gitlab::SecretDetectionLogger.build end + def log_audit_event + # WIP, need MR id + end + def format_response(response) # Try to retrieve file path and commit sha for the blobs found. if [ -- GitLab From 4660039253f471752b578e0a879e16c0276f081f Mon Sep 17 00:00:00 2001 From: Serena Fang Date: Mon, 25 Mar 2024 20:08:02 -0500 Subject: [PATCH 02/12] Add secret detection audit event yml and doc --- doc/administration/audit_event_types.md | 6 ++++++ .../types/skip_pre_receive_secret_detection.yml | 10 ++++++++++ 2 files changed, 16 insertions(+) create mode 100644 ee/config/audit_events/types/skip_pre_receive_secret_detection.yml diff --git a/doc/administration/audit_event_types.md b/doc/administration/audit_event_types.md index 83cda27a0b9f28..250a73186e3164 100644 --- a/doc/administration/audit_event_types.md +++ b/doc/administration/audit_event_types.md @@ -385,6 +385,12 @@ Audit event types belong to the following product categories. |:------------|:------------|:------------------|:---------|:--------------|:--------------| | [`set_runner_associated_projects`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/97666) | Event triggered on successful assignment of associated projects to a CI runner| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.4](https://gitlab.com/gitlab-org/gitlab/-/issues/359958) | User | +### Secret detection + +| Name | Description | Saved to database | Streamed | Introduced in | Scope | +|:------------|:------------|:------------------|:---------|:--------------|:--------------| +| [`skip_pre_receive_secret_detection`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/147855) | Triggered when pre-receive secret detection is skipped by the user| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [16.11](https://gitlab.com/gitlab-org/gitlab/-/issues/441185) | Project | + ### Security policy management | Name | Description | Saved to database | Streamed | Introduced in | Scope | diff --git a/ee/config/audit_events/types/skip_pre_receive_secret_detection.yml b/ee/config/audit_events/types/skip_pre_receive_secret_detection.yml new file mode 100644 index 00000000000000..832cf96a4a8cfa --- /dev/null +++ b/ee/config/audit_events/types/skip_pre_receive_secret_detection.yml @@ -0,0 +1,10 @@ +--- +name: skip_pre_receive_secret_detection +description: Triggered when pre-receive secret detection is skipped by the user +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/441185 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/147855 +feature_category: secret_detection +milestone: '16.11' +saved_to_database: true +streamed: true +scope: [Project] -- GitLab From e9b9797c41d57e98c2e71a8be60a56ee34145158 Mon Sep 17 00:00:00 2001 From: Serena Fang Date: Mon, 25 Mar 2024 21:11:24 -0500 Subject: [PATCH 03/12] Fix message format --- ee/lib/gitlab/checks/secrets_check.rb | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/ee/lib/gitlab/checks/secrets_check.rb b/ee/lib/gitlab/checks/secrets_check.rb index 3f12ed81e10e85..aada48c3165dc1 100644 --- a/ee/lib/gitlab/checks/secrets_check.rb +++ b/ee/lib/gitlab/checks/secrets_check.rb @@ -49,7 +49,7 @@ def validate! # Skip if any commit has the special bypass flag `[skip secret detection]` if skip_secret_detection? - log_audit_event + log_audit_event("commit message") return end @@ -86,8 +86,18 @@ def secret_detection_logger @secret_detection_logger ||= ::Gitlab::SecretDetectionLogger.build end - def log_audit_event - # WIP, need MR id + def log_audit_event(skip_method) + message = "Pre-receive secret detection skipped by user via #{skip_method}" + + audit_context = { + name: "skip_pre_receive_secret_detection", + author: changes_access.user_access.user, + target: project, + scope: project, + message: _(message) + } + + ::Gitlab::Audit::Auditor.audit(audit_context) end def format_response(response) -- GitLab From 402d117bc1fb52dc5ecff2db008fa147b47a95c4 Mon Sep 17 00:00:00 2001 From: Serena Fang Date: Tue, 26 Mar 2024 19:25:33 -0500 Subject: [PATCH 04/12] Add prsd to application settings for audit event --- app/helpers/application_settings_helper.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb index 242bde1fb69747..6cd85ad3206238 100644 --- a/app/helpers/application_settings_helper.rb +++ b/app/helpers/application_settings_helper.rb @@ -347,6 +347,7 @@ def visible_attributes :diagramsnet_enabled, :diagramsnet_url, :polling_interval_multiplier, + :pre_receive_secret_detection_enabled, :project_export_enabled, :prometheus_metrics_enabled, :recaptcha_enabled, -- GitLab From 80907e7e00469b2e83d022da17faa66322e80cb2 Mon Sep 17 00:00:00 2001 From: Serena Fang Date: Tue, 26 Mar 2024 19:37:00 -0500 Subject: [PATCH 05/12] Add skip sd audit event spec --- .../lib/gitlab/secrets_check_shared_examples.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/ee/spec/support/shared_examples/lib/gitlab/secrets_check_shared_examples.rb b/ee/spec/support/shared_examples/lib/gitlab/secrets_check_shared_examples.rb index 8ea908f3aa8e29..f7e636a2eeccbf 100644 --- a/ee/spec/support/shared_examples/lib/gitlab/secrets_check_shared_examples.rb +++ b/ee/spec/support/shared_examples/lib/gitlab/secrets_check_shared_examples.rb @@ -1009,4 +1009,9 @@ expect { subject.validate! }.not_to raise_error end end + + it 'creates an audit event' do + expect { subject.validate! }.to change { AuditEvent.count }.by(1) + expect(AuditEvent.last.details).to include("Pre-receive secret detection skipped by user via commit message") + end end -- GitLab From ef622acc28d79efad17bbf4ee17a51e14a8efe57 Mon Sep 17 00:00:00 2001 From: Serena Fang Date: Tue, 26 Mar 2024 19:39:01 -0500 Subject: [PATCH 06/12] Uncomment stuff that made testing easier --- ee/lib/gitlab/checks/secrets_check.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/ee/lib/gitlab/checks/secrets_check.rb b/ee/lib/gitlab/checks/secrets_check.rb index aada48c3165dc1..4d2a51c9aa613a 100644 --- a/ee/lib/gitlab/checks/secrets_check.rb +++ b/ee/lib/gitlab/checks/secrets_check.rb @@ -40,12 +40,12 @@ def validate! # 1. unless application setting is enabled (regardless of whether it's a gitlab dedicated instance or not) # 2. feature flag is disabled for this project (when instance type is not gitlab dedicated) # 3. license is not ultimate - # return unless ::Gitlab::CurrentSettings.pre_receive_secret_detection_enabled + return unless ::Gitlab::CurrentSettings.pre_receive_secret_detection_enabled - # return unless ::Gitlab::CurrentSettings.gitlab_dedicated_instance || - # ::Feature.enabled?(:pre_receive_secret_detection_push_check, project) + return unless ::Gitlab::CurrentSettings.gitlab_dedicated_instance || + ::Feature.enabled?(:pre_receive_secret_detection_push_check, project) - # return unless project.licensed_feature_available?(:pre_receive_secret_detection) + return unless project.licensed_feature_available?(:pre_receive_secret_detection) # Skip if any commit has the special bypass flag `[skip secret detection]` if skip_secret_detection? -- GitLab From a86bbf45631279857a822f18c0135069c0af7236 Mon Sep 17 00:00:00 2001 From: Serena Fang Date: Tue, 26 Mar 2024 21:31:15 -0500 Subject: [PATCH 07/12] Update audit message --- app/helpers/application_settings_helper.rb | 1 - ee/app/helpers/ee/application_settings_helper.rb | 9 +++++++++ ee/lib/gitlab/checks/secrets_check.rb | 2 +- .../lib/gitlab/secrets_check_shared_examples.rb | 2 +- 4 files changed, 11 insertions(+), 3 deletions(-) diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb index 6cd85ad3206238..242bde1fb69747 100644 --- a/app/helpers/application_settings_helper.rb +++ b/app/helpers/application_settings_helper.rb @@ -347,7 +347,6 @@ def visible_attributes :diagramsnet_enabled, :diagramsnet_url, :polling_interval_multiplier, - :pre_receive_secret_detection_enabled, :project_export_enabled, :prometheus_metrics_enabled, :recaptcha_enabled, diff --git a/ee/app/helpers/ee/application_settings_helper.rb b/ee/app/helpers/ee/application_settings_helper.rb index faa63c87fe0067..c7dabfbe890973 100644 --- a/ee/app/helpers/ee/application_settings_helper.rb +++ b/ee/app/helpers/ee/application_settings_helper.rb @@ -80,6 +80,8 @@ def visible_attributes :zoekt_search_enabled ].tap do |settings| settings.concat(identity_verification_attributes) + settings.concat(pre_receive_secret_detection_setting) + :lock_duo_features_enabled end end @@ -243,5 +245,12 @@ def identity_verification_attributes telesign_customer_xid ] end + + def pre_receive_secret_detection_setting + return [] unless ::Gitlab::CurrentSettings.gitlab_dedicated_instance && + License.feature_available?(:pre_receive_secret_detection) + + [:pre_receive_secret_detection_enabled] + end end end diff --git a/ee/lib/gitlab/checks/secrets_check.rb b/ee/lib/gitlab/checks/secrets_check.rb index 4d2a51c9aa613a..c69e7fa2e61d4a 100644 --- a/ee/lib/gitlab/checks/secrets_check.rb +++ b/ee/lib/gitlab/checks/secrets_check.rb @@ -87,7 +87,7 @@ def secret_detection_logger end def log_audit_event(skip_method) - message = "Pre-receive secret detection skipped by user via #{skip_method}" + message = "Pre-receive secret detection skipped via #{skip_method}" audit_context = { name: "skip_pre_receive_secret_detection", diff --git a/ee/spec/support/shared_examples/lib/gitlab/secrets_check_shared_examples.rb b/ee/spec/support/shared_examples/lib/gitlab/secrets_check_shared_examples.rb index f7e636a2eeccbf..36dafb1c008fac 100644 --- a/ee/spec/support/shared_examples/lib/gitlab/secrets_check_shared_examples.rb +++ b/ee/spec/support/shared_examples/lib/gitlab/secrets_check_shared_examples.rb @@ -1012,6 +1012,6 @@ it 'creates an audit event' do expect { subject.validate! }.to change { AuditEvent.count }.by(1) - expect(AuditEvent.last.details).to include("Pre-receive secret detection skipped by user via commit message") + expect(AuditEvent.last.details[:custom_message]).to eq("Pre-receive secret detection skipped via commit message") end end -- GitLab From 3176d12cd816239bd9db355d0187d1c249ac9873 Mon Sep 17 00:00:00 2001 From: Serena Fang Date: Tue, 26 Mar 2024 21:52:15 -0500 Subject: [PATCH 08/12] Clean up syntax --- ee/app/helpers/ee/application_settings_helper.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/ee/app/helpers/ee/application_settings_helper.rb b/ee/app/helpers/ee/application_settings_helper.rb index c7dabfbe890973..ec3fdced20c772 100644 --- a/ee/app/helpers/ee/application_settings_helper.rb +++ b/ee/app/helpers/ee/application_settings_helper.rb @@ -6,7 +6,7 @@ module ApplicationSettingsHelper override :visible_attributes def visible_attributes - super + [ + attrs = super + [ :allow_group_owners_to_manage_ldap, :automatic_purchased_storage_allocation, :check_namespace_plan, @@ -83,6 +83,8 @@ def visible_attributes settings.concat(pre_receive_secret_detection_setting) :lock_duo_features_enabled end + + attrs end def elasticsearch_objects_options(objects) -- GitLab From 2000a0c6eb6ce17e328ca2f9d653540c88717ab5 Mon Sep 17 00:00:00 2001 From: Serena Fang Date: Tue, 2 Apr 2024 00:15:23 +0000 Subject: [PATCH 09/12] Apply reviewer feedback --- ee/app/helpers/ee/application_settings_helper.rb | 4 +--- ee/lib/gitlab/checks/secrets_check.rb | 6 +++--- locale/gitlab.pot | 6 ++++++ 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/ee/app/helpers/ee/application_settings_helper.rb b/ee/app/helpers/ee/application_settings_helper.rb index ec3fdced20c772..c7dabfbe890973 100644 --- a/ee/app/helpers/ee/application_settings_helper.rb +++ b/ee/app/helpers/ee/application_settings_helper.rb @@ -6,7 +6,7 @@ module ApplicationSettingsHelper override :visible_attributes def visible_attributes - attrs = super + [ + super + [ :allow_group_owners_to_manage_ldap, :automatic_purchased_storage_allocation, :check_namespace_plan, @@ -83,8 +83,6 @@ def visible_attributes settings.concat(pre_receive_secret_detection_setting) :lock_duo_features_enabled end - - attrs end def elasticsearch_objects_options(objects) diff --git a/ee/lib/gitlab/checks/secrets_check.rb b/ee/lib/gitlab/checks/secrets_check.rb index c69e7fa2e61d4a..d2eb292e6deb06 100644 --- a/ee/lib/gitlab/checks/secrets_check.rb +++ b/ee/lib/gitlab/checks/secrets_check.rb @@ -49,7 +49,7 @@ def validate! # Skip if any commit has the special bypass flag `[skip secret detection]` if skip_secret_detection? - log_audit_event("commit message") + log_audit_event(_("commit message")) return end @@ -87,14 +87,14 @@ def secret_detection_logger end def log_audit_event(skip_method) - message = "Pre-receive secret detection skipped via #{skip_method}" + message = "#{_('Pre-receive secret detection skipped via')} #{skip_method}" audit_context = { name: "skip_pre_receive_secret_detection", author: changes_access.user_access.user, target: project, scope: project, - message: _(message) + message: message } ::Gitlab::Audit::Auditor.audit(audit_context) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 40baef0b303784..4cd62ca042935e 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -38163,6 +38163,9 @@ msgstr "" msgid "Pre-defined push rules" msgstr "" +msgid "Pre-receive secret detection skipped via" +msgstr "" + msgid "PreScanVerification|(optional)" msgstr "" @@ -60211,6 +60214,9 @@ msgstr[1] "" msgid "commit %{commit_id}" msgstr "" +msgid "commit message" +msgstr "" + msgid "committed" msgstr "" -- GitLab From 175cd8abf66940c762acb075412a8a8e013e592e Mon Sep 17 00:00:00 2001 From: Serena Fang Date: Thu, 4 Apr 2024 15:18:03 -0500 Subject: [PATCH 10/12] Move lcok duo features --- ee/app/helpers/ee/application_settings_helper.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/ee/app/helpers/ee/application_settings_helper.rb b/ee/app/helpers/ee/application_settings_helper.rb index c7dabfbe890973..fa0a95982dfea0 100644 --- a/ee/app/helpers/ee/application_settings_helper.rb +++ b/ee/app/helpers/ee/application_settings_helper.rb @@ -81,7 +81,6 @@ def visible_attributes ].tap do |settings| settings.concat(identity_verification_attributes) settings.concat(pre_receive_secret_detection_setting) - :lock_duo_features_enabled end end -- GitLab From 30b7c792d3ca4cee0a1e08cd9dfad835f845529c Mon Sep 17 00:00:00 2001 From: Serena Fang Date: Fri, 12 Apr 2024 16:59:18 -0500 Subject: [PATCH 11/12] Fix rubocop whitespace --- ee/app/helpers/ee/application_settings_helper.rb | 8 -------- ee/app/models/ee/application_setting.rb | 8 +++++++- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/ee/app/helpers/ee/application_settings_helper.rb b/ee/app/helpers/ee/application_settings_helper.rb index fa0a95982dfea0..faa63c87fe0067 100644 --- a/ee/app/helpers/ee/application_settings_helper.rb +++ b/ee/app/helpers/ee/application_settings_helper.rb @@ -80,7 +80,6 @@ def visible_attributes :zoekt_search_enabled ].tap do |settings| settings.concat(identity_verification_attributes) - settings.concat(pre_receive_secret_detection_setting) end end @@ -244,12 +243,5 @@ def identity_verification_attributes telesign_customer_xid ] end - - def pre_receive_secret_detection_setting - return [] unless ::Gitlab::CurrentSettings.gitlab_dedicated_instance && - License.feature_available?(:pre_receive_secret_detection) - - [:pre_receive_secret_detection_enabled] - end end end diff --git a/ee/app/models/ee/application_setting.rb b/ee/app/models/ee/application_setting.rb index 7977f148868fe3..1fc3d331269ae4 100644 --- a/ee/app/models/ee/application_setting.rb +++ b/ee/app/models/ee/application_setting.rb @@ -186,7 +186,8 @@ module ApplicationSetting if: :email_confirmation_setting_soft? validates :pre_receive_secret_detection_enabled, - inclusion: { in: [true, false], message: N_('must be a boolean value') } + inclusion: { in: [true, false], message: N_('must be a boolean value') }, + if: :pre_receive_secret_detection_available? validates :instance_level_ai_beta_features_enabled, allow_nil: false, @@ -566,5 +567,10 @@ def check_elasticsearch_url_scheme rescue ::Gitlab::HTTP_V2::UrlBlocker::BlockedUrlError errors.add(:elasticsearch_url, "only supports valid HTTP(S) URLs.") end + + def pre_receive_secret_detection_available? + ::Gitlab::CurrentSettings.gitlab_dedicated_instance && + License.feature_available?(:pre_receive_secret_detection) # rubocop:disable Gitlab/LicenseAvailableUsage -- Does not have cyclical dependency + end end end -- GitLab From 7d1f88f392c762d30bb8b6bce4e26b9ef9a5596d Mon Sep 17 00:00:00 2001 From: Serena Fang Date: Fri, 12 Apr 2024 18:21:57 -0500 Subject: [PATCH 12/12] Fix application setting validation --- ee/app/models/ee/application_setting.rb | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/ee/app/models/ee/application_setting.rb b/ee/app/models/ee/application_setting.rb index 1fc3d331269ae4..0f56716e739302 100644 --- a/ee/app/models/ee/application_setting.rb +++ b/ee/app/models/ee/application_setting.rb @@ -187,7 +187,7 @@ module ApplicationSetting validates :pre_receive_secret_detection_enabled, inclusion: { in: [true, false], message: N_('must be a boolean value') }, - if: :pre_receive_secret_detection_available? + if: :gitlab_dedicated_instance validates :instance_level_ai_beta_features_enabled, allow_nil: false, @@ -567,10 +567,5 @@ def check_elasticsearch_url_scheme rescue ::Gitlab::HTTP_V2::UrlBlocker::BlockedUrlError errors.add(:elasticsearch_url, "only supports valid HTTP(S) URLs.") end - - def pre_receive_secret_detection_available? - ::Gitlab::CurrentSettings.gitlab_dedicated_instance && - License.feature_available?(:pre_receive_secret_detection) # rubocop:disable Gitlab/LicenseAvailableUsage -- Does not have cyclical dependency - end end end -- GitLab