feat: add support for CPE field in SPDX document

[tgagneret-embedded]

Hi,

This PR follows issue #3588.

The goal is to extract information from CPE field (when PURL field is not specified) in SPDX document.

CPE can contain * as version. This PR also brings support for this * as version. In such case it will report all CVE for the product.

[terriko]

Approving the tests to run. I'm heading into meetings and might not get a chance to do code review until quite a bit later, though.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 71.42857% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 80.52%. Comparing base (d6cbe40) to head (9277f53).
Report is 49 commits behind head on main.

Files	Patch %	Lines
cve_bin_tool/cve_scanner.py	59.09%	7 Missing and 2 partials ⚠️
cve_bin_tool/sbom_manager/__init__.py	92.30%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3683      +/-   ##
==========================================
+ Coverage   75.41%   80.52%   +5.11%     
==========================================
  Files         808      808              
  Lines       11983    12003      +20     
  Branches     1598     1602       +4     
==========================================
+ Hits         9037     9666     +629     
+ Misses       2593     1909     -684     
- Partials      353      428      +75     

Flag	Coverage Δ
longtests	75.43% <71.42%> (+0.01%)	⬆️
win-longtests	78.55% <71.42%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tgagneret-embedded]

I have some coverage issue. I added some tests but it is currently skipped (TestExploitScanner and TestSBOM), so it is not taken into account for the coverage.

Could you point me to the correct test file I should update to improve the coverage ?

Thanks

[tgagneret-embedded]

@terriko I updated the PR title that caused the pipeline to fail.

Can I have some feedback on the coverage issue ?

Thanks.

[terriko]

Since we're currently skipping those sbom test files, could you make new tiny files that test your code and write tests that aren't being skipped? I'm guessing it may be another week or more before anyone gets around to fixing those tests.

[tgagneret-embedded]

I rebased my PR since tests seems to be enabled again on main branch. Updated tests in the PR should run and fix the coverage issue.

[tgagneret-embedded]

@terriko Can I have more explaination on why the pipelines run technote-space/get-diff-action ?
https://github.com/tgagneret-embedded/cve-bin-tool/blob/40e51801fe8ae4cabaec0183c072b579dec6e52e/.github/workflows/testing.yml#L210

It uses before that is The SHA of the most recent commit on ref before the push. according to Github documentation. But if you use git push force it causes the pipeline to fail (because it uses the deleted commit to make the diff) which does not make a lot of sense in my opinion. A diff between the commit and main would make a lot more sense.

The only way I see to avoid pipeline to fail is to push force main on my branch (which causes the PR to close) and then push my commit again.

Maybe I missed something, could you help me with this ?

Thank you

[terriko]

@tgagneret-embedded I almost never use git push force so I've never encountered this issue. I squash-merge as a matter of course because I find it un-fun to walk newbies through "how to clean up git history" when I can just do that during merge, and thanks to GSoC and Hacktoberfest I work with a lot of folk who are new to git.

So my first incredibly lazy suggestion is just to avoid force pushing. I don't care if you have 30 tiny commits, I promise! But I know you need force when you rebase and some people just hate having their tiny commits out in the world as a matter of personal pride, so I'd also like to fix the problem if we can.

I wouldn't swear our usage of git-diff is ideal or even functional at the moment -- it was meant to skip some of the longer tests if certain files weren't changed, but I'm not sure how much of a difference it makes in practice (especially since we moved some of the tests that were causing the most problems back then into the network-may-fail job). Do you have any suggestions on what we could do that would work better with your workflow? I'm not adverse to removing it or switching us to a better diff method if keeping it is still useful.

[tgagneret-embedded]

I now understand why you do this. I'm not familiar with Gihtub action or git-diff-action maybe there is a configuration somewhere, but at the time I don't have any suggestion to fix this. I won't push --force anymore :D.

Thanks for the explanation, I'll get back to you if I have any suggestion.

[tgagneret-embedded]

@terriko PR is rebased on main.

[tgagneret-embedded]

@terriko PR Can I have some update on the PR ?

Thanks

[kartben]

ping @terriko?

FWIW I pushed a branch that resolves the rebase conflicts at: https://github.com/kartben/cve-bin-tool/tree/feature/add_cpe_support_for_spdx

[terriko]

Hey, sorry, this one got buried in all the gsoc-related pull rrequests while I was out. I'm probably not going to get to digging into it today but I'm flagging it for myself so I can come back and take a more serious look tomorrow after I'm done moving the simpler PRs.

[terriko]

Trying to fix the merge conflicts here. A lot of the functionality got added as part of the vex cpe support PR: #3990 so this is going to be a bit weird.

[terriko]

Okay, dug into this further. My apologies again -- this is absolutely my fault for not getting things ready to merge in a more timely manner because I was focused on other stuff.

Thee main part of this PR (adding support for CPE) got covered by #3990 which added it for vex data in CycloneDX, but since the same get("externalReference") works in both cases, it looks like it should work correctly for SDPX as well.

If we take that out, there's two groups of things left in this PR:

1. Tests. I'd like to keep your tests to validate that the CycloneDX changes will indeed work for SPDX as you had expected.
2. A change to get all cves with a product if no version is specified. We'd been intentionally not doing this because we currently don't have a way to mass triage these, so it could result in a lot of false positives that were a pain to clean up. I'm not completely adverse to adding this as an option, but I think it would have to be an optional flag to enable it and ideally it'd come with some docs and instructions on how to handle triage in these cases. This should probably be a separate PR.

I'm leaning towards "turn this particular PR into just a test PR and leave everything else for future work" but I wanted to give you some time to look at it in case I missed something.

So... I guess let me know what you'd like to do with this PR, and if I don't hear from you in around a week I'll convert it to tests and merge those?

[tgagneret-embedded]

I will have a look next week and see what I can do.

[tgagneret-embedded]

Ok, we will discard the cpe * support for now.

It seems that the PR you refer to has already updated the tests (spdx and cyclone dx). So I'm not sure which tests I should keep from my PR.

Just one thing, in the other PR the PackageName and PackageVersion is the same than the ones present in ExternalRef cpe fields so you could have a test that passes but in reality it shouldn't. In this PR, I use different name and version in the cpe from ExternalRef, so the test can detect if we use the PackageName instead of the product field in one cpe for example.

Let me know what I should do @terriko

[terriko]

Looks like the right choice is to close this -- it's covered well enough and it's probably not worth doing work to get the remainder into a mergeable state. Thanks again for you work on it, though!