From 5fe1c503f21ea1f2753193e89856015010902503 Mon Sep 17 00:00:00 2001 From: Leo Di Donato <120051+leodido@users.noreply.github.com> Date: Tue, 18 Nov 2025 14:00:10 +0000 Subject: [PATCH 1/2] fix(signing): skip attestation upload when artifact exists Prevents SLSA verification failures caused by attestation overwrites in concurrent builds. When an artifact already exists in remote cache, skip both artifact and attestation upload to preserve the existing artifact's attestation. Co-authored-by: Ona --- pkg/leeway/signing/upload.go | 23 ++++-- pkg/leeway/signing/upload_test.go | 126 +++++++++++++++++++++++++++--- 2 files changed, 129 insertions(+), 20 deletions(-) diff --git a/pkg/leeway/signing/upload.go b/pkg/leeway/signing/upload.go index fe28976b..4ff1bc3d 100644 --- a/pkg/leeway/signing/upload.go +++ b/pkg/leeway/signing/upload.go @@ -99,23 +99,32 @@ func (u *ArtifactUploader) UploadArtifactWithAttestation(ctx context.Context, ar "artifact": artifactPath, "artifact_key": artifactKey, }).Info("Artifact already exists in remote cache, skipping upload") - } else { - // Upload artifact only if it doesn't exist - if err := u.remoteCache.UploadFile(ctx, artifactPath, artifactKey); err != nil { - return fmt.Errorf("failed to upload artifact: %w", err) - } + + // Also skip attestation upload - the existing artifact already has an attestation + // Uploading a new attestation for a different local artifact would cause verification failures log.WithFields(log.Fields{ "artifact": artifactPath, "artifact_key": artifactKey, - }).Info("Successfully uploaded artifact to remote cache") + "att_key": attestationKey, + }).Info("Skipping attestation upload (artifact already exists with attestation)") + return nil + } + + // Upload artifact (only if it doesn't exist) + if err := u.remoteCache.UploadFile(ctx, artifactPath, artifactKey); err != nil { + return fmt.Errorf("failed to upload artifact: %w", err) } + log.WithFields(log.Fields{ + "artifact": artifactPath, + "artifact_key": artifactKey, + }).Info("Successfully uploaded artifact to remote cache") // Check context between uploads if err := ctx.Err(); err != nil { return fmt.Errorf("context cancelled between uploads: %w", err) } - // Always upload attestation file (it's new) + // Upload attestation file (only if artifact was also uploaded) if err := u.remoteCache.UploadFile(ctx, attestationPath, attestationKey); err != nil { return fmt.Errorf("failed to upload .att file: %w", err) } diff --git a/pkg/leeway/signing/upload_test.go b/pkg/leeway/signing/upload_test.go index 402fe112..bd127557 100644 --- a/pkg/leeway/signing/upload_test.go +++ b/pkg/leeway/signing/upload_test.go @@ -382,6 +382,7 @@ func TestArtifactUploader_ConcurrentUploads(t *testing.T) { } // TestArtifactUploader_SkipsExistingArtifacts tests that existing artifacts are not re-uploaded +// and attestation upload is also skipped to prevent overwriting existing attestations func TestArtifactUploader_SkipsExistingArtifacts(t *testing.T) { tmpDir := t.TempDir() artifactPath := filepath.Join(tmpDir, "existing.tar.gz") @@ -406,10 +407,10 @@ func TestArtifactUploader_SkipsExistingArtifacts(t *testing.T) { // CRITICAL: Verify that UploadFile was NOT called for the artifact assert.Equal(t, 0, mockCache.uploadCalls["existing.tar.gz"], "Artifact should not be uploaded when it already exists") - // Verify that the attestation WAS uploaded - assert.Equal(t, 1, mockCache.uploadCalls["existing.tar.gz.att"], "Attestation should be uploaded exactly once") - assert.Contains(t, mockCache.uploadedFiles, "existing.tar.gz.att", "Attestation should be in cache") - assert.Equal(t, attestation, mockCache.uploadedFiles["existing.tar.gz.att"], "Attestation content should match") + // CRITICAL: Verify that the attestation was NOT uploaded (FIXED BEHAVIOR) + // This prevents overwriting the existing artifact's attestation with a potentially different one + assert.Equal(t, 0, mockCache.uploadCalls["existing.tar.gz.att"], "Attestation should NOT be uploaded when artifact already exists") + assert.NotContains(t, mockCache.uploadedFiles, "existing.tar.gz.att", "Attestation should NOT be in cache") } // TestArtifactUploader_UploadsNewArtifacts tests that new artifacts are uploaded @@ -451,7 +452,7 @@ func TestArtifactUploader_SimulatesDownloadedArtifactWorkflow(t *testing.T) { // 1. Build job uploads artifact to S3 // 2. Sign job downloads artifact from S3 // 3. Sign job creates attestation - // 4. Sign job should NOT re-upload artifact, only upload attestation + // 4. Sign job should NOT re-upload artifact OR attestation mockCache := &mockRemoteCacheUpload{ uploadedFiles: make(map[string][]byte), @@ -475,18 +476,17 @@ func TestArtifactUploader_SimulatesDownloadedArtifactWorkflow(t *testing.T) { err = uploader.UploadArtifactWithAttestation(context.Background(), downloadedArtifactPath, attestation) require.NoError(t, err) - // CRITICAL VERIFICATION: Artifact should NOT be re-uploaded + // CRITICAL: Artifact should NOT be re-uploaded // uploadCalls should still be 1 (from build job), not 2 assert.Equal(t, 1, mockCache.uploadCalls["downloaded.tar.gz"], "Downloaded artifact should NOT be re-uploaded by sign job") - // Attestation should be uploaded - assert.Equal(t, 1, mockCache.uploadCalls["downloaded.tar.gz.att"], - "Attestation should be uploaded exactly once") - assert.Contains(t, mockCache.uploadedFiles, "downloaded.tar.gz.att", - "Attestation should be in cache") - assert.Equal(t, attestation, mockCache.uploadedFiles["downloaded.tar.gz.att"], - "Attestation content should match") + // CRITICAL: Attestation should NOT be uploaded + // This prevents overwriting the existing artifact's attestation + assert.Equal(t, 0, mockCache.uploadCalls["downloaded.tar.gz.att"], + "Attestation should NOT be uploaded when artifact already exists") + assert.NotContains(t, mockCache.uploadedFiles, "downloaded.tar.gz.att", + "Attestation should NOT be in cache") // Artifact content should remain unchanged (not overwritten) assert.Equal(t, buildArtifactContent, mockCache.uploadedFiles["downloaded.tar.gz"], @@ -532,3 +532,103 @@ func TestArtifactUploader_SimulatesLocallyBuiltArtifactWorkflow(t *testing.T) { assert.Equal(t, attestation, mockCache.uploadedFiles["local.tar.gz.att"], "Attestation content should match") } + +// TestArtifactUploader_PreventsAttestationOverwrite tests the race condition fix +// where multiple workflows building the same artifact would overwrite each other's attestations +func TestArtifactUploader_PreventsAttestationOverwrite(t *testing.T) { + tmpDir := t.TempDir() + + // Simulate the race condition scenario: + // 1. Workflow A builds artifact (checksum A), uploads to S3, signs it, uploads attestation A + // 2. Workflow B builds artifact (checksum B), finds artifact exists, signs LOCAL artifact B + // 3. WITHOUT THIS: Workflow B would upload attestation B, overwriting attestation A + // 4. WITH THIS: Workflow B skips attestation upload, preserving attestation A + + mockCache := &mockRemoteCacheUpload{ + uploadedFiles: make(map[string][]byte), + uploadCalls: make(map[string]int), + } + + // Step 1: Workflow A uploads artifact and attestation + artifactAContent := []byte("artifact with checksum A") + attestationA := []byte(`{"checksum":"A","workflow":"build-main"}`) + + mockCache.uploadedFiles["shared-artifact.tar.gz"] = artifactAContent + mockCache.uploadedFiles["shared-artifact.tar.gz.att"] = attestationA + mockCache.uploadCalls["shared-artifact.tar.gz"] = 1 + mockCache.uploadCalls["shared-artifact.tar.gz.att"] = 1 + + // Step 2: Workflow B builds a different version locally + artifactBPath := filepath.Join(tmpDir, "shared-artifact.tar.gz") + artifactBContent := []byte("artifact with checksum B") + err := os.WriteFile(artifactBPath, artifactBContent, 0644) + require.NoError(t, err) + + // Step 3: Workflow B tries to upload (should skip both) + uploader := NewArtifactUploader(mockCache) + attestationB := []byte(`{"checksum":"B","workflow":"build-cli"}`) + + err = uploader.UploadArtifactWithAttestation(context.Background(), artifactBPath, attestationB) + require.NoError(t, err) + + // Step 4: Verify the fix + // Artifact upload count should still be 1 (only workflow A uploaded) + assert.Equal(t, 1, mockCache.uploadCalls["shared-artifact.tar.gz"], + "Artifact should not be re-uploaded by workflow B") + + // CRITICAL: Attestation upload count should still be 1 (only workflow A uploaded) + assert.Equal(t, 1, mockCache.uploadCalls["shared-artifact.tar.gz.att"], + "Attestation should not be overwritten by workflow B") + + // Verify original attestation A is preserved (not overwritten by attestation B) + assert.Equal(t, attestationA, mockCache.uploadedFiles["shared-artifact.tar.gz.att"], + "Original attestation A should be preserved, not overwritten by attestation B") + + // Verify original artifact A is preserved + assert.Equal(t, artifactAContent, mockCache.uploadedFiles["shared-artifact.tar.gz"], + "Original artifact A should be preserved") +} + +// TestArtifactUploader_HasFileError tests fallback behavior when HasFile check fails +func TestArtifactUploader_HasFileError(t *testing.T) { + tmpDir := t.TempDir() + artifactPath := filepath.Join(tmpDir, "test.tar.gz") + artifactContent := []byte("test content") + err := os.WriteFile(artifactPath, artifactContent, 0644) + require.NoError(t, err) + + // Create a mock that returns an error for HasFile + mockCache := &mockRemoteCacheUploadWithHasFileError{ + mockRemoteCacheUpload: mockRemoteCacheUpload{ + uploadedFiles: make(map[string][]byte), + uploadCalls: make(map[string]int), + }, + hasFileError: fmt.Errorf("S3 connection failed"), + } + + uploader := NewArtifactUploader(mockCache) + attestation := []byte(`{"test":"attestation"}`) + + // Should proceed with upload despite HasFile error (safe fallback) + err = uploader.UploadArtifactWithAttestation(context.Background(), artifactPath, attestation) + require.NoError(t, err) + + // Verify both were uploaded (fallback behavior assumes artifact doesn't exist) + assert.Equal(t, 1, mockCache.uploadCalls["test.tar.gz"], + "Artifact should be uploaded when HasFile fails") + assert.Equal(t, 1, mockCache.uploadCalls["test.tar.gz.att"], + "Attestation should be uploaded when HasFile fails") +} + +// mockRemoteCacheUploadWithHasFileError extends mockRemoteCacheUpload to simulate HasFile errors +type mockRemoteCacheUploadWithHasFileError struct { + mockRemoteCacheUpload + hasFileError error +} + +func (m *mockRemoteCacheUploadWithHasFileError) HasFile(ctx context.Context, key string) (bool, error) { + if m.hasFileError != nil { + return false, m.hasFileError + } + return m.mockRemoteCacheUpload.HasFile(ctx, key) +} From 2df617fc7c8141545bbd42608906c865592cc94d Mon Sep 17 00:00:00 2001 From: Leo Di Donato <120051+leodido@users.noreply.github.com> Date: Tue, 18 Nov 2025 14:45:29 +0000 Subject: [PATCH 2/2] fix(signing): check attestation existence before skipping upload When artifact exists, check if attestation also exists: - If both exist: skip both uploads (prevents overwrites) - If only artifact exists: upload attestation only (handles old builds) - If HasFile check fails: fallback to upload (safe default) This handles the case where artifacts exist from old builds before SLSA attestation was implemented, ensuring attestations are uploaded for those artifacts. Co-authored-by: Ona --- pkg/leeway/signing/upload.go | 32 ++++++++- pkg/leeway/signing/upload_test.go | 115 ++++++++++++++++++++++++++---- 2 files changed, 131 insertions(+), 16 deletions(-) diff --git a/pkg/leeway/signing/upload.go b/pkg/leeway/signing/upload.go index 4ff1bc3d..90d65e82 100644 --- a/pkg/leeway/signing/upload.go +++ b/pkg/leeway/signing/upload.go @@ -100,13 +100,39 @@ func (u *ArtifactUploader) UploadArtifactWithAttestation(ctx context.Context, ar "artifact_key": artifactKey, }).Info("Artifact already exists in remote cache, skipping upload") - // Also skip attestation upload - the existing artifact already has an attestation - // Uploading a new attestation for a different local artifact would cause verification failures + // Check if attestation also exists + attestationExists, err := u.remoteCache.HasFile(ctx, attestationKey) + if err != nil { + log.WithError(err).WithField("key", attestationKey).Warn("Failed to check if attestation exists, will attempt upload") + attestationExists = false // Assume it doesn't exist and try to upload + } + + if attestationExists { + // Both artifact and attestation exist, skip both uploads + log.WithFields(log.Fields{ + "artifact": artifactPath, + "artifact_key": artifactKey, + "att_key": attestationKey, + }).Info("Skipping attestation upload (artifact and attestation already exist)") + return nil + } + + // Artifact exists but attestation missing, upload attestation only + log.WithFields(log.Fields{ + "artifact": artifactPath, + "artifact_key": artifactKey, + "att_key": attestationKey, + }).Info("Artifact exists but attestation missing, uploading attestation only") + + if err := u.remoteCache.UploadFile(ctx, attestationPath, attestationKey); err != nil { + return fmt.Errorf("failed to upload attestation: %w", err) + } + log.WithFields(log.Fields{ "artifact": artifactPath, "artifact_key": artifactKey, "att_key": attestationKey, - }).Info("Skipping attestation upload (artifact already exists with attestation)") + }).Info("Successfully uploaded attestation") return nil } diff --git a/pkg/leeway/signing/upload_test.go b/pkg/leeway/signing/upload_test.go index bd127557..2a1a21aa 100644 --- a/pkg/leeway/signing/upload_test.go +++ b/pkg/leeway/signing/upload_test.go @@ -382,7 +382,7 @@ func TestArtifactUploader_ConcurrentUploads(t *testing.T) { } // TestArtifactUploader_SkipsExistingArtifacts tests that existing artifacts are not re-uploaded -// and attestation upload is also skipped to prevent overwriting existing attestations +// and attestation upload is also skipped when both artifact and attestation exist func TestArtifactUploader_SkipsExistingArtifacts(t *testing.T) { tmpDir := t.TempDir() artifactPath := filepath.Join(tmpDir, "existing.tar.gz") @@ -395,8 +395,10 @@ func TestArtifactUploader_SkipsExistingArtifacts(t *testing.T) { uploadCalls: make(map[string]int), } - // Pre-populate the cache with the artifact (simulating it already exists) + // Pre-populate the cache with both artifact and attestation (simulating they already exist) + existingAttestation := []byte(`{"existing":"attestation"}`) mockCache.uploadedFiles["existing.tar.gz"] = artifactContent + mockCache.uploadedFiles["existing.tar.gz.att"] = existingAttestation uploader := NewArtifactUploader(mockCache) attestation := []byte(`{"test":"attestation"}`) @@ -407,10 +409,43 @@ func TestArtifactUploader_SkipsExistingArtifacts(t *testing.T) { // CRITICAL: Verify that UploadFile was NOT called for the artifact assert.Equal(t, 0, mockCache.uploadCalls["existing.tar.gz"], "Artifact should not be uploaded when it already exists") - // CRITICAL: Verify that the attestation was NOT uploaded (FIXED BEHAVIOR) - // This prevents overwriting the existing artifact's attestation with a potentially different one - assert.Equal(t, 0, mockCache.uploadCalls["existing.tar.gz.att"], "Attestation should NOT be uploaded when artifact already exists") - assert.NotContains(t, mockCache.uploadedFiles, "existing.tar.gz.att", "Attestation should NOT be in cache") + // CRITICAL: Verify that the attestation was NOT uploaded when both exist + assert.Equal(t, 0, mockCache.uploadCalls["existing.tar.gz.att"], "Attestation should NOT be uploaded when both artifact and attestation exist") + + // Verify existing attestation is preserved + assert.Equal(t, existingAttestation, mockCache.uploadedFiles["existing.tar.gz.att"], "Existing attestation should be preserved") +} + +// TestArtifactUploader_UploadsAttestationWhenMissing tests that attestation is uploaded +// when artifact exists but attestation is missing (e.g., old build before SLSA) +func TestArtifactUploader_UploadsAttestationWhenMissing(t *testing.T) { + tmpDir := t.TempDir() + artifactPath := filepath.Join(tmpDir, "existing.tar.gz") + artifactContent := []byte("existing artifact content") + err := os.WriteFile(artifactPath, artifactContent, 0644) + require.NoError(t, err) + + mockCache := &mockRemoteCacheUpload{ + uploadedFiles: make(map[string][]byte), + uploadCalls: make(map[string]int), + } + + // Pre-populate the cache with artifact only (no attestation - simulating old build) + mockCache.uploadedFiles["existing.tar.gz"] = artifactContent + + uploader := NewArtifactUploader(mockCache) + attestation := []byte(`{"test":"attestation"}`) + + err = uploader.UploadArtifactWithAttestation(context.Background(), artifactPath, attestation) + require.NoError(t, err) + + // Verify that artifact was NOT re-uploaded + assert.Equal(t, 0, mockCache.uploadCalls["existing.tar.gz"], "Artifact should not be re-uploaded when it already exists") + + // CRITICAL: Verify that attestation WAS uploaded (missing attestation case) + assert.Equal(t, 1, mockCache.uploadCalls["existing.tar.gz.att"], "Attestation should be uploaded when missing") + assert.Contains(t, mockCache.uploadedFiles, "existing.tar.gz.att", "Attestation should be in cache") + assert.Equal(t, attestation, mockCache.uploadedFiles["existing.tar.gz.att"], "Attestation content should match") } // TestArtifactUploader_UploadsNewArtifacts tests that new artifacts are uploaded @@ -459,10 +494,13 @@ func TestArtifactUploader_SimulatesDownloadedArtifactWorkflow(t *testing.T) { uploadCalls: make(map[string]int), } - // Step 1: Simulate build job uploading artifact + // Step 1: Simulate build job uploading artifact and attestation buildArtifactContent := []byte("artifact built by build job") + buildAttestation := []byte(`{"build":"attestation"}`) mockCache.uploadedFiles["downloaded.tar.gz"] = buildArtifactContent + mockCache.uploadedFiles["downloaded.tar.gz.att"] = buildAttestation mockCache.uploadCalls["downloaded.tar.gz"] = 1 // Track that build job uploaded it + mockCache.uploadCalls["downloaded.tar.gz.att"] = 1 // Step 2: Simulate sign job downloading artifact (creates local file) downloadedArtifactPath := filepath.Join(tmpDir, "downloaded.tar.gz") @@ -481,12 +519,13 @@ func TestArtifactUploader_SimulatesDownloadedArtifactWorkflow(t *testing.T) { assert.Equal(t, 1, mockCache.uploadCalls["downloaded.tar.gz"], "Downloaded artifact should NOT be re-uploaded by sign job") - // CRITICAL: Attestation should NOT be uploaded - // This prevents overwriting the existing artifact's attestation - assert.Equal(t, 0, mockCache.uploadCalls["downloaded.tar.gz.att"], - "Attestation should NOT be uploaded when artifact already exists") - assert.NotContains(t, mockCache.uploadedFiles, "downloaded.tar.gz.att", - "Attestation should NOT be in cache") + // CRITICAL: Attestation should NOT be uploaded when both exist + assert.Equal(t, 1, mockCache.uploadCalls["downloaded.tar.gz.att"], + "Attestation should NOT be uploaded when both artifact and attestation exist") + + // Verify existing attestation is preserved + assert.Equal(t, buildAttestation, mockCache.uploadedFiles["downloaded.tar.gz.att"], + "Existing attestation should be preserved") // Artifact content should remain unchanged (not overwritten) assert.Equal(t, buildArtifactContent, mockCache.uploadedFiles["downloaded.tar.gz"], @@ -632,3 +671,53 @@ func (m *mockRemoteCacheUploadWithHasFileError) HasFile(ctx context.Context, key } return m.mockRemoteCacheUpload.HasFile(ctx, key) } + +// TestArtifactUploader_AttestationCheckError tests fallback behavior when attestation HasFile check fails +func TestArtifactUploader_AttestationCheckError(t *testing.T) { + tmpDir := t.TempDir() + artifactPath := filepath.Join(tmpDir, "test.tar.gz") + artifactContent := []byte("test content") + err := os.WriteFile(artifactPath, artifactContent, 0644) + require.NoError(t, err) + + // Create a mock where artifact exists but attestation check fails + mockCache := &mockRemoteCacheUploadWithSelectiveError{ + mockRemoteCacheUpload: mockRemoteCacheUpload{ + uploadedFiles: map[string][]byte{ + "test.tar.gz": artifactContent, // Artifact exists + }, + uploadCalls: make(map[string]int), + }, + errorKeys: map[string]error{ + "test.tar.gz.att": fmt.Errorf("S3 connection failed for attestation check"), + }, + } + + uploader := NewArtifactUploader(mockCache) + attestation := []byte(`{"test":"attestation"}`) + + // Should proceed with attestation upload despite HasFile error (safe fallback) + err = uploader.UploadArtifactWithAttestation(context.Background(), artifactPath, attestation) + require.NoError(t, err) + + // Artifact should not be re-uploaded + assert.Equal(t, 0, mockCache.uploadCalls["test.tar.gz"], + "Artifact should not be uploaded when it exists") + + // Attestation should be uploaded (fallback behavior assumes it doesn't exist) + assert.Equal(t, 1, mockCache.uploadCalls["test.tar.gz.att"], + "Attestation should be uploaded when HasFile check fails") +} + +// mockRemoteCacheUploadWithSelectiveError allows different errors for different keys +type mockRemoteCacheUploadWithSelectiveError struct { + mockRemoteCacheUpload + errorKeys map[string]error +} + +func (m *mockRemoteCacheUploadWithSelectiveError) HasFile(ctx context.Context, key string) (bool, error) { + if err, exists := m.errorKeys[key]; exists { + return false, err + } + return m.mockRemoteCacheUpload.HasFile(ctx, key) +}