feat: use source as vcs url

[barblin]

No description provided.

[barblin]

@Shnatsel, what do you think?

[barblin]

@Shnatsel, what do you think?

@justahero ?

[Shnatsel]

Heya, sorry this slipped through the cracks. Could you explain the motivation behind this change? What does this accomplish?

[barblin]

Heya, sorry this slipped through the cracks. Could you explain the motivation behind this change? What does this accomplish?

@Shnatsel attempts to add the repo url as a VCS ExternalReference, to enrich the component information and link it to an origin repository

[Shnatsel]

According to https://doc.rust-lang.org/cargo/commands/cargo-metadata.html#json-format this field is meant to be opaque and its content is not stable between versions and does not conform to any particular format:

        /* The source ID of the package, an "opaque" identifier representing
           where a package is retrieved from. See "Compatibility" above for
           the stability guarantee.

           This is null for path dependencies and workspace members.

           For other dependencies, it is a string with the format:
           - "registry+URL" for registry-based dependencies.
             Example: "registry+https://github.com/rust-lang/crates.io-index"
           - "git+URL" for git-based dependencies.
             Example: "git+https://github.com/rust-lang/cargo?rev=5e85ba14aaa20f8133863373404cb0af69eeef2c#5e85ba14aaa20f8133863373404cb0af69eeef2c"
           - "sparse+URL" for dependencies from a sparse registry
             Example: "sparse+https://my-sparse-registry.org"

           The value after the `+` is not explicitly defined, and may change
           between versions of Cargo and may not directly correlate to other
           things, such as registry definitions in a config file. New source
           kinds may be added in the future which will have different `+`
           prefixed identifiers.
        */

I'm not sure it's a good idea to rely on parsing an explicitly opaque value.

[barblin]

According to https://doc.rust-lang.org/cargo/commands/cargo-metadata.html#json-format this field is meant to be opaque and its content is not stable between versions and does not conform to any particular format:

        /* The source ID of the package, an "opaque" identifier representing
           where a package is retrieved from. See "Compatibility" above for
           the stability guarantee.

           This is null for path dependencies and workspace members.

           For other dependencies, it is a string with the format:
           - "registry+URL" for registry-based dependencies.
             Example: "registry+https://github.com/rust-lang/crates.io-index"
           - "git+URL" for git-based dependencies.
             Example: "git+https://github.com/rust-lang/cargo?rev=5e85ba14aaa20f8133863373404cb0af69eeef2c#5e85ba14aaa20f8133863373404cb0af69eeef2c"
           - "sparse+URL" for dependencies from a sparse registry
             Example: "sparse+https://my-sparse-registry.org"

           The value after the `+` is not explicitly defined, and may change
           between versions of Cargo and may not directly correlate to other
           things, such as registry definitions in a config file. New source
           kinds may be added in the future which will have different `+`
           prefixed identifiers.
        */

I'm not sure it's a good idea to rely on parsing an explicitly opaque value.

Thats why we don't extract the repository url if the field does not exist

[barblin]

@Shnatsel what do you think?

[Shnatsel]

Could this instead be accomplished by parsing the id field, which is fully specified and stable?

https://doc.rust-lang.org/cargo/reference/pkgid-spec.html

[kornelski]

This field is already (not quite correctly) used:

cyclonedx-rust-cargo/cargo-cyclonedx/src/purl.rs

Line 17 in 22cac58

if let Some(source) = &package.source {

[Shnatsel]

I'm going over the outstanding PRs and this is the last one standing. Could you please elaborate on two things:

1. What is the motivation for this change? Specific use cases, etc.
2. Can this be accomplished by parsing the fully specified and stable id field as opposed to the explicitly unstable and opaque source field?

[beer4code]

I'm watching this PR already for some time and can share our primary use-case:

• When we pull an internal crate via git = "https://git.corp.example/team/foo.git", our security team needs to know where that code lives. Without the VCS external reference, they have to parse the PURL, which is cumbersome since every PURL-supported ecosystem uses different qualifiers and there are variations in how these qualifiers are named.
• We use DependencyTrack and Guac to process externalReferences as structured data. These tools don't decompose PURL qualifiers but nicely display VCS links in UI when they are encoded the way foreseen by CycloneDX.

There is also a motivation from a CycloneDX BOM perspective:

• The currently used vcs_url PURL qualifier is not officially part of the PURL specs for cargo (see here).
• To my understanding, the CycloneDX spec defines externalReferences with an explicit vcs type precisely for this use-case, i.e., to provide a first-class link to VCS. Not populating it when the data is available means the SBOM is less complete than it could be.

Regarding the pkgid-spec question, I think you're right. Not sure if the original author wants to follow-up on this but I'd aim for something like the following:

    let id_str = package.id.to_string();
    if id_str.starts_with("git+") {
        let vcs_url = id_str
            .split_once('#')
            .map_or(id_str.as_str(), |(url, _)| url);
        match Uri::try_from(vcs_url.to_string()) {
            Ok(uri) => references.push(ExternalReference::new(ExternalReferenceType::Vcs, uri)),
            Err(e) => log::warn!(
                "Package {} has an invalid repository URI (from id: {}): {} ",
                package.name,
                package.id,
                e
            ),
        }
    }

I guess this should also be adjusted for the PURL identifier to address #799.

[Shnatsel]

@beer4code thanks for the info! Let's not wait on the original author. Would you go ahead and open a new PR with your version?

[beer4code]

Thanks for the fast response. Yes, can do. I'll take care of it in the next few days 👍

[Shnatsel]

The currently used vcs_url PURL qualifier is not officially part of the PURL specs for cargo (see here).

This is defined for all PURL types, see here.

But sure, we can duplicate that info to improve interop.

[Shnatsel]

There's also a big open question about qualifiers. cargo pkgid may include qualifiers after the URL, e.g. git+ssh://git@github.com/rust-lang/regex.git?branch=dev#regex@1.4.3 where ?branch=dev is a qualifier. The implementation proposed in this PR would produce git+ssh://git@github.com/rust-lang/regex.git?branch=dev which can't be git cloned directly, you'd have to strip the qualifier before using that.

If we include the qualifier, it's also not clear in what format it should appear. The PURL spec says that for vcs_url field it should be the SPDX package download location, but its use of @ as a delimiter is odd and doesn't really gel with how URIs work; I'm not sure if it's valid under RFC 3986 at all.

I also can't find any guidance on whether we should include git+ssh://... or just ssh://... or what.

[Shnatsel]

I looked up the ECMA spec since it's more complete than the earlier iterations, but it only says:

The URI (URL or URN) to the external reference. External references are URIs and therefore can accept any URL scheme including https (RFC-7230), mailto (RFC-2368), tel (RFC-3966), and dns (RFC-4501).
External references may also include formally registered URNs such as CycloneDX BOM-Link to reference CycloneDX BOMs or any object within a BOM. BOM-Link transforms applicable external references into relationships that can be expressed in a BOM or across BOMs.

I guess we'll need to look at what existing tools write and what they do with it.

[beer4code]

Thanks for the additional research!

This is defined for all PURL types, see here.

You're correct, I've indeed missed that.

I also can't find any guidance on whether we should include git+ssh://... or just ssh://... or what.

• To my knowledge, the git+ prefix merely tells the package manager which underlying version control system to invoke, both representations are valid though.
• Looking at other CycloneDX plugins, e.g., cyclonedx-gradle-plugin (here) or cyclonedx-javascript-library (here), my understanding is that a basic sanitization check like url::Url::parse(string) would make sense. Curious about your thoughts here.

I'm afraid every vague specification leaves room for interpretation and variation.