Add unittest and fuzzing test for SBOM Conversion #13

wei-deepbits · 2023-09-07T23:02:05Z

This pull request focuses on implementing unit and fuzz tests for SBOM conversion.

For the unit tests, the following steps are performed:

Automatic retrieval of a shared SBOM dataset containing 10,494 SBOM files in SPDX and CycloneDX formats.
Utilizing the sbom-convert tool to convert SBOMs from one format to another.
Verification that the conversion process proceeds without errors.
Comparison of the counts of Package URLs (PURLs) between the original and converted SBOMs.
Comparison of the counts of licenses in the original and converted SBOMs.

The fuzzing tests involve generating new inputs through mutations and evaluating whether the binary encounters failures when exposed to specific corner cases.

jspeed-meyers · 2023-09-08T18:43:53Z

Cool! Do we have CI tests for this repo? I don't see anything running.

wei-deepbits · 2023-09-15T21:24:03Z

@puerco @veramine @manifestori , I would greatly appreciate it if you could take some time to review the changes I've made.

jspeed-meyers · 2023-09-15T21:29:19Z

@wei-deepbits, are you able to add these tests, or some subset of them, to the CI (i.e. put this in a GH action), so that these tests (or some subset of these tests) runs on every PR?

Or did you intentionally not add these tests to GH actions?

wei-deepbits · 2023-09-15T21:37:08Z

@jspeed-meyers , Okay, I will attempt to add them to the GitHub action. I believe I may not have the necessary permissions for the sbom-convert repository, but I will try it on my forked repository first.

jspeed-meyers · 2023-09-15T21:38:53Z

Sounds good. And I should be able to help with permissions, so let me know what you think you need!

jspeed-meyers · 2023-09-15T21:41:24Z

@wei-deepbits, I added you as a maintainer :)

wei-deepbits · 2023-09-15T21:42:03Z

Thanks. I'll let you know once I've figured it out.

wei-deepbits · 2023-09-18T22:57:24Z

Hi @jspeed-meyers , I have added unit tests and fuzzing tests to the GitHub Action for the current code. The current code of protobom is unable to pass both the unit tests and fuzzing tests due to some issues.

Regarding the unit tests, the current code of protobom cannot process all SPDX format SBOM files generated by Syft. It results in a runtime error: "panic: runtime error: invalid memory address or nil pointer dereference."

As for the fuzzing tests, the current code of protobom is unable to handle a simple automatically generated fuzzing input: "{"spdxVersion": "SPDX-2.3"}."

jspeed-meyers · 2023-09-19T00:11:09Z

@wei-deepbits, this is really helpful. Thank you!

@puerco, @manifestori, @houdini91, @veramine: Do any of you have time to diagnose these bugs? We should probably fix these bugs first and then later merge this PR.

Nice work, @wei-deepbits, on finding what appears to be bugs.

veramine · 2023-09-19T01:17:16Z

oh goodness! We should always gracefully fail on bad input, and not panic!

@wei-deepbits can you attach the SBOM files here that cause the panic? If I can reproduce, I will check in fixes!

wei-deepbits · 2023-09-19T06:00:34Z

@veramine , you can download those SBOM files here:
https://drive.google.com/file/d/1aDdDL2elbSFYjSZac8T3k4wruNzNVoTj/view?usp=sharing
https://drive.google.com/file/d/1MmQ0Modw6UQLvXa0aEAw7DtR-oLzC3w0/view?usp=sharing

Thanks.

veramine · 2023-09-19T16:41:58Z

Alright, thanks. protobom reader.ParseFile() worked fine on the first one but panic'd on the second due to a missing spdxDoc.CreationInfo. I added a check before attempting to use it. protobom/protobom#108

Here's the protobom from the first one. Did that error for you? Or did only the second one error?

{
	"metadata": {
		"id": "DOCUMENT",
		"version": "0",
		"name": "/home/wei/code/repos/python/abhiTronix/vidgear",
		"date": {
			"seconds": 1689780012
		},
		"tools": [
			{
				"name": "syft-0.85.0"
			}
		],
		"authors": [
			{
				"name": "Anchore, Inc",
				"is_org": true
			}
		]
	},
	"node_list": {
		"nodes": [
			{
				"id": "Package-python-pyzmq-d345c09cbb0c1e23",
				"name": "pyzmq",
				"version": "24.0.1",
				"url_download": "NOASSERTION",
				"copyright": "NOASSERTION",
				"source_info": "acquired package info from installed python package manifest file: /setup.py",
				"identifiers": {
					"1": "pkg:pypi/[email protected]",
					"3": "cpe:2.3:a:pyzmq:pyzmq:24.0.1:*:*:*:*:*:*:*"
				}
			},
			{
				"id": "File-setup.py-c829033125510d5a",
				"type": 1,
				"name": "/setup.py",
				"license_concluded": "NOASSERTION",
				"hashes": {
					"2": "0000000000000000000000000000000000000000"
				}
			}
		],
		"edges": [
			{
				"type": 30,
				"from": "Package-python-pyzmq-d345c09cbb0c1e23",
				"to": [
					"File-setup.py-c829033125510d5a"
				]
			}
		],
		"root_elements": [
			"DOCUMENT"
		]
	}
}

wei-deepbits · 2023-09-19T17:20:07Z

@veramine The first one also exhibits the panic error in my environment. Here is the log for reference:

wei@black:~/code/online/remote/sbom-convert$ dist/sbom-convert_linux_amd64_v1/sbom-convert /home/wei/Downloads/abhiTronix_vidgear_syft_spdx.json
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x70 pc=0x86bbe8]

goroutine 1 [running]:
github.com/bom-squad/protobom/pkg/native/serializers.(*SerializerCDX).Serialize(0xc00006e918?, {{0xde3660?, 0xc0001472c0?}, 0xc0001b5880?}, 0xc00020a2c0)
	github.com/bom-squad/[email protected]/pkg/native/serializers/serializer_cdx.go:83 +0x588
github.com/bom-squad/protobom/pkg/writer.(*defaultWriterImplementation).SerializeSBOM(0xc00002e900?, {{0x97faec?, 0x2a?}, 0xc0000106d0?}, {0xa372e0, 0xde3660}, 0xc0001fba20?, {0xa36dc0?, 0xc00006e030})
	github.com/bom-squad/[email protected]/pkg/writer/implementation.go:59 +0x5f
github.com/bom-squad/protobom/pkg/writer.(*Writer).WriteStream(0xc0001fba60, 0xc00020a2c0, {0xa36dc0, 0xc00006e030})
	github.com/bom-squad/[email protected]/pkg/writer/writer.go:40 +0xd5
github.com/bom-squad/go-cli/pkg/convert.(*Service).Convert(0xc00006e4f0, {0x0?, 0xc0001e20a0?}, {0xa37ba0?, 0xc00006e4e8}, {0xa36dc0, 0xc00006e030})
	github.com/bom-squad/go-cli/pkg/convert/convert.go:42 +0x11f
github.com/bom-squad/go-cli/cmd/cli.runConvert({0xa38a80, 0xde3660}, 0xc0001ca180, {0xc000039f10, 0x1, 0x0?})
	github.com/bom-squad/go-cli/cmd/cli/convert.go:96 +0x30e
github.com/bom-squad/go-cli/cmd/cli.ConvertCommand.func1(0xc000004600?, {0xc000039f10?, 0x4?, 0x969dba?})
	github.com/bom-squad/go-cli/cmd/cli/convert.go:41 +0x3c
github.com/spf13/cobra.(*Command).execute(0xc000004600, {0xc000039ee0, 0x1, 0x1})
	github.com/spf13/[email protected]/command.go:940 +0x87c
github.com/spf13/cobra.(*Command).ExecuteC(0xc000004300)
	github.com/spf13/[email protected]/command.go:1068 +0x3a5
github.com/spf13/cobra.(*Command).Execute(...)
	github.com/spf13/[email protected]/command.go:992
github.com/spf13/cobra.(*Command).ExecuteContext(...)
	github.com/spf13/[email protected]/command.go:985
github.com/bom-squad/go-cli/cmd/cli.Execute()
	github.com/bom-squad/go-cli/cmd/cli/main.go:36 +0x68
main.main()
	github.com/bom-squad/go-cli/main.go:8 +0xf

veramine · 2023-09-19T17:23:42Z

Aha, that's in the serializing code path. I only tested unserializing. Okay I'll look, thanks!

veramine · 2023-09-19T20:08:01Z

The CDX serializer was unexpectedly not including a root component in this test case. I added that check with protobom/protobom#109

% ./go-cli 2.spdx.json                         
serializing sbom: serializing SBOM to native format: Could not find SBOM root component

wei-deepbits · 2023-09-19T20:17:42Z

@veramine , I appreciate your help in resolving these issues.

veramine · 2023-09-19T20:19:32Z

No problem, thanks for fuzzing!

wei-deepbits · 2023-09-19T20:58:04Z

@veramine , Using a CycloneDX format SBOM as the seed input for fuzzing can lead to the generation of a simple json:

{
    "bomFormat":"CycloneDX",
    "specVersion":"1.4",
    "":[]
}

that triggers a panic in unserializer_cdx14.go:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x28 pc=0x852617]

goroutine 1 [running]:
github.com/bom-squad/protobom/pkg/native/unserializers.(*UnserializerCDX14).ParseStream(0x8e1d60?, 0xc000171890?, {0xa34a00?, 0xc0001184a8})
        github.com/bom-squad/[email protected]/pkg/native/unserializers/unserializer_cdx14.go:40 +0x297
github.com/bom-squad/protobom/pkg/reader.(*defaultParserImplementation).ParseStream(0xc000204101?, {0xa36060?, 0xde3660?}, 0x2a?, {0xa34a00?, 0xc0001184a8?})
        github.com/bom-squad/[email protected]/pkg/reader/implementation.go:77 +0x38
github.com/bom-squad/protobom/pkg/reader.(*Reader).ParseStream(0xc000134630, {0x7f30a85f11e0?, 0xc0001184a8})
        github.com/bom-squad/[email protected]/pkg/reader/reader.go:52 +0x167
github.com/bom-squad/go-cli/pkg/convert.(*Service).Convert(0xc0001184b0, {0x0?, 0xc00012ffd0?}, {0xa37ba0?, 0xc0001184a8}, {0xa36dc0, 0xc00006e030})
        github.com/bom-squad/go-cli/pkg/convert/convert.go:34 +0x89
github.com/bom-squad/go-cli/cmd/cli.runConvert({0xa38a80, 0xde3660}, 0xc000171f20, {0xc00012fe40, 0x1, 0x0?})
        github.com/bom-squad/go-cli/cmd/cli/convert.go:96 +0x30e
github.com/bom-squad/go-cli/cmd/cli.ConvertCommand.func1(0xc0001f0300?, {0xc00012fe40?, 0x4?, 0x969dba?})
        github.com/bom-squad/go-cli/cmd/cli/convert.go:41 +0x3c
github.com/spf13/cobra.(*Command).execute(0xc0001f0300, {0xc00012fe10, 0x1, 0x1})
        github.com/spf13/[email protected]/command.go:940 +0x87c
github.com/spf13/cobra.(*Command).ExecuteC(0xc0001f0000)
        github.com/spf13/[email protected]/command.go:1068 +0x3a5
github.com/spf13/cobra.(*Command).Execute(...)
        github.com/spf13/[email protected]/command.go:992
github.com/spf13/cobra.(*Command).ExecuteContext(...)
        github.com/spf13/[email protected]/command.go:985
github.com/bom-squad/go-cli/cmd/cli.Execute()
        github.com/bom-squad/go-cli/cmd/cli/main.go:36 +0x68
main.main()
        github.com/bom-squad/go-cli/main.go:8 +0xf

Perhaps you could address this issue as well. Thanks!

veramine · 2023-09-19T21:20:38Z

Yep, added two more checks. protobom/protobom#110

wei-deepbits · 2023-09-26T22:42:53Z

Hey @veramine , In pull request protobom/protobom#109, you've implemented a check to return an error if the native serializer doesn't return a valid root node. However, it's worth noting that the native serializer is unable to handle all SPDX format SBOM files generated by Syft. In light of this limitation, I suggest that we should work on finding a way to handle these files correctly, instead of simply returning an error.

wei-deepbits · 2023-10-12T22:32:57Z

Hi @veramine , what are your thoughts on this?

veramine · 2023-10-12T22:46:04Z

Ahhh let me check with @puerco about what to do with an sbom with no root. Could you attach a valid SBOM here that returns an error?

veramine · 2023-10-12T22:53:44Z

I tried the /home/wei/code/repos/python/abhiTronix/vidgear syft-generated SBOM and protobom was able to ingest that ok. But attach some specific troublesome examples here and I'll see if we can find a way to handle them. Thanks!

wei-deepbits · 2023-10-12T23:36:09Z

@veramine , A Syft-generated SPDX SBOM won't trigger an error; instead, it will result in a "serializing SBOM to native format: No SBOM root component" message, halting the conversion to CycloneDX format. It renders our tool unusable for it purposes.

veramine · 2023-10-12T23:39:00Z

Aha, I see that.

veramine · 2023-10-12T23:44:06Z

In that sample, there is a root node, it's below.. why do we think we don't have a root node, I wonder.. Investigating..

{
"spdxVersion": "SPDX-2.3",
"dataLicense": "CC0-1.0",
"SPDXID": "SPDXRef-DOCUMENT",
"name": "/home/wei/code/repos/python/abhiTronix/vidgear",
"documentNamespace": "https://anchore.com/syft/dir/home/wei/code/repos/python/abhiTronix/vidgear-710e3edc-b86e-4c5a-b71a-2f81c876b318",
"creationInfo": {
"licenseListVersion": "3.21",
"creators": [
"Organization: Anchore, Inc",
"Tool: syft-0.85.0"
],
"created": "2023-07-19T15:20:12Z"
}

veramine · 2023-10-13T02:31:29Z

We need to find a root node in this situation. I filed a protobom issue to look at it closer: protobom/protobom#126

Signed-off-by: Wei Song <[email protected]>

2) add parameters for tests Signed-off-by: Wei Song <[email protected]>

Signed-off-by: Wei Song <[email protected]>

wei-deepbits · 2023-11-22T03:38:29Z

@veramine , could you please take a look at this pull request? Thanks.

veramine · 2023-12-01T22:05:35Z

pkg/convert/convert_unit_test.go

+		return
+	}
+
+	SBOM_CONVERT_PATH := "../../dist/sbom-convert_linux_amd64_v1/sbom-convert"


I got stuck here momentarily as I was testing on mac/darwin but figured it out.

veramine

This is fine with me. I was able to replicate unittest and fuzztest locally. I do wonder if unittest might be more suited as a conformance test on protobom directly - you might get more from your effort by joining forces with @puerco on that effort. But if you want to continue to use this repo for testing, that's fine with me. I'll approve this. I'm not sure if we are ready for a deluge of fuzzing related bugs yet 😳 but I don't want to hold this up any longer than I already have!! Thanks for the contribution!

wei-deepbits · 2023-12-01T22:18:21Z

@veramine , I will add platform detection for the path of sbom-convert later and investigate whether we should test protobom directly. Thank you for your review.

manifestori · 2023-12-20T16:30:57Z

Hey sorry for being out for awhile, any updates @wei-deepbits?

adefee · 2023-12-20T16:48:27Z

pkg/convert/convert_unit_test.go

+}
+
+func DownloadSBOMs() {
+	fileURL := "https://drive.usercontent.google.com/download?id=1LgGlq3g_H02mhzkc94cUd0zzxy0JhFim&export=download&authuser=0&confirm=t&uuid=483eac07-f1af-4356-abeb-4ba254e32b86&at=APZUnTWjSNLUgCQ8wwFZjsLS7Y36:1694113089657"


This is scary. Who controls this content? Can it be changed without the end user's knowledge? I don't see a checksum, etc for us to use to know the target is what we expect?

manifestori · 2023-12-20T16:46:36Z

pkg/convert/convert_unit_test.go

Hi @wei-deepbits, I noticed that you are downloading a 100mb SBOM, extracting, converting and drilling into the results. Is this supposed to be a unit test for the convert package?

I appreciate the effort you put in, but I'm afraid this doesn't qualify as a unit test. In unit tests, we test the functionality of the package itself, including the functions and methods, the constructor with happy/sad results, and cover as many cases as possible.

I argue that this is more of an e2e test, but unfortunately, the framework is not there yet. We should test the code state and not the binary we generated. Therefore, we should test the main entry point of the application, or at least the CLI.

Furthermore, we cannot download an external file from Google Drive with fixtures. Please commit a subset of test cases into this repository so we can know those files are secure. Put them under testdata folder, this way go compiler won't read them.

When comparing fixtures to results, you can use snapshots or gocmp with filters. Complex testing code is hard to maintain. Those tools use reflection to compare structs, making most of the comparison functionality obsolete.

I do see the value of counting statistics. How about opening a PR that adds those statistics to the conversion results output? maybe a subcommand or a flag sbom-convert convert ... --show-diff? or sbom-convert convert --benchmark ?
We've discussed those a few times, of a way to show "what is data is missing from the original sbom"

Hi @manifestori , appreciate your valuable suggestions. I will try to fix them accordingly.

manifestori · 2023-12-20T16:58:32Z

pkg/convert/convert_fuzzing_test.go

+	f.Add(string(content))
+
+	f.Fuzz(func(t *testing.T, orig string) {
+		ParseStreamWrapper(orig)


So this is basically the test?

manifestori · 2023-12-20T16:59:45Z

pkg/convert/convert_fuzzing_test.go

I like that, Looking neat. but what are we testing here? can you explain more how it works?

This is just the first step to do the fuzzing tests for our tool. It generates new inputs for the ParseStream() function in protobom and verifies whether it can cause a crash. It helped to find errors in unserializing. we can add more functions for testing.

github-actions · 2024-02-04T02:07:50Z

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

github-actions · 2024-02-15T02:03:50Z

This PR was closed because it has been stalled for 10 days with no activity.

jspeed-meyers requested review from manifestori, puerco and veramine September 8, 2023 00:05

jspeed-meyers assigned wei-deepbits Sep 8, 2023

jspeed-meyers added the enhancement New feature or request label Sep 8, 2023

veramine mentioned this pull request Sep 19, 2023

Ignore CreationInfo if not present protobom/protobom#108

Merged

veramine mentioned this pull request Sep 19, 2023

Return error if native serializer does not return valid root node protobom/protobom#109

Merged

veramine mentioned this pull request Sep 19, 2023

Do not try to use CDX Metadata or Components if nil protobom/protobom#110

Closed

veramine mentioned this pull request Oct 13, 2023

Allow serialization of SBOM with no rootComponent protobom/protobom#128

Merged

wei-deepbits added 9 commits November 21, 2023 19:28

Add unittest and fuzzing test for SBOM Conversion

6fc636c

Signed-off-by: Wei Song <[email protected]>

fix typo

06eda93

Signed-off-by: Wei Song <[email protected]>

Fix fuzzing seed input path error; Change function names

1fecbda

Signed-off-by: Wei Song <[email protected]>

1) add CI tests using GitHub Actions

529cc9f

2) add parameters for tests Signed-off-by: Wei Song <[email protected]>

fix errors in tests.yml

ac3e8ce

Signed-off-by: Wei Song <[email protected]>

convert SBOM to a file, instead of stdout

d95eb3b

Signed-off-by: Wei Song <[email protected]>

use relative path

a437edf

Signed-off-by: Wei Song <[email protected]>

disable fuzzing in CI

4d4bc5f

Signed-off-by: Wei Song <[email protected]>

disable fuzzing in CI

ec66c1c

Signed-off-by: Wei Song <[email protected]>

wei-deepbits force-pushed the unittest_fuzztest branch from fd9e6d8 to ec66c1c Compare November 22, 2023 03:28

enable Fuzzing in Makefile

0ff26da

Signed-off-by: Wei Song <[email protected]>

veramine reviewed Dec 1, 2023

View reviewed changes

veramine approved these changes Dec 1, 2023

View reviewed changes

adefee reviewed Dec 20, 2023

View reviewed changes

manifestori requested changes Dec 20, 2023

View reviewed changes

github-actions bot added the Stale label Feb 4, 2024

github-actions bot closed this Feb 15, 2024

Add unittest and fuzzing test for SBOM Conversion #13

Add unittest and fuzzing test for SBOM Conversion #13

Conversation

wei-deepbits commented Sep 7, 2023

jspeed-meyers commented Sep 8, 2023

wei-deepbits commented Sep 15, 2023

jspeed-meyers commented Sep 15, 2023

wei-deepbits commented Sep 15, 2023

jspeed-meyers commented Sep 15, 2023

jspeed-meyers commented Sep 15, 2023

wei-deepbits commented Sep 15, 2023

wei-deepbits commented Sep 18, 2023

jspeed-meyers commented Sep 19, 2023

veramine commented Sep 19, 2023

wei-deepbits commented Sep 19, 2023

veramine commented Sep 19, 2023 • edited Loading

wei-deepbits commented Sep 19, 2023

veramine commented Sep 19, 2023

veramine commented Sep 19, 2023

wei-deepbits commented Sep 19, 2023

veramine commented Sep 19, 2023

wei-deepbits commented Sep 19, 2023 • edited Loading

veramine commented Sep 19, 2023 • edited Loading

wei-deepbits commented Sep 26, 2023

wei-deepbits commented Oct 12, 2023

veramine commented Oct 12, 2023

veramine commented Oct 12, 2023 • edited Loading

wei-deepbits commented Oct 12, 2023

veramine commented Oct 12, 2023

veramine commented Oct 12, 2023

veramine commented Oct 13, 2023

wei-deepbits commented Nov 22, 2023

veramine Dec 1, 2023 • edited Loading

Choose a reason for hiding this comment

veramine left a comment

Choose a reason for hiding this comment

wei-deepbits commented Dec 1, 2023

manifestori commented Dec 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 4, 2024

github-actions bot commented Feb 15, 2024

veramine commented Sep 19, 2023 •

edited

Loading

wei-deepbits commented Sep 19, 2023 •

edited

Loading

veramine commented Sep 19, 2023 •

edited

Loading

veramine commented Oct 12, 2023 •

edited

Loading

veramine Dec 1, 2023 •

edited

Loading