- 
                Notifications
    
You must be signed in to change notification settings  - Fork 33
 
sbomtool merge and filter features #297
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a quick first pass before diving into the core of this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| normalized := strings.TrimPrefix(cpe, "cpe:") | ||
| 
               | 
          ||
| // Ensure consistent format (cpe:2.3 standard) | ||
| if !strings.HasPrefix(normalized, "2.3:") { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double check this 2.3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing this altogether
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1405ae5    to
    f591edc      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another pass on feat: add deduplication package for SBOM processing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another pass of feat: add core SBOM processing functionality
| 
               | 
          ||
| // Normalize all paths for Syft compatibility | ||
| for _, filePath := range result.AllFiles { | ||
| normalized, err := BuildrootToInstalled(filePath, buildrootPath) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit but IMO this would be better as a method on a buildrootPath type to avoid the footgun of mis-ordering these method arguments:
buildrootPath.InstalledPath(filepath)
or even model the buildrootPath on the Scanner and call as
s.BuildrootToInstalled(filePath)
d40f795    to
    64ad488      
    Compare
  
    | } | ||
| 
               | 
          ||
| // ValidateFilePath validates that a file path exists and is readable. | ||
| func ValidateFilePath(path string, purpose string, mustExist bool) error { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this and the next functions are a bit extra. If either reads or writes fail, you will get an error from standard Go explaining what failed in the operation, so you really don't have to spend extra IO checking the files permissions. The same with the other Validation performed in later, for the output directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the output I actually think it's probably worth it since generating the sbom can be expensive, and it's probably better to validate where it's going at the beginning rather then after we've generated it.
| packageNames := make([]string, len(packages)) | ||
| for i, p := range packages { | ||
| packageNames[i] = p.Name | ||
| } | ||
| assert.Contains(t, packageNames, "pkg1") | ||
| assert.Contains(t, packageNames, "pkg2") | ||
| assert.Contains(t, packageNames, "pkg3") | ||
| assert.Contains(t, packageNames, "app1") // Metadata component | ||
| assert.Contains(t, packageNames, "app2") // Metadata component | ||
| } | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking there is a way to write this in a more idiomatic way, but realized that it will loose the context of what item actually is missing.
| "github.com/stretchr/testify/require" | ||
| ) | ||
| 
               | 
          ||
| func TestMergeCommandMetadata(t *testing.T) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't bother with this test, as it only tests the messages for the usage, and not the actual behavior of the test.
| for p := range result.MergedSBOM.Artifacts.Packages.Enumerate() { | ||
| packages = append(packages, p) | ||
| } | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as elsewhere, you don't really need Packages as a new slice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, the slice makes the test much easier to deal with.
64ad488    to
    5a7ae93      
    Compare
  
    | 
           Just a couple of nits, otherwise LGTM  | 
    
Signed-off-by: Richard Kelly <[email protected]>
When generating a large SBOM, and especially when merging multiple SBOMs together, it is likely that the same package will be found multiple times. The deduplication code feature replaces those duplicates with a single merged package, and reroutes all existing relationships to the new merged package Signed-off-by: Richard Kelly <[email protected]>
- Move generate, merge, logging, and validate packages to internal/ - Update go.mod dependencies Signed-off-by: Richard Kelly <[email protected]>
Processor functionality added to enable future merging and deduplication functionality. This lays the groundwork for sbomtool to read and manipulate existing sboms Signed-off-by: Richard Kelly <[email protected]>
During initial SBOM creation all sources in the initial build are added to the SBOM, however, many of these sources are never actually built or added to the resulting Bottlerocket rpm. Buildroot filtering takes the initial large SBOM as well as the buildroot of actually installed files to limit the packages contained in the SBOM to only those we care about Signed-off-by: Richard Kelly <[email protected]>
Add filtering and deduplication calls to the cli, allowing tool to remove extra and unnecessary information from generated SBOMs Signed-off-by: Richard Kelly <[email protected]>
sbomtool merge feature allows multiple sboms of the same type to be merged into a single sbom. This change allows us to merge the sboms of multiple individual packages into a large sbom for an image. Signed-off-by: Richard Kelly <[email protected]>
5a7ae93    to
    11bb9b9      
    Compare
  
    
Issue number:
#296
Description of changes:
Adds the following features to
sbomtool:Testing done:
make allTerms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.