Draft: Add support for secret DAST profile variables
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)
- proof of concept: !47943 (closed)
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
-
Changelog entry -
Documentation (if required) -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Availability and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. -
Tested in all supported browsers -
Informed Infrastructure department of a default or new setting change, if applicable per definition of done
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