Merge branch 'dz-error-tracking-dsn-auth' into 'master'

Add error tracking client key

See merge request gitlab-org/gitlab!66466

app/models/error_tracking/client_key.rb

+# frozen_string_literal: true
+
+class ErrorTracking::ClientKey < ApplicationRecord
+  belongs_to :project
+
+  validates :project, presence: true
+  validates :public_key, presence: true, length: { maximum: 255 }
+
+  scope :active, -> { where(active: true) }
+
+  after_initialize :generate_key
+
+  def self.find_by_public_key(key)
+    find_by(public_key: key)
+  end
+
+  private
+
+  def generate_key
+    self.public_key = "glet_#{SecureRandom.hex}"
+  end
+end

app/models/project.rb

@@ -378,6 +378,7 @@ class Project < ApplicationRecord
   has_many :operations_feature_flags_user_lists, class_name: 'Operations::FeatureFlags::UserList'
 
   has_many :error_tracking_errors, inverse_of: :project, class_name: 'ErrorTracking::Error'
+  has_many :error_tracking_client_keys, inverse_of: :project, class_name: 'ErrorTracking::ClientKey'
 
   has_many :timelogs
 

db/migrate/20210720130006_create_error_tracking_client_keys.rb

+# frozen_string_literal: true
+
+class CreateErrorTrackingClientKeys < ActiveRecord::Migration[6.1]
+  include Gitlab::Database::MigrationHelpers
+
+  def up
+    create_table_with_constraints :error_tracking_client_keys do |t|
+      t.references :project,
+        index: true,
+        null: false,
+        foreign_key: { on_delete: :cascade }
+
+      t.boolean :active, default: true, null: false
+      t.text :public_key, null: false
+      t.text_limit :public_key, 255
+
+      t.timestamps_with_timezone
+    end
+  end
+
+  def down
+    drop_table :error_tracking_client_keys
+  end
+end

db/schema_migrations/20210720130006

+03d86d635c54b53bd540443f0a911d4f0ae59ec3494be23952490c5df70dd28c
\ No newline at end of file

db/structure.sql

@@ -12783,6 +12783,25 @@ CREATE SEQUENCE epics_id_seq
 
 ALTER SEQUENCE epics_id_seq OWNED BY epics.id;
 
+CREATE TABLE error_tracking_client_keys (
+    id bigint NOT NULL,
+    project_id bigint NOT NULL,
+    active boolean DEFAULT true NOT NULL,
+    public_key text NOT NULL,
+    created_at timestamp with time zone NOT NULL,
+    updated_at timestamp with time zone NOT NULL,
+    CONSTRAINT check_840b719790 CHECK ((char_length(public_key) <= 255))
+);
+
+CREATE SEQUENCE error_tracking_client_keys_id_seq
+    START WITH 1
+    INCREMENT BY 1
+    NO MINVALUE
+    NO MAXVALUE
+    CACHE 1;
+
+ALTER SEQUENCE error_tracking_client_keys_id_seq OWNED BY error_tracking_client_keys.id;
+
 CREATE TABLE error_tracking_error_events (
     id bigint NOT NULL,
     error_id bigint NOT NULL,
@@ -20114,6 +20133,8 @@ ALTER TABLE ONLY epic_user_mentions ALTER COLUMN id SET DEFAULT nextval('epic_us
 
 ALTER TABLE ONLY epics ALTER COLUMN id SET DEFAULT nextval('epics_id_seq'::regclass);
 
+ALTER TABLE ONLY error_tracking_client_keys ALTER COLUMN id SET DEFAULT nextval('error_tracking_client_keys_id_seq'::regclass);
+
 ALTER TABLE ONLY error_tracking_error_events ALTER COLUMN id SET DEFAULT nextval('error_tracking_error_events_id_seq'::regclass);
 
 ALTER TABLE ONLY error_tracking_errors ALTER COLUMN id SET DEFAULT nextval('error_tracking_errors_id_seq'::regclass);
@@ -21443,6 +21464,9 @@ ALTER TABLE ONLY epic_user_mentions
 ALTER TABLE ONLY epics
     ADD CONSTRAINT epics_pkey PRIMARY KEY (id);
 
+ALTER TABLE ONLY error_tracking_client_keys
+    ADD CONSTRAINT error_tracking_client_keys_pkey PRIMARY KEY (id);
+
 ALTER TABLE ONLY error_tracking_error_events
     ADD CONSTRAINT error_tracking_error_events_pkey PRIMARY KEY (id);
 
@@ -23625,6 +23649,8 @@ CREATE INDEX index_epics_on_start_date_sourcing_epic_id ON epics USING btree (st
 
 CREATE INDEX index_epics_on_start_date_sourcing_milestone_id ON epics USING btree (start_date_sourcing_milestone_id);
 
+CREATE INDEX index_error_tracking_client_keys_on_project_id ON error_tracking_client_keys USING btree (project_id);
+
 CREATE INDEX index_error_tracking_error_events_on_error_id ON error_tracking_error_events USING btree (error_id);
 
 CREATE INDEX index_error_tracking_errors_on_project_id ON error_tracking_errors USING btree (project_id);
@@ -27556,6 +27582,9 @@ ALTER TABLE ONLY board_project_recent_visits
 ALTER TABLE ONLY clusters_kubernetes_namespaces
     ADD CONSTRAINT fk_rails_98fe21e486 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE SET NULL;
 
+ALTER TABLE ONLY error_tracking_client_keys
+    ADD CONSTRAINT fk_rails_99342d1d54 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE;
+
 ALTER TABLE ONLY pages_deployments
     ADD CONSTRAINT fk_rails_993b88f59a FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE;
 

lib/api/error_tracking_collector.rb

@@ -13,6 +13,7 @@ module API
     before do
       not_found!('Project') unless project
       not_found! unless feature_enabled?
+      not_found! unless active_client_key?
     end
 
     helpers do
@@ -24,6 +25,22 @@ module API
         ::Feature.enabled?(:integrated_error_tracking, project) &&
           project.error_tracking_setting&.enabled?
       end
+
+      def find_client_key(public_key)
+        return unless public_key.present?
+
+        project.error_tracking_client_keys.active.find_by_public_key(public_key)
+      end
+
+      def active_client_key?
+        begin
+          public_key = ::ErrorTracking::Collector::SentryAuthParser.parse(request)[:public_key]
+        rescue StandardError
+          bad_request!('Failed to parse sentry request')
+        end
+
+        find_client_key(public_key)
+      end
     end
 
     desc 'Submit error tracking event to the project' do
@@ -46,7 +63,7 @@ module API
       begin
         parsed_request = ::ErrorTracking::Collector::SentryRequestParser.parse(request)
       rescue StandardError
-        render_api_error!('Failed to parse sentry request', 400)
+        bad_request!('Failed to parse sentry request')
       end
 
       type = parsed_request[:request_type]
@@ -67,6 +84,9 @@ module API
           .execute
       end
 
+      # Collector should never return any information back.
+      # Because DSN and public key are designed for public use,
+      # it is safe only for submission of new events.
       no_content!
     end
   end

lib/error_tracking/collector/sentry_auth_parser.rb

+# frozen_string_literal: true
+
+module ErrorTracking
+  module Collector
+    class SentryAuthParser
+      def self.parse(request)
+        # Sentry client sends auth in X-Sentry-Auth header
+        #
+        # Example of content:
+        # "Sentry sentry_version=7, sentry_client=sentry-ruby/4.5.1, sentry_timestamp=1623923398,
+        #         sentry_key=afadk312..., sentry_secret=123456asd32131..."
+        auth = request.headers['X-Sentry-Auth']
+
+        # Sentry DSN contains key and secret.
+        # The key is required while secret is optional.
+        # We are going to use only the key since secret is deprecated.
+        public_key = auth[/sentry_key=(\w+)/, 1]
+
+        {
+          public_key: public_key
+        }
+      end
+    end
+  end
+end

spec/factories/error_tracking/client_key.rb

+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :error_tracking_client_key, class: 'ErrorTracking::ClientKey' do
+    project
+    active { true }
+
+    trait :disabled do
+      active { false }
+    end
+  end
+end

spec/lib/error_tracking/collector/sentry_auth_parser_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ErrorTracking::Collector::SentryAuthParser do
+  describe '.parse' do
+    let(:headers) { { 'X-Sentry-Auth' => "Sentry sentry_key=glet_1fedb514e17f4b958435093deb02048c" } }
+    let(:request) { double('request', headers: headers) }
+
+    subject { described_class.parse(request) }
+
+    context 'empty headers' do
+      let(:headers) { {} }
+
+      it 'fails with exception' do
+        expect { subject }.to raise_error(StandardError)
+      end
+    end
+
+    context 'missing sentry_key' do
+      let(:headers) { { 'X-Sentry-Auth' => "Sentry foo=bar" } }
+
+      it 'returns empty value for public_key' do
+        expect(subject[:public_key]).to be_nil
+      end
+    end
+
+    it 'returns correct value for public_key' do
+      expect(subject[:public_key]).to eq('glet_1fedb514e17f4b958435093deb02048c')
+    end
+  end
+end

spec/lib/gitlab/import_export/all_models.yml

@@ -579,6 +579,7 @@ project:
 - security_orchestration_policy_configuration
 - timelogs
 - error_tracking_errors
+- error_tracking_client_keys
 award_emoji:
 - awardable
 - user

spec/models/error_tracking/client_key_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ErrorTracking::ClientKey, type: :model do
+  describe 'relationships' do
+    it { is_expected.to belong_to(:project) }
+  end
+
+  describe 'validations' do
+    it { is_expected.to validate_presence_of(:public_key) }
+    it { is_expected.to validate_length_of(:public_key).is_at_most(255) }
+  end
+
+  describe '#generate_key' do
+    it { expect(subject.public_key).to be_present }
+    it { expect(subject.public_key).to start_with('glet_') }
+  end
+end

spec/models/project_spec.rb

@@ -135,6 +135,8 @@ RSpec.describe Project, factory_default: :keep do
     it { is_expected.to have_many(:pipeline_artifacts) }
     it { is_expected.to have_many(:terraform_states).class_name('Terraform::State').inverse_of(:project) }
     it { is_expected.to have_many(:timelogs) }
+    it { is_expected.to have_many(:error_tracking_errors).class_name('ErrorTracking::Error') }
+    it { is_expected.to have_many(:error_tracking_client_keys).class_name('ErrorTracking::ClientKey') }
 
     # GitLab Pages
     it { is_expected.to have_many(:pages_domains) }

spec/requests/api/error_tracking_collector_spec.rb

@@ -5,14 +5,16 @@ require 'spec_helper'
 RSpec.describe API::ErrorTrackingCollector do
   let_it_be(:project) { create(:project, :private) }
   let_it_be(:setting) { create(:project_error_tracking_setting, project: project) }
+  let_it_be(:client_key) { create(:error_tracking_client_key, project: project) }
 
   describe "POST /error_tracking/collector/api/:id/envelope" do
     let_it_be(:raw_event) { fixture_file('error_tracking/event.txt') }
     let_it_be(:url) { "/error_tracking/collector/api/#{project.id}/envelope" }
 
     let(:params) { raw_event }
+    let(:headers) { { 'X-Sentry-Auth' => "Sentry sentry_key=#{client_key.public_key}" } }
 
-    subject { post api(url), params: params }
+    subject { post api(url), params: params, headers: headers }
 
     RSpec.shared_examples 'not found' do
       it 'reponds with 404' do
@@ -46,6 +48,24 @@ RSpec.describe API::ErrorTrackingCollector do
       it_behaves_like 'not found'
     end
 
+    context 'auth headers are missing' do
+      let(:headers) { {} }
+
+      it_behaves_like 'bad request'
+    end
+
+    context 'public key is wrong' do
+      let(:headers) { { 'X-Sentry-Auth' => "Sentry sentry_key=glet_1fedb514e17f4b958435093deb02048c" } }
+
+      it_behaves_like 'not found'
+    end
+
+    context 'public key is inactive' do
+      let(:client_key) { create(:error_tracking_client_key, :disabled, project: project) }
+
+      it_behaves_like 'not found'
+    end
+
     context 'empty body' do
       let(:params) { '' }