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

.NET integration follow-up considerations #693

Open
4 of 13 tasks
Tracked by #12497
JonDouglas opened this issue Aug 30, 2024 · 16 comments
Open
4 of 13 tasks
Tracked by #12497

.NET integration follow-up considerations #693

JonDouglas opened this issue Aug 30, 2024 · 16 comments
Labels
.NET Pull requests that update .net code

Comments

@jalkire jalkire added the .NET Pull requests that update .net code label Sep 5, 2024
@KalleOlaviNiemitalo

This comment has been minimized.

@KalleOlaviNiemitalo

This comment has been minimized.

@KalleOlaviNiemitalo

This comment has been minimized.

@KalleOlaviNiemitalo

This comment has been minimized.

@afscrome
Copy link

afscrome commented Sep 10, 2024

The GenerateSbomTarget target should include IsPackable in it's condition. If you include this target in a project with IsPackable = false, the targets will try to generate the Sbom, even though there's no package was ever generated.

image

@KalleOlaviNiemitalo
Copy link

KalleOlaviNiemitalo commented Sep 10, 2024

The GenerateSbomTarget target should include IsPackable in it's condition.

However, Microsoft.Sbom.Targets.targets should still define the GenerateSbom task, even if $(IsPackable) == 'false'. I can see myself using the task in custom targets for non-NuGet packaging (e.g. Web Deploy or plain zip).

@afscrome, out of curiosity, are you setting GenerateSBOM as a global property, like dotnet pack -p:GenerateSBOM=true?

@afscrome
Copy link

afscrome commented Sep 10, 2024

My original mistake was that I expected this to be usable on Publishing as well as packable, so added the following to my Directory.Build.Targets file

<GenerateSBOM Condition="$(IsPublishable) or $(IsPackable)" >true</GenerateSBOM>

The project in question was Publishable, but not Packable, so I ended up with GenerateSbom=true on a non packable project.

That said, I would very much like to set this globally - e.g. dotnet pack FooBar.sln -p:GenerateSBOM=true and have sboms only generated for packable projects (and publishable ones when that gets implemented). We have some central template we use for building a variety of products, and as we work on SLSA V1 compliance, would like to forcibly generate the SBOMs as part of those templates, rather than relying on individual teams opting in (and then not opting out / breaking things).

@baronfel
Copy link
Member

We hear you loud and clear about publishable projects - the first iteration of this integration in .NET for 9.0.100 is aimed squarely at NuGet packages, but we hope to follow in a subsequent release for published applications. This is great feedback to hear.

@KalleOlaviNiemitalo thank you for kicking the tires! I generally agree with everything you've said here (and especially thank you for trying it in VS).

@baronfel
Copy link
Member

Microsoft.Sbom.Targets 2.2.8 doesn't find the names of referenced NuGet packages when I use it with .NET SDK 8.0.304 in a project that specifies artifacts output layout in Directory.Build.props:

  <PropertyGroup>
    <UseArtifactsOutput>true</UseArtifactsOutput>
    <ArtifactsPath>$(MSBuildThisFileDirectory).artifacts</ArtifactsPath>
  </PropertyGroup>

Specifically, the NuGet and NuGetProjectCentric component detectors do not detect any components in this case.

It seems this can be fixed by setting <SbomGenerationBuildComponentPath>$(BaseIntermediateOutputPath)</SbomGenerationBuildComponentPath>, but I'm not sure whether that could break something else.

I think this is a single instance of a general problem - all of the properties here should be lazily computed when the SBOM generation task runs instead of being 'pinned' when the targets file is read. This lets user logic/packages/etc influence those properties safely.

@KalleOlaviNiemitalo

This comment has been minimized.

@KalleOlaviNiemitalo

This comment has been minimized.

@afscrome
Copy link

My org has a number of packages with metadata configured along the lines of

<Company>Acme Inc</Company>
<Authors>Team Name</Authors>

In my view it would make more sense to base SbomGenerationPackageSupplier on the Company field than the Authors. (or at least to prefer Company over Authors. This could also provide a bit more consistency when support is implemented for publishing as Authors may not be defined on non packable projects (as it doesn't do anything outside of a nuget package)

https://github.com/microsoft/sbom-tool/blob/08ba73d303228eb4d92a6a5f75350d78230bca30/src/Microsoft.Sbom.Targets/Microsoft.Sbom.Targets.targets#L21C51-L21C80

@KalleOlaviNiemitalo

This comment has been minimized.

@KalleOlaviNiemitalo

This comment has been minimized.

@KalleOlaviNiemitalo
Copy link

The UnzipGuid, ShortUnzipGuidFolder, and NugetPackageUnzip properties should preferably be renamed to something that includes "Sbom", to minimise the risk of conflicts with properties used for other purposes.

But I wonder how necessary a random number even is here. Perhaps the value of $(NugetPackageUnzip) could be just something like $(IntermediateOutputPath)sbom.tmp with no randomness at all. That change might make the unzipped files less likely to exceed the Windows PATH_MAX limit, too.

@JonDouglas
Copy link
Member Author

JonDouglas commented Sep 19, 2024

hi folks,

i have filed a handful of issues here to track various things so far. i would encourage y'all who feel strongly about certain topics to file issues you're passionate for and we can add them to this mini-epic to track for now. thanks again for all the feedback so far!

edit: thank you so much @KalleOlaviNiemitalo (that was fast)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.NET Pull requests that update .net code
Projects
None yet
Development

No branches or pull requests

5 participants