Convert a SPDX purl externalReference into the item level purl field #394
Conversation
Signed-off-by: David Leadbeater <dgl@dgl.cx>
andreas-hilti
left a comment
andreas-hilti
left a comment
There was a problem hiding this comment.
The readme states:
SPDX supports multiple CPEs and PURLs for a package. But doesn't support specifying if any are a component identifier.
Can you comment on this (and in particular the current state)?
| "licenseDeclared": "NOASSERTION", | ||
| "name": "Jena", | ||
| "primaryPackagePurpose": "APPLICATION", | ||
| "purl": "pkg:maven/org.apache.jena/apache-jena@3.12.0", |
There was a problem hiding this comment.
Is this valid here? (I don't see it in the SPDX schema.)
| } | ||
| component.Properties.AddSpdxElement(refPropName, refPropValue); | ||
|
|
||
| if (refPropName == PropertyTaxonomy.EXTERNAL_REFERENCE_PACKAGE_MANAGER_PURL) |
There was a problem hiding this comment.
As far as I can see, SPDX (in principle) allows multiple external references of type PURL. How do we want to handle this case?
There was a problem hiding this comment.
I suspect it should take the first, while not ideal and entirely correct, it makes it work.
SPDX 3.0 does add a proper packageUrl field so if later support is added for 3.0 the CDX and SPDX 3.0 conversion can be more correct.
|
|
||
| if (refPropName == PropertyTaxonomy.EXTERNAL_REFERENCE_PACKAGE_MANAGER_PURL) | ||
| { | ||
| component.Purl = refPropValue; |
There was a problem hiding this comment.
Do we still want to do component.Properties.AddSpdxElement(refPropName, refPropValue); (i.e. add it to the properties) if we end up here?
There was a problem hiding this comment.
No, but we need to properly roundtrip if we do that. I've updated the PR to do so.
Signed-off-by: David Leadbeater <dgl@dgl.cx>
In SPDX 2 it is true that external ref is the only way to represent a PURL, but it is common for there to only be one purl (although I don't have a massive sample of SPDX files to confirm that). In SPDX 3 there is support for using a purl as an identifier: |
|
I’d mostly go with Andreas' judgment regarding this PR |
There was a problem hiding this comment.
I'm not very familiar with SPDX, but having a Purl is quite crucial to do the vulnerability analysis. Thus, I think this is ok as a compromise.
However, we should document this in the readme.
Maybe we should get some feedback from other users of the SPDX to CDX conversion.
@dgl could you document this in the readme as suggested @andreas-hilti ? |
|
@dgl quick bump - one tiny thing left before we can merge 👍 |
3 participants
This (partly, depending if the request for
cpein a comment is considered) addresses CycloneDX/cyclonedx-cli#424.