Merge branch 'philipcunningham-add-dast-secret-variables-table-225406' into 'master'

Add Dast::SiteProfileSecretVariables and associations

See merge request gitlab-org/gitlab!56067

changelogs/unreleased/philipcunningham-add-ci-secrets-225406.yml

+---
+title: Add dast_profile_secret_variables table
+merge_request: 56067
+author:
+type: added

db/migrate/20210305031822_create_dast_site_profile_variables.rb

+# frozen_string_literal: true
+
+class CreateDastSiteProfileVariables < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    table_comment = { owner: 'group::dynamic analysis', description: 'Secret variables used in DAST on-demand scans' }
+
+    encrypted_value_constraint_name = check_constraint_name(:dast_site_profile_secret_variables, 'encrypted_value', 'max_length')
+    encrypted_value_iv_constraint_name = check_constraint_name(:dast_site_profile_secret_variables, 'encrypted_value_iv', 'max_length')
+
+    create_table_with_constraints :dast_site_profile_secret_variables, comment: table_comment.to_json do |t|
+      t.references :dast_site_profile, null: false, foreign_key: { on_delete: :cascade }, index: false
+
+      t.timestamps_with_timezone
+
+      t.integer :variable_type, null: false, default: 1, limit: 2
+
+      t.text :key, null: false
+      t.binary :encrypted_value, null: false
+      t.binary :encrypted_value_iv, null: false, unique: true
+
+      t.index [:dast_site_profile_id, :key], unique: true, name: :index_site_profile_secret_variables_on_site_profile_id_and_key
+
+      t.text_limit :key, 255
+
+      # This does not currently have first-class support via create_table_with_constraints
+      t.check_constraint encrypted_value_constraint_name, 'length(encrypted_value) <= 13352'
+      t.check_constraint encrypted_value_iv_constraint_name, 'length(encrypted_value_iv) <= 17'
+    end
+  end
+
+  def down
+    drop_table :dast_site_profile_secret_variables
+  end
+end

db/schema_migrations/20210305031822

+d91eb442db670adef6d610a2c79259377709e5c98615ba10b85eb998715b3130
\ No newline at end of file

db/structure.sql

@@ -11808,6 +11808,31 @@ CREATE SEQUENCE dast_scanner_profiles_id_seq
 
 ALTER SEQUENCE dast_scanner_profiles_id_seq OWNED BY dast_scanner_profiles.id;
 
+CREATE TABLE dast_site_profile_secret_variables (
+    id bigint NOT NULL,
+    dast_site_profile_id bigint NOT NULL,
+    created_at timestamp with time zone NOT NULL,
+    updated_at timestamp with time zone NOT NULL,
+    variable_type smallint DEFAULT 1 NOT NULL,
+    key text NOT NULL,
+    encrypted_value bytea NOT NULL,
+    encrypted_value_iv bytea NOT NULL,
+    CONSTRAINT check_236213f179 CHECK ((length(encrypted_value) <= 13352)),
+    CONSTRAINT check_8cbef204b2 CHECK ((char_length(key) <= 255)),
+    CONSTRAINT check_b49080abbf CHECK ((length(encrypted_value_iv) <= 17))
+);
+
+COMMENT ON TABLE dast_site_profile_secret_variables IS '{"owner":"group::dynamic analysis","description":"Secret variables used in DAST on-demand scans"}';
+
+CREATE SEQUENCE dast_site_profile_secret_variables_id_seq
+    START WITH 1
+    INCREMENT BY 1
+    NO MINVALUE
+    NO MAXVALUE
+    CACHE 1;
+
+ALTER SEQUENCE dast_site_profile_secret_variables_id_seq OWNED BY dast_site_profile_secret_variables.id;
+
 CREATE TABLE dast_site_profiles (
     id bigint NOT NULL,
     project_id bigint NOT NULL,
@@ -19141,6 +19166,8 @@ ALTER TABLE ONLY dast_profiles ALTER COLUMN id SET DEFAULT nextval('dast_profile
 
 ALTER TABLE ONLY dast_scanner_profiles ALTER COLUMN id SET DEFAULT nextval('dast_scanner_profiles_id_seq'::regclass);
 
+ALTER TABLE ONLY dast_site_profile_secret_variables ALTER COLUMN id SET DEFAULT nextval('dast_site_profile_secret_variables_id_seq'::regclass);
+
 ALTER TABLE ONLY dast_site_profiles ALTER COLUMN id SET DEFAULT nextval('dast_site_profiles_id_seq'::regclass);
 
 ALTER TABLE ONLY dast_site_tokens ALTER COLUMN id SET DEFAULT nextval('dast_site_tokens_id_seq'::regclass);
@@ -20341,6 +20368,9 @@ ALTER TABLE ONLY dast_profiles
 ALTER TABLE ONLY dast_scanner_profiles
     ADD CONSTRAINT dast_scanner_profiles_pkey PRIMARY KEY (id);
 
+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
     ADD CONSTRAINT dast_site_profiles_pkey PRIMARY KEY (id);
 
@@ -23681,6 +23711,8 @@ CREATE UNIQUE INDEX index_services_on_unique_group_id_and_type ON services USING
 
 CREATE UNIQUE INDEX index_shards_on_name ON shards USING btree (name);
 
+CREATE UNIQUE INDEX index_site_profile_secret_variables_on_site_profile_id_and_key ON dast_site_profile_secret_variables USING btree (dast_site_profile_id, key);
+
 CREATE INDEX index_slack_integrations_on_service_id ON slack_integrations USING btree (service_id);
 
 CREATE UNIQUE INDEX index_slack_integrations_on_team_id_and_alias ON slack_integrations USING btree (team_id, alias);
@@ -25562,6 +25594,9 @@ ALTER TABLE ONLY operations_strategies_user_lists
 ALTER TABLE ONLY lfs_file_locks
     ADD CONSTRAINT fk_rails_43df7a0412 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE;
 
+ALTER TABLE ONLY dast_site_profile_secret_variables
+    ADD CONSTRAINT fk_rails_43e2897950 FOREIGN KEY (dast_site_profile_id) REFERENCES dast_site_profiles(id) ON DELETE CASCADE;
+
 ALTER TABLE ONLY merge_request_assignees
     ADD CONSTRAINT fk_rails_443443ce6f FOREIGN KEY (merge_request_id) REFERENCES merge_requests(id) ON DELETE CASCADE;
 

ee/app/models/dast/profile.rb

@@ -8,6 +8,8 @@ module Dast
     belongs_to :dast_site_profile
     belongs_to :dast_scanner_profile
 
+    has_many :secret_variables, through: :dast_site_profile, class_name: 'Dast::SiteProfileSecretVariable'
+
     validates :description, length: { maximum: 255 }
     validates :name, length: { maximum: 255 }, uniqueness: { scope: :project_id }, presence: true
     validates :branch_name, length: { maximum: 255 }

ee/app/models/dast/site_profile_secret_variable.rb

+# frozen_string_literal: true
+
+module Dast
+  class SiteProfileSecretVariable < ApplicationRecord
+    include Ci::HasVariable
+    include Ci::Maskable
+
+    self.table_name = 'dast_site_profile_secret_variables'
+
+    belongs_to :dast_site_profile
+    delegate :project, to: :dast_site_profile, allow_nil: false
+
+    attribute :masked, default: true
+
+    attr_encrypted :value,
+      mode: :per_attribute_iv,
+      algorithm: 'aes-256-gcm',
+      key: Settings.attr_encrypted_db_key_base_32,
+      encode: false # No need to encode for binary column https://github.com/attr-encrypted/attr_encrypted#the-encode-encode_iv-encode_salt-and-default_encoding-options
+
+    # Secret variables must be masked to prevent them being readable in CI jobs
+    validates :masked, inclusion: { in: [true] }
+    validates :variable_type, inclusion: { in: ['env_var'] }
+
+    validates :key, uniqueness: { scope: :dast_site_profile_id, message: "(%{value}) has already been taken" }
+
+    # Since user input is base64 encoded before being encrypted, we must validate against the encoded length
+    MAX_VALUE_LENGTH = 10_000
+    MAX_ENCODED_VALUE_LENGTH = ((4 * MAX_VALUE_LENGTH / 3) + 3) & ~3
+
+    validates :value, length: {
+      maximum: MAX_ENCODED_VALUE_LENGTH, # encoded user input length
+      too_long: -> (object, data) { "exceeds the #{MAX_VALUE_LENGTH} character limit" } # user input length
+    }
+
+    # User input is base64 encoded before being encrypted in order to allow it to be masked by default
+    def raw_value=(new_value)
+      self.value = Base64.strict_encode64(new_value)
+    end
+
+    # Use #raw_value= to ensure value is maskable
+    private :value=
+  end
+end

ee/app/models/dast_site_profile.rb

@@ -4,6 +4,8 @@ class DastSiteProfile < ApplicationRecord
   belongs_to :project
   belongs_to :dast_site
 
+  has_many :secret_variables, class_name: 'Dast::SiteProfileSecretVariable'
+
   validates :name, length: { maximum: 255 }, uniqueness: { scope: :project_id }, presence: true
   validates :project_id, :dast_site_id, presence: true
   validate :dast_site_project_id_fk

ee/spec/factories/dast/site_profile_secret_variables.rb

+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :dast_site_profile_secret_variable, class: 'Dast::SiteProfileSecretVariable' do
+    dast_site_profile
+
+    sequence(:key) { |n| "VARIABLE_#{n}" }
+    raw_value { 'VARIABLE_VALUE' }
+  end
+end

ee/spec/models/dast/profile_spec.rb

@@ -9,6 +9,7 @@ RSpec.describe Dast::Profile, type: :model do
     it { is_expected.to belong_to(:project) }
     it { is_expected.to belong_to(:dast_site_profile) }
     it { is_expected.to belong_to(:dast_scanner_profile) }
+    it { is_expected.to have_many(:secret_variables).through(:dast_site_profile).class_name('Dast::SiteProfileSecretVariable') }
   end
 
   describe 'validations' do

ee/spec/models/dast/site_profile_secret_variable_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Dast::SiteProfileSecretVariable, type: :model do
+  let_it_be(:dast_site_profile) { create(:dast_site_profile) }
+
+  subject { create(:dast_site_profile_secret_variable, dast_site_profile: dast_site_profile) }
+
+  it_behaves_like 'CI variable'
+
+  describe 'constants' do
+    describe 'MAX_ENCODED_VALUE_LENGTH' do
+      it 'correctly expresses the relationship between input and encoded length' do
+        raw_value = SecureRandom.alphanumeric(described_class::MAX_VALUE_LENGTH)
+
+        expect(described_class::MAX_ENCODED_VALUE_LENGTH).to eq(Base64.strict_encode64(raw_value).length)
+      end
+    end
+  end
+
+  describe 'associations' do
+    it { is_expected.to belong_to(:dast_site_profile) }
+  end
+
+  describe 'validations' do
+    it { is_expected.to be_valid }
+    it { is_expected.to include_module(Ci::Maskable) }
+    it { is_expected.to include_module(Ci::HasVariable) }
+    it { is_expected.to validate_inclusion_of(:masked).in_array([true]) }
+    it { is_expected.to validate_uniqueness_of(:key).scoped_to(:dast_site_profile_id).with_message(/\(\w+\) has already been taken/) }
+
+    it 'only allows records where variable_type=env_var', :aggregate_failures do
+      subject = build(:dast_site_profile_secret_variable, variable_type: :file)
+
+      expect(subject).not_to be_valid
+      expect(subject.errors.full_messages).to include('Variable type is not included in the list')
+    end
+
+    describe '#value' do
+      subject { build(:dast_site_profile_secret_variable, dast_site_profile: dast_site_profile, raw_value: raw_value) }
+
+      context 'when the value is over the limit' do
+        let(:raw_value) { SecureRandom.alphanumeric(10_003) }
+
+        it 'is not valid', :aggregate_failures do
+          expect(subject).not_to be_valid
+          expect(subject.errors.full_messages).to include('Value exceeds the 10000 character limit')
+        end
+
+        it 'raises a database level error' do
+          allow(subject).to receive(:valid?).and_return(true)
+
+          expect { subject.save! }.to raise_error(ActiveRecord::StatementInvalid)
+        end
+      end
+
+      context 'when value is under the limit' do
+        let(:raw_value) { SecureRandom.alphanumeric(10_000) }
+
+        it 'is valid' do
+          expect(subject).to be_valid
+        end
+
+        it 'does not raise database level error' do
+          allow(subject).to receive(:valid?).and_return(true)
+
+          expect { subject.save! }.not_to raise_error
+        end
+      end
+    end
+  end
+
+  describe '#masked' do
+    it 'defaults to true', :aggregate_failures do
+      expect(subject.masked).to eq(true)
+      expect(described_class.new.masked).to eq(true)
+    end
+  end
+
+  describe '#project' do
+    it 'delegates to dast_site_profile' do
+      expect(subject.project).to eq(subject.dast_site_profile.project)
+    end
+  end
+
+  describe '#raw_value=' do
+    it 'pre-encodes the value' do
+      value = SecureRandom.alphanumeric
+
+      subject = create(:dast_site_profile_secret_variable, raw_value: value)
+
+      expect(Base64.strict_decode64(subject.value)).to eq(value)
+    end
+  end
+
+  describe '#value=' do
+    it 'raises an error because #raw_value= should be used instead' do
+      expect { subject.value = SecureRandom.alphanumeric }.to raise_error(NoMethodError, /private method `value=' called for/)
+    end
+  end
+
+  describe '#variable_type' do
+    it 'defaults to env_var', :aggregate_failures do
+      variable_type = 'env_var'
+
+      expect(subject.variable_type).to eq(variable_type)
+      expect(described_class.new.variable_type).to eq(variable_type)
+    end
+  end
+end

ee/spec/models/dast_site_profile_spec.rb

@@ -8,6 +8,7 @@ RSpec.describe DastSiteProfile, type: :model do
   describe 'associations' do
     it { is_expected.to belong_to(:project) }
     it { is_expected.to belong_to(:dast_site) }
+    it { is_expected.to have_many(:secret_variables).class_name('Dast::SiteProfileSecretVariable') }
   end
 
   describe 'validations' do