Fix intake of SBOM from JSON #241

luhring · 2021-03-02T02:07:29Z

This PR:

fixes Can't use syft JSON output as input #235
decomposes Syft provider logic into discrete tasks

Notes:

This PR also adds the beginnings of "CLI-level" tests — to ensure that the built binary behaves correctly from a CLI context, such as the handling of data piped to the command's STDIN.

What's done so far:

introduced a new "cli" make target to run these tests
introduces tests to cover the cases listed in Can't use syft JSON output as input #235 (with the exception of ~ home directory processing)
target snapshot builds for CLI tests
included these tests in the CI pipeline

What's left for consideration:

factoring out generalized CLI test helpers to a new directory or project

alfredodeza · 2021-03-02T21:42:14Z

test/cli/sbom_input_test.go

+		path string
+	}{
+		{
+			"absolute path",


I wasn't aware that you could define structs without their field names. When is this style preferred?

I don't think I have a good answer to that question offhand. In this case, I had two reasons for this syntax, 1) the field names appeared immediately before the data (in the struct type literal), and 2) in my editor (GoLand), the field names actually do appear as hints next to the field values.

Having said all that, I don't mind changing to include field names 😃

alfredodeza · 2021-03-02T21:44:44Z

grype/pkg/syft_json_provider.go

@@ -160,10 +185,6 @@ func (p *partialSyftPackage) UnmarshalJSON(b []byte) error {
 			PomGroupID:    group,
 			ManifestName:  name,
 		}
-	case "":


was this part of the issue as well?

Yes — I forget the exact execution path that was causing trouble, but I can dig that up if you'd like

alfredodeza · 2021-03-02T21:46:14Z

grype/pkg/syft_json_provider.go

+}
+
+func openSbom(path string) (*os.File, error) {
+	expandedPath, err := homedir.Expand(path)


more of an open-ended question, but how does this expand when using sudo? is it acceptable to expand to /root instead of /home/myusername ?

Interesting question... I would guess so. The homedir library tries its best to rely on the OS's own behavior for "home directory" lookup. (E.g. see here -> https://github.com/mitchellh/go-homedir/blob/af06845cf3004701891bf4fdb884bfe4920b3727/homedir.go#L89)

This can be tricky in system calls using shell and interpreting back into code (I've run into issues like this in Python). Just tried it out with a quick test and it checks out. Thanks for the link the source code!

alfredodeza · 2021-03-02T21:47:00Z

grype/pkg/syft_json_provider.go

+func openSbom(path string) (*os.File, error) {
+	expandedPath, err := homedir.Expand(path)
+	if err != nil {
+		return nil, fmt.Errorf("unable to open SBOM: %w", err)


I think this error should be specific to expanding the path, not to open the SBOM

I can make that change. 👍

This is one area I think it'd be good to seek alignment on among the group. Errors are interesting... (especially in Go). If function A calls function B, and thus has to handle getting an error back from function B, which function reports that there was a problem with function B's responsibility? Lately, I've been preferring function B to do this, since that consolidates the message to function B, and any number of callers of function B can rely on this error message rather than have them all say something like "there was a problem with function B" in their own function bodies.

In this specific case, if you look into the code for the function homedir.Expand(path), you'll notice it returns errors like "cannot expand user-specific home dir". So in my function, openSbom, rather than fmt.Errorf("error expanding the home dir in the path": %w", err), which would produce:

ERROR: error expanding the home dir in the path: cannot expand user-specific home dir

I decided to have my error reflect the failure in my function's job, which is to open an SBOM.

Again, happy to change it, but just wanted to share those thoughts!

That explanation makes sense to me, and I agree with all your points. The caller of this function mentions something similar which seemed repetitive:

return nil, fmt.Errorf("unable to use specified SBOM: %w", err)

Re-using a bit your example, that would mean it would produce this error:

ERROR: unable to use specified SBOM: unable to open SBOM: cannot expand user-specific home dir

This reads somewhat repetitive. I don't have a good suggestion or alternative here, but that repetition is what made me think that perhaps the messaging should change.

However! I'm fine with whatever you decide, this is not a hard comment :)

That's an interesting point. I'll think about that. I think my thinking was that there could be multiple reasons why an SBOM is unusable (e.g. invalid JSON syntax). getSyftJSON doesn't care why the SBOM is unusable, it just needs to know that it's not usable. The inner error unable to open SBOM... in openSbom is a specific reason why the SBOM was unusable — specifically that the SBOM couldn't be opened.

Signed-off-by: Dan Luhring <dan.luhring@anchore.com>

wagoodman · 2021-03-15T15:07:25Z

grype/pkg/provider.go

-
-			// this reader is a combination of previous bytes read from stdin by other providers as well as what still is
-			// available from stdin. The Tee reader is to ensure that any read bytes from stdin are preserved.
-			cfg.reader = io.MultiReader(&previousStdin, io.TeeReader(os.Stdin, &previousStdin))


taking this out has means that we must read all of stdin to determine which provider to use, where the former implementation was "peeking" as far as necessary. Was there a reason to change this to read all stdin bytes into memory?

The main goal here was to simplify. To be honest, I was having trouble keeping track of the logic, which was notable to me because the lack of availability of SBOM JSON readers was central to these bugs. For now, we only have one provider that makes use of the stdin bytes reader, so the multireader + teereader weren't being leveraged beyond providing an io.Reader for stdin. I could see this changing if we see a need for more providers, and at that point, it'd be good to reassess the symmetry we're seeing among the providers.

If there's a more straightforward way to improve this code, I'd welcome that!

wagoodman

nice addition 💯

alfredodeza reviewed Mar 2, 2021

View reviewed changes

luhring added 3 commits March 9, 2021 08:58

Fix SBOM input and refactor

12646be

Signed-off-by: Dan Luhring <dan.luhring@anchore.com>

Add basic CLI tests for SBOM input

49b04f1

Signed-off-by: Dan Luhring <dan.luhring@anchore.com>

Use snapshot build for pipeline CLI testing

e93bf99

Signed-off-by: Dan Luhring <dan.luhring@anchore.com>

luhring force-pushed the fix-sbom-input branch from 38e9b4b to e93bf99 Compare March 9, 2021 13:58

Skip signing for snapshot builds

a82ae1f

Signed-off-by: Dan Luhring <dan.luhring@anchore.com>

luhring marked this pull request as ready for review March 9, 2021 14:29

alfredodeza approved these changes Mar 9, 2021

View reviewed changes

wagoodman reviewed Mar 15, 2021

View reviewed changes

wagoodman approved these changes Mar 15, 2021

View reviewed changes

luhring merged commit 75c0d36 into main Mar 15, 2021

luhring deleted the fix-sbom-input branch March 15, 2021 15:50

wagoodman mentioned this pull request Mar 15, 2021

Suppress version constraint token scanner errors #247

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix intake of SBOM from JSON #241

Fix intake of SBOM from JSON #241

luhring commented Mar 2, 2021 •

edited

Loading

alfredodeza Mar 2, 2021

luhring Mar 3, 2021

alfredodeza Mar 2, 2021

luhring Mar 3, 2021

alfredodeza Mar 2, 2021

luhring Mar 3, 2021

alfredodeza Mar 3, 2021

alfredodeza Mar 2, 2021

luhring Mar 3, 2021

alfredodeza Mar 3, 2021

luhring Mar 9, 2021

wagoodman Mar 15, 2021

luhring Mar 15, 2021

wagoodman left a comment

Fix intake of SBOM from JSON #241

Fix intake of SBOM from JSON #241

Conversation

luhring commented Mar 2, 2021 • edited Loading

Notes:

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

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

wagoodman left a comment

Choose a reason for hiding this comment

luhring commented Mar 2, 2021 •

edited

Loading