[BUG] Relationships in SPDX sbom pointing in wrong direction

[antonbauhofer]

Is there an existing issue for this?

• I have searched the existing issues

This issue exists in the latest npm version

• I am using the latest npm

Current Behavior

When creating an SPDX sbom, some of the contained relationships are incorrect.
For example, for the npm/cli repository, the following relationship is determined:
{
"spdxElementId": "SPDXRef-Package-npm-10.1.0",
"relatedSpdxElement": "SPDXRef-Package-npmcli.eslint-config-4.0.2",
"relationshipType": "DEV_DEPENDENCY_OF"
}

Expected Behavior

According to the SPDX specification, the relationship should point in the other direction:
{
"spdxElementId": "SPDXRef-Package-npmcli.eslint-config-4.0.2",
"relatedSpdxElement": "SPDXRef-Package-npm-10.1.0",
"relationshipType": "DEV_DEPENDENCY_OF"
}

Steps To Reproduce

1. Clone the latest unstable master of npm/cli repository according to the contributing manual
2. Create an SPDX sbom with node . sbom --sbom-format spdx
3. Search for any relationships with "relationshipType": "DEV_DEPENDENCY_OF"
4. Compare the identified relationships with those specified in the respective package.json files

Environment

• npm: 10.2.0
• Node.js: 18.12.1
• OS Name: macOS Ventura 13.4
• System Model Name: Irrelevant
• npm config: Irrelevant

[wraithgar]

I don't think this is wrong, this was brought up during the implementation PR and the current direction matches guidance from the SPDX community, namely that

"directionality" of the relationship is implied by the spdxElementId/relatedSpdxElement fields not by the relationshipType

#6801

[bdehamer]

The current implementation was based on guidance that we received from the SPDX maintainers. The way it was explained to me is that the relationship names should be considered non-directional. Most tooling which consumes SBOMs will treat the relationships as a directed graph so the important bit is to make sure that the parent (spdxElementId) / child (relatedSpdxElement) relationship is accurately represented. Switching the references like you've done in your PR breaks the graph.

The fact that some of the relationship labels have inverse variants (DEPENDS_ON vs DEPENDENCY_OF) while others do not (DEV_DEPENDENCY_OF vs ??) make things extra confusing -- but I understand that this will be cleaned-up in SPDX 3.

[maxhbr]

This issue is not resolved and my comment in #6801 was probably missunderstood.

Can you please reopen the ticket? The currently produced SPDX files do not represent the correct dependency tree.

[maxhbr]

I don't think this is wrong, this was brought up during the implementation PR and the current direction matches guidance from the SPDX community, namely that

"directionality" of the relationship is implied by the spdxElementId/relatedSpdxElement fields not by the relationshipType

Hey @wraithgar, that statement is wrong.

There are two parts to that determine "directionality",

1. the type of the relationship and
2. the from and to (which are called spdxElementId and relatedSpdxElement)

Therefore the relationships

  {
      "spdxElementId": "SPDXRef-Package-d-1.0.0",
      "relatedSpdxElement": "SPDXRef-Package-a-1.0.0",
      "relationshipType": "DEPENDENCY_OF"
    }

and

 {
      "spdxElementId": "SPDXRef-Package-a-1.0.0",
      "relatedSpdxElement": "SPDXRef-Package-d-1.0.0",
      "relationshipType": "DEPENDS_ON"
 }

are equivalent, since the label was inverted and the from and to were swapped.

But the following relationships are not equivalent:

  {
      "spdxElementId": "SPDXRef-Package-d-1.0.0",
      "relatedSpdxElement": "SPDXRef-Package-a-1.0.0",
      "relationshipType": "DEPENDENCY_OF"
    },

and

 {
      "spdxElementId": "SPDXRef-Package-d-1.0.0",
      "relatedSpdxElement": "SPDXRef-Package-a-1.0.0",
      "relationshipType": "DEPENDS_ON"
 }

So, the choice of label ("DEPENDENCY_OF" vs "DEPENDS_ON") matters and is done inconsistently in the current implementation (mixing "*_OF" with "*_ON").

[maxhbr]

Most tooling which consumes SBOMs will treat the relationships as a directed graph so the important bit is to make sure that the parent (spdxElementId) / child (relatedSpdxElement) relationship is accurately represented

I agree that the direction of the arrows should not be inverted. Nevertheless the relationshipTypes need to be fixed

[antonbauhofer]

We created a new PR to address the issue and keep the order of (spdxElementId) and (relatedSpdxElement) as intuitive as possible, while adhering to the SPDX specifications.

[bdehamer]

Let me explain the thought process and the guidance I received that yielded the current implementation . . .

Setting aside the issue of the relationshipType label for a moment, I think the most important requirement is that the directed graph of dependencies consistently represent the parent (spdxElementId) / child (relatedSpdxElement) relationship.

For example, take the following:

"relationships": [
  {
    "spdxElementId": "SPDXRef-DOCUMENT",
    "relatedSpdxElement": "SPDXRef-Package-hello-world-1.0.0",
    "relationshipType": "DESCRIBES"
  },
  {
    "spdxElementId": "SPDXRef-Package-hello-world-1.0.0",
    "relatedSpdxElement": "SPDXRef-Package-ms-2.1.3",
    "relationshipType": "HAS_PREREQUISITE"
  },
  {
    "spdxElementId": "SPDXRef-Package-hello-world-1.0.0",
    "relatedSpdxElement": "SPDXRef-Package-ci-info-3.9.0",
    "relationshipType": "DEPENDS_ON"
  }
]

It's pretty clear that this represents the following tree of dependencies:

hello-world@1.0.0
├── ci-info@3.9.0
└── ms@2.1.3

Now you could argue that the following is an equivalent representation (note how some of the relationships are reversed similar to #6871):

"relationships": [
  {
    "spdxElementId": "SPDXRef-DOCUMENT",
    "relatedSpdxElement": "SPDXRef-Package-hello-world-1.0.0",
    "relationshipType": "DESCRIBES"
  },
  {
    "spdxElementId": "SPDXRef-Package-ms-2.1.3",
    "relatedSpdxElement": "SPDXRef-Package-hello-world-1.0.0",
    "relationshipType": "PREQUISITE_FOR"
  },
  {
    "spdxElementId": "SPDXRef-Package-ci-info-3.9.0",
    "relatedSpdxElement": "SPDXRef-Package-hello-world-1.0.0",
    "relationshipType": "DEPENDENCY_OF"
  }
]

While both forms are valid according to the SPDX schema (and semantically equivalent to any person who might read them), in practice, I've found that much of the tooling written to consume SPDX documents can't make any sense of the second version because it breaks the expectations for how a directed-graph should be represented -- the source/parent --> destination/child relationship is not consistently represented.

Take for example, the bom utility . . . running the first example through the utility yields:

$ bom document outline ./spdx.json       
               _      
 ___ _ __   __| |_  __
/ __| '_ \ / _` \ \/ /
\__ \ |_) | (_| |>  < 
|___/ .__/ \__,_/_/\_\
    |_|               

 📂 SPDX Document hello-world@1.0.0
  │ 
  │ 📦 DESCRIBES 1 Packages
  │ 
  ├ hello-world@1.0.0
  │  │ 🔗 2 Relationships
  │  ├ HAS_PREREQUISITE PACKAGE ms@2.1.3
  │  └ DEPENDS_ON PACKAGE ci-info@3.9.0
  │ 
  └ 📄 DESCRIBES 0 Files

While the second example results in the following:

$ bom document outline ./spdx.json
WARN 2 packages could not be assigned to the SBOM 
               _      
 ___ _ __   __| |_  __
/ __| '_ \ / _` \ \/ /
\__ \ |_) | (_| |>  < 
|___/ .__/ \__,_/_/\_\
    |_|               

 📂 SPDX Document hello-world@1.0.0
  │ 
  │ 📦 DESCRIBES 1 Packages
  │ 
  ├ hello-world@1.0.0
  │  └ 🔗 0 Relationships
  └ 📄 DESCRIBES 0 Files

With that constraint of representing all of the parent/child relationships consistently, the next question is what labels (or relationshipTypes) should be assigned.

For prod and peer dependencies, the answer is fairly straightforward given that there exists relationship types that read naturally with the direction of the relationship: DEPENDS_ON/HAS_PREREQUISITE.

Unfortunately, the story isn't as clear for development and optional dependencies -- the corresponding relationship types have only a single variant and don't seem to imply the correct relationship "directionality": DEV_DEPENDENCY_OF/OPTIONAL_DEPENDENCY_OF (If only there existed HAS_DEVELOPMENT_DEPENDENCY and HAS_OPTIONAL_DEPENDENCY we wouldn't even be having this conversation 😆)

When discussing this with Adolfo Veytia as part of the RFC process, the guidance I got was that, despite their name, the relationship kinds are non-directional.

He suggested that, even if it didn't read naturally, it was preferable to have a backwards label over having disconnected nodes in the graph.

With that in mind, I implemented the graph consistently and used the somewhat clunky relationship types for the development and optional dependencies.

$ bom document outline ./spdx.json       
               _      
 ___ _ __   __| |_  __
/ __| '_ \ / _` \ \/ /
\__ \ |_) | (_| |>  < 
|___/ .__/ \__,_/_/\_\
    |_|               

 📂 SPDX Document hello-world@1.0.0
  │ 
  │ 📦 DESCRIBES 1 Packages
  │ 
  ├ hello-world@1.0.0
  │  │ 🔗 4 Relationships
  │  ├ HAS_PREREQUISITE PACKAGE ms@2.1.3
  │  ├ DEPENDS_ON PACKAGE ci-info@3.9.0
  │  ├ OPTIONAL_DEPENDENCY_OF PACKAGE lodash@4.17.21
  │  └ DEV_DEPENDENCY_OF PACKAGE exit@0.1.2
  │ 
  └ 📄 DESCRIBES 0 Files

I agree that some of those labels don't read nicely, but it seems preferable to the alternative:

$ bom document outline ./spdx.json       
WARN 2 packages could not be assigned to the SBOM 
               _      
 ___ _ __   __| |_  __
/ __| '_ \ / _` \ \/ /
\__ \ |_) | (_| |>  < 
|___/ .__/ \__,_/_/\_\
    |_|               

 📂 SPDX Document hello-world@1.0.0
  │ 
  │ 📦 DESCRIBES 1 Packages
  │ 
  ├ hello-world@1.0.0
  │  │ 🔗 2 Relationships
  │  ├ HAS_PREREQUISITE PACKAGE ms@2.1.3
  │  └ DEPENDS_ON PACKAGE ci-info@3.9.0
  │ 
  └ 📄 DESCRIBES 0 Files

[maxhbr]

Thanks for this detailed commend and you have a good point here. You are right that the correct representation is more complex to parse, as one can not just follow edges of an graph ignoring the types.

I created kubernetes-sigs/bom#354 to track that bug in the bom tool.

[maxhbr]

He suggested that, even if it didn't read naturally, it was preferable to have a backwards label over having disconnected nodes in the graph.

I do not think that it is an issue of readability. I think the statement that is encoded is wrong. Lets look at the definition of OPTIONAL_DEPENDENCY_OF:

Relationship	Description	Example
OPTIONAL_DEPENDENCY_OF	Is to be used when SPDXRef-A is an optional dependency of SPDXRef-B.	Use when building the code will proceed even if a dependency cannot be found, fails to install, or is only installed on a specific platform. For example, A is in the optionalDependencies scope of npm project B.

So, if I substitute it with the example from above I get the statement "hello-world@1.0.0 is an optional dependency of lodash@4.17.21.", which is not what should be expressed. Note that the Example actually talks about a similar npm example.