Skip to content
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.

Sign up for GitHub

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

refactor(sbom): add sbom prefix + filepaths for decode log messages #7074

Merged
merged 6 commits into from
Jul 3, 2024
Merged

refactor(sbom): add sbom prefix + filepaths for decode log messages #7074

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a4b1b73
fix(sbom): use logger with `sbom` prefix
DmitriyLewen Jul 2, 2024
47c0e0b
feat: add paths to sbom files using context
DmitriyLewen Jul 2, 2024
82c2620
fix linter error
DmitriyLewen Jul 2, 2024
e0b36ca
refactor(sbom): remove context filepath for tests
DmitriyLewen Jul 2, 2024
21ec7cd
Merge branch 'main' of github.com:DmitriyLewen/trivy into fix-sbom/ad…
DmitriyLewen Jul 2, 2024
cf30a86
refactor: use `log.FilePath()`
DmitriyLewen Jul 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions pkg/fanal/analyzer/sbom/sbom.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (

"github.com/aquasecurity/trivy/pkg/fanal/analyzer"
ftypes "github.com/aquasecurity/trivy/pkg/fanal/types"
"github.com/aquasecurity/trivy/pkg/log"
"github.com/aquasecurity/trivy/pkg/sbom"
"github.com/aquasecurity/trivy/pkg/types"
)
Expand All @@ -29,14 +30,15 @@ var requiredSuffixes = []string{

type sbomAnalyzer struct{}

func (a sbomAnalyzer) Analyze(_ context.Context, input analyzer.AnalysisInput) (*analyzer.AnalysisResult, error) {
func (a sbomAnalyzer) Analyze(ctx context.Context, input analyzer.AnalysisInput) (*analyzer.AnalysisResult, error) {
// Format auto-detection
format, err := sbom.DetectFormat(input.Content)
if err != nil {
return nil, xerrors.Errorf("failed to detect SBOM format: %w", err)
}

bom, err := sbom.Decode(input.Content, format)
ctx = log.WithContextAttrs(ctx, log.String("file", input.FilePath))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if adding log.FilePath(filePath string) in pkg/log and using it so we can use the consistent key name?

func FilePath(filePath string) slog.Attr {
    return String("file_path", filePath)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added (5f3f694 + ae1e4d8)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added (5f3f694 + ae1e4d8)

I think they should be another PR. And merge the change into this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. The number of filepaths used was more than i expected.
I created #7080

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Merged #7080.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated this PR

bom, err := sbom.Decode(ctx, input.Content, format)
if err != nil {
return nil, xerrors.Errorf("SBOM decode error: %w", err)
}
Expand Down
5 changes: 3 additions & 2 deletions pkg/fanal/artifact/sbom/sbom.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ func NewArtifact(filePath string, c cache.ArtifactCache, opt artifact.Option) (a
}, nil
}

func (a Artifact) Inspect(_ context.Context) (artifact.Reference, error) {
func (a Artifact) Inspect(ctx context.Context) (artifact.Reference, error) {
f, err := os.Open(a.filePath)
if err != nil {
return artifact.Reference{}, xerrors.Errorf("failed to open sbom file error: %w", err)
Expand All @@ -51,7 +51,8 @@ func (a Artifact) Inspect(_ context.Context) (artifact.Reference, error) {
}
log.Info("Detected SBOM format", log.String("format", string(format)))

bom, err := sbom.Decode(f, format)
ctx = log.WithContextAttrs(ctx, log.String("file", a.filePath))
bom, err := sbom.Decode(ctx, f, format)
if err != nil {
return artifact.Reference{}, xerrors.Errorf("SBOM decode error: %w", err)
}
Expand Down
3 changes: 2 additions & 1 deletion pkg/fanal/handler/unpackaged/unpackaged.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,8 @@ func (h unpackagedHook) Handle(ctx context.Context, res *analyzer.AnalysisResult
}

// Parse the fetched SBOM
bom, err := sbom.Decode(bytes.NewReader(raw), format)
ctx = log.WithContextAttrs(ctx, log.String("file", filePath))
bom, err := sbom.Decode(ctx, bytes.NewReader(raw), format)
if err != nil {
return err
}
Expand Down
5 changes: 4 additions & 1 deletion pkg/sbom/cyclonedx/unmarshal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package cyclonedx_test

import (
"context"
"encoding/json"
"os"
"testing"
Expand All @@ -10,6 +11,7 @@ import (
"github.com/stretchr/testify/require"

ftypes "github.com/aquasecurity/trivy/pkg/fanal/types"
"github.com/aquasecurity/trivy/pkg/log"
"github.com/aquasecurity/trivy/pkg/purl"
"github.com/aquasecurity/trivy/pkg/sbom/cyclonedx"
sbomio "github.com/aquasecurity/trivy/pkg/sbom/io"
Expand Down Expand Up @@ -757,7 +759,8 @@ func TestUnmarshaler_Unmarshal(t *testing.T) {
require.NoError(t, err)

var got types.SBOM
err = sbomio.NewDecoder(cdx.BOM).Decode(&got)
ctx := log.WithContextAttrs(context.Background(), log.String("filepath", tt.inputFile))
err = sbomio.NewDecoder(cdx.BOM).Decode(ctx, &got)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to pass attributes in tests.

Suggested change
ctx := log.WithContextAttrs(context.Background(), log.String("filepath", tt.inputFile))
err = sbomio.NewDecoder(cdx.BOM).Decode(ctx, &got)
err = sbomio.NewDecoder(cdx.BOM).Decode(context.Background(), &got)

Copy link
Contributor Author

@DmitriyLewen DmitriyLewen Jul 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed e0b36ca

require.NoError(t, err)

got.BOM = nil
Expand Down
33 changes: 17 additions & 16 deletions pkg/sbom/io/decode.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package io

import (
"context"
"errors"
"slices"
"sort"
Expand Down Expand Up @@ -46,14 +47,14 @@ func NewDecoder(bom *core.BOM) *Decoder {
}
}

func (m *Decoder) Decode(sbom *types.SBOM) error {
func (m *Decoder) Decode(ctx context.Context, sbom *types.SBOM) error {
// Parse the root component
if err := m.decodeRoot(sbom); err != nil {
return xerrors.Errorf("failed to decode root component: %w", err)
}

// Parse all components
if err := m.decodeComponents(sbom); err != nil {
if err := m.decodeComponents(ctx, sbom); err != nil {
return xerrors.Errorf("failed to decode components: %w", err)
}

Expand All @@ -67,7 +68,7 @@ func (m *Decoder) Decode(sbom *types.SBOM) error {
m.addLangPkgs(sbom)

// Add remaining packages
if err := m.addOrphanPkgs(sbom); err != nil {
if err := m.addOrphanPkgs(ctx, sbom); err != nil {
return xerrors.Errorf("failed to aggregate packages: %w", err)
}

Expand Down Expand Up @@ -109,9 +110,9 @@ func (m *Decoder) decodeRoot(s *types.SBOM) error {
return nil
}

func (m *Decoder) decodeComponents(sbom *types.SBOM) error {
func (m *Decoder) decodeComponents(ctx context.Context, sbom *types.SBOM) error {
onceMultiOSWarn := sync.OnceFunc(func() {
m.logger.Warn("Multiple OS components are not supported, taking the first one and ignoring the rest")
m.logger.WarnContext(ctx, "Multiple OS components are not supported, taking the first one and ignoring the rest")
})

for id, c := range m.bom.Components() {
Expand All @@ -136,7 +137,7 @@ func (m *Decoder) decodeComponents(sbom *types.SBOM) error {

// Third-party SBOMs may contain packages in types other than "Library"
if c.Type == core.TypeLibrary || c.PkgIdentifier.PURL != nil {
pkg, err := m.decodeLibrary(c)
pkg, err := m.decodeLibrary(ctx, c)
if errors.Is(err, ErrUnsupportedType) || errors.Is(err, ErrPURLEmpty) {
continue
} else if err != nil {
Expand Down Expand Up @@ -183,17 +184,17 @@ func (m *Decoder) decodeApplication(c *core.Component) *ftypes.Application {
return &app
}

func (m *Decoder) decodeLibrary(c *core.Component) (*ftypes.Package, error) {
func (m *Decoder) decodeLibrary(ctx context.Context, c *core.Component) (*ftypes.Package, error) {
p := (*purl.PackageURL)(c.PkgIdentifier.PURL)
if p == nil {
log.Debug("Skipping a component without PURL",
m.logger.DebugContext(ctx, "Skipping a component without PURL",
log.String("name", c.Name), log.String("version", c.Version))
return nil, ErrPURLEmpty
}

pkg := p.Package()
if p.Class() == types.ClassUnknown {
log.Debug("Skipping a component with an unsupported type",
m.logger.DebugContext(ctx, "Skipping a component with an unsupported type",
log.String("name", c.Name), log.String("version", c.Version), log.String("type", p.Type))
return nil, ErrUnsupportedType
}
Expand Down Expand Up @@ -240,7 +241,7 @@ func (m *Decoder) decodeLibrary(c *core.Component) (*ftypes.Package, error) {
}

if p.Class() == types.ClassOSPkg {
m.fillSrcPkg(c, pkg)
m.fillSrcPkg(ctx, c, pkg)
}

return pkg, nil
Expand Down Expand Up @@ -278,11 +279,11 @@ func (m *Decoder) pkgName(pkg *ftypes.Package, c *core.Component) string {
return c.Name
}

func (m *Decoder) fillSrcPkg(c *core.Component, pkg *ftypes.Package) {
func (m *Decoder) fillSrcPkg(ctx context.Context, c *core.Component, pkg *ftypes.Package) {
if c.SrcName != "" && pkg.SrcName == "" {
pkg.SrcName = c.SrcName
}
m.parseSrcVersion(pkg, c.SrcVersion)
m.parseSrcVersion(ctx, pkg, c.SrcVersion)

// Source info was added from component or properties
if pkg.SrcName != "" && pkg.SrcVersion != "" {
Expand All @@ -305,7 +306,7 @@ func (m *Decoder) fillSrcPkg(c *core.Component, pkg *ftypes.Package) {
}

// parseSrcVersion parses the version of the source package.
func (m *Decoder) parseSrcVersion(pkg *ftypes.Package, ver string) {
func (m *Decoder) parseSrcVersion(ctx context.Context, pkg *ftypes.Package, ver string) {
if ver == "" {
return
}
Expand All @@ -318,7 +319,7 @@ func (m *Decoder) parseSrcVersion(pkg *ftypes.Package, ver string) {
case packageurl.TypeDebian:
v, err := debver.NewVersion(ver)
if err != nil {
log.Debug("Failed to parse Debian version", log.Err(err))
m.logger.DebugContext(ctx, "Failed to parse Debian version", log.Err(err))
return
}
pkg.SrcEpoch = v.Epoch()
Expand Down Expand Up @@ -361,7 +362,7 @@ func (m *Decoder) addLangPkgs(sbom *types.SBOM) {

// addOrphanPkgs adds orphan packages.
// Orphan packages are packages that are not related to any components.
func (m *Decoder) addOrphanPkgs(sbom *types.SBOM) error {
func (m *Decoder) addOrphanPkgs(ctx context.Context, sbom *types.SBOM) error {
osPkgMap := make(map[string]ftypes.Packages)
langPkgMap := make(map[ftypes.LangType]ftypes.Packages)
for _, pkg := range m.pkgs {
Expand All @@ -382,7 +383,7 @@ func (m *Decoder) addOrphanPkgs(sbom *types.SBOM) error {
// Add OS packages only when OS is detected.
for _, pkgs := range osPkgMap {
if sbom.Metadata.OS == nil || !sbom.Metadata.OS.Detected() {
log.Warn("Ignore the OS package as no OS is detected.")
m.logger.WarnContext(ctx, "Ignore the OS package as no OS is detected.")
break
}

Expand Down
5 changes: 3 additions & 2 deletions pkg/sbom/sbom.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package sbom

import (
"bufio"
"context"
"encoding/json"
"encoding/xml"
"io"
Expand Down Expand Up @@ -180,7 +181,7 @@ func decodeAttestCycloneDXJSONFormat(r io.ReadSeeker) (Format, bool) {
return FormatAttestCycloneDXJSON, true
}

func Decode(f io.Reader, format Format) (types.SBOM, error) {
func Decode(ctx context.Context, f io.Reader, format Format) (types.SBOM, error) {
var (
v any
bom = core.NewBOM(core.Options{})
Expand Down Expand Up @@ -227,7 +228,7 @@ func Decode(f io.Reader, format Format) (types.SBOM, error) {
}

var sbom types.SBOM
if err := sbomio.NewDecoder(bom).Decode(&sbom); err != nil {
if err := sbomio.NewDecoder(bom).Decode(ctx, &sbom); err != nil {
return types.SBOM{}, xerrors.Errorf("failed to decode: %w", err)
}

Expand Down
5 changes: 4 additions & 1 deletion pkg/sbom/spdx/unmarshal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package spdx_test

import (
"context"
"encoding/json"
"os"
"sort"
Expand All @@ -11,6 +12,7 @@ import (
"github.com/stretchr/testify/require"

ftypes "github.com/aquasecurity/trivy/pkg/fanal/types"
"github.com/aquasecurity/trivy/pkg/log"
sbomio "github.com/aquasecurity/trivy/pkg/sbom/io"
"github.com/aquasecurity/trivy/pkg/sbom/spdx"
"github.com/aquasecurity/trivy/pkg/types"
Expand Down Expand Up @@ -357,7 +359,8 @@ func TestUnmarshaler_Unmarshal(t *testing.T) {
}

var got types.SBOM
err = sbomio.NewDecoder(v.BOM).Decode(&got)
ctx := log.WithContextAttrs(context.Background(), log.String("filepath", tt.inputFile))
err = sbomio.NewDecoder(v.BOM).Decode(ctx, &got)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Copy link
Contributor Author

@DmitriyLewen DmitriyLewen Jul 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed in e0b36ca

require.NoError(t, err)

// Not compare BOM
Expand Down