From 731f3b89f0a582c7a88a779ff71ba3b2af68347c Mon Sep 17 00:00:00 2001 From: Darby Frey Date: Mon, 10 Jan 2022 11:31:08 -0600 Subject: [PATCH 1/8] Adding Secure Files data model and file uploader Changelog: added --- app/models/ci/secure_file.rb | 32 +++++++++ app/models/project.rb | 1 + app/uploaders/ci/secure_file_uploader.rb | 46 ++++++++++++ config/gitlab.yml.example | 12 ++++ config/initializers/1_settings.rb | 8 +++ config/object_store_settings.rb | 2 +- .../20220110170953_create_ci_secure_files.rb | 19 +++++ db/schema_migrations/20220110170953 | 1 + db/structure.sql | 33 +++++++++ .../uploaders/every_gitlab_uploader_spec.rb | 1 + lib/gitlab/database/gitlab_schemas.yml | 1 + spec/factories/ci/secure_files.rb | 10 +++ .../ci_secure_files/upload-keystore.jks | Bin 0 -> 2760 bytes spec/lib/gitlab/import_export/all_models.yml | 1 + spec/models/ci/secure_file_spec.rb | 43 ++++++++++++ spec/support/helpers/stub_object_storage.rb | 6 ++ .../uploaders/ci/secure_file_uploader_spec.rb | 66 ++++++++++++++++++ 17 files changed, 281 insertions(+), 1 deletion(-) create mode 100644 app/models/ci/secure_file.rb create mode 100644 app/uploaders/ci/secure_file_uploader.rb create mode 100644 db/migrate/20220110170953_create_ci_secure_files.rb create mode 100644 db/schema_migrations/20220110170953 create mode 100644 spec/factories/ci/secure_files.rb create mode 100644 spec/fixtures/ci_secure_files/upload-keystore.jks create mode 100644 spec/models/ci/secure_file_spec.rb create mode 100644 spec/uploaders/ci/secure_file_uploader_spec.rb diff --git a/app/models/ci/secure_file.rb b/app/models/ci/secure_file.rb new file mode 100644 index 00000000000000..e8a2a04cdb500a --- /dev/null +++ b/app/models/ci/secure_file.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +module Ci + class SecureFile < Ci::ApplicationRecord + include FileStoreMounter + + FILE_SIZE_LIMIT = 5.megabytes.freeze + CHECKSUM_ALGORITHM = 'sha256' + + belongs_to :project, optional: false + + validates :file, presence: true, file_size: { maximum: FILE_SIZE_LIMIT } + + before_validation :assign_checksum + + enum permissions: { read_only: 0, read_write: 1, execute: 2 } + + default_value_for(:file_store) { Ci::SecureFileUploader.default_store } + + mount_file_store_uploader Ci::SecureFileUploader + + def checksum_algorithm + CHECKSUM_ALGORITHM + end + + private + + def assign_checksum + self.checksum = file.checksum if file.present? && file_changed? + end + end +end \ No newline at end of file diff --git a/app/models/project.rb b/app/models/project.rb index 5c4ffd083043ca..3bdb1ab142459e 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -340,6 +340,7 @@ def self.integration_association_name(name) has_many :runners, through: :runner_projects, source: :runner, class_name: 'Ci::Runner' has_many :variables, class_name: 'Ci::Variable' has_many :triggers, class_name: 'Ci::Trigger' + has_many :secure_files, class_name: 'Ci::SecureFile' has_many :environments has_many :environments_for_dashboard, -> { from(with_rank.unfoldered.available, :environments).where('rank <= 3') }, class_name: 'Environment' has_many :deployments diff --git a/app/uploaders/ci/secure_file_uploader.rb b/app/uploaders/ci/secure_file_uploader.rb new file mode 100644 index 00000000000000..4059fc41889074 --- /dev/null +++ b/app/uploaders/ci/secure_file_uploader.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +module Ci + class SecureFileUploader < GitlabUploader + include ObjectStorage::Concern + + storage_options Gitlab.config.ci_secure_files + + # Use Lockbox to encrypt/decrypt the stored file (registers CarrierWave callbacks) + encrypt(key: :key) + + def key + OpenSSL::HMAC.digest('SHA256', Gitlab::Application.secrets.db_key_base, model.project_id.to_s) + end + + def checksum + @checksum ||= Digest::SHA256.hexdigest(model.file.read) + end + + def store_dir + dynamic_segment + end + + private + + def dynamic_segment + Gitlab::HashedPath.new('secure_files', model.id, root_hash: model.project_id) + end + + class << self + # direct upload is disabled since the file + # must always be encrypted + def direct_upload_enabled? + false + end + + def background_upload_enabled? + false + end + + def default_store + object_store_enabled? ? ObjectStorage::Store::REMOTE : ObjectStorage::Store::LOCAL + end + end + end +end \ No newline at end of file diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example index 4ff5c3b5179561..2b67572c470244 100644 --- a/config/gitlab.yml.example +++ b/config/gitlab.yml.example @@ -1425,6 +1425,18 @@ test: aws_secret_access_key: AWS_SECRET_ACCESS_KEY region: us-east-1 + ci_secure_files: + enabled: true + storage_path: tmp/tests/ci_secure_files + object_store: + enabled: false + remote_directory: ci_secure_files + connection: + provider: AWS # Only AWS supported at the moment + aws_access_key_id: AWS_ACCESS_KEY_ID + aws_secret_access_key: AWS_SECRET_ACCESS_KEY + region: us-east-1 + gitlab: host: localhost port: 80 diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index 2587347719a8c6..f65c76d8b6b0d2 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -246,6 +246,14 @@ Settings.gitlab_ci['builds_path'] = Settings.absolute(Settings.gitlab_ci['builds_path'] || "builds/") Settings.gitlab_ci['url'] ||= Settings.__send__(:build_gitlab_ci_url) +# +# CI Secure Files +# +Settings['ci_secure_files'] ||= Settingslogic.new({}) +Settings.ci_secure_files['enabled'] = true if Settings.ci_secure_files['enabled'].nil? +Settings.ci_secure_files['storage_path'] = Settings.absolute(Settings.ci_secure_files['storage_path'] || File.join(Settings.shared['path'], "ci_secure_files")) +Settings.ci_secure_files['object_store'] = ObjectStoreSettings.legacy_parse(Settings.ci_secure_files['object_store']) + # # Reply by email # diff --git a/config/object_store_settings.rb b/config/object_store_settings.rb index 8cbb3451a1658e..53fbfb088db44b 100644 --- a/config/object_store_settings.rb +++ b/config/object_store_settings.rb @@ -2,7 +2,7 @@ # Set default values for object_store settings class ObjectStoreSettings - SUPPORTED_TYPES = %w(artifacts external_diffs lfs uploads packages dependency_proxy terraform_state pages).freeze + SUPPORTED_TYPES = %w(artifacts external_diffs lfs uploads packages dependency_proxy terraform_state pages secure_files).freeze ALLOWED_OBJECT_STORE_OVERRIDES = %w(bucket enabled proxy_download).freeze # To ensure the one Workhorse credential matches the Rails config, we diff --git a/db/migrate/20220110170953_create_ci_secure_files.rb b/db/migrate/20220110170953_create_ci_secure_files.rb new file mode 100644 index 00000000000000..e0a5dbf7b1946d --- /dev/null +++ b/db/migrate/20220110170953_create_ci_secure_files.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class CreateCiSecureFiles < Gitlab::Database::Migration[1.0] + def up + create_table :ci_secure_files do |t| + t.references :project, index: true, null: false, foreign_key: { on_delete: :cascade } + t.text :name, null: false, limit: 255 + t.integer :file_store, limit: 2, null: false, default: 1 + t.text :file, null: false, limit: 255 + t.binary :checksum, null: false + t.integer :permissions, null: false, default: 0, limit: 2 + t.timestamps_with_timezone null: false + end + end + + def down + drop_table :ci_secure_files, if_exists: true + end +end diff --git a/db/schema_migrations/20220110170953 b/db/schema_migrations/20220110170953 new file mode 100644 index 00000000000000..d4c2aa5fcf26e5 --- /dev/null +++ b/db/schema_migrations/20220110170953 @@ -0,0 +1 @@ +da1c6f2db7cee1e4cb8b477d1892fa7206a95157a84864ad3d6022ab6cffbd1f \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index e39a7e2ccbf2fc..a0365bb2e8266a 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -12207,6 +12207,29 @@ CREATE SEQUENCE ci_running_builds_id_seq ALTER SEQUENCE ci_running_builds_id_seq OWNED BY ci_running_builds.id; +CREATE TABLE ci_secure_files ( + id bigint NOT NULL, + project_id bigint NOT NULL, + name text NOT NULL, + file_store smallint DEFAULT 1 NOT NULL, + file text NOT NULL, + checksum bytea NOT NULL, + permissions smallint DEFAULT 0 NOT NULL, + created_at timestamp with time zone NOT NULL, + updated_at timestamp with time zone NOT NULL, + CONSTRAINT check_320790634d CHECK ((char_length(file) <= 255)), + CONSTRAINT check_402c7b4a56 CHECK ((char_length(name) <= 255)) +); + +CREATE SEQUENCE ci_secure_files_id_seq + START WITH 1 + INCREMENT BY 1 + NO MINVALUE + NO MAXVALUE + CACHE 1; + +ALTER SEQUENCE ci_secure_files_id_seq OWNED BY ci_secure_files.id; + CREATE TABLE ci_sources_pipelines ( id integer NOT NULL, project_id integer, @@ -21438,6 +21461,8 @@ ALTER TABLE ONLY ci_runners ALTER COLUMN id SET DEFAULT nextval('ci_runners_id_s ALTER TABLE ONLY ci_running_builds ALTER COLUMN id SET DEFAULT nextval('ci_running_builds_id_seq'::regclass); +ALTER TABLE ONLY ci_secure_files ALTER COLUMN id SET DEFAULT nextval('ci_secure_files_id_seq'::regclass); + ALTER TABLE ONLY ci_sources_pipelines ALTER COLUMN id SET DEFAULT nextval('ci_sources_pipelines_id_seq'::regclass); ALTER TABLE ONLY ci_sources_projects ALTER COLUMN id SET DEFAULT nextval('ci_sources_projects_id_seq'::regclass); @@ -22928,6 +22953,9 @@ ALTER TABLE ONLY ci_runners ALTER TABLE ONLY ci_running_builds ADD CONSTRAINT ci_running_builds_pkey PRIMARY KEY (id); +ALTER TABLE ONLY ci_secure_files + ADD CONSTRAINT ci_secure_files_pkey PRIMARY KEY (id); + ALTER TABLE ONLY ci_sources_pipelines ADD CONSTRAINT ci_sources_pipelines_pkey PRIMARY KEY (id); @@ -25621,6 +25649,8 @@ CREATE INDEX index_ci_running_builds_on_project_id ON ci_running_builds USING bt CREATE INDEX index_ci_running_builds_on_runner_id ON ci_running_builds USING btree (runner_id); +CREATE INDEX index_ci_secure_files_on_project_id ON ci_secure_files USING btree (project_id); + CREATE INDEX index_ci_sources_pipelines_on_pipeline_id ON ci_sources_pipelines USING btree (pipeline_id); CREATE INDEX index_ci_sources_pipelines_on_project_id ON ci_sources_pipelines USING btree (project_id); @@ -31369,6 +31399,9 @@ ALTER TABLE ONLY experiment_subjects ALTER TABLE ONLY ci_daily_build_group_report_results ADD CONSTRAINT fk_rails_ee072d13b3 FOREIGN KEY (last_pipeline_id) REFERENCES ci_pipelines(id) ON DELETE CASCADE; +ALTER TABLE ONLY ci_secure_files + ADD CONSTRAINT fk_rails_eee98d5857 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE; + ALTER TABLE ONLY packages_debian_group_architectures ADD CONSTRAINT fk_rails_ef667d1b03 FOREIGN KEY (distribution_id) REFERENCES packages_debian_group_distributions(id) ON DELETE CASCADE; diff --git a/ee/spec/uploaders/every_gitlab_uploader_spec.rb b/ee/spec/uploaders/every_gitlab_uploader_spec.rb index 7c36853ee9d479..3942afe0d63273 100644 --- a/ee/spec/uploaders/every_gitlab_uploader_spec.rb +++ b/ee/spec/uploaders/every_gitlab_uploader_spec.rb @@ -72,6 +72,7 @@ def klass_from_path(path, root) # Please see https://gitlab.com/gitlab-org/gitlab/-/issues/328491 for more details. def known_unimplemented_uploader?(uploader) [ + Ci::SecureFileUploader, # TODO: Add Geo support for Secure Files https://gitlab.com/gitlab-org/gitlab/-/issues/349893 DeletedObjectUploader, DependencyProxy::FileUploader, Packages::Composer::CacheUploader, diff --git a/lib/gitlab/database/gitlab_schemas.yml b/lib/gitlab/database/gitlab_schemas.yml index beabca812a5c35..f4cb49c20fc5b0 100644 --- a/lib/gitlab/database/gitlab_schemas.yml +++ b/lib/gitlab/database/gitlab_schemas.yml @@ -107,6 +107,7 @@ ci_runner_projects: :gitlab_ci ci_runners: :gitlab_ci ci_running_builds: :gitlab_ci ci_sources_pipelines: :gitlab_ci +ci_secure_files: :gitlab_ci ci_sources_projects: :gitlab_ci ci_stages: :gitlab_ci ci_subscriptions_projects: :gitlab_ci diff --git a/spec/factories/ci/secure_files.rb b/spec/factories/ci/secure_files.rb new file mode 100644 index 00000000000000..9198ea61d1400a --- /dev/null +++ b/spec/factories/ci/secure_files.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :ci_secure_file, class: 'Ci::SecureFile' do + name { 'filename' } + file { fixture_file_upload('spec/fixtures/ci_secure_files/upload-keystore.jks', 'application/octet-stream') } + checksum { 'foo1234' } + project + end +end diff --git a/spec/fixtures/ci_secure_files/upload-keystore.jks b/spec/fixtures/ci_secure_files/upload-keystore.jks new file mode 100644 index 0000000000000000000000000000000000000000..715adad4a8925e6a43633f2ec97ccd231ef54a69 GIT binary patch literal 2760 zcmXqL;yS{_$ZXKWmB+@Z)#lOmotKfFaX}MTJWCT-v_TVBxIq)^Iut3^`7BMWvkjV9 zryDe}PG;kV>f+&IWLnU~>R`~sYGaTF*TKqb5NTk6;PMz~vxv-jG`n(Nm(bry`6p$T z+s>U+Q=-w$#H233#He7v!^QzIk%^O)!9bRcGoj6cF_oExQHw=j;S&Ap1*N64dIVc{ z>8aK9vox_>P<;B}Uivh{^E;22|6E)4zL+5CH8~a0#k{X}DPCAV zvoqG{m6g<`&NFNKUl#s#pLI6x?A6B`3uesDGL2&OSz*XAIVYdzkKUxHw5yipj|`_t zJend^ohVazp-eAJ*yL)S$m5x76Xr`S6v$y*q$QRT)8Mr-`%15u@b&QjyBv-^{NvY?JMUUg@VUPQK#zBvo~EgcbFX^rX%cTbICM8#Ve8zMGO z({-L3$ENVdWs$A>rGHteG@p%XS%Wmp-J6Gs1@0dgHG+*VrzqS<~42b3X ze%$n4ee=Dozt`KM=0wc-O`_`SqKk^wZvt zinljjeee73N8-$h+(&;$tWFJjxLNGL*~$;fDRRLZALs_9tA2eSBrniEQSalC@Rn11 zmb%`3oPX%i#Kz9bFA*Om7-w-Vn(dk1T-EkhcK(6A^2tAKOV)g5s(0oQQhc^DsjG9@ zO`EPLPyfJlh7V@*|DNmH`|!=;lg}3Y{{8B)tn=TQx1ZD}cHXqUaB!lVqGqqny2}Ub zFNy4OVUKHw-CoCJQ++!1{{3AAKNa28PF%RQnelRG(fuNew)k~xG9@fmoO++>aI~W1 z+KO8zD}PYy^v)z@0jr4qO;*zoJ5?u-aI-8s&0kmVecfW4fBlm5-Wma$=|MLwPxQY3 zR^&a)(d>bKge6CHr-(A+xyCrYMJDV`T~B!#b#}*h?RqVyz5VE8zcuI2vOHXHtp3R6 zLf>ZTs8HrzUu*g+`nYsf3N0u-!51ra&d9-IkLI!f*}kitXXEN$v8`2{ot5Ev!QM!- zNg(o;l^}b=6wM2hOV& z>Ax|N-^{u4lkT)Ht5T-$|36W@eeuJHK-b^`qg6`3j&GCY&YNW$vbO%K?CFVzU&}Lh z^6KWDne6VrAaTi$@9KB=8((%md+}4-o{%G40>5u4{SJJ#MbN?0h{yRBGXvvB^YG$% z(>|A8p{Gofqv}JOxISEa_)$W8P zd}><%{b}bZ{ZDOSpK987)IJq7zufS2W`KgrX@@@>`HC+x1|7X4JZ^)z!W(TFK~uP_&KO+6Ld=(_&x z#~c6iCKx&xNW+UuP7y;sIUa^mh608hhJ1!Zh7wXG%k$T{mAjo z^@E3rDW-2fI=8#k>T{eQ*9G~{84Fk%npgy^)s8*sm5SM{rOoO;edR0*`$P8+Xr(i{ zJno8UQN3NCfA`w?f{zX<+YfVuKAx~s+}N(>*WTpQe%aSd+!N&j@>`QNFD`B0`eb3G z?7X<$_v(3eSZf|>WLFN3lKl4T^{v{Ld!Cnh;v`l5V~;Brh_&vCWtZb%s{T}bk}FU@ zsQ4JyOv941JTGfD8mLasSU>YmSQ&ea@e^ z2m2ZiXMZ?oadG}L-rFCwk~e!T_|PD&QxvjJ#o!jFi{0)Gg1MKs=P%}6!IiRh!&b#y znHk5GqyknQ{dz?3u494rq}sHMl@4vW+plkX9Dl%PTIB4=8?lu?W>^Jj_x^abSjKZE z`&_xjaj!K?Y;RetSXVOXyZE$odg_Z9MLTw`TKdv_L9JHck?HsCeI9F^jWnu>xN=|j zta;SSgSnRrEa%!5WibG#C@H9 z2fOB$8v@2F1;7UdXB@<{t{C~(Tz{Lesct*dQQJ_@~x6? z|G!_RH5UqtyYzO>c{QIUa!tjzBUheJe|3HJ&Iz%LUo8&YV5;47FnvcLtDl2#y>9l5 z$oH2-6J=}k<8$!(86GoW-(B z%V(?Eu5x3_P&D0t@WClPcGtK00w=CW2CG<{(y?1nXW;(DrfnJ{d*jNzHSCog9Hr~O zz7#EHich_~EViI$YwSORoKHP(j(Tb@eFYKHwa=UfCT3OsyrVfWNvDTVhyDIDR+q}oEFIVPAEuq;w2BG$ zdD1XVUbx2OqIh8O-A5Vs_a7BGm;U96+#i8rTZ^W%QXRgW8o#Fru`0)QvmTvrX5Yig zrzWi7t2DblEt{QvjY*EtMdPApyb)_qQr?=D_c0$N{uR1dp4f1=du7`0hMvFqjPvdJ zp3GdAv}~b7sCfQ)i8b22o@YCc>Y6@Wx=H2bTpI&l149E|czW@LL literal 0 HcmV?d00001 diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index fb7eb4668b9a9b..625e94e3d77038 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -602,6 +602,7 @@ project: - bulk_import_exports - ci_project_mirror - sync_events +- secure_files award_emoji: - awardable - user diff --git a/spec/models/ci/secure_file_spec.rb b/spec/models/ci/secure_file_spec.rb new file mode 100644 index 00000000000000..c6e65ce9a032cc --- /dev/null +++ b/spec/models/ci/secure_file_spec.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::SecureFile do + let(:sample_file) { fixture_file('ci_secure_files/upload-keystore.jks') } + + subject { create(:ci_secure_file) } + + before do + stub_ci_secure_file_object_storage + end + + it { is_expected.to be_a FileStoreMounter } + + it { is_expected.to belong_to(:project).required } + + it_behaves_like 'having unique enum values' + + context 'file storage' do + describe '#permissions' do + it 'defaults to read_only file permssions' do + expect(subject.permissions).to eq('read_only') + end + end + + describe '#checksum' do + it 'computes SHA256 checksum on the file before encrypted' do + subject.file = CarrierWaveStringFile.new(sample_file) + subject.save! + expect(subject.checksum).to eq(Digest::SHA256.hexdigest(sample_file)) + end + end + + describe '#file' do + it 'returns the saved file' do + subject.file = CarrierWaveStringFile.new(sample_file) + subject.save! + expect(Base64.encode64(subject.file.read)).to eq(Base64.encode64(sample_file)) + end + end + end +end diff --git a/spec/support/helpers/stub_object_storage.rb b/spec/support/helpers/stub_object_storage.rb index 5e86b08aa45bea..d49a14f7f5b20b 100644 --- a/spec/support/helpers/stub_object_storage.rb +++ b/spec/support/helpers/stub_object_storage.rb @@ -91,6 +91,12 @@ def stub_uploads_object_storage(uploader = described_class, **params) **params) end + def stub_ci_secure_file_object_storage(**params) + stub_object_storage_uploader(config: Gitlab.config.ci_secure_files.object_store, + uploader: Ci::SecureFileUploader, + **params) + end + def stub_terraform_state_object_storage(**params) stub_object_storage_uploader(config: Gitlab.config.terraform_state.object_store, uploader: Terraform::StateUploader, diff --git a/spec/uploaders/ci/secure_file_uploader_spec.rb b/spec/uploaders/ci/secure_file_uploader_spec.rb new file mode 100644 index 00000000000000..e1457600cbed8b --- /dev/null +++ b/spec/uploaders/ci/secure_file_uploader_spec.rb @@ -0,0 +1,66 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::SecureFileUploader do + subject { ci_secure_file.file } + + let(:project) { create(:project) } + let(:ci_secure_file) { create(:ci_secure_file) } + let(:sample_file) { fixture_file('ci_secure_files/upload-keystore.jks') } + + before do + stub_ci_secure_file_object_storage + end + + describe '#key' do + it 'creates a digest with a secret key and the project id' do + expect(OpenSSL::HMAC) + .to receive(:digest) + .with('SHA256', Gitlab::Application.secrets.db_key_base, ci_secure_file.project_id.to_s) + .and_return('digest') + + expect(subject.key).to eq('digest') + end + end + + describe '.checksum' do + it 'returns a SHA256 checksum for the unencrypted file' do + expect(subject.checksum).to eq(Digest::SHA256.hexdigest(sample_file)) + end + end + + describe 'encryption' do + it 'encrypts the stored file' do + expect(Base64.encode64(subject.file.read)).not_to eq(Base64.encode64(sample_file)) + end + + it 'decrypts the file when reading' do + expect(Base64.encode64(subject.read)).to eq(Base64.encode64(sample_file)) + end + end + + describe '.direct_upload_enabled?' do + it 'returns false' do + expect(described_class.direct_upload_enabled?).to eq(false) + end + end + + describe '.default_store' do + context 'when object storage is enabled' do + it 'returns REMOTE' do + expect(described_class.default_store).to eq(ObjectStorage::Store::REMOTE) + end + end + + context 'when object storage is disabled' do + before do + stub_ci_secure_file_object_storage(enabled: false) + end + + it 'returns LOCAL' do + expect(described_class.default_store).to eq(ObjectStorage::Store::LOCAL) + end + end + end +end -- GitLab From 3272e7a0dedfc30580a05606558bc5fa2e9e06f3 Mon Sep 17 00:00:00 2001 From: Darby Frey Date: Mon, 10 Jan 2022 12:04:11 -0600 Subject: [PATCH 2/8] Fixing linter errors --- app/models/ci/secure_file.rb | 2 +- app/uploaders/ci/secure_file_uploader.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/ci/secure_file.rb b/app/models/ci/secure_file.rb index e8a2a04cdb500a..28ad4453333bc9 100644 --- a/app/models/ci/secure_file.rb +++ b/app/models/ci/secure_file.rb @@ -29,4 +29,4 @@ def assign_checksum self.checksum = file.checksum if file.present? && file_changed? end end -end \ No newline at end of file +end diff --git a/app/uploaders/ci/secure_file_uploader.rb b/app/uploaders/ci/secure_file_uploader.rb index 4059fc41889074..514d88dd177dac 100644 --- a/app/uploaders/ci/secure_file_uploader.rb +++ b/app/uploaders/ci/secure_file_uploader.rb @@ -43,4 +43,4 @@ def default_store end end end -end \ No newline at end of file +end -- GitLab From fdc86d37b68a2d34705d3da164619d20d346f4d7 Mon Sep 17 00:00:00 2001 From: Darby Frey Date: Mon, 10 Jan 2022 14:08:47 -0600 Subject: [PATCH 3/8] Adding test coverage --- spec/models/ci/secure_file_spec.rb | 38 ++++++++++--------- .../uploaders/ci/secure_file_uploader_spec.rb | 6 +++ 2 files changed, 27 insertions(+), 17 deletions(-) diff --git a/spec/models/ci/secure_file_spec.rb b/spec/models/ci/secure_file_spec.rb index c6e65ce9a032cc..39a239931c0771 100644 --- a/spec/models/ci/secure_file_spec.rb +++ b/spec/models/ci/secure_file_spec.rb @@ -17,27 +17,31 @@ it_behaves_like 'having unique enum values' - context 'file storage' do - describe '#permissions' do - it 'defaults to read_only file permssions' do - expect(subject.permissions).to eq('read_only') - end + describe '#permissions' do + it 'defaults to read_only file permssions' do + expect(subject.permissions).to eq('read_only') end + end - describe '#checksum' do - it 'computes SHA256 checksum on the file before encrypted' do - subject.file = CarrierWaveStringFile.new(sample_file) - subject.save! - expect(subject.checksum).to eq(Digest::SHA256.hexdigest(sample_file)) - end + describe '#checksum' do + it 'computes SHA256 checksum on the file before encrypted' do + subject.file = CarrierWaveStringFile.new(sample_file) + subject.save! + expect(subject.checksum).to eq(Digest::SHA256.hexdigest(sample_file)) end + end + + describe '#checksum_algorithm' do + it 'returns the configured checksum_algorithm' do + expect(subject.checksum_algorithm).to eq('sha256') + end + end - describe '#file' do - it 'returns the saved file' do - subject.file = CarrierWaveStringFile.new(sample_file) - subject.save! - expect(Base64.encode64(subject.file.read)).to eq(Base64.encode64(sample_file)) - end + describe '#file' do + it 'returns the saved file' do + subject.file = CarrierWaveStringFile.new(sample_file) + subject.save! + expect(Base64.encode64(subject.file.read)).to eq(Base64.encode64(sample_file)) end end end diff --git a/spec/uploaders/ci/secure_file_uploader_spec.rb b/spec/uploaders/ci/secure_file_uploader_spec.rb index e1457600cbed8b..3be4f742a24533 100644 --- a/spec/uploaders/ci/secure_file_uploader_spec.rb +++ b/spec/uploaders/ci/secure_file_uploader_spec.rb @@ -46,6 +46,12 @@ end end + describe '.background_upload_enabled?' do + it 'returns false' do + expect(described_class.background_upload_enabled?).to eq(false) + end + end + describe '.default_store' do context 'when object storage is enabled' do it 'returns REMOTE' do -- GitLab From 21cb9a511529704c189a6a6400340df01ebac527 Mon Sep 17 00:00:00 2001 From: Darby Frey Date: Wed, 12 Jan 2022 10:21:40 -0600 Subject: [PATCH 4/8] Updated column ordering in ci_secure_files table --- db/migrate/20220110170953_create_ci_secure_files.rb | 6 +++--- db/structure.sql | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/db/migrate/20220110170953_create_ci_secure_files.rb b/db/migrate/20220110170953_create_ci_secure_files.rb index e0a5dbf7b1946d..877a51d8e9edf6 100644 --- a/db/migrate/20220110170953_create_ci_secure_files.rb +++ b/db/migrate/20220110170953_create_ci_secure_files.rb @@ -4,12 +4,12 @@ class CreateCiSecureFiles < Gitlab::Database::Migration[1.0] def up create_table :ci_secure_files do |t| t.references :project, index: true, null: false, foreign_key: { on_delete: :cascade } - t.text :name, null: false, limit: 255 + t.timestamps_with_timezone null: false t.integer :file_store, limit: 2, null: false, default: 1 + t.integer :permissions, null: false, default: 0, limit: 2 + t.text :name, null: false, limit: 255 t.text :file, null: false, limit: 255 t.binary :checksum, null: false - t.integer :permissions, null: false, default: 0, limit: 2 - t.timestamps_with_timezone null: false end end diff --git a/db/structure.sql b/db/structure.sql index a0365bb2e8266a..f85c3b281025a1 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -12210,13 +12210,13 @@ ALTER SEQUENCE ci_running_builds_id_seq OWNED BY ci_running_builds.id; CREATE TABLE ci_secure_files ( id bigint NOT NULL, project_id bigint NOT NULL, - name text NOT NULL, + created_at timestamp with time zone NOT NULL, + updated_at timestamp with time zone NOT NULL, file_store smallint DEFAULT 1 NOT NULL, + permissions smallint DEFAULT 0 NOT NULL, + name text NOT NULL, file text NOT NULL, checksum bytea NOT NULL, - permissions smallint DEFAULT 0 NOT NULL, - created_at timestamp with time zone NOT NULL, - updated_at timestamp with time zone NOT NULL, CONSTRAINT check_320790634d CHECK ((char_length(file) <= 255)), CONSTRAINT check_402c7b4a56 CHECK ((char_length(name) <= 255)) ); -- GitLab From 8b69e9aaf614d5590420429f411e4bc7727c3490 Mon Sep 17 00:00:00 2001 From: Darby Frey Date: Wed, 12 Jan 2022 10:56:47 -0600 Subject: [PATCH 5/8] Fixing linter error in db migration --- db/migrate/20220110170953_create_ci_secure_files.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/migrate/20220110170953_create_ci_secure_files.rb b/db/migrate/20220110170953_create_ci_secure_files.rb index 877a51d8e9edf6..04b1f4fd6263f9 100644 --- a/db/migrate/20220110170953_create_ci_secure_files.rb +++ b/db/migrate/20220110170953_create_ci_secure_files.rb @@ -6,7 +6,7 @@ def up t.references :project, index: true, null: false, foreign_key: { on_delete: :cascade } t.timestamps_with_timezone null: false t.integer :file_store, limit: 2, null: false, default: 1 - t.integer :permissions, null: false, default: 0, limit: 2 + t.integer :permissions, null: false, default: 0, limit: 2 t.text :name, null: false, limit: 255 t.text :file, null: false, limit: 255 t.binary :checksum, null: false -- GitLab From 65931c225c33806ffac7a875616487a71c5fb5ef Mon Sep 17 00:00:00 2001 From: Darby Frey Date: Wed, 12 Jan 2022 13:04:49 -0600 Subject: [PATCH 6/8] Setting up loose foreign key for project_id relationship --- db/migrate/20220110170953_create_ci_secure_files.rb | 2 +- db/structure.sql | 3 --- lib/gitlab/database/gitlab_loose_foreign_keys.yml | 4 ++++ 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/db/migrate/20220110170953_create_ci_secure_files.rb b/db/migrate/20220110170953_create_ci_secure_files.rb index 04b1f4fd6263f9..1498a2d0212daf 100644 --- a/db/migrate/20220110170953_create_ci_secure_files.rb +++ b/db/migrate/20220110170953_create_ci_secure_files.rb @@ -3,7 +3,7 @@ class CreateCiSecureFiles < Gitlab::Database::Migration[1.0] def up create_table :ci_secure_files do |t| - t.references :project, index: true, null: false, foreign_key: { on_delete: :cascade } + t.bigint :project_id, index: true, null: false t.timestamps_with_timezone null: false t.integer :file_store, limit: 2, null: false, default: 1 t.integer :permissions, null: false, default: 0, limit: 2 diff --git a/db/structure.sql b/db/structure.sql index ef05573e42f15f..7260ba76891762 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -31407,9 +31407,6 @@ ALTER TABLE ONLY experiment_subjects ALTER TABLE ONLY ci_daily_build_group_report_results ADD CONSTRAINT fk_rails_ee072d13b3 FOREIGN KEY (last_pipeline_id) REFERENCES ci_pipelines(id) ON DELETE CASCADE; -ALTER TABLE ONLY ci_secure_files - ADD CONSTRAINT fk_rails_eee98d5857 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE; - ALTER TABLE ONLY packages_debian_group_architectures ADD CONSTRAINT fk_rails_ef667d1b03 FOREIGN KEY (distribution_id) REFERENCES packages_debian_group_distributions(id) ON DELETE CASCADE; diff --git a/lib/gitlab/database/gitlab_loose_foreign_keys.yml b/lib/gitlab/database/gitlab_loose_foreign_keys.yml index 1c45bfbd0dcc53..01f0d0006b36e8 100644 --- a/lib/gitlab/database/gitlab_loose_foreign_keys.yml +++ b/lib/gitlab/database/gitlab_loose_foreign_keys.yml @@ -123,3 +123,7 @@ security_scans: - table: ci_builds column: build_id on_delete: async_delete +ci_secure_files: + - table: projects + column: project_id + on_delete: async_delete -- GitLab From 4b36ffbbc13e7a6ef3bbbbc59cec139bce5ff780 Mon Sep 17 00:00:00 2001 From: Darby Frey Date: Wed, 12 Jan 2022 14:44:29 -0600 Subject: [PATCH 7/8] Triggering build -- GitLab From 785beda3f77366dbfa1f41176cd35b9a174c9fe4 Mon Sep 17 00:00:00 2001 From: Darby Frey Date: Thu, 13 Jan 2022 12:09:49 -0600 Subject: [PATCH 8/8] Adding validations and validation test for the SecureFile model --- app/models/ci/secure_file.rb | 1 + spec/models/ci/secure_file_spec.rb | 8 ++++++++ 2 files changed, 9 insertions(+) diff --git a/app/models/ci/secure_file.rb b/app/models/ci/secure_file.rb index 28ad4453333bc9..56f632b6232ad3 100644 --- a/app/models/ci/secure_file.rb +++ b/app/models/ci/secure_file.rb @@ -10,6 +10,7 @@ class SecureFile < Ci::ApplicationRecord belongs_to :project, optional: false validates :file, presence: true, file_size: { maximum: FILE_SIZE_LIMIT } + validates :checksum, :file_store, :name, :permissions, :project_id, presence: true before_validation :assign_checksum diff --git a/spec/models/ci/secure_file_spec.rb b/spec/models/ci/secure_file_spec.rb index 39a239931c0771..ae57b63e7a4903 100644 --- a/spec/models/ci/secure_file_spec.rb +++ b/spec/models/ci/secure_file_spec.rb @@ -17,6 +17,14 @@ it_behaves_like 'having unique enum values' + describe 'validations' do + it { is_expected.to validate_presence_of(:checksum) } + it { is_expected.to validate_presence_of(:file_store) } + it { is_expected.to validate_presence_of(:name) } + it { is_expected.to validate_presence_of(:permissions) } + it { is_expected.to validate_presence_of(:project_id) } + end + describe '#permissions' do it 'defaults to read_only file permssions' do expect(subject.permissions).to eq('read_only') -- GitLab