.NET integration follow-up considerations

JonDouglas commented 2 weeks ago

Here is an issue regarding a recent integration effort into the .NET SDK & providing SBOMs for NuGet packages:

WIP / Done

KalleOlaviNiemitalo commented 1 week ago

Moved from https://github.com/microsoft/sbom-tool/pull/674#discussion_r1751708750 to https://github.com/microsoft/sbom-tool/issues/714

Should Microsoft.Sbom.Targets.csproj also set <DevelopmentDependency>true</DevelopmentDependency>? So that, when dotnet add package adds the Microsoft.Sbom.Targets package to a project, the PackageReference item gets this kind of metadata by default:

    <PackageReference Include="Microsoft.Sbom.Targets" Version="2.2.8">
      <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
      <PrivateAssets>all</PrivateAssets>
    </PackageReference>

Documented in https://learn.microsoft.com/nuget/reference/msbuild-targets#pack-target and https://github.com/NuGet/Home/wiki/DevelopmentDependency-support-for-PackageReference.

KalleOlaviNiemitalo commented 1 week ago

Moved to https://github.com/microsoft/sbom-tool/issues/719

I tried adding the Microsoft.Sbom.Targets 2.2.8 package to a project (thus not using the upcoming feature https://github.com/dotnet/sdk/pull/43151) and setting some properties. With .NET SDK 8.0.304, dotnet build and dotnet pack work OK, and the resulting NuGet package contains an SBOM file (although it doesn't contain all the information I expected). However, packing in Visual Studio 2022 does not work:

[REDACTED]\.nuget\packages\microsoft.sbom.targets\2.2.8\buildMultiTargeting\Microsoft.Sbom.Targets.targets(57,5): error MSB6004: The specified task executable location "[REDACTED]\.nuget\packages\microsoft.sbom.targets\2.2.8\buildMultiTargeting\..\tasks\net472\sbom-tool\Microsoft.Sbom.Tool.exe" is invalid.

So it's apparently trying to execute the SBOM CLI Tool as described in https://github.com/microsoft/sbom-tool/pull/674#issue-2469238950:

SbomCLIToolTask.cs is invoked if the MSBuild version targets the "Full" (.NET Framework) runtime bundled with Visual Studio. Because the SBOM API does not support .NET Framework, this class utilizes the SBOM CLI Tool to generate an SBOM.

but the tool is not included in the package.

KalleOlaviNiemitalo commented 1 week ago

Moved to https://github.com/microsoft/sbom-tool/issues/715

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. Perhaps it'll break detection of third-party non-NuGet components that have been copied into the project source directory. Fixing this properly may require changing https://github.com/microsoft/component-detection/ so that the artifacts path can be passed as a separate parameter and each component detector can then decide whether to search for files in the source directory, in the artifacts directory, or both.

KalleOlaviNiemitalo commented 1 week ago

Moved to https://github.com/microsoft/sbom-tool/issues/716

The version number generated by Nerdbank.GitVersioning 3.6.133 does not automatically propagate into the SBOM file generated by Microsoft.Sbom.Targets 2.2.8, which instead defaults to version 1.0.0. I put this thing in my project to fix that:

  <Target Name="SetSbomProperties" BeforeTargets="GenerateSbomTarget" DependsOnTargets="GetBuildVersion">
    <PropertyGroup>
      <SbomGenerationPackageVersion>$(NuGetPackageVersion)</SbomGenerationPackageVersion>
    </PropertyGroup>
  </Target>

but it would be nice if it worked out of the box.

afscrome commented 1 week ago

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.

KalleOlaviNiemitalo commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

Moved to https://github.com/microsoft/sbom-tool/issues/715

Well the default value here is $(MSBuildProjectDirectory)

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

which just won't be able to find project.assets.json in the artifacts output layout, no matter whether SbomGenerationBuildComponentPath is s set eagerly or lazily.

KalleOlaviNiemitalo commented 1 week ago

Moved to https://github.com/microsoft/sbom-tool/issues/713

Spaces between words of $(SbomGenerationPackageSupplier) are removed, e.g. "Contoso Catering GmbH" becomes "ContosoCateringGmbH". I guess it's done here: https://github.com/microsoft/sbom-tool/blob/08ba73d303228eb4d92a6a5f75350d78230bca30/src/Microsoft.Sbom.Targets/SbomInputValidator.cs#L44

Why was it implemented like that? The sbom-tool -ps option preserves internal spaces; and if the NuGetComponentDetector finds a *.nuspec file and reads the authors element, that too preserves internal spaces.

afscrome commented 1 week ago

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 commented 1 week ago

Moved to https://github.com/microsoft/sbom-tool/issues/718

These properties don't take effect when I set them in my project:

<PropertyGroup>
    <SbomGenerationFetchLicenseInformation>true</SbomGenerationFetchLicenseInformation>
    <SbomGenerationEnablePackageMetadataParsing>true</SbomGenerationEnablePackageMetadataParsing>
</PropertyGroup>

They are passed to parameters of the task, but the resulting SBOM contains no license information, and the SBOMTelemetry log entry shows FetchLicenseInformation=null, EnablePackageMetadataParsing=null.

If I instead use sbom-tool with -pm true -li true, then the SBOM shows "licenseConcluded": "MIT" and/or "licenseDeclared": "MIT" for some packages, so the bug is in the MSBuild integration.

AFAICT, src/Microsoft.Sbom.Targets/SbomCLIToolTask.cs for .NET Framework uses those parameters (but then fails because of https://github.com/microsoft/sbom-tool/issues/693#issuecomment-2340410911), but src/Microsoft.Sbom.Targets/GenerateSbomTask.cs for .NET Core ignores the parameters.

KalleOlaviNiemitalo commented 1 week ago

Re computing the properties lazily, I'd prefer doing that in a new target, say SetSbomDefaultProperties, on which GenerateSbomTarget would then depend. That way, if a custom target for non-NuGet packaging uses the GenerateSbom task, it could likewise depend on the SetSbomDefaultProperties target and share the defaulting logic.

KalleOlaviNiemitalo commented 1 week ago

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 commented 2 hours ago

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)

microsoft / sbom-tool

.NET integration follow-up considerations #693