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.

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

"Unable to locate valid bom ref" when AssemblyName is != project name in referenced project #833

Closed
ag-lls opened this issue Jan 18, 2024 · 15 comments
Labels
bug Something isn't working

Comments

@ag-lls
Copy link

ag-lls commented Jan 18, 2024

We are facing an issue where the the cyclonedx-dotnet app exits with the message Unable to locate valid bom ref and code 7.

The problem can be triggered by the following setup:

  • Project1 references Project2
  • Project2 has the AssemblyName Test.Project2
  • Project2 references Project3

@mtsfoni I have once again created a test project for you to reproduce the problem.
https://github.com/ag-lls/RefTest

The problem does not trigger if Project2 does not reference Project3 or if Project2's AssemblyName is equal to the project name exactly.

We think the issue maybe has something to do with how a referenced project is getting read out as a project reference and as a dependency.


As a project reference, the name of the project is used (in this case Project2), while as a dependency the AssemblyName is used (in this case Test.Project2). This results in the two entries not getting caught by the UnionWith in line 254 in ProjectFileService.cs.

image

image

This seems to be the case because for a dependency (getDotnetDependencys), the tool reads out the projects project.assets.json file, in which the "projectName" field has the AssemblyName as a value.

@github-actions github-actions bot added the triage Don't know what to do with this yet label Jan 18, 2024
@mtsfoni
Copy link
Contributor

mtsfoni commented Jan 18, 2024

What argument do you call it with?

I stumbled over the same problem when using -rs and -ipr in a solution that uses packages.config after updating from 3.0.4 to 3.0.5 and already built a test-case.

We think the issue maybe has something to do with RecursivelyGetProjectReferencesAsync(?).
And in some of the lists/dicts used in RecursivelyGetProjectDotnetDependencysAsync, sometimes projects appears two times, once "normal" with a normal key/name and a version and once with a not "normal" key/name (the full project path) and a missing version. We think that this is the root of the problem, since for that "broken" entry no bom-ref can be found.

#834 should be a fix for this.
But I want to test some more cases before making a version as I feel that by fixing one hole I am always opening another one.

@mtsfoni mtsfoni added bug Something isn't working and removed triage Don't know what to do with this yet labels Jan 18, 2024
@ag-lls
Copy link
Author

ag-lls commented Jan 18, 2024

@mtsfoni i just updated the issue with a better description.
But yes, i also called with -rs and -ipr

@mtsfoni
Copy link
Contributor

mtsfoni commented Jan 18, 2024

Should be the same issue indeed.
Interesting aspect about the AssemblyName, but it should also be utilized with this fix.

I am curious: Do you think the Component's name in the SBOM should be the project or the assembly name? I can see argument for both sides.

@ag-lls
Copy link
Author

ag-lls commented Jan 18, 2024

That's a good question. I can also see arguments for both sides.
On one hand the SBOM shows what gets delivered, on the other hand it shows dependencies in the libraries/executables. So to show what actually gets delivered, assembly name would be better. But to show the dependencies and the.. "connectedness", the project name would maybe be better.
But I really don't know what the right answer would be. I think this is one of those questions where you get a different answer from each person you ask 😅.

I will check out and try the PR and report back 👍

@ag-lls
Copy link
Author

ag-lls commented Jan 18, 2024

@mtsfoni I have tried #834 and it does indeed not exit like before. However what I noticed is that the generated SBOM is not entirely correct (?).

Here is the resulting SBOM:
{
  "bomFormat": "CycloneDX",
  "specVersion": "1.5",
  "serialNumber": "urn:uuid:cf6a69f1-0f78-4e4c-b19c-08244c6545a7",
  "version": 1,
  "metadata": {
    "timestamp": "2024-01-18T13:40:43Z",
    "tools": [
      {
        "vendor": "CycloneDX",
        "name": "CycloneDX module for .NET",
        "version": "1.0.0.0"
      }
    ],
    "component": {
      "type": "application",
      "bom-ref": "Project1@24.1",
      "name": "Project1",
      "version": "24.1"
    }
  },
  "components": [
    {
      "type": "library",
      "bom-ref": "Project2@1.0.0",
      "name": "Project2",
      "version": "1.0.0",
      "scope": "required"
    },
    {
      "type": "library",
      "bom-ref": "Project3@1.0.0",
      "name": "Project3",
      "version": "1.0.0",
      "scope": "required"
    },
    {
      "type": "library",
      "bom-ref": "Test.Project2@1.0.0",
      "name": "Test.Project2",
      "version": "1.0.0",
      "scope": "required"
    }
  ],
  "dependencies": [
    {
      "ref": "Project1@24.1",
      "dependsOn": [
        "Project3@1.0.0",
        "Test.Project2@1.0.0",
        "Project2@1.0.0"
      ]
    },
    {
      "ref": "Project2@1.0.0",
      "dependsOn": [
        "Project3@1.0.0"
      ]
    },
    {
      "ref": "Project3@1.0.0",
      "dependsOn": []
    },
    {
      "ref": "Test.Project2@1.0.0",
      "dependsOn": [
        "Project3@1.0.0"
      ]
    }
  ]
}

What we can see is that Project2 is added two times to the components array (once as Project2 and once as Test.Project2) and two times in the dependency tree. This means our list of components (and the dependency tree) is factually wrong, since only one project (Project2) exists. This also causes Project3 to be a dependency for two components. This could also cause problems when digesting the SBOMs in tools like Dependency-Track, where we would have more components and issues then we actually have, as well as a wrongly displayed Dependency-Graph.

@mtsfoni
Copy link
Contributor

mtsfoni commented Jan 18, 2024

I believe one of them comes from the project.assets.json (which probably use the project name "project2") while the other comes from the recursive scan, where I choose to take the assembly name.

However, I believe the problem reaches a little deeper and there is also some other dependency graph related problems, coming from the fact, that when iterating through the projects I don't know if they have assets file or packages.config. I have to ask you for a few days of patience, until I find the time to dig into that again.

Thank you for trying the PR and bringing the problems to my awareness.

@ag-lls
Copy link
Author

ag-lls commented Mar 21, 2024

Hey @mtsfoni any update on this?

@mtsfoni
Copy link
Contributor

mtsfoni commented Mar 21, 2024

I am quite swamped currently but looking ahead at a week of vacation before the holidays

Expect another release in a week or so

@ag-lls
Copy link
Author

ag-lls commented Mar 27, 2024

Hey @mtsfoni,
i noticed you are working on this currently, i just checked out your latest code. I noticed something.
While the tool doesnt crash, the name (that now seems to be the assembly name), is sometimes not really correct if placeholders are used in the assembly name. So, in short - this:
image
becomes this:
image

You could fix this with maybe something like this?

if (assemblyName.Contains("MSBuildProjectName"))
         {
             assemblyName = assemblyName.Replace("$(MSBuildProjectName)", projectName);
         }

Where projectName would be the.. projectname 😄

@mtsfoni
Copy link
Contributor

mtsfoni commented Mar 27, 2024

Oh hey,

yes caught me :)

Think that's a good workaround for now, thank you for bringing it up before I release something :)

Maybe generally if the name contains "$(" at all, just use the project name as a fallback. Now one would say what else but $(MSBuildProjectName) should be in there, but in the last months of maintaining this, I learned people do all sort of weird things with their project files.

This should be solved with the current code, correct? I seem to have done that 2 months ago, and I have zero memory of it.

@ag-lls
Copy link
Author

ag-lls commented Mar 27, 2024

This should be solved with the current code, correct?

Yes! 👍

I seem to have done that 2 months ago, and I have zero memory of it.

I know that feeling lol 😆

Well, if you would just use the name as fallback, we would miss something in our case. Since its "ABC.Project" and it would then be just "Project", which in turn wouldnt be correct sbom-wise, or would it? So if we replace that part we would still have "ABC.Project" in the end.

But yeah, people do loads of different things. But i hope/feel like the $(MSBuildProjectName) is some of the less wild stuff out there :D

@mtsfoni
Copy link
Contributor

mtsfoni commented Mar 27, 2024

Well, if you would just use the name as fallback, we would miss something in our case. Since its "ABC.Project" and it would then be just "Project", which in turn wouldnt be correct sbom-wise, or would it? So if we replace that part we would still have "ABC.Project" in the end.

Got it, I built it your way. I might add a few more tests, but I intend to release it as it is now, if everything works out.

Thanks for your support.

@ag-lls
Copy link
Author

ag-lls commented Mar 28, 2024

Thanks for fixing this and being on this! 👍

@mtsfoni
Copy link
Contributor

mtsfoni commented Mar 28, 2024

Release is out. I'd appreciate feedback if it works for you as expected.

@ag-lls
Copy link
Author

ag-lls commented Mar 28, 2024

Just tested the new release, works like a charm! 👍

@ag-lls ag-lls closed this as completed Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants