Add error tracking client key

To authenticate error tracking events in collector we need to make sure
request has the same public key as the one project has. Keys are safe to
to keep public because they only allow submission of new events.
We do not allow read access to any information for those keys.
So it should not be used anywhere else except error tracking collector.
Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>

Changelog: added
Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>

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

@@ -12757,6 +12757,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,
@@ -20064,6 +20083,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);
@@ -21388,6 +21409,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);
 
@@ -23563,6 +23587,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);
@@ -27481,6 +27507,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) { '' }