Merge branch 'eurie/509776-add-metrics-for-token-revocation' into 'master'

Add telemetry to token revocation

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/183593



Merged-by: Ethan Urie <eurie@gitlab.com>
Approved-by: Andrew Jung <ajung@gitlab.com>
Approved-by: Allen Cook <acook@gitlab.com>
Approved-by: Andrew Evans <aevans@gitlab.com>
Approved-by: Niko Belokolodov <nbelokolodov@gitlab.com>
Reviewed-by: GitLab Duo <gitlab-duo@gitlab.com>

ee/app/services/security/token_revocation_service.rb

@@ -7,10 +7,13 @@ module Security
   # Service for alerting revocation service of leaked security tokens
   #
   class TokenRevocationService < ::BaseService
+    include Gitlab::InternalEventsTracking
+
     RevocationFailedError = Class.new(StandardError)
 
-    def initialize(revocable_keys:)
+    def initialize(revocable_keys:, project:)
       @revocable_keys = revocable_keys
+      @project = project
     end
 
     def execute
@@ -24,7 +27,12 @@ def execute
       return success if revoke_token_body.blank?
 
       response = revoke_tokens
-      response.success? ? success : error('Failed to revoke tokens')
+
+      return error('Failed to revoke tokens') unless response.success?
+
+      @revocable_keys.each { |key| log_token_revocation(key_type: key[:type]) }
+
+      success
     rescue RevocationFailedError => exception
       error(exception.message)
     rescue StandardError => exception
@@ -34,6 +42,8 @@ def execute
 
     private
 
+    attr_reader :project
+
     # Deduplicate pats before revocation regardless of file location
     def revoke_glpats(tokens)
       tokens
@@ -55,6 +65,8 @@ def revoke_glpat(token)
 
       raise RevocationFailedError, result[:message] if result[:status] == :error
 
+      log_token_revocation(key_type: GLPAT_KEY_TYPE)
+
       return unless token[:vulnerability].present?
 
       SystemNoteService.change_vulnerability_state(
@@ -129,5 +141,19 @@ def revocation_comment
       s_("TokenRevocation|This personal access token has been automatically revoked on detection. " \
          "Consider investigating and rotating before marking this vulnerability as resolved.")
     end
+
+    #################################################################
+    ## Internal tracking methods
+
+    def log_token_revocation(key_type:)
+      track_internal_event(
+        'revoke_leaked_token_after_vulnerability_report_is_ingested',
+        project: project,
+        namespace: project.namespace,
+        additional_properties: {
+          label: key_type
+        }
+      )
+    end
   end
 end

ee/app/workers/scan_security_report_secrets_worker.rb

@@ -28,7 +28,10 @@ def perform(pipeline_id)
     keys = revocable_keys(pipeline)
 
     if keys.present?
-      executed_result = Security::TokenRevocationService.new(revocable_keys: keys).execute
+      executed_result = Security::TokenRevocationService.new(
+        revocable_keys: keys,
+        project: pipeline.project
+      ).execute
 
       raise ScanSecurityReportSecretsWorkerError, executed_result[:message] if executed_result[:status] == :error
     end

ee/config/events/revoke_leaked_token_after_vulnerability_report_is_ingested.yml

+---
+description: Leaked tokens that are automatically revoked
+internal_events: true
+action: revoke_leaked_token_after_vulnerability_report_is_ingested
+identifiers:
+- project
+- namespace
+additional_properties:
+  label:
+    description: The token type that is revoked.
+product_group: secret_detection
+product_categories:
+- vulnerability_management
+milestone: '17.10'
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/183593
+tiers:
+- ultimate

ee/config/metrics/counts_all/count_distinct_label_from_revoke_leaked_token_after_vulnerability_report_is_ingested.yml

+---
+key_path: redis_hll_counters.count_distinct_label_from_revoke_leaked_token_after_vulnerability_report_is_ingested
+description: Count of unique token types revoked
+product_group: secret_detection
+product_categories:
+- vulnerability_management
+performance_indicator_type: []
+value_type: number
+status: active
+milestone: '17.10'
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/183593
+time_frame:
+- 28d
+- 7d
+data_source: internal_events
+data_category: optional
+tiers:
+- ultimate
+events:
+- name: revoke_leaked_token_after_vulnerability_report_is_ingested
+  unique: label

ee/config/metrics/counts_all/count_distinct_namespace_id_from_revoke_leaked_token_after_vulnerability_report_is_ingested.yml

+---
+key_path: redis_hll_counters.count_distinct_namespace_id_from_revoke_leaked_token_after_vulnerability_report_is_ingested
+description: Count of unique namespaces with leaked tokens that were revoked.
+product_group: secret_detection
+product_categories:
+- vulnerability_management
+performance_indicator_type: []
+value_type: number
+status: active
+milestone: '17.10'
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/183593
+time_frame:
+- 28d
+- 7d
+data_source: internal_events
+data_category: optional
+tiers:
+- ultimate
+events:
+- name: revoke_leaked_token_after_vulnerability_report_is_ingested
+  unique: namespace.id

ee/config/metrics/counts_all/count_distinct_project_id_from_revoke_leaked_token_after_vulnerability_report_is_ingested.yml

+---
+key_path: redis_hll_counters.count_distinct_project_id_from_revoke_leaked_token_after_vulnerability_report_is_ingested
+description: Count of unique projects with leaked tokens that were revoked.
+product_group: secret_detection
+product_categories:
+- vulnerability_management
+performance_indicator_type: []
+value_type: number
+status: active
+milestone: '17.10'
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/183593
+time_frame:
+- 28d
+- 7d
+data_source: internal_events
+data_category: optional
+tiers:
+- ultimate
+events:
+- name: revoke_leaked_token_after_vulnerability_report_is_ingested
+  unique: project.id

ee/spec/services/security/token_revocation_service_spec.rb

@@ -5,6 +5,7 @@
 RSpec.describe Security::TokenRevocationService, '#execute', feature_category: :security_policy_management do
   let_it_be(:revocation_token_types_url) { 'https://myhost.com/api/v1/token_types' }
   let_it_be(:token_revocation_url) { 'https://myhost.com/api/v1/revoke' }
+  let_it_be(:project) { build(:project) }
 
   let_it_be(:revocable_keys) do
     [{
@@ -28,7 +29,7 @@
     { types: %w[aws_key_id aws_secret gcp_key_id gcp_secret] }
   end
 
-  subject { described_class.new(revocable_keys: revocable_keys).execute }
+  subject(:token_revocation_service) { described_class.new(revocable_keys:, project:).execute }
 
   before do
     stub_application_setting(secret_detection_revocation_token_types_url: revocation_token_types_url)
@@ -38,10 +39,7 @@
 
   context 'when revoking a glpat token' do
     let_it_be(:glpat_token) { create(:personal_access_token) }
-
-    let_it_be(:vulnerability) do
-      double('Vulnerability') # rubocop:disable RSpec/VerifiedDoubles
-    end
+    let_it_be(:vulnerability) { create(:vulnerability, project: project) }
 
     let_it_be(:revocable_keys) do
       [
@@ -60,6 +58,12 @@
       ]
     end
 
+    it_behaves_like 'internal event tracking' do
+      let(:event) { 'revoke_leaked_token_after_vulnerability_report_is_ingested' }
+      let(:category) { described_class.name }
+      let(:additional_properties) { { label: 'gitleaks_rule_id_gitlab_personal_access_token' } }
+    end
+
     it 'returns success' do
       expect(PersonalAccessTokens::RevokeService).to receive(:new).once.and_call_original
 
@@ -100,6 +104,12 @@
 
         subject
       end
+
+      it_behaves_like 'internal event tracking' do
+        let(:event) { 'revoke_leaked_token_after_vulnerability_report_is_ingested' }
+        let(:category) { described_class.name }
+        let(:additional_properties) { { label: 'gitleaks_rule_id_gitlab_personal_access_token' } }
+      end
     end
   end
 
@@ -114,6 +124,10 @@
       expect(subject[:status]).to be(:error)
       expect(subject[:message]).to eql('Failed to revoke tokens')
     end
+
+    it 'does not create internal tracking events' do
+      expect { subject }.not_to trigger_internal_events('revoke_leaked_token_after_vulnerability_report_is_ingested')
+    end
   end
 
   context 'when revocation token types API returns empty list of types' do
@@ -141,7 +155,28 @@
       end
 
       context 'when there is a list of tokens to be revoked' do
-        specify { expect(subject[:status]).to be(:success) }
+        describe 'status is success' do
+          specify { expect(described_class.new(revocable_keys:, project:).execute[:status]).to be(:success) }
+        end
+
+        # rubocop:disable Layout/LineLength -- metric names are very long
+        it 'creates internal tracking event' do
+          expect { described_class.new(revocable_keys:, project:).execute }.to trigger_internal_events(
+            'revoke_leaked_token_after_vulnerability_report_is_ingested').with(
+              project: project, namespace: project.namespace, additional_properties: { label: 'aws_key_id' }
+            ).and trigger_internal_events('revoke_leaked_token_after_vulnerability_report_is_ingested').with(
+              project: project, namespace: project.namespace, additional_properties: { label: 'aws_secret' }
+            ).twice.and increment_usage_metrics(
+              'redis_hll_counters.count_distinct_namespace_id_from_revoke_leaked_token_after_vulnerability_report_is_ingested_monthly',
+              'redis_hll_counters.count_distinct_project_id_from_revoke_leaked_token_after_vulnerability_report_is_ingested_monthly',
+              'redis_hll_counters.count_distinct_namespace_id_from_revoke_leaked_token_after_vulnerability_report_is_ingested_weekly',
+              'redis_hll_counters.count_distinct_project_id_from_revoke_leaked_token_after_vulnerability_report_is_ingested_weekly'
+            ).by(1).and increment_usage_metrics(
+              'redis_hll_counters.count_distinct_label_from_revoke_leaked_token_after_vulnerability_report_is_ingested_monthly',
+              'redis_hll_counters.count_distinct_label_from_revoke_leaked_token_after_vulnerability_report_is_ingested_weekly'
+            ).by(2)
+        end
+        # rubocop:enable Layout/LineLength
       end
 
       context 'when token_revocation_url is missing' do
@@ -152,6 +187,12 @@
         end
 
         specify { expect(subject).to eql({ message: 'Missing revocation token data', status: :error }) }
+
+        it 'does not create internal tracking events' do
+          expect { subject }.not_to trigger_internal_events(
+            'revoke_leaked_token_after_vulnerability_report_is_ingested'
+          )
+        end
       end
 
       context 'when token_types_url is missing' do
@@ -162,6 +203,12 @@
         end
 
         specify { expect(subject).to eql({ message: 'Missing revocation token data', status: :error }) }
+
+        it 'does not create internal tracking events' do
+          expect { subject }.not_to trigger_internal_events(
+            'revoke_leaked_token_after_vulnerability_report_is_ingested'
+          )
+        end
       end
 
       context 'when revocation_api_token is missing' do
@@ -172,6 +219,12 @@
         end
 
         specify { expect(subject).to eql({ message: 'Missing revocation token data', status: :error }) }
+
+        it 'does not create internal tracking events' do
+          expect { subject }.not_to trigger_internal_events(
+            'revoke_leaked_token_after_vulnerability_report_is_ingested'
+          )
+        end
       end
 
       context 'when there is no token to be revoked' do
@@ -180,6 +233,12 @@
         end
 
         specify { expect(subject).to eql({ status: :success }) }
+
+        it 'does not create internal tracking events' do
+          expect { subject }.not_to trigger_internal_events(
+            'revoke_leaked_token_after_vulnerability_report_is_ingested'
+          )
+        end
       end
     end
 
@@ -189,6 +248,12 @@
       end
 
       specify { expect(subject).to eql({ message: 'Failed to get revocation token types', status: :error }) }
+
+      it 'does not create internal tracking events' do
+        expect { subject }.not_to trigger_internal_events(
+          'revoke_leaked_token_after_vulnerability_report_is_ingested'
+        )
+      end
     end
   end