diff --git a/pkg/leeway/signing/upload.go b/pkg/leeway/signing/upload.go index fe28976b..90d65e82 100644 --- a/pkg/leeway/signing/upload.go +++ b/pkg/leeway/signing/upload.go @@ -99,23 +99,58 @@ 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) + + // 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, - }).Info("Successfully uploaded artifact to remote cache") + "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("Successfully uploaded 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..2a1a21aa 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 when both artifact and attestation exist func TestArtifactUploader_SkipsExistingArtifacts(t *testing.T) { tmpDir := t.TempDir() artifactPath := filepath.Join(tmpDir, "existing.tar.gz") @@ -394,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"}`) @@ -406,8 +409,41 @@ 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") + // 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") } @@ -451,17 +487,20 @@ 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), 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") @@ -475,18 +514,18 @@ 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 + // CRITICAL: Attestation should NOT be uploaded when both exist 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") + "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"], @@ -532,3 +571,153 @@ 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) +} + +// 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) +}