Fix SBOM pushing in ML merge workflow and remove field from SBOM

.github/workflows/tox-tests.yml

@@ -101,7 +101,8 @@ jobs:
         run: tox -e py311
       - name: Upload coverage to Codecov
         uses: codecov/[email protected]
-        env: CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}
+        env:
+          CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}
         with:
           fail_ci_if_error: true
           verbose: true

pubtools/_quay/security_manifest_pusher.py

@@ -312,6 +312,34 @@ def get_destination_repos(self, item: Any) -> List[str]:
 
         return list(set(dest_repos))
 
+    def security_manifest_remove_incompleteness_reasons(self, security_manifest_path: str) -> str:
+        """
+        Remove the field "incompleteness_reasons" from the security manifest.
+
+        The field is for internal use only, and isn't a part of the CycloneDX spec.
+
+        Args:
+            security_manifest_path (str):
+                Path to the extracted security manifest.
+
+        Returns (str):
+            Path to a file containing the modified security manifest.
+        """
+        with open(security_manifest_path, "r") as f1:
+            security_manifest = json.load(f1)
+
+        if "incompleteness_reasons" in security_manifest:
+            del security_manifest["incompleteness_reasons"]
+
+        modified_security_manifest_path = os.path.join(
+            os.path.dirname(security_manifest_path),
+            f"sanitized_security_manifest_{uuid.uuid4().hex}.json",
+        )
+        with open(modified_security_manifest_path, "w") as f2:
+            json.dump(security_manifest, f2, indent=4)
+
+        return modified_security_manifest_path
+
     def security_manifest_add_products(
         self, security_manifest_path: str, products: Set[str]
     ) -> str:
@@ -428,16 +456,19 @@ def merge_and_push_security_manifest(
                 self.delete_existing_attestation(image_ref, dir_path)
                 products = products | existing_products
 
+            sanitized_security_manifest_path = self.security_manifest_remove_incompleteness_reasons(
+                image_manifest.security_manifest_path
+            )
             if products:
                 full_security_manifest_path = self.security_manifest_add_products(
-                    image_manifest.security_manifest_path, products
+                    sanitized_security_manifest_path, products
                 )
             else:
                 LOG.warning(
                     f"Push item {item} doesn't contain a product name. A new attestation "
                     "will be created without this information."
                 )
-                full_security_manifest_path = image_manifest.security_manifest_path
+                full_security_manifest_path = sanitized_security_manifest_path
 
             self.cosign_attest_security_manifest(
                 full_security_manifest_path,
@@ -501,10 +532,11 @@ def push_manifest_list_security_manifests(self, item: Any, dir_path: str) -> Non
                         self.target_settings.get("cosign_sbom_skip_verify_rekor", False),
                     )
                     if not arch_attestation_exist:
-                        raise ValueError(
+                        LOG.warning(
                             f"Arch image {arch_ref} that is a part of {dest_ref} "
                             "doesn't have an attestation"
                         )
+                        continue
                     tag_attestations.append(attestation_file)
 
                 attestation_file = os.path.join(dir_path, f"attestation_{uuid.uuid4().hex}.json")

tests/test_security_manifest_pusher.py

@@ -399,6 +399,26 @@ def test_get_destination_repos(target_settings, container_multiarch_push_item):
     assert dest_repos == ["quay.io/some-namespace/target----repo"]
 
 
+@mock.patch("uuid.uuid4")
+@mock.patch("json.dump")
+@mock.patch("json.load")
+@mock.patch("pubtools._quay.security_manifest_pusher.open", mock.mock_open())
+def test_security_manifest_remove_incompleteness_reasons(
+    mock_load, mock_dump, mock_uuid, target_settings, container_multiarch_push_item
+):
+    pusher = security_manifest_pusher.SecurityManifestPusher(
+        [container_multiarch_push_item], target_settings
+    )
+    mock_load.return_value = {"specVersion": "1.4", "incompleteness_reasons": []}
+    mock_uuid.return_value.hex = "abcd"
+
+    res = pusher.security_manifest_remove_incompleteness_reasons(__file__)
+    assert res == os.path.join(os.path.dirname(__file__), "sanitized_security_manifest_abcd.json")
+    mock_load.assert_called_once()
+    mock_dump.assert_called_once()
+    assert mock_dump.call_args_list[0][0][0] == {"specVersion": "1.4"}
+
+
 @mock.patch("uuid.uuid4")
 @mock.patch("json.dump")
 @mock.patch("json.load")
@@ -451,6 +471,10 @@ def test_delete_existing_attestation(
 @mock.patch(
     "pubtools._quay.security_manifest_pusher.SecurityManifestPusher.security_manifest_add_products"
 )
+@mock.patch(
+    "pubtools._quay.security_manifest_pusher.SecurityManifestPusher"
+    ".security_manifest_remove_incompleteness_reasons"
+)
 @mock.patch(
     "pubtools._quay.security_manifest_pusher.SecurityManifestPusher."
     "get_security_manifest_from_attestation"
@@ -463,6 +487,7 @@ def test_merge_and_push_security_manifest(
     mock_uuid,
     mock_get_attestation,
     mock_get_manifest,
+    mock_remove_incompleteness,
     mock_add_products,
     mock_attest,
     mock_delete_attestation,
@@ -479,6 +504,7 @@ def test_merge_and_push_security_manifest(
     mock_get_attestation.return_value = True
 
     mock_get_manifest.return_value = security_manifest
+    mock_remove_incompleteness.return_value = "/path/to/sanitized/manifest.json"
 
     mock_uuid.return_value.hex = "abcd"
     mock_add_products.return_value = "/path/to/final/manifest.json"
@@ -494,8 +520,9 @@ def test_merge_and_push_security_manifest(
         False,
     )
     mock_get_manifest.assert_called_once_with("/temp/temp_path/attestation_abcd.json")
+    mock_remove_incompleteness.assert_called_once_with("path/to/manifest")
     mock_add_products.assert_called_once_with(
-        "path/to/manifest", {"new-product", "product1", "product2"}
+        "/path/to/sanitized/manifest.json", {"new-product", "product1", "product2"}
     )
     mock_delete_attestation.assert_called_once_with("quay.io/org/repo@abcdef", "/temp/temp_path")
     mock_attest.assert_called_once_with(
@@ -512,6 +539,10 @@ def test_merge_and_push_security_manifest(
 @mock.patch(
     "pubtools._quay.security_manifest_pusher.SecurityManifestPusher.security_manifest_add_products"
 )
+@mock.patch(
+    "pubtools._quay.security_manifest_pusher.SecurityManifestPusher"
+    ".security_manifest_remove_incompleteness_reasons"
+)
 @mock.patch(
     "pubtools._quay.security_manifest_pusher.SecurityManifestPusher."
     "get_security_manifest_from_attestation"
@@ -524,6 +555,7 @@ def test_merge_and_push_security_manifest_no_existing_attestation_also_disable_r
     mock_uuid,
     mock_get_attestation,
     mock_get_manifest,
+    mock_remove_incompleteness,
     mock_add_products,
     mock_attest,
     mock_delete_attestation,
@@ -542,6 +574,7 @@ def test_merge_and_push_security_manifest_no_existing_attestation_also_disable_r
     mock_get_attestation.return_value = False
 
     mock_get_manifest.return_value = security_manifest
+    mock_remove_incompleteness.return_value = "/path/to/sanitized/manifest.json"
 
     mock_uuid.return_value.hex = "abcd"
     mock_add_products.return_value = "/path/to/final/manifest.json"
@@ -558,7 +591,8 @@ def test_merge_and_push_security_manifest_no_existing_attestation_also_disable_r
     )
     mock_get_manifest.assert_not_called()
     mock_delete_attestation.assert_not_called()
-    mock_add_products.assert_called_once_with("path/to/manifest", {"new-product"})
+    mock_remove_incompleteness.assert_called_once_with("path/to/manifest")
+    mock_add_products.assert_called_once_with("/path/to/sanitized/manifest.json", {"new-product"})
     mock_attest.assert_called_once_with(
         "/path/to/final/manifest.json", "quay.io/org/repo@abcdef", "https://some-rekor.com", True
     )
@@ -573,6 +607,10 @@ def test_merge_and_push_security_manifest_no_existing_attestation_also_disable_r
 @mock.patch(
     "pubtools._quay.security_manifest_pusher.SecurityManifestPusher.security_manifest_add_products"
 )
+@mock.patch(
+    "pubtools._quay.security_manifest_pusher.SecurityManifestPusher"
+    ".security_manifest_remove_incompleteness_reasons"
+)
 @mock.patch(
     "pubtools._quay.security_manifest_pusher.SecurityManifestPusher."
     "get_security_manifest_from_attestation"
@@ -585,6 +623,7 @@ def test_merge_and_push_security_manifest_already_pushed(
     mock_uuid,
     mock_get_attestation,
     mock_get_manifest,
+    mock_remove_incompleteness,
     mock_add_products,
     mock_attest,
     mock_delete_attestation,
@@ -602,6 +641,7 @@ def test_merge_and_push_security_manifest_already_pushed(
     mock_get_attestation.return_value = True
 
     mock_get_manifest.return_value = security_manifest
+    mock_remove_incompleteness.return_value = "/path/to/sanitized/manifest.json"
 
     mock_uuid.return_value.hex = "abcd"
     mock_add_products.return_value = "/path/to/final/manifest.json"
@@ -618,6 +658,7 @@ def test_merge_and_push_security_manifest_already_pushed(
     )
     mock_get_manifest.assert_called_once_with("/temp/temp_path/attestation_abcd.json")
     mock_delete_attestation.assert_not_called()
+    mock_remove_incompleteness.assert_not_called()
     mock_add_products.assert_not_called()
     mock_attest.assert_not_called()
 
@@ -631,6 +672,10 @@ def test_merge_and_push_security_manifest_already_pushed(
 @mock.patch(
     "pubtools._quay.security_manifest_pusher.SecurityManifestPusher.security_manifest_add_products"
 )
+@mock.patch(
+    "pubtools._quay.security_manifest_pusher.SecurityManifestPusher"
+    ".security_manifest_remove_incompleteness_reasons"
+)
 @mock.patch(
     "pubtools._quay.security_manifest_pusher.SecurityManifestPusher."
     "get_security_manifest_from_attestation"
@@ -643,6 +688,7 @@ def test_merge_and_push_security_manifest_no_product_existing_attestation(
     mock_uuid,
     mock_get_attestation,
     mock_get_manifest,
+    mock_remove_incompleteness,
     mock_add_products,
     mock_attest,
     mock_delete_attestation,
@@ -658,6 +704,7 @@ def test_merge_and_push_security_manifest_no_product_existing_attestation(
 
     digest_manifest = security_manifest_pusher.DigestSecurityManifest("abcdef", "path/to/manifest")
     mock_get_attestation.return_value = True
+    mock_remove_incompleteness.return_value = "/path/to/sanitized/manifest.json"
 
     mock_get_manifest.return_value = security_manifest
 
@@ -677,6 +724,7 @@ def test_merge_and_push_security_manifest_no_product_existing_attestation(
     mock_get_manifest.assert_called_once_with("/temp/temp_path/attestation_abcd.json")
     mock_add_products.assert_not_called()
     mock_delete_attestation.assert_not_called()
+    mock_remove_incompleteness.assert_not_called()
     mock_attest.assert_not_called()
 
 
@@ -689,6 +737,10 @@ def test_merge_and_push_security_manifest_no_product_existing_attestation(
 @mock.patch(
     "pubtools._quay.security_manifest_pusher.SecurityManifestPusher.security_manifest_add_products"
 )
+@mock.patch(
+    "pubtools._quay.security_manifest_pusher.SecurityManifestPusher"
+    ".security_manifest_remove_incompleteness_reasons"
+)
 @mock.patch(
     "pubtools._quay.security_manifest_pusher.SecurityManifestPusher."
     "get_security_manifest_from_attestation"
@@ -701,6 +753,7 @@ def test_merge_and_push_security_manifest_no_product_no_existing_attestation(
     mock_uuid,
     mock_get_attestation,
     mock_get_manifest,
+    mock_remove_incompleteness,
     mock_add_products,
     mock_attest,
     mock_delete_attestation,
@@ -718,6 +771,7 @@ def test_merge_and_push_security_manifest_no_product_no_existing_attestation(
     mock_get_attestation.return_value = False
 
     mock_get_manifest.return_value = security_manifest
+    mock_remove_incompleteness.return_value = "/path/to/sanitized/manifest.json"
 
     mock_uuid.return_value.hex = "abcd"
     mock_add_products.return_value = "/path/to/final/manifest.json"
@@ -734,9 +788,13 @@ def test_merge_and_push_security_manifest_no_product_no_existing_attestation(
     )
     mock_get_manifest.assert_not_called()
     mock_add_products.assert_not_called()
+    mock_remove_incompleteness.assert_called_once_with("path/to/manifest")
     mock_delete_attestation.assert_not_called()
     mock_attest.assert_called_once_with(
-        "path/to/manifest", "quay.io/org/repo@abcdef", "https://some-rekor.com", False
+        "/path/to/sanitized/manifest.json",
+        "quay.io/org/repo@abcdef",
+        "https://some-rekor.com",
+        False,
     )
 
 
@@ -862,16 +920,15 @@ def test_push_manifest_list_security_manifests_arch_att_not_exist(
     pusher = security_manifest_pusher.SecurityManifestPusher(
         [container_multiarch_push_item], target_settings
     )
-    mock_get_attestation.side_effect = [True, True, False]
+    mock_get_attestation.side_effect = [True, True, False, True, True, True]
     mock_quay_client.return_value.get_manifest_digest.return_value = "sha256:abcdef"
     mock_quay_client.return_value.get_manifest.return_value = manifest_list_data
     mock_uuid.return_value.hex = "abcd"
 
-    with pytest.raises(ValueError, match=".*doesn't have an attestation.*"):
-        pusher.push_manifest_list_security_manifests(
-            container_multiarch_push_item,
-            "/some",
-        )
+    pusher.push_manifest_list_security_manifests(
+        container_multiarch_push_item,
+        "/some",
+    )
 
     mock_quay_client.return_value.get_manifest_digest.assert_called_once_with(
         "quay.io/some-namespace/target----repo:latest-test-tag",
@@ -882,9 +939,11 @@ def test_push_manifest_list_security_manifests_arch_att_not_exist(
         media_type=mock_quay_client.return_value.MANIFEST_LIST_TYPE,
     )
 
-    assert mock_get_attestation.call_count == 3
-    mock_delete_attestation.assert_not_called()
-    mock_attest.assert_not_called()
+    assert mock_get_attestation.call_count == 6
+    mock_delete_attestation.assert_called_once_with(
+        "quay.io/some-namespace/target----repo@sha256:abcdef", "/some"
+    )
+    assert mock_attest.call_count == 4
 
 
 @mock.patch(