From 39e1610385028c09c40b226c01dc07902c2b56ce Mon Sep 17 00:00:00 2001 From: Aaron Huntsman Date: Sun, 7 Sep 2025 19:47:44 -0500 Subject: [PATCH 01/30] Add file upload capability to Attestation model Allows attestation files generated by GitLab to be stored as attachments to the `SupplyChain::Attestation` model. Changelog: added --- app/models/supply_chain/attestation.rb | 11 ++++ db/docs/slsa_attestation_uploads.yml | 14 +++++ ...905134037_add_file_to_slsa_attestations.rb | 12 ++++ ...ate_slsa_attestations_uploads_partition.rb | 18 ++++++ db/schema_migrations/20250905134037 | 1 + db/schema_migrations/20250905190541 | 1 + db/structure.sql | 62 +++++++++++++++++++ spec/factories/supply_chain/attestations.rb | 1 + spec/fixtures/supply_chain/attestation.json | 33 ++++++++++ spec/models/supply_chain/attestation_spec.rb | 13 +++- 10 files changed, 164 insertions(+), 2 deletions(-) create mode 100644 db/docs/slsa_attestation_uploads.yml create mode 100644 db/migrate/20250905134037_add_file_to_slsa_attestations.rb create mode 100644 db/migrate/20250905190541_create_slsa_attestations_uploads_partition.rb create mode 100644 db/schema_migrations/20250905134037 create mode 100644 db/schema_migrations/20250905190541 create mode 100644 spec/fixtures/supply_chain/attestation.json diff --git a/app/models/supply_chain/attestation.rb b/app/models/supply_chain/attestation.rb index b0d7bebe66173f..ed8fd97d7db484 100644 --- a/app/models/supply_chain/attestation.rb +++ b/app/models/supply_chain/attestation.rb @@ -11,9 +11,12 @@ class Attestation < ::ApplicationRecord validates :predicate_kind, presence: true validates :predicate_type, presence: true validates :subject_digest, presence: true, length: { minimum: 64, maximum: 255 } + validates :file, presence: true validates :subject_digest, uniqueness: { scope: [:project_id, :predicate_kind] } + mount_uploader :file, AttachmentUploader + enum :status, { success: 0, error: 1 @@ -23,5 +26,13 @@ class Attestation < ::ApplicationRecord provenance: 0, sbom: 1 } + + def uploads_sharding_key + { project_id: project_id } + end + + def retrieve_upload(_identifier, paths) + Upload.find_by(model: self, path: paths) + end end end diff --git a/db/docs/slsa_attestation_uploads.yml b/db/docs/slsa_attestation_uploads.yml new file mode 100644 index 00000000000000..cad95d4cdb5c49 --- /dev/null +++ b/db/docs/slsa_attestation_uploads.yml @@ -0,0 +1,14 @@ +--- +table_name: slsa_attestation_uploads +classes: +- SupplyChain::Attesatation +- Upload +feature_categories: +- artifact_security +description: Stores uploads for SupplyChain::Attestation model +introduced_by_url: +milestone: '18.4' +table_size: small +gitlab_schema: gitlab_main_user +sharding_key: + project_id: projects diff --git a/db/migrate/20250905134037_add_file_to_slsa_attestations.rb b/db/migrate/20250905134037_add_file_to_slsa_attestations.rb new file mode 100644 index 00000000000000..7b496b53ef6a3b --- /dev/null +++ b/db/migrate/20250905134037_add_file_to_slsa_attestations.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +class AddFileToSlsaAttestations < Gitlab::Database::Migration[2.3] + milestone '18.4' + + disable_ddl_transaction! + + def change + add_column :slsa_attestations, :file, :text, null: false, default: '' + add_text_limit :slsa_attestations, :file, 1024 + end +end diff --git a/db/migrate/20250905190541_create_slsa_attestations_uploads_partition.rb b/db/migrate/20250905190541_create_slsa_attestations_uploads_partition.rb new file mode 100644 index 00000000000000..10befe44c80829 --- /dev/null +++ b/db/migrate/20250905190541_create_slsa_attestations_uploads_partition.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +class CreateSlsaAttestationsUploadsPartition < Gitlab::Database::Migration[2.3] + include Gitlab::Database::PartitioningMigrationHelpers::TableManagementHelpers + + milestone '18.4' + + def up + create_list_partitions( + 'uploads_9ba88c4165', + { slsa_attestation: "'SupplyChain::Attestation'" }, + '%{partition_name}_uploads') + end + + def down + drop_table :slsa_attestation_uploads + end +end diff --git a/db/schema_migrations/20250905134037 b/db/schema_migrations/20250905134037 new file mode 100644 index 00000000000000..14b0d996866c4a --- /dev/null +++ b/db/schema_migrations/20250905134037 @@ -0,0 +1 @@ +dc727e6fd032b112466d37d87552efe773cddd6c20671c03b3372aac82b7ff5a \ No newline at end of file diff --git a/db/schema_migrations/20250905190541 b/db/schema_migrations/20250905190541 new file mode 100644 index 00000000000000..522f3e04d53d98 --- /dev/null +++ b/db/schema_migrations/20250905190541 @@ -0,0 +1 @@ +b87eda3aa27e0a04fb4656b7ef264c3733d3255e5301d64ea626e7dfe7bbdbca \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 853a7b497d6981..0a86a829b1c33d 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -25157,6 +25157,27 @@ CREATE SEQUENCE slack_integrations_scopes_id_seq ALTER SEQUENCE slack_integrations_scopes_id_seq OWNED BY slack_integrations_scopes.id; +CREATE TABLE slsa_attestation_uploads ( + id bigint NOT NULL, + size bigint NOT NULL, + model_id bigint NOT NULL, + uploaded_by_user_id bigint, + organization_id bigint, + namespace_id bigint, + project_id bigint, + created_at timestamp without time zone, + store integer DEFAULT 1 NOT NULL, + version integer DEFAULT 1, + path text NOT NULL, + checksum text, + model_type text NOT NULL, + uploader text NOT NULL, + mount_point text, + secret text, + CONSTRAINT check_2849dedce7 CHECK ((char_length(path) <= 511)), + CONSTRAINT check_b888b1df14 CHECK ((char_length(checksum) <= 64)) +); + CREATE TABLE slsa_attestations ( id bigint NOT NULL, created_at timestamp with time zone NOT NULL, @@ -25168,6 +25189,8 @@ CREATE TABLE slsa_attestations ( predicate_kind smallint DEFAULT 0 NOT NULL, predicate_type text NOT NULL, subject_digest text NOT NULL, + file text DEFAULT ''::text NOT NULL, + CONSTRAINT check_3575e9121e CHECK ((char_length(file) <= 1024)), CONSTRAINT check_dec11b603a CHECK ((char_length(subject_digest) <= 255)), CONSTRAINT check_ea0d61030d CHECK ((char_length(predicate_type) <= 255)) ); @@ -29200,6 +29223,8 @@ ALTER TABLE ONLY ci_runners ATTACH PARTITION project_type_ci_runners FOR VALUES ALTER TABLE ONLY uploads_9ba88c4165 ATTACH PARTITION project_uploads FOR VALUES IN ('Project'); +ALTER TABLE ONLY uploads_9ba88c4165 ATTACH PARTITION slsa_attestation_uploads FOR VALUES IN ('SupplyChain::Attestation'); + ALTER TABLE ONLY uploads_9ba88c4165 ATTACH PARTITION snippet_uploads FOR VALUES IN ('Snippet'); ALTER TABLE ONLY uploads_9ba88c4165 ATTACH PARTITION user_permission_export_upload_uploads FOR VALUES IN ('UserPermissionExportUpload'); @@ -33866,6 +33891,9 @@ ALTER TABLE ONLY slack_integrations ALTER TABLE ONLY slack_integrations_scopes ADD CONSTRAINT slack_integrations_scopes_pkey PRIMARY KEY (id); +ALTER TABLE ONLY slsa_attestation_uploads + ADD CONSTRAINT slsa_attestation_uploads_pkey PRIMARY KEY (id, model_type); + ALTER TABLE ONLY slsa_attestations ADD CONSTRAINT slsa_attestations_pkey PRIMARY KEY (id); @@ -42252,6 +42280,22 @@ CREATE UNIQUE INDEX security_findings_uuid_scan_id_partition_number_idx ON ONLY CREATE INDEX security_policy_approval_mr_rule_index_merge_request_id ON approval_merge_request_rules USING btree (merge_request_id) WHERE (report_type = ANY (ARRAY[4, 2, 5])); +CREATE INDEX slsa_attestation_uploads_checksum_idx ON slsa_attestation_uploads USING btree (checksum); + +CREATE INDEX slsa_attestation_uploads_model_id_model_type_uploader_creat_idx ON slsa_attestation_uploads USING btree (model_id, model_type, uploader, created_at); + +CREATE INDEX slsa_attestation_uploads_namespace_id_idx ON slsa_attestation_uploads USING btree (namespace_id); + +CREATE INDEX slsa_attestation_uploads_organization_id_idx ON slsa_attestation_uploads USING btree (organization_id); + +CREATE INDEX slsa_attestation_uploads_project_id_idx ON slsa_attestation_uploads USING btree (project_id); + +CREATE INDEX slsa_attestation_uploads_store_idx ON slsa_attestation_uploads USING btree (store); + +CREATE INDEX slsa_attestation_uploads_uploaded_by_user_id_idx ON slsa_attestation_uploads USING btree (uploaded_by_user_id); + +CREATE INDEX slsa_attestation_uploads_uploader_path_idx ON slsa_attestation_uploads USING btree (uploader, path); + CREATE INDEX snippet_uploads_checksum_idx ON snippet_uploads USING btree (checksum); CREATE INDEX snippet_uploads_model_id_model_type_uploader_created_at_idx ON snippet_uploads USING btree (model_id, model_type, uploader, created_at); @@ -45334,6 +45378,24 @@ ALTER INDEX index_uploads_9ba88c4165_on_uploaded_by_user_id ATTACH PARTITION pro ALTER INDEX index_uploads_9ba88c4165_on_uploader_and_path ATTACH PARTITION project_uploads_uploader_path_idx; +ALTER INDEX index_uploads_9ba88c4165_on_checksum ATTACH PARTITION slsa_attestation_uploads_checksum_idx; + +ALTER INDEX index_uploads_9ba88c4165_on_model_uploader_created_at ATTACH PARTITION slsa_attestation_uploads_model_id_model_type_uploader_creat_idx; + +ALTER INDEX index_uploads_9ba88c4165_on_namespace_id ATTACH PARTITION slsa_attestation_uploads_namespace_id_idx; + +ALTER INDEX index_uploads_9ba88c4165_on_organization_id ATTACH PARTITION slsa_attestation_uploads_organization_id_idx; + +ALTER INDEX uploads_9ba88c4165_pkey ATTACH PARTITION slsa_attestation_uploads_pkey; + +ALTER INDEX index_uploads_9ba88c4165_on_project_id ATTACH PARTITION slsa_attestation_uploads_project_id_idx; + +ALTER INDEX index_uploads_9ba88c4165_on_store ATTACH PARTITION slsa_attestation_uploads_store_idx; + +ALTER INDEX index_uploads_9ba88c4165_on_uploaded_by_user_id ATTACH PARTITION slsa_attestation_uploads_uploaded_by_user_id_idx; + +ALTER INDEX index_uploads_9ba88c4165_on_uploader_and_path ATTACH PARTITION slsa_attestation_uploads_uploader_path_idx; + ALTER INDEX index_uploads_9ba88c4165_on_checksum ATTACH PARTITION snippet_uploads_checksum_idx; ALTER INDEX index_uploads_9ba88c4165_on_model_uploader_created_at ATTACH PARTITION snippet_uploads_model_id_model_type_uploader_created_at_idx; diff --git a/spec/factories/supply_chain/attestations.rb b/spec/factories/supply_chain/attestations.rb index 13e3e1bf47fdcd..185ad39ecb8094 100644 --- a/spec/factories/supply_chain/attestations.rb +++ b/spec/factories/supply_chain/attestations.rb @@ -7,5 +7,6 @@ predicate_kind { :provenance } predicate_type { "https://slsa.dev/provenance/v1" } subject_digest { Digest::SHA256.hexdigest("abc") } + file { fixture_file_upload('spec/fixtures/supply_chain/attestation.json') } end end diff --git a/spec/fixtures/supply_chain/attestation.json b/spec/fixtures/supply_chain/attestation.json new file mode 100644 index 00000000000000..7143e8f5f9b828 --- /dev/null +++ b/spec/fixtures/supply_chain/attestation.json @@ -0,0 +1,33 @@ +{ + "mediaType": "application/vnd.dev.sigstore.bundle.v0.3+json", + "verificationMaterial": { + "certificate": { + "rawBytes": "[example]" + }, + "tlogEntries": [ + { + "logIndex": "12345", + "logId": { + "keyId": "[example]" + }, + "kindVersion": { + "kind": "dsse", + "version": "0.0.1" + }, + "inclusionProof": { + "logIndex": "12345" + }, + "canonicalizedBody": "[example]" + } + ] + }, + "dsseEnvelope": { + "payload": "[example]", + "payloadType": "application/vnd.in-toto+json", + "signatures": [ + { + "sig": "[signature]" + } + ] + } +} diff --git a/spec/models/supply_chain/attestation_spec.rb b/spec/models/supply_chain/attestation_spec.rb index 9e457826f92c97..0439a5e4defe95 100644 --- a/spec/models/supply_chain/attestation_spec.rb +++ b/spec/models/supply_chain/attestation_spec.rb @@ -3,9 +3,9 @@ require 'spec_helper' RSpec.describe SupplyChain::Attestation, feature_category: :artifact_security do - describe "validations" do - subject { create(:supply_chain_attestation) } + subject { create(:supply_chain_attestation) } + describe "validations" do it { is_expected.to belong_to(:project) } it { is_expected.to belong_to(:build) } @@ -13,7 +13,16 @@ it { is_expected.to validate_presence_of(:predicate_kind) } it { is_expected.to validate_presence_of(:predicate_type) } it { is_expected.to validate_presence_of(:subject_digest) } + it { is_expected.to validate_presence_of(:file) } it { is_expected.to validate_uniqueness_of(:subject_digest).scoped_to([:project_id, :predicate_kind]) } end + + describe "uploads" do + it_behaves_like "model with uploads", false do + let(:model_object) { create(:supply_chain_attestation) } + let(:upload_attribute) { :file } + let(:uploader_class) { AttachmentUploader } + end + end end -- GitLab From 0364bcdaded8ec0dc95361a5a26c570e4265da31 Mon Sep 17 00:00:00 2001 From: Aaron Huntsman Date: Sun, 7 Sep 2025 20:20:54 -0500 Subject: [PATCH 02/30] Fix irreversible migration --- db/migrate/20250905134037_add_file_to_slsa_attestations.rb | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/db/migrate/20250905134037_add_file_to_slsa_attestations.rb b/db/migrate/20250905134037_add_file_to_slsa_attestations.rb index 7b496b53ef6a3b..4a223d5496af32 100644 --- a/db/migrate/20250905134037_add_file_to_slsa_attestations.rb +++ b/db/migrate/20250905134037_add_file_to_slsa_attestations.rb @@ -5,8 +5,13 @@ class AddFileToSlsaAttestations < Gitlab::Database::Migration[2.3] disable_ddl_transaction! - def change + def up add_column :slsa_attestations, :file, :text, null: false, default: '' add_text_limit :slsa_attestations, :file, 1024 end + + def down + remove_text_limit :slsa_attestations, :file + remove_column :slsa_attestations, :file, if_exists: true + end end -- GitLab From 18b910644b6e69a3493b2d9e5faf6c1ba11398bf Mon Sep 17 00:00:00 2001 From: Aaron Huntsman Date: Sun, 7 Sep 2025 23:01:25 -0500 Subject: [PATCH 03/30] Update partition exceptions in specs --- db/docs/slsa_attestation_uploads.yml | 6 +++--- spec/db/schema_spec.rb | 1 + spec/lib/gitlab/database/sharding_key_spec.rb | 2 ++ 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/db/docs/slsa_attestation_uploads.yml b/db/docs/slsa_attestation_uploads.yml index cad95d4cdb5c49..0b397a52f5bb3f 100644 --- a/db/docs/slsa_attestation_uploads.yml +++ b/db/docs/slsa_attestation_uploads.yml @@ -1,14 +1,14 @@ --- table_name: slsa_attestation_uploads classes: -- SupplyChain::Attesatation - Upload +- SupplyChain::Attesatation feature_categories: - artifact_security description: Stores uploads for SupplyChain::Attestation model -introduced_by_url: +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/204116 milestone: '18.4' table_size: small -gitlab_schema: gitlab_main_user +gitlab_schema: gitlab_main_org sharding_key: project_id: projects diff --git a/spec/db/schema_spec.rb b/spec/db/schema_spec.rb index 56f92996e86aa4..87e86fe831868e 100644 --- a/spec/db/schema_spec.rb +++ b/spec/db/schema_spec.rb @@ -214,6 +214,7 @@ project_import_export_relation_export_upload_uploads: %w[model_id], project_topic_uploads: %w[model_id], project_uploads: %w[model_id], + slsa_attestation_uploads: %w[model_id], snippet_uploads: %w[model_id], user_permission_export_upload_uploads: %w[model_id], user_uploads: %w[model_id], diff --git a/spec/lib/gitlab/database/sharding_key_spec.rb b/spec/lib/gitlab/database/sharding_key_spec.rb index 42999dd9b63571..ff368e8c37aa6e 100644 --- a/spec/lib/gitlab/database/sharding_key_spec.rb +++ b/spec/lib/gitlab/database/sharding_key_spec.rb @@ -48,6 +48,7 @@ project_import_export_relation_export_upload_uploads.project_id project_topic_uploads.organization_id project_uploads.project_id + slsa_attestation_uploads.project_id snippet_uploads.organization_id vulnerability_export_part_uploads.organization_id vulnerability_export_uploads.organization_id @@ -275,6 +276,7 @@ "project_import_export_relation_export_upload_uploads" => "https://gitlab.com/gitlab-org/gitlab/-/issues/398199", "project_topic_uploads" => "https://gitlab.com/gitlab-org/gitlab/-/issues/398199", "project_uploads" => "https://gitlab.com/gitlab-org/gitlab/-/issues/398199", + "slsa_attestation_uploads" => "https://gitlab.com/gitlab-org/gitlab/-/issues/398199", "snippet_uploads" => "https://gitlab.com/gitlab-org/gitlab/-/issues/398199", "uploads_9ba88c4165" => "https://gitlab.com/gitlab-org/gitlab/-/issues/398199", "user_permission_export_upload_uploads" => "https://gitlab.com/gitlab-org/gitlab/-/issues/398199", -- GitLab From aa8073543ae56db6f1fa40fa845a166bdf778486 Mon Sep 17 00:00:00 2001 From: Sam Joan Roque-Worcel Date: Mon, 15 Sep 2025 15:03:26 +1200 Subject: [PATCH 04/30] Avoid logging attestation as it is large --- app/services/ci/slsa/publish_provenance_service.rb | 2 +- spec/services/ci/slsa/publish_provenance_service_spec.rb | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/app/services/ci/slsa/publish_provenance_service.rb b/app/services/ci/slsa/publish_provenance_service.rb index 1cb9f91fd4a0a2..95fb2c04301f14 100644 --- a/app/services/ci/slsa/publish_provenance_service.rb +++ b/app/services/ci/slsa/publish_provenance_service.rb @@ -40,7 +40,7 @@ def execute id_token: id_token) @logger.info(class: self.class.name, message: "Attestation successful", hash: hash, blob_name: blob_name, - attestation: attestation, duration: duration, build_id: @build.id) + duration: duration, build_id: @build.id) end ServiceResponse.success(message: "OK") diff --git a/spec/services/ci/slsa/publish_provenance_service_spec.rb b/spec/services/ci/slsa/publish_provenance_service_spec.rb index d5404012eef177..9c7083bcf84468 100644 --- a/spec/services/ci/slsa/publish_provenance_service_spec.rb +++ b/spec/services/ci/slsa/publish_provenance_service_spec.rb @@ -71,7 +71,6 @@ message: "Attestation successful", hash: hash, blob_name: File.basename(path), - attestation: signature_bundle, duration: expected_duration, build_id: build.id })) -- GitLab From 20177715ce9da79a936dd08f9c60a9ff7c9cf7c9 Mon Sep 17 00:00:00 2001 From: Sam Joan Roque-Worcel Date: Tue, 16 Sep 2025 11:02:14 +1200 Subject: [PATCH 05/30] Refactor publish_provenance_service.rb file In this commit, I've changed the structure of the file to allow for the changes I will make on it shortly. --- .../ci/slsa/publish_provenance_service.rb | 22 +++++++++++-------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/app/services/ci/slsa/publish_provenance_service.rb b/app/services/ci/slsa/publish_provenance_service.rb index 95fb2c04301f14..4f230d3850d219 100644 --- a/app/services/ci/slsa/publish_provenance_service.rb +++ b/app/services/ci/slsa/publish_provenance_service.rb @@ -13,7 +13,6 @@ class PublishProvenanceService < ::BaseService def initialize(build) @build = build - @logger = Gitlab::AppJsonLogger end def execute @@ -26,24 +25,29 @@ def execute id_token = @build.variables["SIGSTORE_ID_TOKEN"]&.value return ServiceResponse.error(message: "Missing required variable SIGSTORE_ID_TOKEN") unless id_token - reader = SupplyChain::ArtifactsReader.new(@build) + attest_all_artifacts(build: @build, id_token: id_token) + + ServiceResponse.success(message: "OK") + end + + def attest_all_artifacts(build:, id_token:) + logger = Gitlab::AppJsonLogger + reader = SupplyChain::ArtifactsReader.new(build) reader.files do |artifact_path, file_input_stream| blob_name = File.basename(artifact_path) hash = hash(file_input_stream) - predicate = SupplyChain::Slsa::ProvenanceStatement::Predicate.from_build(@build).to_json + predicate = SupplyChain::Slsa::ProvenanceStatement::Predicate.from_build(build).to_json - @logger.info(class: self.class.name, message: "Performing attestation for artifact", hash: hash, - path: artifact_path, build_id: @build.id) + logger.info(class: self.class.name, message: "Performing attestation for artifact", hash: hash, + path: artifact_path, build_id: build.id) attestation, duration = attest_blob!(blob_name: blob_name, hash: hash, predicate: predicate, id_token: id_token) - @logger.info(class: self.class.name, message: "Attestation successful", hash: hash, blob_name: blob_name, - duration: duration, build_id: @build.id) + logger.info(class: self.class.name, message: "Attestation successful", hash: hash, blob_name: blob_name, + duration: duration, build_id: build.id) end - - ServiceResponse.success(message: "OK") end def hash(file_input_stream) -- GitLab From a991ba6aeb39dbae015f0b75f81153ba8199944e Mon Sep 17 00:00:00 2001 From: Sam Joan Roque-Worcel Date: Tue, 16 Sep 2025 11:29:54 +1200 Subject: [PATCH 06/30] Make methods private, add memoization In this commit, I've made two changes: - Add `strong_memoize_attr` to the `predicate` method. This is doable because only the subject of the attestation references the artifact, and this is done by our call to the `cosign` command. - Make private methods explicitly private. --- app/services/ci/slsa/publish_provenance_service.rb | 10 +++++++++- .../ci/slsa/publish_provenance_service_spec.rb | 8 ++++---- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/app/services/ci/slsa/publish_provenance_service.rb b/app/services/ci/slsa/publish_provenance_service.rb index 4f230d3850d219..c0cd84a1362ed4 100644 --- a/app/services/ci/slsa/publish_provenance_service.rb +++ b/app/services/ci/slsa/publish_provenance_service.rb @@ -5,6 +5,8 @@ module Ci module Slsa class PublishProvenanceService < ::BaseService + include Gitlab::Utils::StrongMemoize + HASH_READ_CHUNK_SIZE = 1.megabyte Error = Class.new(StandardError) @@ -30,6 +32,13 @@ def execute ServiceResponse.success(message: "OK") end + private + + def predicate + SupplyChain::Slsa::ProvenanceStatement::Predicate.from_build(@build).to_json + end + strong_memoize_attr :predicate + def attest_all_artifacts(build:, id_token:) logger = Gitlab::AppJsonLogger reader = SupplyChain::ArtifactsReader.new(build) @@ -37,7 +46,6 @@ def attest_all_artifacts(build:, id_token:) reader.files do |artifact_path, file_input_stream| blob_name = File.basename(artifact_path) hash = hash(file_input_stream) - predicate = SupplyChain::Slsa::ProvenanceStatement::Predicate.from_build(build).to_json logger.info(class: self.class.name, message: "Performing attestation for artifact", hash: hash, path: artifact_path, build_id: build.id) diff --git a/spec/services/ci/slsa/publish_provenance_service_spec.rb b/spec/services/ci/slsa/publish_provenance_service_spec.rb index 9c7083bcf84468..e3df9229e4471a 100644 --- a/spec/services/ci/slsa/publish_provenance_service_spec.rb +++ b/spec/services/ci/slsa/publish_provenance_service_spec.rb @@ -201,7 +201,7 @@ let(:blob_name) { "test.txt" } subject(:attest_blob) do - service.attest_blob!(blob_name: 'test.txt', hash: hash, predicate: "{}", id_token: id_token) + service.send(:attest_blob!, blob_name: 'test.txt', hash: hash, predicate: "{}", id_token: id_token) end context "when called normally" do @@ -216,7 +216,7 @@ end describe '#validate_id_token!' do - subject(:validate_id_token) { service.validate_id_token!(id_token) } + subject(:validate_id_token) { service.send(:validate_id_token!, id_token) } context "when an valid looking JWT is passed" do it 'does not raise_error when a valid JWT is passed' do @@ -242,7 +242,7 @@ end describe '#validate_hash!' do - subject(:validate_hash) { service.validate_hash!(hash) } + subject(:validate_hash) { service.send(:validate_hash!, hash) } context "when an valid SHA-256 is passed" do let(:hash) { "5db1fee4b5703808c48078a76768b155b421b210c0761cd6a5d223f4d99f1eaa" } @@ -262,7 +262,7 @@ end describe '#validate_blob_name!' do - subject(:validate_blob_name) { service.validate_blob_name!(blob_name) } + subject(:validate_blob_name) { service.send(:validate_blob_name!, blob_name) } context "when a valid base name is passed" do let(:blob_name) { "artifact.tar.gz" } -- GitLab From 760c6f2d81da5122677b57ed30530573f27fd85a Mon Sep 17 00:00:00 2001 From: Sam Joan Roque-Worcel Date: Tue, 16 Sep 2025 12:49:21 +1200 Subject: [PATCH 07/30] Add wrapper for storing attestation failures In this commit, I've added a basic wrapper for attesting failures, as well as making use of caching in the attest_blob method. I also added a specific check for caching that ensures we only call from_build one time. --- .../ci/slsa/publish_provenance_service.rb | 18 +++++---- .../slsa/publish_provenance_service_spec.rb | 38 ++++++++++++------- 2 files changed, 36 insertions(+), 20 deletions(-) diff --git a/app/services/ci/slsa/publish_provenance_service.rb b/app/services/ci/slsa/publish_provenance_service.rb index c0cd84a1362ed4..1e7b5c3eee514f 100644 --- a/app/services/ci/slsa/publish_provenance_service.rb +++ b/app/services/ci/slsa/publish_provenance_service.rb @@ -47,14 +47,18 @@ def attest_all_artifacts(build:, id_token:) blob_name = File.basename(artifact_path) hash = hash(file_input_stream) - logger.info(class: self.class.name, message: "Performing attestation for artifact", hash: hash, - path: artifact_path, build_id: build.id) + begin + logger.info(class: self.class.name, message: "Performing attestation for artifact", hash: hash, + path: artifact_path, build_id: build.id) - attestation, duration = attest_blob!(blob_name: blob_name, hash: hash, predicate: predicate, - id_token: id_token) + _attestation, duration = attest_blob!(blob_name: blob_name, hash: hash, id_token: id_token) - logger.info(class: self.class.name, message: "Attestation successful", hash: hash, blob_name: blob_name, - duration: duration, build_id: build.id) + logger.info(class: self.class.name, message: "Attestation successful", hash: hash, blob_name: blob_name, + duration: duration, build_id: build.id) + rescue StandardError => e + Gitlab::ErrorTracking.track_exception(e, project_id: build.project.id) + persist_attestation!(build: build, status: :error, subject_digest: hash) + end end end @@ -68,7 +72,7 @@ def ci_server_url Gitlab.config.gitlab.url end - def attest_blob!(blob_name:, hash:, predicate:, id_token:) + def attest_blob!(blob_name:, hash:, id_token:) validate_id_token!(id_token) validate_blob_name!(blob_name) validate_hash!(hash) diff --git a/spec/services/ci/slsa/publish_provenance_service_spec.rb b/spec/services/ci/slsa/publish_provenance_service_spec.rb index e3df9229e4471a..438449af25a1dd 100644 --- a/spec/services/ci/slsa/publish_provenance_service_spec.rb +++ b/spec/services/ci/slsa/publish_provenance_service_spec.rb @@ -12,11 +12,11 @@ end let_it_be(:signature_bundle) { File.read(SLSA_ATTESTATION_BUNDLE) } - let_it_be(:expected_stderr) { "expected stderr outuput string" } + let_it_be(:expected_stderr) { "expecte stderr outuput string" } let_it_be(:expected_stdout) { "expected stderr outuput string" } let_it_be(:tmp_file_path) { "/tmp/folder/file.bundle" } let_it_be(:expected_duration) { 1.33337 } - let(:expected_predicate) { SupplyChain::Slsa::ProvenanceStatement::Predicate.from_build(build).to_json } + let(:expected_predicate) { service.send(:predicate) } let(:popen_success) { true } let(:process_status) do process_status = instance_double(Process::Status) @@ -56,6 +56,9 @@ it 'logs the right hash and attestation' do allow(Gitlab::AppJsonLogger).to receive(:info) + predicate_class = SupplyChain::Slsa::ProvenanceStatement::Predicate + expect(predicate_class).to receive(:from_build).exactly(1).time.and_call_original + expect(execute[:status]).to eq(:success) expect(execute[:message]).to eq("OK") @@ -81,8 +84,7 @@ it 'calls attest with the right parameters' do expected_hashes.each do |path, hash| - expect(service).to receive(:attest_blob!).with(blob_name: path, id_token: id_token, - predicate: expected_predicate, hash: hash) + expect(service).to receive(:attest_blob!).with(blob_name: path, id_token: id_token, hash: hash) end expect(execute[:message]).to eq("OK") @@ -141,14 +143,6 @@ end end - context 'when popen returns an error' do - let(:popen_success) { false } - - it 'raises the appropriate exception' do - expect { execute }.to raise_exception(described_class::AttestationFailure) - end - end - context "when the build does not have SIGSTORE_ID_TOKEN" do let(:yaml_variables) do [ @@ -194,6 +188,16 @@ expect(execute[:message]).to eq("Attestation is only enabled for public projects") end end + + context "when attestation fails" do + before do + allow(service).to receive(:validate_blob_name!).with(any_args).and_raise(StandardError) + end + + it 'persists an attestation' do + expect(service).to be(false) # TODO + end + end end describe '#attest_blob!' do @@ -201,7 +205,7 @@ let(:blob_name) { "test.txt" } subject(:attest_blob) do - service.send(:attest_blob!, blob_name: 'test.txt', hash: hash, predicate: "{}", id_token: id_token) + service.send(:attest_blob!, blob_name: 'test.txt', hash: hash, id_token: id_token) end context "when called normally" do @@ -213,6 +217,14 @@ attest_blob end end + + context 'when popen returns an error' do + let(:popen_success) { false } + + it 'raises the appropriate exception' do + expect { attest_blob }.to raise_exception(described_class::AttestationFailure) + end + end end describe '#validate_id_token!' do -- GitLab From 661c3d134bd54ee296f53fb5623ef4262b8a4b23 Mon Sep 17 00:00:00 2001 From: Sam Joan Roque-Worcel Date: Tue, 16 Sep 2025 15:45:24 +1200 Subject: [PATCH 08/30] Implement persistence for :error attestations In this commit, I've implemented attestation persistence for failure cases. I've also removed the requirement for files to be present when there is a failure in the attestation, and modified the ServiceResponse objects to include the attestations in the payload. --- app/models/supply_chain/attestation.rb | 2 +- .../supply_chain/slsa/provenance_statement.rb | 4 +- .../ci/slsa/publish_provenance_service.rb | 30 ++++++++++-- .../slsa/publish_provenance_service_spec.rb | 46 ++++++++++++------- 4 files changed, 60 insertions(+), 22 deletions(-) diff --git a/app/models/supply_chain/attestation.rb b/app/models/supply_chain/attestation.rb index ed8fd97d7db484..2a3ecd99141735 100644 --- a/app/models/supply_chain/attestation.rb +++ b/app/models/supply_chain/attestation.rb @@ -11,7 +11,7 @@ class Attestation < ::ApplicationRecord validates :predicate_kind, presence: true validates :predicate_type, presence: true validates :subject_digest, presence: true, length: { minimum: 64, maximum: 255 } - validates :file, presence: true + validates :file, presence: true, unless: :error? validates :subject_digest, uniqueness: { scope: [:project_id, :predicate_kind] } diff --git a/app/models/supply_chain/slsa/provenance_statement.rb b/app/models/supply_chain/slsa/provenance_statement.rb index 9f72b1a8e96cf2..926031ca075d73 100644 --- a/app/models/supply_chain/slsa/provenance_statement.rb +++ b/app/models/supply_chain/slsa/provenance_statement.rb @@ -24,6 +24,8 @@ class ProvenanceStatement attr_accessor :_type, :subject, :predicate_type, :predicate + PREDICATE_TYPE_V1 = "https://slsa.dev/provenance/v1" + def self.from_build(build) archives = build.job_artifacts.filter { |artifact| artifact.file_type == "archive" } raise ArgumentError, 'artifacts associated with build do not contain a single archive' if archives.length != 1 @@ -40,7 +42,7 @@ def self.from_build(build) def initialize @_type = "https://in-toto.io/Statement/v1" - @predicate_type = "https://slsa.dev/provenance/v1" + @predicate_type = PREDICATE_TYPE end def as_json(options = nil) diff --git a/app/services/ci/slsa/publish_provenance_service.rb b/app/services/ci/slsa/publish_provenance_service.rb index 1e7b5c3eee514f..2db39d27c7a0ad 100644 --- a/app/services/ci/slsa/publish_provenance_service.rb +++ b/app/services/ci/slsa/publish_provenance_service.rb @@ -28,8 +28,6 @@ def execute return ServiceResponse.error(message: "Missing required variable SIGSTORE_ID_TOKEN") unless id_token attest_all_artifacts(build: @build, id_token: id_token) - - ServiceResponse.success(message: "OK") end private @@ -43,6 +41,8 @@ def attest_all_artifacts(build:, id_token:) logger = Gitlab::AppJsonLogger reader = SupplyChain::ArtifactsReader.new(build) + all_successful = true + attestations = [] reader.files do |artifact_path, file_input_stream| blob_name = File.basename(artifact_path) hash = hash(file_input_stream) @@ -57,9 +57,33 @@ def attest_all_artifacts(build:, id_token:) duration: duration, build_id: build.id) rescue StandardError => e Gitlab::ErrorTracking.track_exception(e, project_id: build.project.id) - persist_attestation!(build: build, status: :error, subject_digest: hash) + + attestation = persist_attestation!(build: build, status: :error, subject_digest: hash) + attestations << attestation + + all_successful = false end end + + return ServiceResponse.success(message: "OK", payload: { attestations: attestations }) if all_successful + + ServiceResponse.error(message: "Attestation failure", payload: { attestations: attestations }) + end + + def persist_attestation!(build:, status:, subject_digest:) + attestation = SupplyChain::Attestation.new do |att| + att.subject_digest = subject_digest + att.project = build.project + att.build = build + att.status = status + att.expire_at = 2.years.from_now # TODO: adjust based on outcomes of ticket discussion. + att.predicate_kind = :provenance + att.predicate_type = SupplyChain::Slsa::ProvenanceStatement::PREDICATE_TYPE_V1 + end + + attestation.save! + + attestation end def hash(file_input_stream) diff --git a/spec/services/ci/slsa/publish_provenance_service_spec.rb b/spec/services/ci/slsa/publish_provenance_service_spec.rb index 438449af25a1dd..73dd796acf77de 100644 --- a/spec/services/ci/slsa/publish_provenance_service_spec.rb +++ b/spec/services/ci/slsa/publish_provenance_service_spec.rb @@ -43,7 +43,9 @@ end describe '#execute' do - subject(:execute) { service.execute } + subject(:result) { service.execute } + + let(:attestations) { result.payload[:attestations] } let(:expected_hashes) do { @@ -59,8 +61,8 @@ predicate_class = SupplyChain::Slsa::ProvenanceStatement::Predicate expect(predicate_class).to receive(:from_build).exactly(1).time.and_call_original - expect(execute[:status]).to eq(:success) - expect(execute[:message]).to eq("OK") + expect(result[:status]).to eq(:success) + expect(result[:message]).to eq("OK") expected_hashes.each do |path, hash| expect(Gitlab::AppJsonLogger).to have_received(:info).with(a_hash_including({ @@ -87,7 +89,7 @@ expect(service).to receive(:attest_blob!).with(blob_name: path, id_token: id_token, hash: hash) end - expect(execute[:message]).to eq("OK") + expect(result[:message]).to eq("OK") end it 'calls cosign with the appropriate parameters' do @@ -99,7 +101,7 @@ expect(Gitlab::Popen).to receive(:popen_with_detail).with(expected_parameters).and_return(popen_result) end - expect(execute[:message]).to eq("OK") + expect(result[:message]).to eq("OK") end context 'when environment variables for optional parameters exist' do @@ -122,7 +124,7 @@ expect(Gitlab::Popen).to receive(:popen_with_detail).with(expected_parameters).and_return(popen_result) end - expect(execute[:message]).to eq("OK") + expect(result[:message]).to eq("OK") end end @@ -138,7 +140,7 @@ expect(Gitlab::Popen).to receive(:popen_with_detail).with(expected_parameters).and_return(popen_result) end - expect(execute[:message]).to eq("OK") + expect(result[:message]).to eq("OK") end end end @@ -151,8 +153,8 @@ end it "returns an error" do - expect(execute[:status]).to eq(:error) - expect(execute[:message]).to eq("Missing required variable SIGSTORE_ID_TOKEN") + expect(result[:status]).to eq(:error) + expect(result[:message]).to eq("Missing required variable SIGSTORE_ID_TOKEN") end end @@ -160,8 +162,8 @@ let(:service) { described_class.new(nil) } it "returns an error" do - expect(execute[:status]).to eq(:error) - expect(execute[:message]).to eq("Unable to find build") + expect(result[:status]).to eq(:error) + expect(result[:message]).to eq("Unable to find build") end end @@ -172,8 +174,8 @@ end it "returns an error" do - expect(execute[:status]).to eq(:error) - expect(execute[:message]).to eq("Attestation is only enabled for public projects") + expect(result[:status]).to eq(:error) + expect(result[:message]).to eq("Attestation is only enabled for public projects") end end @@ -184,8 +186,8 @@ end it "returns an error" do - expect(execute[:status]).to eq(:error) - expect(execute[:message]).to eq("Attestation is only enabled for public projects") + expect(result[:status]).to eq(:error) + expect(result[:message]).to eq("Attestation is only enabled for public projects") end end @@ -194,10 +196,20 @@ allow(service).to receive(:validate_blob_name!).with(any_args).and_raise(StandardError) end - it 'persists an attestation' do - expect(service).to be(false) # TODO + it 'persists a :failed attestation' do + expect(result[:status]).to eq(:error) + expect(result[:message]).to eq("Attestation failure") + + expect(attestations.length).to eq(3) + expect(attestations).to all(be_presisted) + expect(attestations).to all(be_a(SupplyChain::Attestation)) + + # TODO: validate attributes of attestation end end + + # TODO: test duplicate attestation for predicate. + # TODO: add test that ensures we log the exception and don't swallow it. end describe '#attest_blob!' do -- GitLab From 868dba60f1d04c25ddbfc0ae5506c5a131f40959 Mon Sep 17 00:00:00 2001 From: Sam Joan Roque-Worcel Date: Wed, 17 Sep 2025 10:52:18 +1200 Subject: [PATCH 09/30] Validate the properties of Attestation objects --- .../slsa/publish_provenance_service_spec.rb | 20 +++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/spec/services/ci/slsa/publish_provenance_service_spec.rb b/spec/services/ci/slsa/publish_provenance_service_spec.rb index 73dd796acf77de..48c36326ac328c 100644 --- a/spec/services/ci/slsa/publish_provenance_service_spec.rb +++ b/spec/services/ci/slsa/publish_provenance_service_spec.rb @@ -192,19 +192,35 @@ end context "when attestation fails" do + let(:expected_predicate_type) { SupplyChain::Slsa::ProvenanceStatement::PREDICATE_TYPE_V1 } + before do allow(service).to receive(:validate_blob_name!).with(any_args).and_raise(StandardError) end it 'persists a :failed attestation' do + expect(Gitlab::ErrorTracking).to receive(:track_exception).exactly(3).times + expect(result[:status]).to eq(:error) expect(result[:message]).to eq("Attestation failure") expect(attestations.length).to eq(3) - expect(attestations).to all(be_presisted) + expect(attestations).to all(be_a(SupplyChain::Attestation)) + expect(attestations).to all(be_persisted) + expect(attestations).to all(be_error) + expect(attestations).to all(be_provenance) + + attestations.each do |att| + expect(att.project_id).to eq(project.id) + expect(att.build_id).to eq(build.id) + expect(att.predicate_type).to eq(expected_predicate_type) + expect(att.file.read).to be_nil + end - # TODO: validate attributes of attestation + expected_hashes.each_value do |hash| + expect(attestations).to include(an_object_having_attributes(subject_digest: hash)) + end end end -- GitLab From 2f87ef86154ac96ff741846f80eba83f849904bc Mon Sep 17 00:00:00 2001 From: Sam Joan Roque-Worcel Date: Thu, 18 Sep 2025 11:12:39 +1200 Subject: [PATCH 10/30] Refactor provenance service, persist successful attestations In this commit, I've slightly refactored the PublishProvenanceService class to make it simpler to persist the successful attestations. I've also implemented that feature, tests upcoming. --- app/models/supply_chain/attestation.rb | 2 +- .../ci/slsa/publish_provenance_service.rb | 61 ++++++++++--------- .../slsa/publish_provenance_service_spec.rb | 24 ++++++-- 3 files changed, 51 insertions(+), 36 deletions(-) diff --git a/app/models/supply_chain/attestation.rb b/app/models/supply_chain/attestation.rb index 2a3ecd99141735..ac6fbecfdb726d 100644 --- a/app/models/supply_chain/attestation.rb +++ b/app/models/supply_chain/attestation.rb @@ -11,7 +11,7 @@ class Attestation < ::ApplicationRecord validates :predicate_kind, presence: true validates :predicate_type, presence: true validates :subject_digest, presence: true, length: { minimum: 64, maximum: 255 } - validates :file, presence: true, unless: :error? + #validates :file, presence: true, unless: :error? validates :subject_digest, uniqueness: { scope: [:project_id, :predicate_kind] } diff --git a/app/services/ci/slsa/publish_provenance_service.rb b/app/services/ci/slsa/publish_provenance_service.rb index 2db39d27c7a0ad..14c51eb0e09d85 100644 --- a/app/services/ci/slsa/publish_provenance_service.rb +++ b/app/services/ci/slsa/publish_provenance_service.rb @@ -37,6 +37,10 @@ def predicate end strong_memoize_attr :predicate + def ci_server_url + Gitlab.config.gitlab.url + end + def attest_all_artifacts(build:, id_token:) logger = Gitlab::AppJsonLogger reader = SupplyChain::ArtifactsReader.new(build) @@ -51,14 +55,15 @@ def attest_all_artifacts(build:, id_token:) logger.info(class: self.class.name, message: "Performing attestation for artifact", hash: hash, path: artifact_path, build_id: build.id) - _attestation, duration = attest_blob!(blob_name: blob_name, hash: hash, id_token: id_token) + attestation, duration = attest_blob!(blob_name: blob_name, hash: hash, id_token: id_token) + attestations << attestation logger.info(class: self.class.name, message: "Attestation successful", hash: hash, blob_name: blob_name, duration: duration, build_id: build.id) rescue StandardError => e Gitlab::ErrorTracking.track_exception(e, project_id: build.project.id) - attestation = persist_attestation!(build: build, status: :error, subject_digest: hash) + attestation = persist_attestation!(status: :error, subject_digest: hash) attestations << attestation all_successful = false @@ -70,32 +75,6 @@ def attest_all_artifacts(build:, id_token:) ServiceResponse.error(message: "Attestation failure", payload: { attestations: attestations }) end - def persist_attestation!(build:, status:, subject_digest:) - attestation = SupplyChain::Attestation.new do |att| - att.subject_digest = subject_digest - att.project = build.project - att.build = build - att.status = status - att.expire_at = 2.years.from_now # TODO: adjust based on outcomes of ticket discussion. - att.predicate_kind = :provenance - att.predicate_type = SupplyChain::Slsa::ProvenanceStatement::PREDICATE_TYPE_V1 - end - - attestation.save! - - attestation - end - - def hash(file_input_stream) - sha = Digest::SHA256.new - sha << file_input_stream.read(HASH_READ_CHUNK_SIZE) until file_input_stream.eof? - sha.hexdigest - end - - def ci_server_url - Gitlab.config.gitlab.url - end - def attest_blob!(blob_name:, hash:, id_token:) validate_id_token!(id_token) validate_blob_name!(blob_name) @@ -124,8 +103,7 @@ def attest_blob!(blob_name:, hash:, id_token:) stdin.write(predicate) end - bundle_file.rewind - attestation = bundle_file.read + attestation = persist_attestation!(status: :success, subject_digest: hash, bundle_file: bundle_file) end return attestation, result.duration if result.status.success? @@ -134,6 +112,29 @@ def attest_blob!(blob_name:, hash:, id_token:) raise AttestationFailure, "Attestation for #{hash} failed after #{result.duration}s: #{error}" end + def persist_attestation!(status:, subject_digest:, bundle_file: nil) + attestation = SupplyChain::Attestation.new do |att| + att.subject_digest = subject_digest + att.project = @build.project + att.build = @build + att.status = status + att.expire_at = 2.years.from_now # TODO: adjust based on outcomes of ticket discussion. + att.predicate_kind = :provenance + att.predicate_type = SupplyChain::Slsa::ProvenanceStatement::PREDICATE_TYPE_V1 + att.file = bundle_file + end + + attestation.save! + + attestation + end + + def hash(file_input_stream) + sha = Digest::SHA256.new + sha << file_input_stream.read(HASH_READ_CHUNK_SIZE) until file_input_stream.eof? + sha.hexdigest + end + def validate_id_token!(id_token) # Can be path or literal according to documentation. Gitlab::PathTraversal.check_path_traversal!(id_token) diff --git a/spec/services/ci/slsa/publish_provenance_service_spec.rb b/spec/services/ci/slsa/publish_provenance_service_spec.rb index 48c36326ac328c..2f82502085910f 100644 --- a/spec/services/ci/slsa/publish_provenance_service_spec.rb +++ b/spec/services/ci/slsa/publish_provenance_service_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' require 'digest' -SLSA_ATTESTATION_BUNDLE = 'app/validators/json_schemas/slsa/in_toto_v1/provenance_v1.json' +SLSA_ATTESTATION_BUNDLE = 'spec/fixtures/slsa/attestation.bundle' RSpec.describe Ci::Slsa::PublishProvenanceService, feature_category: :artifact_security do let(:service) { described_class.new(build) } @@ -33,11 +33,16 @@ include_context 'with build, pipeline and artifacts' - before do - file = instance_double(File) + let(:bundle_file) do + fh = Tempfile.new + #allow(fh).to receive_messages(read: signature_bundle, rewind: nil, path: tmp_file_path) + fh.write(signature_bundle) + fh.flush + fh + end - allow(Tempfile).to receive(:create).and_yield(file) - allow(file).to receive_messages(read: signature_bundle, path: tmp_file_path, rewind: nil) + before do + allow(Tempfile).to receive(:create).and_yield(bundle_file) allow(Gitlab::Popen).to receive(:popen_with_detail).with(any_args).and_yield(popen_stdin_file) .and_return(popen_result) end @@ -55,6 +60,13 @@ } end + it 'persists the attestations' do + p attestations + p attestations[0].file.read + p "wat" + + end + it 'logs the right hash and attestation' do allow(Gitlab::AppJsonLogger).to receive(:info) @@ -224,6 +236,8 @@ end end + # TODO: test correct handling of duplicate attestation for predicate. + # TODO: test mixture of successful and unsuccessful attestations. # TODO: test duplicate attestation for predicate. # TODO: add test that ensures we log the exception and don't swallow it. end -- GitLab From 40b7f502cb6872caa39fe394d8d989b4f3eab7e1 Mon Sep 17 00:00:00 2001 From: Sam Joan Roque-Worcel Date: Thu, 18 Sep 2025 12:00:44 +1200 Subject: [PATCH 11/30] Improve rspec tests for successful attestations --- .../ci/slsa/publish_provenance_service.rb | 2 + .../slsa/publish_provenance_service_spec.rb | 49 ++++++++++++++----- 2 files changed, 39 insertions(+), 12 deletions(-) diff --git a/app/services/ci/slsa/publish_provenance_service.rb b/app/services/ci/slsa/publish_provenance_service.rb index 14c51eb0e09d85..359ebebb6a0494 100644 --- a/app/services/ci/slsa/publish_provenance_service.rb +++ b/app/services/ci/slsa/publish_provenance_service.rb @@ -103,6 +103,8 @@ def attest_blob!(blob_name:, hash:, id_token:) stdin.write(predicate) end + pry + attestation = persist_attestation!(status: :success, subject_digest: hash, bundle_file: bundle_file) end diff --git a/spec/services/ci/slsa/publish_provenance_service_spec.rb b/spec/services/ci/slsa/publish_provenance_service_spec.rb index 2f82502085910f..a88e1ea1c12fa0 100644 --- a/spec/services/ci/slsa/publish_provenance_service_spec.rb +++ b/spec/services/ci/slsa/publish_provenance_service_spec.rb @@ -16,6 +16,7 @@ let_it_be(:expected_stdout) { "expected stderr outuput string" } let_it_be(:tmp_file_path) { "/tmp/folder/file.bundle" } let_it_be(:expected_duration) { 1.33337 } + let_it_be(:expected_predicate_type) { SupplyChain::Slsa::ProvenanceStatement::PREDICATE_TYPE_V1 } let(:expected_predicate) { service.send(:predicate) } let(:popen_success) { true } let(:process_status) do @@ -31,22 +32,32 @@ fh end - include_context 'with build, pipeline and artifacts' + # object_storage.rb reads the file from disk rather than use our file handle. + # This file can't be passed directly either + let(:real_tmp_file) do + file = Tempfile.new + file.write(signature_bundle) + file.flush - let(:bundle_file) do - fh = Tempfile.new - #allow(fh).to receive_messages(read: signature_bundle, rewind: nil, path: tmp_file_path) - fh.write(signature_bundle) - fh.flush - fh + file end + + include_context 'with build, pipeline and artifacts' + before do + bundle_file = instance_double(File) + allow(bundle_file).to receive_messages(path: tmp_file_path) + allow(Tempfile).to receive(:create).and_yield(bundle_file) allow(Gitlab::Popen).to receive(:popen_with_detail).with(any_args).and_yield(popen_stdin_file) .and_return(popen_result) end + after do + real_tmp_file.close + end + describe '#execute' do subject(:result) { service.execute } @@ -61,10 +72,26 @@ end it 'persists the attestations' do - p attestations - p attestations[0].file.read - p "wat" + expect(result[:status]).to eq(:success) + expect(result[:message]).to eq("OK") + + expect(attestations.length).to eq(3) + + expect(attestations).to all(be_a(SupplyChain::Attestation)) + expect(attestations).to all(be_persisted) + expect(attestations).to all(be_success) + expect(attestations).to all(be_provenance) + attestations.each do |att| + expect(att.project_id).to eq(project.id) + expect(att.build_id).to eq(build.id) + expect(att.predicate_type).to eq(expected_predicate_type) + expect(att.file.read).to eq(signature_bundle) + end + + expected_hashes.each_value do |hash| + expect(attestations).to include(an_object_having_attributes(subject_digest: hash)) + end end it 'logs the right hash and attestation' do @@ -204,8 +231,6 @@ end context "when attestation fails" do - let(:expected_predicate_type) { SupplyChain::Slsa::ProvenanceStatement::PREDICATE_TYPE_V1 } - before do allow(service).to receive(:validate_blob_name!).with(any_args).and_raise(StandardError) end -- GitLab From f499d14ae631bca97663bfb15bbde54f8c1343c5 Mon Sep 17 00:00:00 2001 From: Sam Joan Roque-Worcel Date: Fri, 19 Sep 2025 07:09:43 +1200 Subject: [PATCH 12/30] Modify error handling, fix tests --- app/models/supply_chain/attestation.rb | 2 +- .../ci/slsa/publish_provenance_service.rb | 4 +- .../slsa/publish_provenance_service_spec.rb | 46 ++++++++++--------- 3 files changed, 26 insertions(+), 26 deletions(-) diff --git a/app/models/supply_chain/attestation.rb b/app/models/supply_chain/attestation.rb index ac6fbecfdb726d..2a3ecd99141735 100644 --- a/app/models/supply_chain/attestation.rb +++ b/app/models/supply_chain/attestation.rb @@ -11,7 +11,7 @@ class Attestation < ::ApplicationRecord validates :predicate_kind, presence: true validates :predicate_type, presence: true validates :subject_digest, presence: true, length: { minimum: 64, maximum: 255 } - #validates :file, presence: true, unless: :error? + validates :file, presence: true, unless: :error? validates :subject_digest, uniqueness: { scope: [:project_id, :predicate_kind] } diff --git a/app/services/ci/slsa/publish_provenance_service.rb b/app/services/ci/slsa/publish_provenance_service.rb index 359ebebb6a0494..f150ee07186679 100644 --- a/app/services/ci/slsa/publish_provenance_service.rb +++ b/app/services/ci/slsa/publish_provenance_service.rb @@ -61,7 +61,7 @@ def attest_all_artifacts(build:, id_token:) logger.info(class: self.class.name, message: "Attestation successful", hash: hash, blob_name: blob_name, duration: duration, build_id: build.id) rescue StandardError => e - Gitlab::ErrorTracking.track_exception(e, project_id: build.project.id) + Gitlab::ErrorTracking.track_and_raise_for_dev_exception(e, project_id: build.project.id) attestation = persist_attestation!(status: :error, subject_digest: hash) attestations << attestation @@ -103,8 +103,6 @@ def attest_blob!(blob_name:, hash:, id_token:) stdin.write(predicate) end - pry - attestation = persist_attestation!(status: :success, subject_digest: hash, bundle_file: bundle_file) end diff --git a/spec/services/ci/slsa/publish_provenance_service_spec.rb b/spec/services/ci/slsa/publish_provenance_service_spec.rb index a88e1ea1c12fa0..342e2bf088870e 100644 --- a/spec/services/ci/slsa/publish_provenance_service_spec.rb +++ b/spec/services/ci/slsa/publish_provenance_service_spec.rb @@ -14,7 +14,6 @@ let_it_be(:signature_bundle) { File.read(SLSA_ATTESTATION_BUNDLE) } let_it_be(:expected_stderr) { "expecte stderr outuput string" } let_it_be(:expected_stdout) { "expected stderr outuput string" } - let_it_be(:tmp_file_path) { "/tmp/folder/file.bundle" } let_it_be(:expected_duration) { 1.33337 } let_it_be(:expected_predicate_type) { SupplyChain::Slsa::ProvenanceStatement::PREDICATE_TYPE_V1 } let(:expected_predicate) { service.send(:predicate) } @@ -32,30 +31,36 @@ fh end - # object_storage.rb reads the file from disk rather than use our file handle. - # This file can't be passed directly either - let(:real_tmp_file) do - file = Tempfile.new - file.write(signature_bundle) - file.flush + let(:real_tmp_files) do + files = [] + 3.times do + file = Tempfile.new + file.write(signature_bundle) + file.flush - file - end + files << file + end + files + end include_context 'with build, pipeline and artifacts' before do - bundle_file = instance_double(File) - allow(bundle_file).to receive_messages(path: tmp_file_path) + # object_storage.rb moves the file on disk rather than use our file handle. + # Because of this, we need to provide one Tempfile per artifact. + nb = 0 + allow(Tempfile).to receive(:create) do |&block| + block.call(real_tmp_files[nb]) + nb += 1 + end - allow(Tempfile).to receive(:create).and_yield(bundle_file) allow(Gitlab::Popen).to receive(:popen_with_detail).with(any_args).and_yield(popen_stdin_file) .and_return(popen_result) end after do - real_tmp_file.close + real_tmp_files.each(&:close) end describe '#execute' do @@ -94,7 +99,7 @@ end end - it 'logs the right hash and attestation' do + it 'logs the right values' do allow(Gitlab::AppJsonLogger).to receive(:info) predicate_class = SupplyChain::Slsa::ProvenanceStatement::Predicate @@ -135,7 +140,7 @@ expected_hashes.each do |path, hash| expected_parameters = ["cosign", "attest-blob", "--new-bundle-format", "--predicate", "-", "--type", "slsaprovenance1", "--hash", hash, "--identity-token", id_token, "--oidc-issuer", - "http://localhost", "--yes", "--bundle", tmp_file_path, "--", "./#{File.basename(path)}"] + "http://localhost", "--yes", "--bundle", anything, "--", "./#{File.basename(path)}"] expect(Gitlab::Popen).to receive(:popen_with_detail).with(expected_parameters).and_return(popen_result) end @@ -157,7 +162,7 @@ expected_hashes.each do |path, hash| expected_parameters = ["cosign", "attest-blob", "--new-bundle-format", "--predicate", "-", "--type", "slsaprovenance1", "--hash", hash, "--identity-token", id_token, "--oidc-issuer", - "http://localhost", "--yes", "--bundle", tmp_file_path, '--fulcio-url', fulcio_url, + "http://localhost", "--yes", "--bundle", anything, '--fulcio-url', fulcio_url, '--rekor-url', rekor_url, "--", "./#{File.basename(path)}"] expect(Gitlab::Popen).to receive(:popen_with_detail).with(expected_parameters).and_return(popen_result) @@ -174,7 +179,7 @@ expected_hashes.each do |path, hash| expected_parameters = ["cosign", "attest-blob", "--new-bundle-format", "--predicate", "-", "--type", "slsaprovenance1", "--hash", hash, "--identity-token", id_token, "--oidc-issuer", - "http://localhost", "--yes", "--bundle", tmp_file_path, "--", "./#{File.basename(path)}"] + "http://localhost", "--yes", "--bundle", anything, "--", "./#{File.basename(path)}"] expect(Gitlab::Popen).to receive(:popen_with_detail).with(expected_parameters).and_return(popen_result) end @@ -231,12 +236,9 @@ end context "when attestation fails" do - before do - allow(service).to receive(:validate_blob_name!).with(any_args).and_raise(StandardError) - end - it 'persists a :failed attestation' do - expect(Gitlab::ErrorTracking).to receive(:track_exception).exactly(3).times + expect(service).to receive(:validate_blob_name!).with(any_args).exactly(3).times.and_raise(StandardError) + expect(Gitlab::ErrorTracking).to receive(:track_and_raise_for_dev_exception).exactly(3).times expect(result[:status]).to eq(:error) expect(result[:message]).to eq("Attestation failure") -- GitLab From 7162946cc4807f8ed6d2328d93f540ba980d0846 Mon Sep 17 00:00:00 2001 From: Sam Joan Roque-Worcel Date: Fri, 19 Sep 2025 11:09:40 +1200 Subject: [PATCH 13/30] Address edge cases around :subject_digest In this commit, I've implemented the basic logic for dealing with duplicate subject_digest values. May be changed later. I've also made logging less verbose. --- .../ci/slsa/publish_provenance_service.rb | 29 ++++++++++++++--- .../slsa/publish_provenance_service_spec.rb | 32 ++++++++++++++++--- 2 files changed, 53 insertions(+), 8 deletions(-) diff --git a/app/services/ci/slsa/publish_provenance_service.rb b/app/services/ci/slsa/publish_provenance_service.rb index f150ee07186679..503112ef11e24c 100644 --- a/app/services/ci/slsa/publish_provenance_service.rb +++ b/app/services/ci/slsa/publish_provenance_service.rb @@ -41,6 +41,10 @@ def ci_server_url Gitlab.config.gitlab.url end + def attestation_by_hash(hash) + SupplyChain::Attestation.find_by(project: @build.project, predicate_kind: :provenance, subject_digest: hash) # TODO: migrate to model + end + def attest_all_artifacts(build:, id_token:) logger = Gitlab::AppJsonLogger reader = SupplyChain::ArtifactsReader.new(build) @@ -50,16 +54,33 @@ def attest_all_artifacts(build:, id_token:) reader.files do |artifact_path, file_input_stream| blob_name = File.basename(artifact_path) hash = hash(file_input_stream) + log_args = { + class: self.class.name, + hash: hash, + path: artifact_path, + build_id: build.id, + blob_name: blob_name + } + + previous_attestation = attestation_by_hash(hash) + if previous_attestation + if previous_attestation.success? + logger.info(**log_args, message: "Skipping attestation, attestation already exists") + next + else + logger.info(**log_args, message: "Reattempting previously failed attestation") + previous_attestation.destroy + end + end begin - logger.info(class: self.class.name, message: "Performing attestation for artifact", hash: hash, - path: artifact_path, build_id: build.id) + logger.info(**log_args, message: "Performing attestation for artifact") attestation, duration = attest_blob!(blob_name: blob_name, hash: hash, id_token: id_token) attestations << attestation - logger.info(class: self.class.name, message: "Attestation successful", hash: hash, blob_name: blob_name, - duration: duration, build_id: build.id) + logger.info(**log_args, message: "Attestation successful", duration: duration) + rescue StandardError => e Gitlab::ErrorTracking.track_and_raise_for_dev_exception(e, project_id: build.project.id) diff --git a/spec/services/ci/slsa/publish_provenance_service_spec.rb b/spec/services/ci/slsa/publish_provenance_service_spec.rb index 342e2bf088870e..989ae073694f72 100644 --- a/spec/services/ci/slsa/publish_provenance_service_spec.rb +++ b/spec/services/ci/slsa/publish_provenance_service_spec.rb @@ -263,10 +263,34 @@ end end - # TODO: test correct handling of duplicate attestation for predicate. - # TODO: test mixture of successful and unsuccessful attestations. - # TODO: test duplicate attestation for predicate. - # TODO: add test that ensures we log the exception and don't swallow it. + context "when duplicate attestations are attempted" do + before do + dup_hash = "5db1fee4b5703808c48078a76768b155b421b210c0761cd6a5d223f4d99f1eaa" + allow(service).to receive(:hash).exactly(3).times.and_return(dup_hash) + end + + it "skips attestation" do + expect(attestations.length).to be(1) + end + end + + context "when a previous :error attestation exists" do + it "deletes it" do + expect(attestations.length).to be(1) + end + end + + context "when a mixture of successful and unsuccessful attestations happen" do + it "persists a mixture of :error and :success attestations" do + expect(result).to be(false) + end + end + + context "when validation errors happen" do + it "creates a failed attestation" do + expect(service).to be(1) + end + end end describe '#attest_blob!' do -- GitLab From 699c2f04acc0195b74d6f9630949abe761a1641a Mon Sep 17 00:00:00 2001 From: Sam Joan Roque-Worcel Date: Fri, 19 Sep 2025 11:17:36 +1200 Subject: [PATCH 14/30] Add test for validation errors specifically --- .../slsa/publish_provenance_service_spec.rb | 26 +++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/spec/services/ci/slsa/publish_provenance_service_spec.rb b/spec/services/ci/slsa/publish_provenance_service_spec.rb index 989ae073694f72..342f7c9e2c1838 100644 --- a/spec/services/ci/slsa/publish_provenance_service_spec.rb +++ b/spec/services/ci/slsa/publish_provenance_service_spec.rb @@ -287,8 +287,30 @@ end context "when validation errors happen" do - it "creates a failed attestation" do - expect(service).to be(1) + it 'persists a :failed attestation' do + expect(service).to receive(:validate_blob_name!).with(any_args).exactly(3).times.and_raise(ActiveRecord::RecordInvalid) + expect(Gitlab::ErrorTracking).to receive(:track_and_raise_for_dev_exception).exactly(3).times + + expect(result[:status]).to eq(:error) + expect(result[:message]).to eq("Attestation failure") + + expect(attestations.length).to eq(3) + + expect(attestations).to all(be_a(SupplyChain::Attestation)) + expect(attestations).to all(be_persisted) + expect(attestations).to all(be_error) + expect(attestations).to all(be_provenance) + + attestations.each do |att| + expect(att.project_id).to eq(project.id) + expect(att.build_id).to eq(build.id) + expect(att.predicate_type).to eq(expected_predicate_type) + expect(att.file.read).to be_nil + end + + expected_hashes.each_value do |hash| + expect(attestations).to include(an_object_having_attributes(subject_digest: hash)) + end end end end -- GitLab From 619995dd0e9d6317f68929207bf60e0f4200564b Mon Sep 17 00:00:00 2001 From: Sam Joan Roque-Worcel Date: Fri, 19 Sep 2025 14:45:07 +1200 Subject: [PATCH 15/30] Add test for duplicate attestation, previous :error --- .../slsa/publish_provenance_service_spec.rb | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/spec/services/ci/slsa/publish_provenance_service_spec.rb b/spec/services/ci/slsa/publish_provenance_service_spec.rb index 342f7c9e2c1838..9c752b02ac0202 100644 --- a/spec/services/ci/slsa/publish_provenance_service_spec.rb +++ b/spec/services/ci/slsa/publish_provenance_service_spec.rb @@ -267,16 +267,39 @@ before do dup_hash = "5db1fee4b5703808c48078a76768b155b421b210c0761cd6a5d223f4d99f1eaa" allow(service).to receive(:hash).exactly(3).times.and_return(dup_hash) + allow(Gitlab::AppJsonLogger).to receive(:info) end it "skips attestation" do expect(attestations.length).to be(1) + + expect(Gitlab::AppJsonLogger).to have_received(:info).exactly(2).times.with(a_hash_including({ + message: "Skipping attestation, attestation already exists" + })) end end context "when a previous :error attestation exists" do + let(:duplicate_hash) { "5db1fee4b5703808c48078a76768b155b421b210c0761cd6a5d223f2d99f1eaa" } + let(:existing_attestation) { create(:supply_chain_attestation, subject_digest: duplicate_hash, status: :error) } + + before do + allow_next_instance_of(SupplyChain::ArtifactsReader) do |instance| + allow(instance).to receive(:files).and_yield("path", nil) + end + allow(service).to receive(:hash).exactly(1).time.and_return(duplicate_hash) + allow(Gitlab::AppJsonLogger).to receive(:info) + end + it "deletes it" do + previous = existing_attestation expect(attestations.length).to be(1) + + expect { previous.reload }.to raise_exception(ActiveRecord::RecordNotFound) + + expect(Gitlab::AppJsonLogger).to have_received(:info).with(a_hash_including({ + message: "Reattempting previously failed attestation" + })) end end -- GitLab From 682908010e987a073d9c0db8b16c75ee23bbc08e Mon Sep 17 00:00:00 2001 From: Sam Joan Roque-Worcel Date: Mon, 22 Sep 2025 07:08:23 +1200 Subject: [PATCH 16/30] Make all tests pass --- .../slsa/publish_provenance_service_spec.rb | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/spec/services/ci/slsa/publish_provenance_service_spec.rb b/spec/services/ci/slsa/publish_provenance_service_spec.rb index 9c752b02ac0202..6cd371fd9d739c 100644 --- a/spec/services/ci/slsa/publish_provenance_service_spec.rb +++ b/spec/services/ci/slsa/publish_provenance_service_spec.rb @@ -292,11 +292,10 @@ end it "deletes it" do - previous = existing_attestation + expect(existing_attestation).to receive(:destroy).and_call_original + allow(service).to receive(:attestation_by_hash).with(duplicate_hash).and_return(existing_attestation) expect(attestations.length).to be(1) - expect { previous.reload }.to raise_exception(ActiveRecord::RecordNotFound) - expect(Gitlab::AppJsonLogger).to have_received(:info).with(a_hash_including({ message: "Reattempting previously failed attestation" })) @@ -305,13 +304,24 @@ context "when a mixture of successful and unsuccessful attestations happen" do it "persists a mixture of :error and :success attestations" do - expect(result).to be(false) + nb = 0 + expect(service).to receive(:validate_blob_name!).with(any_args).exactly(3).times do + nb += 1 + raise StandardError if nb == 2 + end + + expect(Gitlab::ErrorTracking).to receive(:track_and_raise_for_dev_exception).once + + expect(attestations.count(&:success?)).to be(2) + expect(attestations.count(&:error?)).to be(1) end end context "when validation errors happen" do it 'persists a :failed attestation' do - expect(service).to receive(:validate_blob_name!).with(any_args).exactly(3).times.and_raise(ActiveRecord::RecordInvalid) + expect(service).to receive(:validate_blob_name!).with(any_args).exactly(3).times \ + .and_raise(ActiveRecord::RecordInvalid) + expect(Gitlab::ErrorTracking).to receive(:track_and_raise_for_dev_exception).exactly(3).times expect(result[:status]).to eq(:error) -- GitLab From 9aaf0291da60c7c995257657530ea73d9fe81297 Mon Sep 17 00:00:00 2001 From: Sam Joan Roque-Worcel Date: Mon, 22 Sep 2025 07:10:11 +1200 Subject: [PATCH 17/30] Revert "Merge remote-tracking branch 'origin/create_attestation_file_uploader' into 567885-modify-publishprovenanceservice-to-persist-attestation" This reverts commit de3d47b278de96bdb657761e6a6f86b08270c363, reversing changes made to 81d6e2e041cb26192103d2b78a3d03361f90750b. --- app/models/supply_chain/attestation.rb | 38 ------------ db/docs/slsa_attestation_uploads.yml | 14 ----- ...905134037_add_file_to_slsa_attestations.rb | 17 ----- ...ate_slsa_attestations_uploads_partition.rb | 18 ------ db/schema_migrations/20250905134037 | 1 - db/schema_migrations/20250905190541 | 1 - db/structure.sql | 62 ------------------- spec/db/schema_spec.rb | 1 - spec/factories/supply_chain/attestations.rb | 1 - spec/fixtures/supply_chain/attestation.json | 33 ---------- spec/lib/gitlab/database/sharding_key_spec.rb | 2 - spec/models/supply_chain/attestation_spec.rb | 13 +--- 12 files changed, 2 insertions(+), 199 deletions(-) delete mode 100644 app/models/supply_chain/attestation.rb delete mode 100644 db/docs/slsa_attestation_uploads.yml delete mode 100644 db/migrate/20250905134037_add_file_to_slsa_attestations.rb delete mode 100644 db/migrate/20250905190541_create_slsa_attestations_uploads_partition.rb delete mode 100644 db/schema_migrations/20250905134037 delete mode 100644 db/schema_migrations/20250905190541 delete mode 100644 spec/fixtures/supply_chain/attestation.json diff --git a/app/models/supply_chain/attestation.rb b/app/models/supply_chain/attestation.rb deleted file mode 100644 index 2a3ecd99141735..00000000000000 --- a/app/models/supply_chain/attestation.rb +++ /dev/null @@ -1,38 +0,0 @@ -# frozen_string_literal: true - -module SupplyChain - class Attestation < ::ApplicationRecord - self.table_name = 'slsa_attestations' - - belongs_to :project - belongs_to :build, class_name: 'Ci::Build', optional: true - - validates :project_id, presence: true - validates :predicate_kind, presence: true - validates :predicate_type, presence: true - validates :subject_digest, presence: true, length: { minimum: 64, maximum: 255 } - validates :file, presence: true, unless: :error? - - validates :subject_digest, uniqueness: { scope: [:project_id, :predicate_kind] } - - mount_uploader :file, AttachmentUploader - - enum :status, { - success: 0, - error: 1 - } - - enum :predicate_kind, { - provenance: 0, - sbom: 1 - } - - def uploads_sharding_key - { project_id: project_id } - end - - def retrieve_upload(_identifier, paths) - Upload.find_by(model: self, path: paths) - end - end -end diff --git a/db/docs/slsa_attestation_uploads.yml b/db/docs/slsa_attestation_uploads.yml deleted file mode 100644 index 0b397a52f5bb3f..00000000000000 --- a/db/docs/slsa_attestation_uploads.yml +++ /dev/null @@ -1,14 +0,0 @@ ---- -table_name: slsa_attestation_uploads -classes: -- Upload -- SupplyChain::Attesatation -feature_categories: -- artifact_security -description: Stores uploads for SupplyChain::Attestation model -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/204116 -milestone: '18.4' -table_size: small -gitlab_schema: gitlab_main_org -sharding_key: - project_id: projects diff --git a/db/migrate/20250905134037_add_file_to_slsa_attestations.rb b/db/migrate/20250905134037_add_file_to_slsa_attestations.rb deleted file mode 100644 index 4a223d5496af32..00000000000000 --- a/db/migrate/20250905134037_add_file_to_slsa_attestations.rb +++ /dev/null @@ -1,17 +0,0 @@ -# frozen_string_literal: true - -class AddFileToSlsaAttestations < Gitlab::Database::Migration[2.3] - milestone '18.4' - - disable_ddl_transaction! - - def up - add_column :slsa_attestations, :file, :text, null: false, default: '' - add_text_limit :slsa_attestations, :file, 1024 - end - - def down - remove_text_limit :slsa_attestations, :file - remove_column :slsa_attestations, :file, if_exists: true - end -end diff --git a/db/migrate/20250905190541_create_slsa_attestations_uploads_partition.rb b/db/migrate/20250905190541_create_slsa_attestations_uploads_partition.rb deleted file mode 100644 index 10befe44c80829..00000000000000 --- a/db/migrate/20250905190541_create_slsa_attestations_uploads_partition.rb +++ /dev/null @@ -1,18 +0,0 @@ -# frozen_string_literal: true - -class CreateSlsaAttestationsUploadsPartition < Gitlab::Database::Migration[2.3] - include Gitlab::Database::PartitioningMigrationHelpers::TableManagementHelpers - - milestone '18.4' - - def up - create_list_partitions( - 'uploads_9ba88c4165', - { slsa_attestation: "'SupplyChain::Attestation'" }, - '%{partition_name}_uploads') - end - - def down - drop_table :slsa_attestation_uploads - end -end diff --git a/db/schema_migrations/20250905134037 b/db/schema_migrations/20250905134037 deleted file mode 100644 index 14b0d996866c4a..00000000000000 --- a/db/schema_migrations/20250905134037 +++ /dev/null @@ -1 +0,0 @@ -dc727e6fd032b112466d37d87552efe773cddd6c20671c03b3372aac82b7ff5a \ No newline at end of file diff --git a/db/schema_migrations/20250905190541 b/db/schema_migrations/20250905190541 deleted file mode 100644 index 522f3e04d53d98..00000000000000 --- a/db/schema_migrations/20250905190541 +++ /dev/null @@ -1 +0,0 @@ -b87eda3aa27e0a04fb4656b7ef264c3733d3255e5301d64ea626e7dfe7bbdbca \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 2e3c31275c87ff..ccb6fb98cbb24f 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -24404,27 +24404,6 @@ CREATE SEQUENCE slack_integrations_scopes_id_seq ALTER SEQUENCE slack_integrations_scopes_id_seq OWNED BY slack_integrations_scopes.id; -CREATE TABLE slsa_attestation_uploads ( - id bigint NOT NULL, - size bigint NOT NULL, - model_id bigint NOT NULL, - uploaded_by_user_id bigint, - organization_id bigint, - namespace_id bigint, - project_id bigint, - created_at timestamp without time zone, - store integer DEFAULT 1 NOT NULL, - version integer DEFAULT 1, - path text NOT NULL, - checksum text, - model_type text NOT NULL, - uploader text NOT NULL, - mount_point text, - secret text, - CONSTRAINT check_2849dedce7 CHECK ((char_length(path) <= 511)), - CONSTRAINT check_b888b1df14 CHECK ((char_length(checksum) <= 64)) -); - CREATE TABLE slsa_attestations ( id bigint NOT NULL, created_at timestamp with time zone NOT NULL, @@ -24436,8 +24415,6 @@ CREATE TABLE slsa_attestations ( predicate_kind smallint DEFAULT 0 NOT NULL, predicate_type text NOT NULL, subject_digest text NOT NULL, - file text DEFAULT ''::text NOT NULL, - CONSTRAINT check_3575e9121e CHECK ((char_length(file) <= 1024)), CONSTRAINT check_dec11b603a CHECK ((char_length(subject_digest) <= 255)), CONSTRAINT check_ea0d61030d CHECK ((char_length(predicate_type) <= 255)) ); @@ -28349,8 +28326,6 @@ ALTER TABLE ONLY ci_runners ATTACH PARTITION project_type_ci_runners FOR VALUES ALTER TABLE ONLY uploads_9ba88c4165 ATTACH PARTITION project_uploads FOR VALUES IN ('Project'); -ALTER TABLE ONLY uploads_9ba88c4165 ATTACH PARTITION slsa_attestation_uploads FOR VALUES IN ('SupplyChain::Attestation'); - ALTER TABLE ONLY uploads_9ba88c4165 ATTACH PARTITION snippet_uploads FOR VALUES IN ('Snippet'); ALTER TABLE ONLY uploads_9ba88c4165 ATTACH PARTITION user_permission_export_upload_uploads FOR VALUES IN ('UserPermissionExportUpload'); @@ -32846,9 +32821,6 @@ ALTER TABLE ONLY slack_integrations ALTER TABLE ONLY slack_integrations_scopes ADD CONSTRAINT slack_integrations_scopes_pkey PRIMARY KEY (id); -ALTER TABLE ONLY slsa_attestation_uploads - ADD CONSTRAINT slsa_attestation_uploads_pkey PRIMARY KEY (id, model_type); - ALTER TABLE ONLY slsa_attestations ADD CONSTRAINT slsa_attestations_pkey PRIMARY KEY (id); @@ -40753,22 +40725,6 @@ CREATE UNIQUE INDEX security_findings_uuid_scan_id_partition_number_idx ON ONLY CREATE INDEX security_policy_approval_mr_rule_index_merge_request_id ON approval_merge_request_rules USING btree (merge_request_id) WHERE (report_type = ANY (ARRAY[4, 2, 5])); -CREATE INDEX slsa_attestation_uploads_checksum_idx ON slsa_attestation_uploads USING btree (checksum); - -CREATE INDEX slsa_attestation_uploads_model_id_model_type_uploader_creat_idx ON slsa_attestation_uploads USING btree (model_id, model_type, uploader, created_at); - -CREATE INDEX slsa_attestation_uploads_namespace_id_idx ON slsa_attestation_uploads USING btree (namespace_id); - -CREATE INDEX slsa_attestation_uploads_organization_id_idx ON slsa_attestation_uploads USING btree (organization_id); - -CREATE INDEX slsa_attestation_uploads_project_id_idx ON slsa_attestation_uploads USING btree (project_id); - -CREATE INDEX slsa_attestation_uploads_store_idx ON slsa_attestation_uploads USING btree (store); - -CREATE INDEX slsa_attestation_uploads_uploaded_by_user_id_idx ON slsa_attestation_uploads USING btree (uploaded_by_user_id); - -CREATE INDEX slsa_attestation_uploads_uploader_path_idx ON slsa_attestation_uploads USING btree (uploader, path); - CREATE INDEX snippet_uploads_checksum_idx ON snippet_uploads USING btree (checksum); CREATE INDEX snippet_uploads_model_id_model_type_uploader_created_at_idx ON snippet_uploads USING btree (model_id, model_type, uploader, created_at); @@ -43213,24 +43169,6 @@ ALTER INDEX index_uploads_9ba88c4165_on_uploaded_by_user_id ATTACH PARTITION pro ALTER INDEX index_uploads_9ba88c4165_on_uploader_and_path ATTACH PARTITION project_uploads_uploader_path_idx; -ALTER INDEX index_uploads_9ba88c4165_on_checksum ATTACH PARTITION slsa_attestation_uploads_checksum_idx; - -ALTER INDEX index_uploads_9ba88c4165_on_model_uploader_created_at ATTACH PARTITION slsa_attestation_uploads_model_id_model_type_uploader_creat_idx; - -ALTER INDEX index_uploads_9ba88c4165_on_namespace_id ATTACH PARTITION slsa_attestation_uploads_namespace_id_idx; - -ALTER INDEX index_uploads_9ba88c4165_on_organization_id ATTACH PARTITION slsa_attestation_uploads_organization_id_idx; - -ALTER INDEX uploads_9ba88c4165_pkey ATTACH PARTITION slsa_attestation_uploads_pkey; - -ALTER INDEX index_uploads_9ba88c4165_on_project_id ATTACH PARTITION slsa_attestation_uploads_project_id_idx; - -ALTER INDEX index_uploads_9ba88c4165_on_store ATTACH PARTITION slsa_attestation_uploads_store_idx; - -ALTER INDEX index_uploads_9ba88c4165_on_uploaded_by_user_id ATTACH PARTITION slsa_attestation_uploads_uploaded_by_user_id_idx; - -ALTER INDEX index_uploads_9ba88c4165_on_uploader_and_path ATTACH PARTITION slsa_attestation_uploads_uploader_path_idx; - ALTER INDEX index_uploads_9ba88c4165_on_checksum ATTACH PARTITION snippet_uploads_checksum_idx; ALTER INDEX index_uploads_9ba88c4165_on_model_uploader_created_at ATTACH PARTITION snippet_uploads_model_id_model_type_uploader_created_at_idx; diff --git a/spec/db/schema_spec.rb b/spec/db/schema_spec.rb index 5b33b94c1b8dc3..17ed6099b49b54 100644 --- a/spec/db/schema_spec.rb +++ b/spec/db/schema_spec.rb @@ -217,7 +217,6 @@ project_import_export_relation_export_upload_uploads: %w[model_id], project_topic_uploads: %w[model_id], project_uploads: %w[model_id], - slsa_attestation_uploads: %w[model_id], snippet_uploads: %w[model_id], user_permission_export_upload_uploads: %w[model_id], user_uploads: %w[model_id], diff --git a/spec/factories/supply_chain/attestations.rb b/spec/factories/supply_chain/attestations.rb index 185ad39ecb8094..13e3e1bf47fdcd 100644 --- a/spec/factories/supply_chain/attestations.rb +++ b/spec/factories/supply_chain/attestations.rb @@ -7,6 +7,5 @@ predicate_kind { :provenance } predicate_type { "https://slsa.dev/provenance/v1" } subject_digest { Digest::SHA256.hexdigest("abc") } - file { fixture_file_upload('spec/fixtures/supply_chain/attestation.json') } end end diff --git a/spec/fixtures/supply_chain/attestation.json b/spec/fixtures/supply_chain/attestation.json deleted file mode 100644 index 7143e8f5f9b828..00000000000000 --- a/spec/fixtures/supply_chain/attestation.json +++ /dev/null @@ -1,33 +0,0 @@ -{ - "mediaType": "application/vnd.dev.sigstore.bundle.v0.3+json", - "verificationMaterial": { - "certificate": { - "rawBytes": "[example]" - }, - "tlogEntries": [ - { - "logIndex": "12345", - "logId": { - "keyId": "[example]" - }, - "kindVersion": { - "kind": "dsse", - "version": "0.0.1" - }, - "inclusionProof": { - "logIndex": "12345" - }, - "canonicalizedBody": "[example]" - } - ] - }, - "dsseEnvelope": { - "payload": "[example]", - "payloadType": "application/vnd.in-toto+json", - "signatures": [ - { - "sig": "[signature]" - } - ] - } -} diff --git a/spec/lib/gitlab/database/sharding_key_spec.rb b/spec/lib/gitlab/database/sharding_key_spec.rb index deb190224809bf..bb6e7771ce5927 100644 --- a/spec/lib/gitlab/database/sharding_key_spec.rb +++ b/spec/lib/gitlab/database/sharding_key_spec.rb @@ -48,7 +48,6 @@ project_import_export_relation_export_upload_uploads.project_id project_topic_uploads.organization_id project_uploads.project_id - slsa_attestation_uploads.project_id snippet_uploads.organization_id vulnerability_export_part_uploads.organization_id vulnerability_export_uploads.organization_id @@ -276,7 +275,6 @@ "project_import_export_relation_export_upload_uploads" => "https://gitlab.com/gitlab-org/gitlab/-/issues/398199", "project_topic_uploads" => "https://gitlab.com/gitlab-org/gitlab/-/issues/398199", "project_uploads" => "https://gitlab.com/gitlab-org/gitlab/-/issues/398199", - "slsa_attestation_uploads" => "https://gitlab.com/gitlab-org/gitlab/-/issues/398199", "snippet_uploads" => "https://gitlab.com/gitlab-org/gitlab/-/issues/398199", "uploads_9ba88c4165" => "https://gitlab.com/gitlab-org/gitlab/-/issues/398199", "user_permission_export_upload_uploads" => "https://gitlab.com/gitlab-org/gitlab/-/issues/398199", diff --git a/spec/models/supply_chain/attestation_spec.rb b/spec/models/supply_chain/attestation_spec.rb index 0439a5e4defe95..9e457826f92c97 100644 --- a/spec/models/supply_chain/attestation_spec.rb +++ b/spec/models/supply_chain/attestation_spec.rb @@ -3,9 +3,9 @@ require 'spec_helper' RSpec.describe SupplyChain::Attestation, feature_category: :artifact_security do - subject { create(:supply_chain_attestation) } - describe "validations" do + subject { create(:supply_chain_attestation) } + it { is_expected.to belong_to(:project) } it { is_expected.to belong_to(:build) } @@ -13,16 +13,7 @@ it { is_expected.to validate_presence_of(:predicate_kind) } it { is_expected.to validate_presence_of(:predicate_type) } it { is_expected.to validate_presence_of(:subject_digest) } - it { is_expected.to validate_presence_of(:file) } it { is_expected.to validate_uniqueness_of(:subject_digest).scoped_to([:project_id, :predicate_kind]) } end - - describe "uploads" do - it_behaves_like "model with uploads", false do - let(:model_object) { create(:supply_chain_attestation) } - let(:upload_attribute) { :file } - let(:uploader_class) { AttachmentUploader } - end - end end -- GitLab From 9076f93d9e2e719e162046ec497fdcc0105824c8 Mon Sep 17 00:00:00 2001 From: Sam Joan Roque-Worcel Date: Mon, 22 Sep 2025 07:12:59 +1200 Subject: [PATCH 18/30] Undo accidental deletion of attestation model --- app/models/supply_chain/attestation.rb | 38 ++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 app/models/supply_chain/attestation.rb diff --git a/app/models/supply_chain/attestation.rb b/app/models/supply_chain/attestation.rb new file mode 100644 index 00000000000000..2a3ecd99141735 --- /dev/null +++ b/app/models/supply_chain/attestation.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +module SupplyChain + class Attestation < ::ApplicationRecord + self.table_name = 'slsa_attestations' + + belongs_to :project + belongs_to :build, class_name: 'Ci::Build', optional: true + + validates :project_id, presence: true + validates :predicate_kind, presence: true + validates :predicate_type, presence: true + validates :subject_digest, presence: true, length: { minimum: 64, maximum: 255 } + validates :file, presence: true, unless: :error? + + validates :subject_digest, uniqueness: { scope: [:project_id, :predicate_kind] } + + mount_uploader :file, AttachmentUploader + + enum :status, { + success: 0, + error: 1 + } + + enum :predicate_kind, { + provenance: 0, + sbom: 1 + } + + def uploads_sharding_key + { project_id: project_id } + end + + def retrieve_upload(_identifier, paths) + Upload.find_by(model: self, path: paths) + end + end +end -- GitLab From c8c50702a254ca5af8a01dcd6756fd4c51c1cacb Mon Sep 17 00:00:00 2001 From: Sam Joan Roque-Worcel Date: Mon, 22 Sep 2025 07:13:55 +1200 Subject: [PATCH 19/30] Revert attestation to what is on master --- app/models/supply_chain/attestation.rb | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/app/models/supply_chain/attestation.rb b/app/models/supply_chain/attestation.rb index 2a3ecd99141735..b0d7bebe66173f 100644 --- a/app/models/supply_chain/attestation.rb +++ b/app/models/supply_chain/attestation.rb @@ -11,12 +11,9 @@ class Attestation < ::ApplicationRecord validates :predicate_kind, presence: true validates :predicate_type, presence: true validates :subject_digest, presence: true, length: { minimum: 64, maximum: 255 } - validates :file, presence: true, unless: :error? validates :subject_digest, uniqueness: { scope: [:project_id, :predicate_kind] } - mount_uploader :file, AttachmentUploader - enum :status, { success: 0, error: 1 @@ -26,13 +23,5 @@ class Attestation < ::ApplicationRecord provenance: 0, sbom: 1 } - - def uploads_sharding_key - { project_id: project_id } - end - - def retrieve_upload(_identifier, paths) - Upload.find_by(model: self, path: paths) - end end end -- GitLab From d0b4871bd18bc45f92ba7b1769e4892de6b7315e Mon Sep 17 00:00:00 2001 From: Sam Joan Roque-Worcel Date: Fri, 26 Sep 2025 11:40:11 +1200 Subject: [PATCH 20/30] Fix typo in ProvenanceStatement --- app/models/supply_chain/slsa/provenance_statement.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/supply_chain/slsa/provenance_statement.rb b/app/models/supply_chain/slsa/provenance_statement.rb index 4b383a5c2fb6a6..8fc00a0e4633cd 100644 --- a/app/models/supply_chain/slsa/provenance_statement.rb +++ b/app/models/supply_chain/slsa/provenance_statement.rb @@ -42,7 +42,7 @@ def self.from_build(build) def initialize @_type = "https://in-toto.io/Statement/v1" - @predicate_type = PREDICATE_TYPE + @predicate_type = PREDICATE_TYPE_V1 end def as_json(options = nil) -- GitLab From 5fee0b05a82ec1058f01838564bef478cb6c9c75 Mon Sep 17 00:00:00 2001 From: Sam Joan Roque-Worcel Date: Fri, 26 Sep 2025 13:22:23 +1200 Subject: [PATCH 21/30] Make all linting, tests pass In this commit I've made tests pass by moving code to `SupplyChain::Attestation` where appropriate, fixing a validation as discussed in the issue, and adding and fixing tests. --- app/models/supply_chain/attestation.rb | 10 +++++- .../ci/slsa/publish_provenance_service.rb | 14 ++++----- spec/models/supply_chain/attestation_spec.rb | 31 +++++++++++++++++++ .../slsa/publish_provenance_service_spec.rb | 11 ++++++- 4 files changed, 56 insertions(+), 10 deletions(-) diff --git a/app/models/supply_chain/attestation.rb b/app/models/supply_chain/attestation.rb index 99a60406582e1a..c1e4141d2f0181 100644 --- a/app/models/supply_chain/attestation.rb +++ b/app/models/supply_chain/attestation.rb @@ -13,13 +13,17 @@ class Attestation < ::ApplicationRecord belongs_to :build, class_name: 'Ci::Build', optional: true validates :project_id, presence: true - validates :file, presence: true + validates :file, presence: true, unless: :error? validates :predicate_kind, presence: true validates :predicate_type, presence: true validates :subject_digest, presence: true, length: { minimum: 64, maximum: 255 } validates :subject_digest, uniqueness: { scope: [:project_id, :predicate_kind] } + scope :for_project, ->(project_id) { where(project_id: project_id) } + scope :with_digest, ->(subject_digest) { where(subject_digest: subject_digest) } + scope :with_predicate_kind, ->(predicate_kind) { where(predicate_kind: predicate_kind) } + attribute :file_store, default: -> { AttestationUploader.default_store } mount_file_store_uploader AttestationUploader @@ -33,5 +37,9 @@ class Attestation < ::ApplicationRecord provenance: 0, sbom: 1 } + + def self.find_existing_attestation(project:, predicate_kind:, subject_digest:) + for_project(project).with_predicate_kind(predicate_kind).with_digest(subject_digest).take + end end end diff --git a/app/services/ci/slsa/publish_provenance_service.rb b/app/services/ci/slsa/publish_provenance_service.rb index 503112ef11e24c..6387fb783d3387 100644 --- a/app/services/ci/slsa/publish_provenance_service.rb +++ b/app/services/ci/slsa/publish_provenance_service.rb @@ -41,10 +41,6 @@ def ci_server_url Gitlab.config.gitlab.url end - def attestation_by_hash(hash) - SupplyChain::Attestation.find_by(project: @build.project, predicate_kind: :provenance, subject_digest: hash) # TODO: migrate to model - end - def attest_all_artifacts(build:, id_token:) logger = Gitlab::AppJsonLogger reader = SupplyChain::ArtifactsReader.new(build) @@ -62,14 +58,16 @@ def attest_all_artifacts(build:, id_token:) blob_name: blob_name } - previous_attestation = attestation_by_hash(hash) - if previous_attestation - if previous_attestation.success? + existing_attestation = SupplyChain::Attestation.find_existing_attestation(project: @build.project, + subject_digest: hash, predicate_kind: 'provenance') + + if existing_attestation + if existing_attestation.success? logger.info(**log_args, message: "Skipping attestation, attestation already exists") next else logger.info(**log_args, message: "Reattempting previously failed attestation") - previous_attestation.destroy + existing_attestation.destroy end end diff --git a/spec/models/supply_chain/attestation_spec.rb b/spec/models/supply_chain/attestation_spec.rb index 482e1dec0efbbe..94b388eecb2797 100644 --- a/spec/models/supply_chain/attestation_spec.rb +++ b/spec/models/supply_chain/attestation_spec.rb @@ -35,4 +35,35 @@ expect(attestation.file.read).to eq(sample_file) end end + + describe '#find_existing_attestation' do + let(:subject_digest) { "5db1fee4b5703808c48078a76768b155b421b210c0761cd6a5d223f2d99f1eaa" } + + subject(:attestation) do + create(:supply_chain_attestation, predicate_kind: "provenance", subject_digest: subject_digest) + end + + context "when the right parameters are passed" do + let(:result) do + described_class.find_existing_attestation(project: attestation.project, predicate_kind: "provenance", + subject_digest: subject_digest) + end + + it 'finds the required attestation if the attestation exists' do + expect(result).to be_a(described_class) + expect(result.id).to eq(attestation.id) + end + end + + context "when incorrect parameters are passed" do + let(:result) do + described_class.find_existing_attestation(project: attestation.project, predicate_kind: "provenance", + subject_digest: "f3d9bb2a27422532b5264e1e1e22010ef9d71f604ca5de574a42a3ec07c27721") + end + + it 'finds the required attestation if the attestation exists' do + expect(result).to be_nil + end + end + end end diff --git a/spec/services/ci/slsa/publish_provenance_service_spec.rb b/spec/services/ci/slsa/publish_provenance_service_spec.rb index 6cd371fd9d739c..0a693541799aa1 100644 --- a/spec/services/ci/slsa/publish_provenance_service_spec.rb +++ b/spec/services/ci/slsa/publish_provenance_service_spec.rb @@ -12,7 +12,7 @@ end let_it_be(:signature_bundle) { File.read(SLSA_ATTESTATION_BUNDLE) } - let_it_be(:expected_stderr) { "expecte stderr outuput string" } + let_it_be(:expected_stderr) { "expected stderr outuput string" } let_it_be(:expected_stdout) { "expected stderr outuput string" } let_it_be(:expected_duration) { 1.33337 } let_it_be(:expected_predicate_type) { SupplyChain::Slsa::ProvenanceStatement::PREDICATE_TYPE_V1 } @@ -294,6 +294,15 @@ it "deletes it" do expect(existing_attestation).to receive(:destroy).and_call_original allow(service).to receive(:attestation_by_hash).with(duplicate_hash).and_return(existing_attestation) + + expected_args = { + project: project, + predicate_kind: "provenance", + subject_digest: duplicate_hash + } + allow(SupplyChain::Attestation).to receive(:find_existing_attestation).with(expected_args) + .and_return(existing_attestation) + expect(attestations.length).to be(1) expect(Gitlab::AppJsonLogger).to have_received(:info).with(a_hash_including({ -- GitLab From 50b567f6d9ce9cbca41b9fc8482cf0c7a36beeb7 Mon Sep 17 00:00:00 2001 From: Sam Joan Roque-Worcel Date: Mon, 29 Sep 2025 16:28:01 +1300 Subject: [PATCH 22/30] Persist :success attestation only on success --- app/services/ci/slsa/publish_provenance_service.rb | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/app/services/ci/slsa/publish_provenance_service.rb b/app/services/ci/slsa/publish_provenance_service.rb index 6387fb783d3387..1521c04d30e932 100644 --- a/app/services/ci/slsa/publish_provenance_service.rb +++ b/app/services/ci/slsa/publish_provenance_service.rb @@ -80,12 +80,12 @@ def attest_all_artifacts(build:, id_token:) logger.info(**log_args, message: "Attestation successful", duration: duration) rescue StandardError => e - Gitlab::ErrorTracking.track_and_raise_for_dev_exception(e, project_id: build.project.id) - attestation = persist_attestation!(status: :error, subject_digest: hash) attestations << attestation all_successful = false + + Gitlab::ErrorTracking.track_and_raise_for_dev_exception(e, project_id: build.project.id) end end @@ -122,7 +122,10 @@ def attest_blob!(blob_name:, hash:, id_token:) stdin.write(predicate) end - attestation = persist_attestation!(status: :success, subject_digest: hash, bundle_file: bundle_file) + if result.status.success? + attestation = persist_attestation!(status: :success, subject_digest: hash, + bundle_file: bundle_file) + end end return attestation, result.duration if result.status.success? -- GitLab From bbf2104c10582d0d2ddaa5e880daa536799c891e Mon Sep 17 00:00:00 2001 From: Sam Joan Roque-Worcel Date: Tue, 30 Sep 2025 10:28:08 +1300 Subject: [PATCH 23/30] Rename 'find_existing_attestation' to find_provenance In this commit I've renamed the method within the Attestation model so that it looks better. --- app/models/supply_chain/attestation.rb | 4 ++-- app/services/ci/slsa/publish_provenance_service.rb | 3 +-- spec/models/supply_chain/attestation_spec.rb | 9 ++++----- spec/services/ci/slsa/publish_provenance_service_spec.rb | 3 +-- 4 files changed, 8 insertions(+), 11 deletions(-) diff --git a/app/models/supply_chain/attestation.rb b/app/models/supply_chain/attestation.rb index c1e4141d2f0181..51c7e03e9b73e2 100644 --- a/app/models/supply_chain/attestation.rb +++ b/app/models/supply_chain/attestation.rb @@ -38,8 +38,8 @@ class Attestation < ::ApplicationRecord sbom: 1 } - def self.find_existing_attestation(project:, predicate_kind:, subject_digest:) - for_project(project).with_predicate_kind(predicate_kind).with_digest(subject_digest).take + def self.find_provenance(project:, subject_digest:) + for_project(project).with_predicate_kind("provenance").with_digest(subject_digest).take end end end diff --git a/app/services/ci/slsa/publish_provenance_service.rb b/app/services/ci/slsa/publish_provenance_service.rb index 1521c04d30e932..c41c67885b80dc 100644 --- a/app/services/ci/slsa/publish_provenance_service.rb +++ b/app/services/ci/slsa/publish_provenance_service.rb @@ -58,8 +58,7 @@ def attest_all_artifacts(build:, id_token:) blob_name: blob_name } - existing_attestation = SupplyChain::Attestation.find_existing_attestation(project: @build.project, - subject_digest: hash, predicate_kind: 'provenance') + existing_attestation = SupplyChain::Attestation.find_provenance(project: @build.project, subject_digest: hash) if existing_attestation if existing_attestation.success? diff --git a/spec/models/supply_chain/attestation_spec.rb b/spec/models/supply_chain/attestation_spec.rb index 94b388eecb2797..8e1da9db985771 100644 --- a/spec/models/supply_chain/attestation_spec.rb +++ b/spec/models/supply_chain/attestation_spec.rb @@ -36,17 +36,16 @@ end end - describe '#find_existing_attestation' do + describe '#find_provenance' do let(:subject_digest) { "5db1fee4b5703808c48078a76768b155b421b210c0761cd6a5d223f2d99f1eaa" } subject(:attestation) do - create(:supply_chain_attestation, predicate_kind: "provenance", subject_digest: subject_digest) + create(:supply_chain_attestation, subject_digest: subject_digest) end context "when the right parameters are passed" do let(:result) do - described_class.find_existing_attestation(project: attestation.project, predicate_kind: "provenance", - subject_digest: subject_digest) + described_class.find_provenance(project: attestation.project, subject_digest: subject_digest) end it 'finds the required attestation if the attestation exists' do @@ -57,7 +56,7 @@ context "when incorrect parameters are passed" do let(:result) do - described_class.find_existing_attestation(project: attestation.project, predicate_kind: "provenance", + described_class.find_provenance(project: attestation.project, subject_digest: "f3d9bb2a27422532b5264e1e1e22010ef9d71f604ca5de574a42a3ec07c27721") end diff --git a/spec/services/ci/slsa/publish_provenance_service_spec.rb b/spec/services/ci/slsa/publish_provenance_service_spec.rb index 0a693541799aa1..057c3d2d8e850d 100644 --- a/spec/services/ci/slsa/publish_provenance_service_spec.rb +++ b/spec/services/ci/slsa/publish_provenance_service_spec.rb @@ -297,10 +297,9 @@ expected_args = { project: project, - predicate_kind: "provenance", subject_digest: duplicate_hash } - allow(SupplyChain::Attestation).to receive(:find_existing_attestation).with(expected_args) + allow(SupplyChain::Attestation).to receive(:find_provenance).with(expected_args) .and_return(existing_attestation) expect(attestations.length).to be(1) -- GitLab From aafacc89508951ee1049c5f96de0860c2023156b Mon Sep 17 00:00:00 2001 From: Sam Joan Roque-Worcel Date: Wed, 1 Oct 2025 07:29:27 +1300 Subject: [PATCH 24/30] Remove noisy logging In this commit, I've removed logging that may pollute the logs if run many times. --- app/services/ci/slsa/publish_provenance_service.rb | 10 +++------- .../ci/slsa/publish_provenance_service_spec.rb | 10 +--------- 2 files changed, 4 insertions(+), 16 deletions(-) diff --git a/app/services/ci/slsa/publish_provenance_service.rb b/app/services/ci/slsa/publish_provenance_service.rb index c41c67885b80dc..febca68a02e418 100644 --- a/app/services/ci/slsa/publish_provenance_service.rb +++ b/app/services/ci/slsa/publish_provenance_service.rb @@ -61,13 +61,9 @@ def attest_all_artifacts(build:, id_token:) existing_attestation = SupplyChain::Attestation.find_provenance(project: @build.project, subject_digest: hash) if existing_attestation - if existing_attestation.success? - logger.info(**log_args, message: "Skipping attestation, attestation already exists") - next - else - logger.info(**log_args, message: "Reattempting previously failed attestation") - existing_attestation.destroy - end + next if existing_attestation.success? + + existing_attestation.destroy end begin diff --git a/spec/services/ci/slsa/publish_provenance_service_spec.rb b/spec/services/ci/slsa/publish_provenance_service_spec.rb index 057c3d2d8e850d..1a379a43a6718e 100644 --- a/spec/services/ci/slsa/publish_provenance_service_spec.rb +++ b/spec/services/ci/slsa/publish_provenance_service_spec.rb @@ -271,11 +271,7 @@ end it "skips attestation" do - expect(attestations.length).to be(1) - - expect(Gitlab::AppJsonLogger).to have_received(:info).exactly(2).times.with(a_hash_including({ - message: "Skipping attestation, attestation already exists" - })) + expect(attestations.length).to eq(1) end end @@ -303,10 +299,6 @@ .and_return(existing_attestation) expect(attestations.length).to be(1) - - expect(Gitlab::AppJsonLogger).to have_received(:info).with(a_hash_including({ - message: "Reattempting previously failed attestation" - })) end end -- GitLab From cece9089c58807a4aea17f802537e444097e0e6c Mon Sep 17 00:00:00 2001 From: Sam Joan Roque-Worcel Date: Wed, 1 Oct 2025 07:33:28 +1300 Subject: [PATCH 25/30] Make code more idiomatic --- app/services/ci/slsa/publish_provenance_service.rb | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/app/services/ci/slsa/publish_provenance_service.rb b/app/services/ci/slsa/publish_provenance_service.rb index febca68a02e418..ffddc705dbbea0 100644 --- a/app/services/ci/slsa/publish_provenance_service.rb +++ b/app/services/ci/slsa/publish_provenance_service.rb @@ -60,11 +60,9 @@ def attest_all_artifacts(build:, id_token:) existing_attestation = SupplyChain::Attestation.find_provenance(project: @build.project, subject_digest: hash) - if existing_attestation - next if existing_attestation.success? + next if existing_attestation&.success? - existing_attestation.destroy - end + existing_attestation&.destroy begin logger.info(**log_args, message: "Performing attestation for artifact") -- GitLab From d124b3245be6b6fac5d3258a3557949d0febfe7f Mon Sep 17 00:00:00 2001 From: Sam Joan Roque-Worcel Date: Wed, 1 Oct 2025 07:36:42 +1300 Subject: [PATCH 26/30] Change ServiceResponse message --- app/services/ci/slsa/publish_provenance_service.rb | 5 ++++- .../ci/slsa/publish_provenance_service_spec.rb | 13 +++++++------ 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/app/services/ci/slsa/publish_provenance_service.rb b/app/services/ci/slsa/publish_provenance_service.rb index ffddc705dbbea0..57552e539d28c4 100644 --- a/app/services/ci/slsa/publish_provenance_service.rb +++ b/app/services/ci/slsa/publish_provenance_service.rb @@ -82,7 +82,10 @@ def attest_all_artifacts(build:, id_token:) end end - return ServiceResponse.success(message: "OK", payload: { attestations: attestations }) if all_successful + if all_successful + return ServiceResponse.success(message: "Attestations persisted", + payload: { attestations: attestations }) + end ServiceResponse.error(message: "Attestation failure", payload: { attestations: attestations }) end diff --git a/spec/services/ci/slsa/publish_provenance_service_spec.rb b/spec/services/ci/slsa/publish_provenance_service_spec.rb index 1a379a43a6718e..c09f6c18760afa 100644 --- a/spec/services/ci/slsa/publish_provenance_service_spec.rb +++ b/spec/services/ci/slsa/publish_provenance_service_spec.rb @@ -7,6 +7,7 @@ RSpec.describe Ci::Slsa::PublishProvenanceService, feature_category: :artifact_security do let(:service) { described_class.new(build) } + let(:success_message) { "Attestations persisted" } let(:popen_result) do Gitlab::Popen::Result.new([], expected_stdout, expected_stderr, process_status, expected_duration) end @@ -78,7 +79,7 @@ it 'persists the attestations' do expect(result[:status]).to eq(:success) - expect(result[:message]).to eq("OK") + expect(result[:message]).to eq(success_message) expect(attestations.length).to eq(3) @@ -106,7 +107,7 @@ expect(predicate_class).to receive(:from_build).exactly(1).time.and_call_original expect(result[:status]).to eq(:success) - expect(result[:message]).to eq("OK") + expect(result[:message]).to eq(success_message) expected_hashes.each do |path, hash| expect(Gitlab::AppJsonLogger).to have_received(:info).with(a_hash_including({ @@ -133,7 +134,7 @@ expect(service).to receive(:attest_blob!).with(blob_name: path, id_token: id_token, hash: hash) end - expect(result[:message]).to eq("OK") + expect(result[:message]).to eq(success_message) end it 'calls cosign with the appropriate parameters' do @@ -145,7 +146,7 @@ expect(Gitlab::Popen).to receive(:popen_with_detail).with(expected_parameters).and_return(popen_result) end - expect(result[:message]).to eq("OK") + expect(result[:message]).to eq(success_message) end context 'when environment variables for optional parameters exist' do @@ -168,7 +169,7 @@ expect(Gitlab::Popen).to receive(:popen_with_detail).with(expected_parameters).and_return(popen_result) end - expect(result[:message]).to eq("OK") + expect(result[:message]).to eq(success_message) end end @@ -184,7 +185,7 @@ expect(Gitlab::Popen).to receive(:popen_with_detail).with(expected_parameters).and_return(popen_result) end - expect(result[:message]).to eq("OK") + expect(result[:message]).to eq(success_message) end end end -- GitLab From 34686b1f538b1d08fa79c529f4f2366e9a1099a1 Mon Sep 17 00:00:00 2001 From: Sam Joan Roque-Worcel Date: Wed, 1 Oct 2025 07:42:21 +1300 Subject: [PATCH 27/30] Rename attest_blob! to attest_blob In this commit, I've renamed this method to make it clear the method will not abort mid way, but rather return normally on failure. --- app/services/ci/slsa/publish_provenance_service.rb | 4 ++-- spec/services/ci/slsa/publish_provenance_service_spec.rb | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/services/ci/slsa/publish_provenance_service.rb b/app/services/ci/slsa/publish_provenance_service.rb index 57552e539d28c4..d30a966488f8c1 100644 --- a/app/services/ci/slsa/publish_provenance_service.rb +++ b/app/services/ci/slsa/publish_provenance_service.rb @@ -67,7 +67,7 @@ def attest_all_artifacts(build:, id_token:) begin logger.info(**log_args, message: "Performing attestation for artifact") - attestation, duration = attest_blob!(blob_name: blob_name, hash: hash, id_token: id_token) + attestation, duration = attest_blob(blob_name: blob_name, hash: hash, id_token: id_token) attestations << attestation logger.info(**log_args, message: "Attestation successful", duration: duration) @@ -90,7 +90,7 @@ def attest_all_artifacts(build:, id_token:) ServiceResponse.error(message: "Attestation failure", payload: { attestations: attestations }) end - def attest_blob!(blob_name:, hash:, id_token:) + def attest_blob(blob_name:, hash:, id_token:) validate_id_token!(id_token) validate_blob_name!(blob_name) validate_hash!(hash) diff --git a/spec/services/ci/slsa/publish_provenance_service_spec.rb b/spec/services/ci/slsa/publish_provenance_service_spec.rb index c09f6c18760afa..08bab49d845256 100644 --- a/spec/services/ci/slsa/publish_provenance_service_spec.rb +++ b/spec/services/ci/slsa/publish_provenance_service_spec.rb @@ -131,7 +131,7 @@ it 'calls attest with the right parameters' do expected_hashes.each do |path, hash| - expect(service).to receive(:attest_blob!).with(blob_name: path, id_token: id_token, hash: hash) + expect(service).to receive(:attest_blob).with(blob_name: path, id_token: id_token, hash: hash) end expect(result[:message]).to eq(success_message) @@ -349,12 +349,12 @@ end end - describe '#attest_blob!' do + describe '#attest_blob' do let(:hash) { "5db1fee4b5703808c48078a76768b155b421b210c0761cd6a5d223f4d99f1eaa" } let(:blob_name) { "test.txt" } subject(:attest_blob) do - service.send(:attest_blob!, blob_name: 'test.txt', hash: hash, id_token: id_token) + service.send(:attest_blob, blob_name: 'test.txt', hash: hash, id_token: id_token) end context "when called normally" do -- GitLab From 38c538078749c6c4f931160ef9ba88f61becaa11 Mon Sep 17 00:00:00 2001 From: Sam Joan Roque-Worcel Date: Mon, 6 Oct 2025 06:32:58 +1300 Subject: [PATCH 28/30] Prefer instance variable over parameter --- app/services/ci/slsa/publish_provenance_service.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/app/services/ci/slsa/publish_provenance_service.rb b/app/services/ci/slsa/publish_provenance_service.rb index d30a966488f8c1..cb8d819a4379a7 100644 --- a/app/services/ci/slsa/publish_provenance_service.rb +++ b/app/services/ci/slsa/publish_provenance_service.rb @@ -27,7 +27,7 @@ def execute id_token = @build.variables["SIGSTORE_ID_TOKEN"]&.value return ServiceResponse.error(message: "Missing required variable SIGSTORE_ID_TOKEN") unless id_token - attest_all_artifacts(build: @build, id_token: id_token) + attest_all_artifacts(id_token: id_token) end private @@ -41,9 +41,9 @@ def ci_server_url Gitlab.config.gitlab.url end - def attest_all_artifacts(build:, id_token:) + def attest_all_artifacts(id_token:) logger = Gitlab::AppJsonLogger - reader = SupplyChain::ArtifactsReader.new(build) + reader = SupplyChain::ArtifactsReader.new(@build) all_successful = true attestations = [] @@ -54,7 +54,7 @@ def attest_all_artifacts(build:, id_token:) class: self.class.name, hash: hash, path: artifact_path, - build_id: build.id, + build_id: @build.id, blob_name: blob_name } @@ -78,7 +78,7 @@ def attest_all_artifacts(build:, id_token:) all_successful = false - Gitlab::ErrorTracking.track_and_raise_for_dev_exception(e, project_id: build.project.id) + Gitlab::ErrorTracking.track_and_raise_for_dev_exception(e, project_id: @build.project.id) end end -- GitLab From cf150d40d3b2af7d3df75d32cab1e2a57d4ea3e5 Mon Sep 17 00:00:00 2001 From: Sam Joan Roque-Worcel Date: Mon, 6 Oct 2025 07:13:49 +1300 Subject: [PATCH 29/30] Refactor attest_all_artifacts In this commit, I've refactored the attest_all_artifacts method so that it is not in charge of so many things. --- .../ci/slsa/publish_provenance_service.rb | 85 ++++++++++++------- .../slsa/publish_provenance_service_spec.rb | 12 +-- 2 files changed, 58 insertions(+), 39 deletions(-) diff --git a/app/services/ci/slsa/publish_provenance_service.rb b/app/services/ci/slsa/publish_provenance_service.rb index cb8d819a4379a7..a0cb02abf9502c 100644 --- a/app/services/ci/slsa/publish_provenance_service.rb +++ b/app/services/ci/slsa/publish_provenance_service.rb @@ -15,6 +15,12 @@ class PublishProvenanceService < ::BaseService def initialize(build) @build = build + + @logger = Gitlab::AppJsonLogger + @logger_base_args = { + class: self.class.name, + build_id: @build&.id + } end def execute @@ -24,14 +30,22 @@ def execute return ServiceResponse.error(message: "Attestation is only enabled for public projects") end - id_token = @build.variables["SIGSTORE_ID_TOKEN"]&.value return ServiceResponse.error(message: "Missing required variable SIGSTORE_ID_TOKEN") unless id_token - attest_all_artifacts(id_token: id_token) + attest_all_artifacts end private + def id_token + @build.variables["SIGSTORE_ID_TOKEN"]&.value + end + strong_memoize_attr :id_token + + def log(**args) + @logger.info(@logger_base_args.merge(args)) + end + def predicate SupplyChain::Slsa::ProvenanceStatement::Predicate.from_build(@build).to_json end @@ -41,45 +55,20 @@ def ci_server_url Gitlab.config.gitlab.url end - def attest_all_artifacts(id_token:) - logger = Gitlab::AppJsonLogger + def attest_all_artifacts reader = SupplyChain::ArtifactsReader.new(@build) all_successful = true attestations = [] reader.files do |artifact_path, file_input_stream| - blob_name = File.basename(artifact_path) hash = hash(file_input_stream) - log_args = { - class: self.class.name, - hash: hash, - path: artifact_path, - build_id: @build.id, - blob_name: blob_name - } - - existing_attestation = SupplyChain::Attestation.find_provenance(project: @build.project, subject_digest: hash) - next if existing_attestation&.success? + next if successful_attestation?(hash) - existing_attestation&.destroy + attestation, success = attest_artifact(artifact_path, hash) + attestations << attestation - begin - logger.info(**log_args, message: "Performing attestation for artifact") - - attestation, duration = attest_blob(blob_name: blob_name, hash: hash, id_token: id_token) - attestations << attestation - - logger.info(**log_args, message: "Attestation successful", duration: duration) - - rescue StandardError => e - attestation = persist_attestation!(status: :error, subject_digest: hash) - attestations << attestation - - all_successful = false - - Gitlab::ErrorTracking.track_and_raise_for_dev_exception(e, project_id: @build.project.id) - end + all_successful = false unless success end if all_successful @@ -90,7 +79,37 @@ def attest_all_artifacts(id_token:) ServiceResponse.error(message: "Attestation failure", payload: { attestations: attestations }) end - def attest_blob(blob_name:, hash:, id_token:) + def successful_attestation?(hash) + existing_attestation = SupplyChain::Attestation.find_provenance(project: @build.project, subject_digest: hash) + return true if existing_attestation&.success? + + existing_attestation&.destroy + + false + end + + def attest_artifact(artifact_path, hash) + log(message: "Performing attestation for artifact", path: artifact_path, hash: hash) + + blob_name = File.basename(artifact_path) + begin + attestation, duration = cosign_attest_blob(blob_name: blob_name, hash: hash) + log(message: "Attestation successful", duration: duration, path: artifact_path, hash: hash, + blob_name: blob_name) + + [attestation, true] + rescue StandardError => e + log(message: "Attestation failure", path: artifact_path, hash: hash, blob_name: blob_name) + + attestation = persist_attestation!(status: :error, subject_digest: hash) + + Gitlab::ErrorTracking.track_and_raise_for_dev_exception(e, project_id: @build.project.id) + + [attestation, false] + end + end + + def cosign_attest_blob(blob_name:, hash:) validate_id_token!(id_token) validate_blob_name!(blob_name) validate_hash!(hash) diff --git a/spec/services/ci/slsa/publish_provenance_service_spec.rb b/spec/services/ci/slsa/publish_provenance_service_spec.rb index 08bab49d845256..5ea577f9567aa0 100644 --- a/spec/services/ci/slsa/publish_provenance_service_spec.rb +++ b/spec/services/ci/slsa/publish_provenance_service_spec.rb @@ -131,7 +131,7 @@ it 'calls attest with the right parameters' do expected_hashes.each do |path, hash| - expect(service).to receive(:attest_blob).with(blob_name: path, id_token: id_token, hash: hash) + expect(service).to receive(:cosign_attest_blob).with(blob_name: path, hash: hash) end expect(result[:message]).to eq(success_message) @@ -349,12 +349,12 @@ end end - describe '#attest_blob' do + describe '#cosign_attest_blob' do let(:hash) { "5db1fee4b5703808c48078a76768b155b421b210c0761cd6a5d223f4d99f1eaa" } let(:blob_name) { "test.txt" } - subject(:attest_blob) do - service.send(:attest_blob, blob_name: 'test.txt', hash: hash, id_token: id_token) + subject(:cosign_attest_blob) do + service.send(:cosign_attest_blob, blob_name: 'test.txt', hash: hash) end context "when called normally" do @@ -363,7 +363,7 @@ expect(service).to receive(:validate_hash!).with(hash) expect(service).to receive(:validate_id_token!).with(id_token) - attest_blob + cosign_attest_blob end end @@ -371,7 +371,7 @@ let(:popen_success) { false } it 'raises the appropriate exception' do - expect { attest_blob }.to raise_exception(described_class::AttestationFailure) + expect { cosign_attest_blob }.to raise_exception(described_class::AttestationFailure) end end end -- GitLab From 3ab70f255ffd209024de1e1fb0063f6bfc2ca2f6 Mon Sep 17 00:00:00 2001 From: Sam Joan Roque-Worcel Date: Mon, 6 Oct 2025 10:31:41 +1300 Subject: [PATCH 30/30] Reduce logging when not necessary As discussed, in this commit I've reduced logging. To avoid silent failures, I've added an explicit log for failure events. --- .../ci/slsa/publish_provenance_service.rb | 2 -- .../ci/slsa/publish_provenance_service_spec.rb | 17 +++++++++-------- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/app/services/ci/slsa/publish_provenance_service.rb b/app/services/ci/slsa/publish_provenance_service.rb index a0cb02abf9502c..4b798e100330e9 100644 --- a/app/services/ci/slsa/publish_provenance_service.rb +++ b/app/services/ci/slsa/publish_provenance_service.rb @@ -89,8 +89,6 @@ def successful_attestation?(hash) end def attest_artifact(artifact_path, hash) - log(message: "Performing attestation for artifact", path: artifact_path, hash: hash) - blob_name = File.basename(artifact_path) begin attestation, duration = cosign_attest_blob(blob_name: blob_name, hash: hash) diff --git a/spec/services/ci/slsa/publish_provenance_service_spec.rb b/spec/services/ci/slsa/publish_provenance_service_spec.rb index 5ea577f9567aa0..686afd0c9e3116 100644 --- a/spec/services/ci/slsa/publish_provenance_service_spec.rb +++ b/spec/services/ci/slsa/publish_provenance_service_spec.rb @@ -110,13 +110,6 @@ expect(result[:message]).to eq(success_message) expected_hashes.each do |path, hash| - expect(Gitlab::AppJsonLogger).to have_received(:info).with(a_hash_including({ - message: "Performing attestation for artifact", - hash: hash, - path: end_with(path), - build_id: build.id - })) - expect(Gitlab::AppJsonLogger).to have_received(:info).with(a_hash_including({ message: "Attestation successful", hash: hash, @@ -238,6 +231,8 @@ context "when attestation fails" do it 'persists a :failed attestation' do + allow(Gitlab::AppJsonLogger).to receive(:info) + expect(service).to receive(:validate_blob_name!).with(any_args).exactly(3).times.and_raise(StandardError) expect(Gitlab::ErrorTracking).to receive(:track_and_raise_for_dev_exception).exactly(3).times @@ -258,8 +253,14 @@ expect(att.file.read).to be_nil end - expected_hashes.each_value do |hash| + expected_hashes.each do |path, hash| expect(attestations).to include(an_object_having_attributes(subject_digest: hash)) + expect(Gitlab::AppJsonLogger).to have_received(:info).with(a_hash_including({ + message: "Attestation failure", + hash: hash, + blob_name: File.basename(path), + build_id: build.id + })) end end end -- GitLab