Merge branch 'philipcunningham-investigate-secret-delivery-via-site-profile-328201' into 'master'

Support DAST secret delivery using DastSiteProfile

See merge request gitlab-org/gitlab!60090

changelogs/unreleased/philipcunningham-investigate-secret-delivery-via-site-profile-328201.yml

+---
+title: Create database table dast_site_profiles_pipelines
+merge_request: 60090
+author:
+type: added

db/migrate/20210423054022_create_dast_site_profiles_pipelines.rb

+# frozen_string_literal: true
+
+class CreateDastSiteProfilesPipelines < ActiveRecord::Migration[6.0]
+  def up
+    table_comment = { owner: 'group::dynamic analysis', description: 'Join table between DAST Site Profiles and CI Pipelines' }
+
+    create_table :dast_site_profiles_pipelines, primary_key: [:dast_site_profile_id, :ci_pipeline_id], comment: table_comment.to_json do |t|
+      t.bigint :dast_site_profile_id, null: false
+      t.bigint :ci_pipeline_id, null: false
+
+      t.index :ci_pipeline_id, unique: true, name: :index_dast_site_profiles_pipelines_on_ci_pipeline_id
+    end
+  end
+
+  def down
+    drop_table :dast_site_profiles_pipelines
+  end
+end

db/migrate/20210423054537_add_dast_site_profile_id_fk_to_dast_site_profiles_pipelines.rb

+# frozen_string_literal: true
+
+class AddDastSiteProfileIdFkToDastSiteProfilesPipelines < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_foreign_key :dast_site_profiles_pipelines, :dast_site_profiles, column: :dast_site_profile_id, on_delete: :cascade
+  end
+
+  def down
+    with_lock_retries do
+      remove_foreign_key :dast_site_profiles_pipelines, column: :dast_site_profile_id
+    end
+  end
+end

db/migrate/20210423054846_add_ci_pipeline_id_fk_to_dast_site_profiles_pipelines.rb

+# frozen_string_literal: true
+
+class AddCiPipelineIdFkToDastSiteProfilesPipelines < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_foreign_key :dast_site_profiles_pipelines, :ci_pipelines, column: :ci_pipeline_id, on_delete: :cascade
+  end
+
+  def down
+    with_lock_retries do
+      remove_foreign_key :dast_site_profiles_pipelines, column: :ci_pipeline_id
+    end
+  end
+end

db/schema_migrations/20210423054022

+84f7b631c9017b286665beca42fb30e064c852d5a21c2f82a8bee6f0d5e62c25
\ No newline at end of file

db/schema_migrations/20210423054537

+0ea5c328f9d15d73744f8847c4b1071e2a360cd52ce0da1216ca6acc768050e5
\ No newline at end of file

db/schema_migrations/20210423054846

+013106237f73a94606f962f54c740af23deac637c8e075471ba03ef5d6c1b953
\ No newline at end of file

db/structure.sql

@@ -12050,6 +12050,13 @@ CREATE SEQUENCE dast_site_profiles_id_seq
 
 ALTER SEQUENCE dast_site_profiles_id_seq OWNED BY dast_site_profiles.id;
 
+CREATE TABLE dast_site_profiles_pipelines (
+    dast_site_profile_id bigint NOT NULL,
+    ci_pipeline_id bigint NOT NULL
+);
+
+COMMENT ON TABLE dast_site_profiles_pipelines IS '{"owner":"group::dynamic analysis","description":"Join table between DAST Site Profiles and CI Pipelines"}';
+
 CREATE TABLE dast_site_tokens (
     id bigint NOT NULL,
     project_id bigint NOT NULL,
@@ -20879,6 +20886,9 @@ ALTER TABLE ONLY dast_scanner_profiles
 ALTER TABLE ONLY dast_site_profile_secret_variables
     ADD CONSTRAINT dast_site_profile_secret_variables_pkey PRIMARY KEY (id);
 
+ALTER TABLE ONLY dast_site_profiles_pipelines
+    ADD CONSTRAINT dast_site_profiles_pipelines_pkey PRIMARY KEY (dast_site_profile_id, ci_pipeline_id);
+
 ALTER TABLE ONLY dast_site_profiles
     ADD CONSTRAINT dast_site_profiles_pkey PRIMARY KEY (id);
 
@@ -22866,6 +22876,8 @@ CREATE INDEX index_dast_site_profiles_on_dast_site_id ON dast_site_profiles USIN
 
 CREATE UNIQUE INDEX index_dast_site_profiles_on_project_id_and_name ON dast_site_profiles USING btree (project_id, name);
 
+CREATE UNIQUE INDEX index_dast_site_profiles_pipelines_on_ci_pipeline_id ON dast_site_profiles_pipelines USING btree (ci_pipeline_id);
+
 CREATE INDEX index_dast_site_tokens_on_project_id ON dast_site_tokens USING btree (project_id);
 
 CREATE INDEX index_dast_site_validations_on_dast_site_token_id ON dast_site_validations USING btree (dast_site_token_id);
@@ -25363,6 +25375,9 @@ ALTER TABLE ONLY alert_management_alerts
 ALTER TABLE ONLY path_locks
     ADD CONSTRAINT fk_5265c98f24 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE;
 
+ALTER TABLE ONLY dast_site_profiles_pipelines
+    ADD CONSTRAINT fk_53849b0ad5 FOREIGN KEY (ci_pipeline_id) REFERENCES ci_pipelines(id) ON DELETE CASCADE;
+
 ALTER TABLE ONLY clusters_applications_prometheus
     ADD CONSTRAINT fk_557e773639 FOREIGN KEY (cluster_id) REFERENCES clusters(id) ON DELETE CASCADE;
 
@@ -25720,6 +25735,9 @@ ALTER TABLE ONLY experiment_subjects
 ALTER TABLE ONLY todos
     ADD CONSTRAINT fk_ccf0373936 FOREIGN KEY (author_id) REFERENCES users(id) ON DELETE CASCADE;
 
+ALTER TABLE ONLY dast_site_profiles_pipelines
+    ADD CONSTRAINT fk_cf05cf8fe1 FOREIGN KEY (dast_site_profile_id) REFERENCES dast_site_profiles(id) ON DELETE CASCADE;
+
 ALTER TABLE ONLY geo_event_log
     ADD CONSTRAINT fk_cff7185ad2 FOREIGN KEY (reset_checksum_event_id) REFERENCES geo_reset_checksum_events(id) ON DELETE CASCADE;
 

ee/app/models/dast/site_profiles_pipeline.rb

+# frozen_string_literal: true
+
+module Dast
+  class SiteProfilesPipeline < ApplicationRecord
+    extend SuppressCompositePrimaryKeyWarning
+
+    self.table_name = 'dast_site_profiles_pipelines'
+
+    belongs_to :ci_pipeline, class_name: 'Ci::Pipeline', optional: false, inverse_of: :dast_site_profiles_pipeline
+    belongs_to :dast_site_profile, class_name: 'DastSiteProfile', optional: false, inverse_of: :dast_site_profiles_pipelines
+
+    validates :ci_pipeline_id, :dast_site_profile_id, presence: true
+
+    validate :project_ids_match
+
+    private
+
+    def project_ids_match
+      return if ci_pipeline.nil? || dast_site_profile.nil?
+
+      unless ci_pipeline.project_id == dast_site_profile.project_id
+        errors.add(:ci_pipeline_id, 'project_id must match dast_site_profile.project_id')
+      end
+    end
+  end
+end

ee/app/models/dast_site_profile.rb

@@ -6,6 +6,9 @@ class DastSiteProfile < ApplicationRecord
 
   has_many :secret_variables, class_name: 'Dast::SiteProfileSecretVariable'
 
+  has_many :dast_site_profiles_pipelines, class_name: 'Dast::SiteProfilesPipeline', foreign_key: :dast_site_profile_id, inverse_of: :dast_site_profile
+  has_many :ci_pipelines, class_name: 'Ci::Pipeline', through: :dast_site_profiles_pipelines
+
   validates :excluded_urls, length: { maximum: 25 }
   validates :auth_url, addressable_url: true, length: { maximum: 1024 }, allow_nil: true
   validates :auth_username_field, :auth_password_field, :auth_username, length: { maximum: 255 }
@@ -25,6 +28,10 @@ class DastSiteProfile < ApplicationRecord
 
   delegate :dast_site_validation, to: :dast_site, allow_nil: true
 
+  def ci_variables
+    ::Gitlab::Ci::Variables::Collection.new(secret_variables)
+  end
+
   def status
     return DastSiteValidation::NONE_STATE unless dast_site_validation
 

ee/app/models/ee/ci/build.rb

@@ -48,8 +48,11 @@ module EE
       def variables
         strong_memoize(:variables) do
           super.tap do |collection|
-            if pipeline.triggered_for_ondemand_dast_scan? && pipeline.dast_profile
-              collection.concat(pipeline.dast_profile.ci_variables)
+            if pipeline.triggered_for_ondemand_dast_scan?
+              # Subject to change. Please see gitlab-org/gitlab#330950 for more info.
+              profile = pipeline.dast_profile || pipeline.dast_site_profile
+
+              collection.concat(profile.ci_variables)
             end
           end
         end

ee/app/models/ee/ci/pipeline.rb

@@ -22,6 +22,11 @@ module EE
 
         has_one :dast_profiles_pipeline, class_name: 'Dast::ProfilesPipeline', foreign_key: :ci_pipeline_id, inverse_of: :ci_pipeline
         has_one :dast_profile, class_name: 'Dast::Profile', through: :dast_profiles_pipeline
+
+        # Temporary location to be moved in the future. Please see gitlab-org/gitlab#330950 for more info.
+        has_one :dast_site_profiles_pipeline, class_name: 'Dast::SiteProfilesPipeline', foreign_key: :ci_pipeline_id, inverse_of: :ci_pipeline
+        has_one :dast_site_profile, class_name: 'DastSiteProfile', through: :dast_site_profiles_pipeline
+
         has_one :source_project, class_name: 'Ci::Sources::Project', foreign_key: :pipeline_id
 
         # Legacy way to fetch security reports based on job name. This has been replaced by the reports feature.

ee/app/services/ci/run_dast_scan_service.rb

@@ -2,13 +2,18 @@
 
 module Ci
   class RunDastScanService < BaseService
-    def execute(branch:, dast_profile: nil, **args)
+    def execute(branch:, dast_profile: nil, dast_site_profile: nil, **args)
       return ServiceResponse.error(message: 'Insufficient permissions') unless allowed?
 
       service = Ci::CreatePipelineService.new(project, current_user, ref: branch)
 
       pipeline = service.execute(:ondemand_dast_scan, content: ci_yaml(args)) do |pipeline|
-        pipeline.dast_profile = dast_profile
+        if dast_profile
+          pipeline.dast_profile = dast_profile
+        else
+          # Subject to change. Please see gitlab-org/gitlab#330950 for more info.
+          pipeline.dast_site_profile = dast_site_profile
+        end
       end
 
       if pipeline.created_successfully?

ee/app/services/dast_on_demand_scans/params_create_service.rb

@@ -62,7 +62,7 @@ module DastOnDemandScans
     end
 
     def default_config
-      { dast_profile: dast_profile, branch: branch }
+      { dast_profile: dast_profile, dast_site_profile: dast_site_profile, branch: branch }
     end
 
     def target_config

ee/spec/factories/dast/site_profiles_pipelines.rb

+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :dast_site_profiles_pipeline, class: 'Dast::SiteProfilesPipeline' do
+    dast_site_profile
+
+    ci_pipeline { association :ci_pipeline, project: dast_site_profile.project }
+  end
+end

ee/spec/graphql/mutations/dast/profiles/update_spec.rb

@@ -8,6 +8,8 @@ RSpec.describe Mutations::Dast::Profiles::Update do
   let_it_be(:project) { create(:project, :repository) }
   let_it_be(:user) { create(:user) }
   let_it_be(:dast_profile, reload: true) { create(:dast_profile, project: project) }
+  let_it_be(:new_dast_site_profile) { create(:dast_site_profile, project: project) }
+  let_it_be(:new_dast_scanner_profile) { create(:dast_scanner_profile, project: project) }
 
   let(:dast_profile_gid) { dast_profile.to_global_id }
   let(:run_after_update) { false }
@@ -18,8 +20,8 @@ RSpec.describe Mutations::Dast::Profiles::Update do
       name: SecureRandom.hex,
       description: SecureRandom.hex,
       branch_name: project.default_branch,
-      dast_site_profile_id: global_id_of(create(:dast_site_profile, project: project)),
-      dast_scanner_profile_id: global_id_of(create(:dast_scanner_profile, project: project)),
+      dast_site_profile_id: global_id_of(new_dast_site_profile),
+      dast_scanner_profile_id: global_id_of(new_dast_scanner_profile),
       run_after_update: run_after_update
     }
   end

ee/spec/models/ci/pipeline_spec.rb

@@ -20,6 +20,8 @@ RSpec.describe Ci::Pipeline do
     it { is_expected.to have_many(:vulnerabilities_finding_pipelines).class_name('Vulnerabilities::FindingPipeline') }
     it { is_expected.to have_one(:dast_profiles_pipeline).class_name('Dast::ProfilesPipeline').with_foreign_key(:ci_pipeline_id).inverse_of(:ci_pipeline) }
     it { is_expected.to have_one(:dast_profile).class_name('Dast::Profile').through(:dast_profiles_pipeline) }
+    it { is_expected.to have_one(:dast_site_profiles_pipeline).class_name('Dast::SiteProfilesPipeline').with_foreign_key(:ci_pipeline_id).inverse_of(:ci_pipeline) }
+    it { is_expected.to have_one(:dast_site_profile).class_name('DastSiteProfile').through(:dast_site_profiles_pipeline) }
   end
 
   describe '.failure_reasons' do

ee/spec/models/dast/site_profiles_pipeline_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Dast::SiteProfilesPipeline, type: :model do
+  subject { create(:dast_site_profiles_pipeline) }
+
+  describe 'associations' do
+    it { is_expected.to belong_to(:ci_pipeline).class_name('Ci::Pipeline').inverse_of(:dast_site_profiles_pipeline).required }
+    it { is_expected.to belong_to(:dast_site_profile).class_name('DastSiteProfile').inverse_of(:dast_site_profiles_pipelines).required }
+  end
+
+  describe 'validations' do
+    it { is_expected.to be_valid }
+    it { is_expected.to validate_presence_of(:ci_pipeline_id) }
+    it { is_expected.to validate_presence_of(:dast_site_profile_id) }
+
+    context 'when the ci_pipeline.project_id and dast_site_profile.project_id do not match' do
+      let(:pipeline) { build(:ci_pipeline, project_id: 1) }
+      let(:site_profile) { build(:dast_site_profile, project_id: 2) }
+
+      subject { build(:dast_site_profiles_pipeline, ci_pipeline: pipeline, dast_site_profile: site_profile) }
+
+      it 'is not valid', :aggregate_failures do
+        expect(subject).not_to be_valid
+        expect(subject.errors.full_messages).to include('Ci pipeline project_id must match dast_site_profile.project_id')
+      end
+    end
+  end
+end

ee/spec/models/dast_site_profile_spec.rb

@@ -193,5 +193,21 @@ RSpec.describe DastSiteProfile, type: :model do
         end
       end
     end
+
+    describe '#ci_variables' do
+      context 'when there are no secret_variables' do
+        it 'returns an empty collection' do
+          expect(subject.ci_variables.size).to be_zero
+        end
+      end
+
+      context 'when there are secret_variables' do
+        it 'returns a collection containing that variable' do
+          variable = create(:dast_site_profile_secret_variable, dast_site_profile: subject)
+
+          expect(subject.ci_variables.to_runner_variables).to include(key: variable.key, value: variable.value, public: false, masked: true)
+        end
+      end
+    end
   end
 end

ee/spec/services/ci/run_dast_scan_service_spec.rb

@@ -6,6 +6,7 @@ RSpec.describe Ci::RunDastScanService do
   let_it_be(:user) { create(:user) }
   let_it_be(:project) { create(:project, :repository, creator: user) }
   let_it_be(:dast_profile) { create(:dast_profile) }
+  let_it_be(:dast_site_profile) { dast_profile.dast_site_profile }
   let_it_be(:branch) { project.default_branch }
   let_it_be(:spider_timeout) { 42 }
   let_it_be(:target_timeout) { 21 }
@@ -35,7 +36,8 @@ RSpec.describe Ci::RunDastScanService do
         auth_username_field: 'session[username]',
         auth_password_field: 'session[password]',
         auth_username: 'tanuki',
-        dast_profile: dast_profile
+        dast_profile: dast_profile,
+        dast_site_profile: dast_site_profile
       )
     end
 
@@ -175,23 +177,37 @@ RSpec.describe Ci::RunDastScanService do
         expect(build.yaml_variables).to contain_exactly(*expected_variables)
       end
 
-      it 'associates the dast_profile with the pipeline' do
-        expect(pipeline.dast_profile).to eq(dast_profile)
+      shared_examples 'transactional creation' do
+        let_it_be(:type_mismatch) { build(:dast_scanner_profile) }
+
+        it 'does not create a Ci::Pipeline' do
+          expect { subject }.to raise_error(ActiveRecord::AssociationTypeMismatch).and change { Ci::Pipeline.count }.by(0)
+        end
       end
 
-      context 'when no dast_profile is provided' do
-        let(:dast_profile) { nil }
+      context 'when the dast_profile and dast_site_profile are provided' do
+        it 'associates the dast_profile with the pipeline' do
+          expect(pipeline.dast_profile).to eq(dast_profile)
+        end
 
-        it 'does not create a new association' do
-          expect(pipeline.dast_profile).to be_nil
+        it 'does associate the dast_site_profile with the pipeline' do
+          expect(pipeline.dast_site_profile).to be_nil
+        end
+
+        it_behaves_like 'transactional creation' do
+          let_it_be(:dast_profile) { type_mismatch }
         end
       end
 
-      context 'when creating the association betweeen the dast_profile and the pipeline fails' do
-        let_it_be(:dast_profile) { build(:dast_site_profile) }
+      context 'when the dast_site_profile is provided' do
+        let(:dast_profile) { nil }
 
-        it 'does not create a Ci::Pipeline' do
-          expect { subject }.to raise_error(ActiveRecord::AssociationTypeMismatch).and change { Ci::Pipeline.count }.by(0)
+        it 'associates the dast_site_profile with the pipeline' do
+          expect(pipeline.dast_site_profile).to eq(dast_site_profile)
+        end
+
+        it_behaves_like 'transactional creation' do
+          let_it_be(:dast_site_profile) { type_mismatch }
         end
       end
 

ee/spec/services/dast_on_demand_scans/create_service_spec.rb

@@ -70,6 +70,7 @@ RSpec.describe DastOnDemandScans::CreateService do
               auth_username_field: dast_site_profile.auth_username_field,
               branch: project.default_branch,
               dast_profile: nil,
+              dast_site_profile: dast_site_profile,
               excluded_urls: dast_site_profile.excluded_urls.join(','),
               full_scan_enabled: false,
               show_debug_messages: false,

ee/spec/services/dast_on_demand_scans/params_create_service_spec.rb

@@ -48,6 +48,7 @@ RSpec.describe DastOnDemandScans::ParamsCreateService do
             auth_url: dast_site_profile.auth_url,
             branch: project.default_branch,
             dast_profile: nil,
+            dast_site_profile: dast_site_profile,
             excluded_urls: excluded_urls,
             target_url: target_url
           )
@@ -65,6 +66,7 @@ RSpec.describe DastOnDemandScans::ParamsCreateService do
             auth_url: dast_site_profile.auth_url,
             branch: project.default_branch,
             dast_profile: nil,
+            dast_site_profile: dast_site_profile,
             excluded_urls: excluded_urls,
             full_scan_enabled: false,
             show_debug_messages: false,
@@ -102,6 +104,7 @@ RSpec.describe DastOnDemandScans::ParamsCreateService do
           expect(subject.payload).to eq(
             branch: project.default_branch,
             dast_profile: nil,
+            dast_site_profile: dast_site_profile,
             excluded_urls: excluded_urls,
             full_scan_enabled: false,
             show_debug_messages: false,
@@ -125,29 +128,38 @@ RSpec.describe DastOnDemandScans::ParamsCreateService do
           expect(subject.payload).to match(expected_payload).and exclude(:target_url)
         end
       end
+
+      context 'when the dast_profile is provided' do
+        let_it_be(:dast_profile) { create(:dast_profile, project: project, dast_site_profile: dast_site_profile, dast_scanner_profile: dast_scanner_profile, branch_name: project.default_branch) }
+
+        let(:params) { { dast_profile: dast_profile } }
+
+        it 'returns prepared scanner params in the payload' do
+          expect(subject.payload).to eq(
+            auth_password_field: dast_site_profile.auth_password_field,
+            auth_username: dast_site_profile.auth_username,
+            auth_username_field: dast_site_profile.auth_username_field,
+            branch: dast_profile.branch_name,
+            auth_url: dast_site_profile.auth_url,
+            dast_profile: dast_profile,
+            dast_site_profile: dast_profile.dast_site_profile,
+            excluded_urls: excluded_urls,
+            full_scan_enabled: false,
+            show_debug_messages: false,
+            spider_timeout: nil,
+            target_timeout: nil,
+            target_url: target_url,
+            use_ajax_spider: false
+          )
+        end
+      end
     end
 
-    context 'when the dast_profile is provided' do
-      let_it_be(:dast_profile) { create(:dast_profile, project: project, dast_site_profile: dast_site_profile, dast_scanner_profile: dast_scanner_profile, branch_name: project.default_branch) }
-
-      let(:params) { { dast_profile: dast_profile } }
-
-      it 'returns prepared scanner params in the payload' do
-        expect(subject.payload).to eq(
-          auth_password_field: dast_site_profile.auth_password_field,
-          auth_username: dast_site_profile.auth_username,
-          auth_username_field: dast_site_profile.auth_username_field,
-          branch: dast_profile.branch_name,
-          auth_url: dast_site_profile.auth_url,
-          dast_profile: dast_profile,
-          excluded_urls: excluded_urls,
-          full_scan_enabled: false,
-          show_debug_messages: false,
-          spider_timeout: nil,
-          target_timeout: nil,
-          target_url: target_url,
-          use_ajax_spider: false
-        )
+    context 'when the branch is provided' do
+      let(:params) { { dast_site_profile: dast_site_profile, dast_scanner_profile: dast_scanner_profile, branch: 'hello-world' } }
+
+      it 'returns the branch in the payload' do
+        expect(subject.payload[:branch]).to match('hello-world')
       end
     end
   end

ee/spec/services/security/security_orchestration_policies/on_demand_scan_pipeline_configuration_service_spec.rb

@@ -55,6 +55,7 @@ RSpec.describe Security::SecurityOrchestrationPolicies::OnDemandScanPipelineConf
         auth_username: site_profile.auth_username,
         auth_username_field: site_profile.auth_username_field,
         dast_profile: nil,
+        dast_site_profile: site_profile,
         branch: project.default_branch,
         excluded_urls: site_profile.excluded_urls.join(','),
         full_scan_enabled: false,

spec/lib/gitlab/import_export/all_models.yml

@@ -260,6 +260,8 @@ ci_pipelines:
 - latest_statuses
 - dast_profile
 - dast_profiles_pipeline
+- dast_site_profile
+- dast_site_profiles_pipeline
 ci_refs:
 - project
 - ci_pipelines