Skip to content

Draft: Add support for secret DAST profile variables

Philip Cunningham requested to merge philipcunningham-add-ci-secrets-225406 into master

What does this MR do?

this merge request adds the ability to store secrets for dast on-demand scans via the introduction of a new table (dast_site_profile_variables) and an association with the ci_pipelines table.

Why?

as part of the work on dast on-demand scans, we want to give users the ability to specify login credentials for their on-demand scans in order to perform authenticated scans.

during security review we identified that ci_variables seemed like the correct place to store this data but that one of the risks associated was that sensitive ci_variables could end up being sent to jobs that should not have access to them (e.g. if someone were set the environment scope incorrectly).

following discussion in review on this merge request, we opted to move away from using the ci_variables and simplify the solution by introducing a new table and extending Ci::Contextable.

Related Issue(s)

Related Merge Request(s)

Database

Migrations

% rails db:migrate:up VERSION=20210305031822 && rails db:migrate:up VERSION=20210305235615 && rails db:migrate:up VERSION=20210306000246 && rails db:migrate:up VERSION=20210306000911 && rails db:migrate:down VERSION=20210306000911 && rails db:migrate:down VERSION=20210306000246 && rails db:migrate:down VERSION=20210305235615 && rails db:migrate:down VERSION=20210305031822

== 20210305031822 CreateDastSiteProfileVariables: migrating ===================
-- create_table(:dast_site_profile_variables, {:comment=>"{\"owner\":\"group::dynamic analysis\",\"description\":\"Variables used in an DAST on-demand scan\"}"})
-- quote_column_name(:key)
   -> 0.0000s
   -> 0.0134s
-- quote_table_name("check_b6df2f9c3d")
   -> 0.0000s
-- quote_table_name(:dast_site_profile_variables)
   -> 0.0000s
-- execute("ALTER TABLE \"dast_site_profile_variables\"\nADD CONSTRAINT \"check_b6df2f9c3d\" CHECK (char_length(\"key\") <= 255)\n")
   -> 0.0009s
== 20210305031822 CreateDastSiteProfileVariables: migrated (0.0208s) ==========

== 20210305235615 AddDastProfileIdToCiPipeline: migrating =====================
-- add_column(:ci_pipelines, :dast_profile_id, :integer)
   -> 0.0014s
== 20210305235615 AddDastProfileIdToCiPipeline: migrated (0.0074s) ============

== 20210306000246 AddIndexToDastProfileIdOnCiPipeline: migrating ==============
-- transaction_open?()
   -> 0.0000s
-- index_exists?(:ci_pipelines, :dast_profile_id, {:name=>:index_ci_pipelines_on_dast_profile_id, :algorithm=>:concurrently})
   -> 0.0079s
-- execute("SET statement_timeout TO 0")
   -> 0.0006s
-- add_index(:ci_pipelines, :dast_profile_id, {:name=>:index_ci_pipelines_on_dast_profile_id, :algorithm=>:concurrently})
   -> 0.0032s
-- execute("RESET ALL")
   -> 0.0005s
== 20210306000246 AddIndexToDastProfileIdOnCiPipeline: migrated (0.0132s) =====

== 20210306000911 AddForeignKeyToDastProfileIdOnCiPipeline: migrating =========
-- transaction_open?()
   -> 0.0000s
-- foreign_keys(:ci_pipelines)
   -> 0.0035s
-- execute("ALTER TABLE ci_pipelines\nADD CONSTRAINT fk_ab78278ad1\nFOREIGN KEY (dast_profile_id)\nREFERENCES dast_profiles (id)\nON DELETE SET NULL\nNOT VALID;\n")
   -> 0.0018s
-- execute("SET statement_timeout TO 0")
   -> 0.0005s
-- execute("ALTER TABLE ci_pipelines VALIDATE CONSTRAINT fk_ab78278ad1;")
   -> 0.0025s
-- execute("RESET ALL")
   -> 0.0005s
== 20210306000911 AddForeignKeyToDastProfileIdOnCiPipeline: migrated (0.0150s)

== 20210306000911 AddForeignKeyToDastProfileIdOnCiPipeline: reverting =========
-- remove_foreign_key(:ci_pipelines, {:column=>:dast_profile_id})
   -> 0.0049s
== 20210306000911 AddForeignKeyToDastProfileIdOnCiPipeline: reverted (0.0111s)

== 20210306000246 AddIndexToDastProfileIdOnCiPipeline: reverting ==============
-- transaction_open?()
   -> 0.0000s
-- index_exists?(:ci_pipelines, :dast_profile_id, {:name=>:index_ci_pipelines_on_dast_profile_id, :algorithm=>:concurrently})
   -> 0.0078s
-- execute("SET statement_timeout TO 0")
   -> 0.0005s
-- remove_index(:ci_pipelines, {:name=>:index_ci_pipelines_on_dast_profile_id, :algorithm=>:concurrently, :column=>:dast_profile_id})
   -> 0.0079s
-- execute("RESET ALL")
   -> 0.0005s
== 20210306000246 AddIndexToDastProfileIdOnCiPipeline: reverted (0.0177s) =====

== 20210305235615 AddDastProfileIdToCiPipeline: reverting =====================
-- remove_column(:ci_pipelines, :dast_profile_id)
   -> 0.0011s
== 20210305235615 AddDastProfileIdToCiPipeline: reverted (0.0058s) ============

== 20210305031822 CreateDastSiteProfileVariables: reverting ===================
-- drop_table(:dast_site_profile_variables)
   -> 0.0035s
== 20210305031822 CreateDastSiteProfileVariables: reverted (0.0035s) ==========

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team
Edited by Philip Cunningham

Merge request reports