fix(signing): skip attestation upload when artifact exists

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)
 	}

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)
+}