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

feat(nuget): Parse deps specified as PackageReference in .csproj files #121

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Sjord
Copy link

@Sjord Sjord commented Aug 8, 2022

C# project files, with the extension .csproj, are XML files that can specify project dependencies in <PackageReference> tags.

See also:

@CLAassistant
Copy link

CLAassistant commented Aug 8, 2022

CLA assistant check
All committers have signed the CLA.

@Sjord
Copy link
Author

Sjord commented Aug 8, 2022

From the CLA:

You represent that each of Your Contributions is Your original creation

Well, I copied pkg/nuget/config to pkg/nuget/csproj and modified things to get it to work on .csproj files. I guess that's OK, right?

C# project files, with the extension .csproj, are XML files that can specify project dependencies in `<PackageReference>` tags.

See also:
* aquasecurity/trivy#2668
* https://docs.microsoft.com/en-us/nuget/consume-packages/package-references-in-project-files
@Sjord Sjord changed the title Parse deps specified as PackageReference in .csproj files feat(nuget): Parse deps specified as PackageReference in .csproj files Aug 8, 2022
@knqyf263
Copy link
Collaborator

knqyf263 commented Aug 8, 2022

Well, I copied pkg/nuget/config to pkg/nuget/csproj and modified things to get it to work on .csproj files. I guess that's OK, right?

Yes, thanks for signing.

@DmitriyLewen Could you review this PR?

pkg/nuget/csproj/parse.go Outdated Show resolved Hide resolved
pkg/nuget/csproj/parse.go Outdated Show resolved Hide resolved
@Sjord
Copy link
Author

Sjord commented Aug 12, 2022

I now put this in the nuget directory, but I later saw that there is also a dotnet directory. Is csproj correct in the nuget dir, or does dotnet make more sense?

PrivateAssets=all, ExcludeAssets=all, ExcludeAssets=runtime. There are more combinations possible, but these seem to be the most common values to indicate dependencies that are either not transitive, only available at compile time, or not available at all.

PrivateAssets and ExcludeAssets can be specified in either an attribute or a tag, where the tag takes precedence. Values are separated by semicolons and case insensitive.
A floating version specifies a range of versions, such as `1.2.*`. This can't be parsed as an actual version. We strip all `.*` at the end, and reduce to a version such as `1.2`. This is not very accurate. Version 1.2 of a package may not even exist. However, I think it is acceptable for trivy when comparing actual versions with vulnerable versions. When packages before 1.2.5 contain a vulnerability, we want to flag 1.2.* as potentially vulnerable.
@DmitriyLewen
Copy link
Collaborator

DmitriyLewen commented Aug 18, 2022

LGTM.

Is csproj correct in the nuget dir, or does dotnet make more sense?

I think nuget is okay

@Sjord
Copy link
Author

Sjord commented Aug 29, 2022

@knqyf263 This has been reviewed, could you merge this?

@knqyf263
Copy link
Collaborator

knqyf263 commented Sep 1, 2022

The result can be incomplete since it extracts only packages having exact versions, right? We're currently discussing whether we should display these incomplete results or encourage users to generate a lock file. If we show any result, it looks like we properly determined installed packages, but it is not the case actually. It can confuse users.
Any thoughts? Please correct me if my above assumption is wrong.

@Sjord
Copy link
Author

Sjord commented Sep 6, 2022

Yes, this is an opportunistic approach where we can verify some dependencies, but in some cases determining the exact version is not possible. I think this is preferable to not scanning dependencies at all, but I agree this may give false confidence. If a lock file is present, that should be scanned instead. If no lock file is present, it would be a good idea to report that to the user.

As a security consultant, I use trivy to scan my client's projects. Finding any vulnerable dependency allows me to point the client to the importance of keeping packages up to date, and advice them to use a lock file. So completeness is not very important to me, as I use this to indicate a general problem instead of finding which packages need updating.

@bjuraga
Copy link

bjuraga commented Nov 17, 2022

Any chance this gets merged? As it is, it does have some limitations, but i would split the implementation to 2 phases, Optimistic and Deterministic. At the moment we have nothing in the SBOM, and with this implementation, we can at least get most of the package names.

@afrischk
Copy link

Any update on this?

@bjuraga
Copy link

bjuraga commented Mar 15, 2023

Any update on this?

Might be better to use the *.deps.json from your bin folder. We have had a great success in doing so, and that file contains the actual dependencies instead of the requested ones.

@afrischk
Copy link

afrischk commented Mar 15, 2023

This holds true for .NET Core and .NET 5+. I was hoping to be able to use Trivy on an older .NET project with no .deps.json available but PackageReference in .csproj files.

@afrischk
Copy link

afrischk commented Mar 16, 2023

Ok, I found a solution. After reading the comment from @knqyf263 carefully I noticed 'encourage users to generate a lock file'. I read through microsofts docs about packages.lock.json and added a Directory.Build.props file with the following contents to the root directory of my solution.

<Project>
 <PropertyGroup>
   <RestorePackagesWithLockFile>true</RestorePackagesWithLockFile>
 </PropertyGroup>
</Project>

All subprojects are now generating a packages.lock.json after a NuGet restore and Trivy happily parses them :-) I hope this is helpful for anybody having the same issue.

@itrf98
Copy link

itrf98 commented Dec 21, 2023

Dear Team,

Any news about this ? Are you planning to merge it ?

Thank you in advance

@afrischk
Copy link

afrischk commented Jan 1, 2024

@itrf98 have you read @bjuraga , @knqyf263 and my comment about the json files? Is this a solution for you?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants