[rust] add crates.io enichment option for rust audit binary, json schema and spdx license updates. #3554

jimmystewpot · 2024-12-31T22:21:29Z

Description

This pull request supports remotely enriching Rust auditable binaries using crates.io. It adds the license, supplier, originator, description, and other fields (optionally if enabled) to the manifest.

This information is unavailable in the cargo lock and binary; if approved, I will add this capability to the other rust cataloger.

Type of change

New feature (non-breaking change which adds functionality)

I've also updated the SPDX license list, as that was failing the make test, and updated the JSON schema version to support the new crates-enriched metadata. There are still some missing unit tests, specifically the mocks for the crates.io lookup and caching functionality. I wanted to submit a PR early, seek guidance, and ensure this would benefit the community before investing more time in standardising it across the Rust catalogers.

The new feature adds a rust key to the configuration that allows the feature to be turned on/off and some settings tuned for site-specific needs.

Checklist:

I have added unit tests that cover changed behaviour partially
I have tested my code in common scenarios and confirmed there are no regressions
I have added comments to my code, particularly in hard-to-understand sections

kzantow

Hi @jimmystewpot, thanks for this enhancement; overall this looks great, and it's much appreciated that you've followed conventions really well. I left a few specific comments, but the biggest takeaways, I think are:

"duplicate" the configuration struct (but it won't be completely duplicated -- for the multilevel configuration, you'll use *bool whereas the rust.CatalogerConfig would have a bool, for example)
we probably don't want to choose between one metadata type or the other, but rather add a way to keep both (though the suggestions I have are only suggestions and I'd like to run these by the team when we start to introduce new patterns for things). maybe the best thing is just to add the fields to the existing structs and not worry about having to support multiple metadata types just yet
you'll need to Sign-off your commit(s) see contributing.md

kzantow · 2025-01-01T03:09:22Z

cmd/syft/internal/options/rust.go

+	"github.com/anchore/syft/syft/pkg/cataloger/rust"
+)
+
+type rustConfig rust.CatalogerConfig


This should not be a typedef of the rust struct but should be a declaration of all fields the CLI allows to be configured -- the idea is that these options define the CLI interface and can evolve separately from the internal configuration structs.

kzantow · 2025-01-01T03:14:58Z

syft/pkg/cataloger/rust/cataloger.go


 // NewCargoLockCataloger returns a new Rust Cargo lock file cataloger object.
-func NewCargoLockCataloger() pkg.Cataloger {
-	return generic.NewCataloger("rust-cargo-lock-cataloger").
+func NewCargoLockCataloger(opts CatalogerConfig) pkg.Cataloger {


it looks like this opts is not used?

kzantow · 2025-01-01T03:17:53Z

syft/pkg/cataloger/rust/config.go

+type CatalogerConfig struct {
+	InsecureSkipTLSVerify bool          `yaml:"insecure-skip-tls-verify" json:"insecure-skip-tls-verify" mapstructure:"insecure-skip-tls-verify"`
+	UseCratesEnrichment   bool          `json:"use-crates-enrichment" yaml:"use-crates-enrichment" mapstructure:"use-crates-enrichment"`
+	Proxy                 string        `yaml:"proxy,omitempty" json:"proxy,omitempty" mapstructure:"proxy"`


Is this equivalent to the go http_proxy environment variable? I don't think we would need a special config for this, rather just advise users to use the environment variable such that it's used for all http calls instead of needing to configure each individually. If there's really some reason that we need configuration other than the environment variable, we should figure out how to set this globally for all http requests.

kzantow · 2025-01-01T03:20:05Z

syft/pkg/cataloger/rust/cataloger.go

 	"github.com/anchore/syft/internal/mimetype"
 	"github.com/anchore/syft/syft/pkg"
 	"github.com/anchore/syft/syft/pkg/cataloger/generic"
 )

-const cargoAuditBinaryCatalogerName = "cargo-auditable-binary-cataloger"
+const (
+	toolName                      = "syft" // used for the user-agent string.


We shouldn't add a string here for the user-agent. Ideally this would come from configuration passed from the API user. The app name is initially passed in here, this just needs to get passed through to the appropriate configuration. Maybe we should make this more convenient somehow. But we really don't want this hardcoded as "syft", since a number of apps use the Syft API and are not, in fact, Syft.

kzantow · 2025-01-01T03:29:22Z

syft/pkg/cataloger/rust/cataloger.go

-const cargoAuditBinaryCatalogerName = "cargo-auditable-binary-cataloger"
+const (
+	toolName                      = "syft" // used for the user-agent string.
+	cargoAuditBinaryCatalogerName = "rust-cargo-auditable-binary-cataloger"


This looks like the right change, but I think this would technically be a breaking change if anyone was using the cargo-auditable-binary-cataloger string. This should probably be reverted to the previous value but add an issue to update it, so we can make sure to do so in Syft 2.0 or whenever appropriate.

kzantow · 2025-01-01T03:51:30Z

syft/pkg/cataloger/rust/crates_resolver.go

+func newCratesResolver(name string, opts CatalogerConfig) *rustCratesResolver {
+	base, err := url.Parse(opts.CratesBaseURL)
+	if err != nil {
+		panic(err)


We should never panic but instead use error returns

kzantow · 2025-01-01T03:53:23Z

syft/pkg/cataloger/rust/crates_resolver.go

+		if r := recover(); r != nil {
+			fmt.Fprintf(os.Stderr, "recovered from panic while resolving license at: \n%s", string(debug.Stack()))
+		}


there shouldn't be a need for panic recovery here -- what is panicking?

kzantow · 2025-01-01T03:54:40Z

syft/pkg/cataloger/rust/crates_resolver.go

+// cratesRemoteMetadata represents the remote metadata for a crate
+// as fetched from crates.io via an API request.
+// This is used for deserialization of the response from crates.io
+type cratesRemoteMetadata struct {


nit: this type should probably be defined above or below methods on rustCratesResolver rather than in the middle

kzantow · 2025-01-01T03:59:36Z

syft/pkg/cataloger/rust/parse_audit_binary.go

+		switch c.opts.UseCratesEnrichment {
+		case true:
+			ctx, cancel := context.WithTimeout(context.Background(), time.Duration(c.opts.CratesTimeout))
+			defer cancel()
+			cratesEnrichment, err := c.cratesResolver.ResolveCrate(ctx, dep.Name, dep.Version)
+			if err != nil {
+				log.Tracef("rust cataloger: failed to resolve crate %s/%s using crates.io: %v", dep.Name, dep.Version, err)
+				// fallback to not using the crates enriched package information.
+				p = newPackageFromAudit(&dep, location.WithAnnotation(pkg.EvidenceAnnotationKey, pkg.PrimaryEvidenceAnnotation))
+				continue
+			}
+			p = newPackageWithEnrichment(&dep, cratesEnrichment, location.WithAnnotation(pkg.EvidenceAnnotationKey, pkg.PrimaryEvidenceAnnotation))
+		case false:
+			p = newPackageFromAudit(&dep, location.WithAnnotation(pkg.EvidenceAnnotationKey, pkg.PrimaryEvidenceAnnotation))
+		}


nit: if/else for boolean

kzantow · 2025-01-01T04:18:03Z

syft/pkg/cataloger/rust/parse_audit_binary.go

+				p = newPackageFromAudit(&dep, location.WithAnnotation(pkg.EvidenceAnnotationKey, pkg.PrimaryEvidenceAnnotation))
+				continue
+			}
+			p = newPackageWithEnrichment(&dep, cratesEnrichment, location.WithAnnotation(pkg.EvidenceAnnotationKey, pkg.PrimaryEvidenceAnnotation))


This is probably the thing that we need to discuss a bit as a team how best to handle: in the case enrichment is enabled, there is no pkg.RustBinaryAuditEntry metadata created, instead populating a richer, but different, metadata struct type. I've long been a proponent of allowing multiple metadata types, but we don't really have a standard way of doing this yet. I don't think we should have less data when enriching, but we would end up in a situation that potentially something is checking for the pkg.RustBinaryAuditEntry type and it's not found in this case.

I've talked with @wagoodman about this, but I don't think we came to a concrete solution, although since metadata types are arbitrary we could easily add a []any or something similar, and maybe have a helper function to find and return metadata. I don't know if we need this yet, but it definitely looks like some of the fields are being read when outputting different formats from the new enriched data.

If it were me, and the restrictions we have today exist, I might think adding a helper function in the syft/pkg package of something like:

func GetMetadata[T any](p *Package) *T { if t, ok := p.Metadata.(T); ok { return &t } if t, ok := p.Metadata.(*T); ok { return t } if metadatas, ok := p.Metadata.([]any); ok { for _, m := range metadatas { if t, ok := m.(T); ok { return &t } if t, ok := m.(*T); ok { return t } } } return nil }

... or something of the sort. which would let us use it fairly simply where we need it, like:

if m := pkg.GetMetadata[pkg.RustBinaryAuditEntry](p); m != nil { // do something with the metadata }

... and we then could set metadata to []any{ RustBinaryAuditEntry{...}, RustCargoMetadata{...} }. And, though it's not directly applicable here, if we migrated usage of the metadata types to this function instead of the direct type assertions we have, we could then also support merging packages more completely without losing certain metadata, etc..

Sorry for the long-winded comment here, just noting this for discussion along with some background.

jimmystewpot added 7 commits December 31, 2024 10:35

minor tidy up to make the different catalogers identical in structure

1de4964

restructure code to be more consistent with other language packages

929701d

add rust crates.io enricher to gather license and other package metadata

289d32e

make generate-json-schema and generated code

30ade01

fix originator vs supplier field mismatch

51941e9

add crates.io baseURL configuration item, bugfix test

fad34b9

update license list using generate

918bf4d

github-actions bot added the json-schema Changes the json schema label Dec 31, 2024

kzantow reviewed Jan 1, 2025

View reviewed changes

willmurphyscode mentioned this pull request Jan 6, 2025

Configurable package merge behavior #3485

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[rust] add crates.io enichment option for rust audit binary, json schema and spdx license updates. #3554

[rust] add crates.io enichment option for rust audit binary, json schema and spdx license updates. #3554

jimmystewpot commented Dec 31, 2024 •

edited

Loading

kzantow left a comment •

edited

Loading

kzantow Jan 1, 2025

kzantow Jan 1, 2025

kzantow Jan 1, 2025

kzantow Jan 1, 2025

kzantow Jan 1, 2025

kzantow Jan 1, 2025

kzantow Jan 1, 2025

kzantow Jan 1, 2025

kzantow Jan 1, 2025

kzantow Jan 1, 2025

[rust] add crates.io enichment option for rust audit binary, json schema and spdx license updates. #3554

Are you sure you want to change the base?

[rust] add crates.io enichment option for rust audit binary, json schema and spdx license updates. #3554

Conversation

jimmystewpot commented Dec 31, 2024 • edited Loading

Description

Type of change

Checklist:

kzantow left a comment • edited Loading

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

jimmystewpot commented Dec 31, 2024 •

edited

Loading

kzantow left a comment •

edited

Loading