anchore / grype

A vulnerability scanner for container images and filesystems
Apache License 2.0
8.15k stars 528 forks source link

Grype appears to be writing v1.6 spec cyclonedx files that grype itself cannot read (affects 0.79.0+) #1951

Open ragaskar opened 2 weeks ago

ragaskar commented 2 weeks ago

What happened: After updating to grype 0.79.1 which now emits v1.6 spec cyclonedx-json (and cascading that spec change down through all our internal utils that deal with cyclonedx and needed the cyclonedx-go library bumped to latest), I discovered that grype was unable to process a file itself had emitted (we scan artifact bits with syft/grype, emit cyclonedx json and store the result in a database, later retrieving just the SBOM and running grype against that in order to refresh vuln results without a full artifact rescan).

What you expected to happen: I expected grype to be able to process cyclonedx SBOMs it emits. (I also kind of -- maybe unreasonably? -- expected grype to emit an older, more compatible cyclonedx spec format unless/until it needs updated spec features ... but I don't think this is default behavior of the cyclonedx-go lib, so might be kind of tricky -- and is certainly more feature request, less bug ;). Also I guess not sure how much of this is on the syft side vs grype side? šŸ¤· ).

How to reproduce it (as minimally and precisely as possible): grype alpine -o cyclonedx-json > /tmp/grype-out.json && grype /tmp/grype-out.json -o cyclonedx-json

which w/ 0.79.1 results in (on my darwin arm machine):

āÆ grype alpine -o cyclonedx-json > /tmp/grype-out.json && grype /tmp/grype-out.json -o cyclonedx-json
 āœ” Vulnerability DB                [no update available]
 āœ” Loaded image                                                                                                                                                                                                                alpine:latest
 āœ” Parsed image                                                                                                                                                      sha256:442043f030d33ff05cf2b6dcb64a3948ce38f2663d146558281a828d78eae5fd
 āœ” Cataloged contents                                                                                                                                                       8946eb426fbf9ed6260ee859e60462b834c8d1d50349f15e73aa17928f7991e8
   ā”œā”€ā”€ āœ” Packages                        [14 packages]
   ā”œā”€ā”€ āœ” File digests                    [77 files]
   ā”œā”€ā”€ āœ” File metadata                   [77 locations]
   ā””ā”€ā”€ āœ” Executables                     [17 executables]
 āœ” Scanned for vulnerabilities     [8 vulnerability matches]
   ā”œā”€ā”€ by severity: 0 critical, 0 high, 6 medium, 0 low, 0 negligible (2 unknown)
   ā””ā”€ā”€ by status:   8 fixed, 0 not-fixed, 0 ignored
 āœ” Vulnerability DB                [no update available]
failed to catalog: unable to decode sbom: sbom format not recognized

0.79.0 is similarly affected, 0.78.0 works as expected (emits an SBOM vs. throwing an error re: SBOM decoding).

Anything else we need to know?: Affects 0.79.0 and 0.79.1 (the move to cyclonedx-go 0.9.0 occurred in 0.79.0). I suspect (but haven't dug into it much yet) that the problem lies in the internal cyclonedxutil lib, which doesn't seem to have any references to the 1.6 spec and appears to be involved in the SBOM decoding workflow.

Thanks for all your work on grype!!!! It's a great tool! Hopefully I'm not coming off too salty in this issue, but I admit I am a bit bummed to have updated libs across our toolchain to achieve 1.6 cdx compatibility just to run into this blocker. :) I guess in the near-term I can pin back to 0.78.0 and at least internally we're up-to-date cyclonedx spec-wise now :)

Environment:

ragaskar commented 2 weeks ago

(sorry, would have liked to show up with more than just a bug report, but a bit late in the evening for me to start digging into the fix -- I'll see if I'm able to poke a bit in the morning).

ragaskar commented 2 weeks ago

Poking a bit this evening and I see that the decoder lives over in syft, so likely a change needs to happen there and get bumped into grype. Haven't dug deep yet, but given what I glanced at it seems straightforward to fix (and I can see a way to write a test that might "track" these kinds of changes better -- i.e. fail on bump -- to help prevent future breakage).

ragaskar commented 2 weeks ago

Haha, after looking at it more, I believe am I somewhat incorrect. At the moment here is what I believe is happening:

1) grype has a presenter that does not use the syft cyclonedx encoder to write cyclonedx files, it simply calls cyclonedx.NewBom() 2) cyclonedx.NewBom() is hardcoded to ship w/ latest supported spec (1.6). 3) grype, when decoding (I think this is the right direction?), does use the syft decoder. The syft decoder, despite using the same 0.9.0 underlying cyclonedxgo lib (that supports 1.6), has -- as aligns with my earlier expectation -- a hardcoded default SpecVersion -- 1.5 -- along with a set of (again, hardcoded) supported by syft cdx versions. 4) Those versions have not yet been adjusted for the new 1.6 specVersion (I suspect that syft would actually not fail the same way grype is; I haven't checked, but it's almost certainly still emitting 1.5 vs. 1.6). Someone could go bump all those versions according to the current pattern and then this grype problem would go away, until the next time that cyclonedxgo releases a SpecVersion bump and then this will happen again.

I think the right fix in Grype is to (probably?) lock the json cyclonedx presenter to the syft default version. This should guarantee with the current code that Grype will always emit an SBOM that it is capable of reading, which is my main concern (someone should also add 1.6 to syft, and ... maybe bump the default version? The former is an easy PR, I feel like the latter is a project decision that has a wide impact and is probably best left to core maintainers).

Anyways, I'll probably take a look at if it's possible to coerce the presenter to use the syft default json spec version without making things too yucky.

ragaskar commented 1 week ago

Haha:

ok. wow, this looked simple but was not.

So. Fun fact: the DefaultVersion constant of cycloneDx that syft uses when deciding what spec version to emit lives in an internal package here (and thus cannot be referenced).

If it was possible to reference this (which would require that package to move out of internal, or ... something?), then one could change the line here to use the EncodeVersion method (instead of Encode) with that default, much like the syft Encoder does here.

There would be increased coupling, but ... the coupling is already there in that Syft decoders are used to parse the input SBOM.

Another, simpler thing to do with less direct coupling is to simply add a test that ensures that grype can read what it emits. I think what that would mean is that you would have to wait until the default spec version was bumped in syft AND you bumped to that syft version before you could upgrade to a version of cyclonedx-go that defaults to a new spec version. Now that I think about it, that's all I really want, so maybe just that test is sufficient, but we can't add it until the criteria I described above is true (syft defaults to 1.6 for cyclonedx). Happy to add after it is possible, but I would appreciate some suggestions on where best to place such a test -- it's kind of integration-y.

spiffcs commented 1 week ago

Thanks for the issue @ragaskar - with the new syft released that's generating CDX 1.6 by default I'm working on getting grype upgraded now so it can read it as an input.

Apologies for this incompatibility as this is definitely a bug. We'll get the update in and release a newer version of grype to go along with the latest syft

[I] hal@ChristophersMBP ~> syft -o cyclonedx-json alpine:latest | grype
 āœ” Loaded image                                                                                                      alpine:latest
 āœ” Parsed image                                            sha256:092561eea88f9f28654bbade209576f5f93efeb8c7ba66a07ac2033c0ddc8ae7
 āœ” Cataloged contents                                             722d50f1c8652372d25427579162c0301e2e78b1876878be3beedca494d09b2b
   ā”œā”€ā”€ āœ” Packages                        [14 packages]
   ā”œā”€ā”€ āœ” File digests                    [77 files]
   ā”œā”€ā”€ āœ” File metadata                   [77 locations]
   ā””ā”€ā”€ āœ” Executables                     [17 executables]
failed to catalog: unable to decode sbom: sbom format not recognized
mydeveloperplanet commented 2 days ago

Wouldn't it be better if a more conservative default strategy was chosen by syft in order to allow users a more smooth transition? This is a breaking change and even grype which is part of Anchore breaks on it. The syft-version had to be 2.0.0 because there an incompatible API change has been made. If I had seen that it was a breaking change, I would have been more cautious to upgrade.

spiffcs commented 1 day ago

Thanks for the comment @mydeveloperplanet - In short, I agree

The current strategy is "when bumping an sbom format in syft, we also release the downstream tooling so compatibility can be maintained" - in this situation grype did not have a follow up release which caused the command to fail as shown above.

There was no grype release that would allow a user to run the default flow when using a cyclone-dx SBOM shown above without using a flag that communicated a format other than cyclone-dx v1.6.

We're working on getting more gates and tests added around this so that this situation cannot happen again and we follow a more conservative strategy that will not break current users workflows.

The current state of the tips of main for each repo now have compatible and correct defaults when using cyclone-dx (both local go bin built from tips of main):

[I] hal@ChristophersMBP ~/d/grype (cyclone-dx-upgrade)> ~/go/bin/syft -o cyclonedx-json alpine:latest | ~/go/bin/grype
 āœ” Loaded image                                                                                                                                                                                                                        alpine:latest
 āœ” Parsed image                                                                                                                                                              sha256:092561eea88f9f28654bbade209576f5f93efeb8c7ba66a07ac2033c0ddc8ae7
 āœ” Cataloged contents                                                                                                                                                               722d50f1c8652372d25427579162c0301e2e78b1876878be3beedca494d09b2b
   ā”œā”€ā”€ āœ” Packages                        [14 packages]
   ā”œā”€ā”€ āœ” File digests                    [77 files]
   ā”œā”€ā”€ āœ” File metadata                   [77 locations]
   ā””ā”€ā”€ āœ” Executables                     [17 executables]

I do disagree that the syft change should have resulted in a 2.0.0. Following this, it would necessitate a new major version update for any SBOM format minor version update as the default output. @wagoodman - what are your thoughts on the release notes showing a compatibility table as we do more releases that update the default minor version of SBOM formats?

What should be backwards compatible? If a user updates a minor version of syft, but does not update grype, should it be communicated in the release notes that they must also update grype?

Happy for any and all thoughts here as we try and make this smoother for the future

spiffcs commented 1 day ago

A new release of grype has been published that uses syft v1.8.0 - this should allow the latest version of grype (v0.79.2) to accept the default cyclone-dx format the syft outputs (v1.6).

This issue will not be closed yet.

I'll be following up on this issue with a future PR that should gate a syft release on the condition of testing with a few more smoke tests of exercising all the formats and their schema validity as grype inputs.

Developers will get a notification if a syft release is about to be published that is incompatible with the current released version of grype and the release will be halted.

Users will be notified in the future release notes if updating syft also necessitates a grype update because of a detected incompatibility.

I think we're still waiting for a few more of the core team to respond so we can agree on the updated release plan going forward.

mydeveloperplanet commented 1 day ago

Thank you for your elaborate answer and actions. I believe a lot will be fixed when extra tests are added.

I wrote that syft had to be 2.0.0 because now it breaks for example grype. But probably also other tooling that interpret the SBOM. I have not investigated why grype breaks but maybe because CycloneDX 1.6 specific items were added to the SBOM? A clear warning in the release notes would also do, as long as I (as a user) am notified that I should take a closer look at the release myself :+1: