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

Add an implicit reference to the Generate SBOM task #43151

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

Conversation

gustavoaca1997
Copy link

@gustavoaca1997 gustavoaca1997 commented Aug 31, 2024

Depends on microsoft/sbom-tool#674 to actually create the Microsoft.Sbom.Targets package.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Infrastructure untriaged Request triage from a team member labels Aug 31, 2024
@gustavoaca1997
Copy link
Author

@dotnet-policy-service agree company="Microsoft"

@@ -89,6 +89,7 @@
<MicrosoftWindowsCsWin32PackageVersion>0.3.49-beta</MicrosoftWindowsCsWin32PackageVersion>
<!-- This is the version of the zip archive for the WiX toolset and is different from the NuGet package version format. -->
<WixVersion>3.14.1.8722</WixVersion>
<MicrosoftSbomTargetsPackageVersion>0.0.0-preview.0</MicrosoftSbomTargetsPackageVersion>
Copy link
Author

@gustavoaca1997 gustavoaca1997 Aug 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be updated once we publish an official Nuget package.

  • Update this with the official version.

@marcpopMSFT
Copy link
Member

@baronfel is going to provide some guidance on how to make the package reference work for CPM and for non-CPM (currently this will fail if the customer has CPM enabled).

@@ -253,6 +253,12 @@ Copyright (c) .NET Foundation. All rights reserved.
Condition="'@(PackageReference->AnyHaveMetadataValue('Identity', 'Microsoft.Net.Compilers.Toolset.Framework'))' == 'true'" />
</Target>

<Target Name="_AddGenerateSbomTask" BeforeTargets="CollectPackageReferences">
<ItemGroup Condition=" '$(GenerateSbom)' == 'true' ">
<PackageReference Include="Microsoft.Sbom.Targets" Version="$(MicrosoftSbomTargetsPackageVersion)" IsImplicitlyDefined="true" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so this will be a problem when users have opted into NuGet Central Package Version Management, because CPM requires that users separate the declaration of PackageReferences and their versions.

This is a good opportunity to develop a pattern. Perhaps something like the following:

<ItemGroup Condition=" '$(GenerateSbom)' == 'true' ">
  <PackageReference Include="Microsoft.Sbom.Targets" Version="$(MicrosoftSbomTargetsPackageVersion)" IsImplicitlyDefined="true" Condition="'$(ManagePackageVersionsCentrally)' != 'true' " />
  <!-- If using CPM create the PackageReference and PackageVersion -->
  <PackageReference Include="Microsoft.Sbom.Targets"  IsImplicitlyDefined="true" Condition="'$(ManagePackageVersionsCentrally)' == 'true' " />
  <PackageVersion Include="Microsoft.Sbom.Targets" Version="$(MicrosoftSbomTargetsPackageVersion)" Condition="'$(ManagePackageVersionsCentrally)' == 'true' "/>
</ItemGroup>

cc @JonDouglas / @nkolev92 for feedback on how we should manage SDK-delivered CPM-able dependencies. This will be a Big Deal when we turn on SBOM generation by default for 10.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gonna defer to @jeffkl on this one. @zivkan may have ideas too. @nkolev92 may when he returns.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to handle CPM. IsImplicitlyDefined="true" will tell NuGet that this package is not managed by the customer, and therefore is expected to explicitly have the version set on the PackageReference.

It also means that in VS we won't show it available for updates, and I think that maybe we disable the install/upgrade button even if you search for the package in the browse tab. I'm not yet aware of partners engaging with us about how to allow customers to upgrade implicitly defined packages, hence there's no feature for that at this time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. What about users using lock files? We currently have a really bad story there with implicit packages and the SDK, and this is one more instance of that pain.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤷

lock files basically have two purposes:

  • to ensure that the same package version is restored
  • the package contents (hash) are the same.

If different versions of the SDK pull in different versions of the package, yes, it'll be a bad time, but it's working precisely as intended. One way customers can mitigate this is by using a global.json to pin to a specific .NET SDK, and that generally necessitates using dotnet-install.[ps1|sh] to install the pinned SDK, rather than using the CI agent installed.

Perhaps our teams need to get together and talk more about lock file experiences and how we can make implicit packages work better. But at the same time lock files are used by customers who want deterministic builds, and different versions of the SDK brining in different package versions conflicts with that goal.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, let's set something up soon to talk about it - maybe a Monday partner sync Monday after next or something? cc @dsplaisted

@@ -89,6 +89,7 @@
<MicrosoftWindowsCsWin32PackageVersion>0.3.49-beta</MicrosoftWindowsCsWin32PackageVersion>
<!-- This is the version of the zip archive for the WiX toolset and is different from the NuGet package version format. -->
<WixVersion>3.14.1.8722</WixVersion>
<MicrosoftSbomTargetsPackageVersion>2.2.8</MicrosoftSbomTargetsPackageVersion>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've mirrored this package over (the job is running now)

Copy link

@JonDouglas JonDouglas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR lgtm

Copy link
Member

@baronfel baronfel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need

@JonDouglas
Copy link

JonDouglas commented Sep 10, 2024

manifest.spdx.json

Here's a SPDX generated for autofac's repo: https://github.com/autofac/Autofac

Some quick feedback I saw:

License information seems missing for all components. although defined as <PackageLicenseExpression>MIT</PackageLicenseExpression> by autofac (may need to investigate this further). We may need to turn on <SbomGenerationFetchLicenseInformation> on by default and see what needs to be enabled to capture NuGet license expression. Unfortunately I couldn't get it to work with either setting:

FetchLicenseInformation (-li)             If set to true, we will attempt to fetch license information of packages detected in the SBOM from the ClearlyDefinedApi.
EnablePackageMetadataParsing (-pm)        If set to true, we will attempt to parse license and supplier info from the packages metadata file (RubyGems, NuGet, Maven, Npm).

`FetchLicenseInformation=null, EnablePackageMetadataParsing=null` even though both properties set to true.

@KalleOlaviNiemitalo
Copy link

We may need to turn on <SbomGenerationFetchLicenseInformation> on by default

Please don't.

@JonDouglas
Copy link

We may need to turn on <SbomGenerationFetchLicenseInformation> on by default

Please don't.

Noted. I was referring to just a simple scenario here where no License information is being generated due to properties still reflecting as null.

@baronfel
Copy link
Member

baronfel commented Sep 18, 2024

The SBOM task also spews all over the console output - that is not good MSBuild behavior. Any output from the task needs to happen through MSBuild's Logging APIs in the Task.

 chusk@Chet-Desktop E:\....\sbom-tests> dotnet pack -bl
Restore succeeded with 1 warning(s) in 3.6s
    E:\Code\Scratch\sbom-tests\sbom-tests.csproj : warning NU1604: Project dependency Microsoft.Sbom.Targets does not contain an inclusive lower bound. Include a lower bound in the dependency version to ensure consistent restore results.
You are using a preview version of .NET. See: https://aka.ms/dotnet-support-policy
  sbom-tests net9.0 succeeded with 1 warning(s) (0.1s)
    E:\Code\Scratch\sbom-tests\sbom-tests.csproj : warning NU1604: Project dependency Microsoft.Sbom.Targets does not contain an inclusive lower bound. Include a lower bound in the dependency version to ensure consistent restore results.
  sbom-tests                                                                                  GenerateSbomTarget (9.5s)
##[information]Finding components...                                                                             (9.9s)
##[information]No instructions received to scan docker images.
  sbom-tests                                                                                 GenerateSbomTarget (10.0s)
##[information]Enumerated 39 files and 22 directories in 00:00:00.0231810                                       (10.2s)
##[information]PipReport: No pip found on system. Pip installation report detection will not run.               (10.8s)
                                                          Detection Summary
┌────────────────────────────────┬────────────────────────────────┬────────────────────────────────┬────────────────────────────────┐
│ Component Detector Id          │ Detection Time                 │ # Components Found             │ # Explicitly Referenced        │
├────────────────────────────────┼────────────────────────────────┼────────────────────────────────┼────────────────────────────────┤
│ CocoaPods                      │ 0.1 seconds                    │ 0                              │ 0                              │
│ ConanLock                      │ 0.096 seconds                  │ 0                              │ 0                              │
│ Go                             │ 0.098 seconds                  │ 0                              │ 0                              │
│ GoWithReplace (Beta)           │ 0.098 seconds                  │ 0                              │ 0                              │
│ Gradle                         │ 0.096 seconds                  │ 0                              │ 0                              │
│ Ivy (Beta)                     │ 0.17 seconds                   │ 0                              │ 0                              │
│ Linux                          │ 0.0077 seconds                 │ 0                              │ 0                              │
│ MvnCli                         │ 0.19 seconds                   │ 0                              │ 0                              │
│ Npm                            │ 0.096 seconds                  │ 0                              │ 0                              │
│ NpmLockfile3                   │ 0.096 seconds                  │ 0                              │ 0                              │
│ NpmWithRoots                   │ 0.096 seconds                  │ 0                              │ 0                              │
│ NuGet                          │ 0.098 seconds                  │ 1                              │ 0                              │
│ NuGetPackagesConfig            │ 0.096 seconds                  │ 0                              │ 0                              │
│ NuGetProjectCentric            │ 0.14 seconds                   │ 2                              │ 2                              │
│ Pip                            │ 0.72 seconds                   │ 0                              │ 0                              │
│ PipReport (Beta)               │ 0.24 seconds                   │ 0                              │ 0                              │
│ Pnpm                           │ 0.096 seconds                  │ 0                              │ 0                              │
│ Poetry (Beta)                  │ 0.096 seconds                  │ 0                              │ 0                              │
│ Ruby                           │ 0.096 seconds                  │ 0                              │ 0                              │
│ RustCli                        │ 0.096 seconds                  │ 0                              │ 0                              │
│ RustCrateDetector              │ 0.096 seconds                  │ 0                              │ 0                              │
│ SPDX22SBOM                     │ 0.096 seconds                  │ 0                              │ 0                              │
│ Vcpkg                          │ 0.096 seconds                  │ 0                              │ 0                              │
│ Yarn                           │ 0.096 seconds                  │ 0                              │ 0                              │
│ ────────────────────────────── │ ────────────────────────────── │ ────────────────────────────── │ ────────────────────────────── │
│ Total                          │ 1.1 seconds                    │ 3                              │ 2                              │
└────────────────────────────────┴────────────────────────────────┴────────────────────────────────┴────────────────────────────────┘
##[information]""
##[information]""
##[information]Detection time: 1.1199817 seconds.                                                               (10.8s)
##[information]Scan Manifest file: "C:\\Users\\chusk\\AppData\\Local\\Temp\\ScanManifest_20240918143541642.json"(11.0s)
##[information]Finished execution of the Generate workflow SBOMTelemetry {Result=Success, Errors=ErrorContainer`1 {Count=0, Errors=[]}, Parameters=Configuration {BuildDropPath=ConfigurationSetting`1 {Value="E:\\Code\\Scratch\\sbom-tests\\bin\\Release\\sbom-tests.1.0.0.1edb7ea3.temp", Source=SBOMApi, IsDefaultSource=False}, BuildComponentPath=ConfigurationSetting`1 {Value="E:\\Code\\Scratch\\sbom-tests", Source=SBOMApi, IsDefaultSource=False}, BuildListFile=null, ManifestPath=null, ManifestDirPath=ConfigurationSetting`1 {Value="E:\\Code\\Scratch\\sbom-tests\\bin\\Release\\sbom-tests.1.0.0.1edb7ea3.temp\\_manifest", Source=Default, IsDefaultSource=True}, OutputPath=null, Parallelism=ConfigurationSetting`1 {Value=8, Source=SBOMApi, IsDefaultSource=False}, Verbosity=ConfigurationSetting`1 {Value=Information, Source=SBOMApi, IsDefaultSource=False}, ConfigFilePath=null, ManifestInfo=ConfigurationSetting`1 {Value=[ManifestInfo {Name="SPDX", Version="2.2"}], Source=SBOMApi, IsDefaultSource=False}, HashAlgorithm=null, RootPathFilter=null, CatalogFilePath=null, ValidateSignature=null, IgnoreMissing=null, ManifestToolAction=Generate, PackageName=ConfigurationSetting`1 {Value="sbom-tests", Source=SBOMApi, IsDefaultSource=False}, PackageVersion=ConfigurationSetting`1 {Value="1.0.0", Source=SBOMApi, IsDefaultSource=False}, PackageSupplier=ConfigurationSetting`1 {Value="sbom-tests", Source=SBOMApi, IsDefaultSource=False}, FilesList=null, PackagesList=null, TelemetryFilePath=null, DockerImagesToScan=null, ExternalDocumentReferenceListFile=null, AdditionalComponentDetectorArgs=null, NamespaceUriUni(11.0s)=ConfigurationSetting`1 {Value=null, Source=SBOMApi, IsDefaultSource
ConfigurationSetting`1 {Value="http://spdx.org/spdxdocs/sbom-tests", Source=SBOMApi, IsDefaultSource=False}, GenerationTimestamp=ConfigurationSetting`1 {Value=null, Source=SBOMApi, IsDefaultSource=False}, FollowSymlinks=ConfigurationSetting`1 {Value=True, Source=SBOMApi, IsDefaultSource=False}, DeleteManifestDirIfPresent=ConfigurationSetting`1 {Value=True, Source=SBOMApi, IsDefaultSource=False}, FailIfNoPackages=null, FetchLicenseInformation=null, EnablePackageMetadataParsing=null, SbomPath=null, SbomDir=null}, SBOMFormatsUsed=[SBOMFile {SbomFormatName=ManifestInfo {Name="SPDX", Version="2.2"}, SbomFilePath="E:\\Code\\Scratch\\sbom-tests\\bin\\Release\\sbom-tests.1.0.0.1edb7ea3.temp\\_manifest\\spdx_2.2\\manifest.spdx.json", FileSizeInBytes=7717, TotalNumberOfPackages=4}], Timings=[Timing {EventName="Metadata build time for SPDX:2.2 format", TimeSpan="00:00:00.0096792"}, Timing {EventName="External document reference generation time", TimeSpan="00:00:00.0027449"}, Timing {EventName="Packages generation time", TimeSpan="00:00:00.0318228"}, Timing {EventName="Relationships generation time", TimeSpan="00:00:00.0043172"}, Timing {EventName="Files generation time", TimeSpan="00:00:01.4718744"}, Timing {EventName="Total generation time", TimeSpan="00:00:01.5549652"}], Switches={}, Exceptions={}  sbom-tests succeeded with 1 warning(s) (11.0s) → bin\Release\net9.0\sbom-tests.dll
    E:\Code\Scratch\sbom-tests\sbom-tests.csproj : warning NU1604: Project dependency Microsoft.Sbom.Targets does not contain an inclusive lower bound. Include a lower bound in the dependency version to ensure consistent restore results.

Build succeeded with 3 warning(s) in 15.1s

@baronfel
Copy link
Member

@dsplaisted / @marcpopMSFT / @joeloff - since this is a change to GenerateBundledVersions.props, how do we get that change to be reflected on the redist SDK? Right now for simple usage (build.cmd, eng/dogfood.ps1, dotnet pack) the redist SDK doesn't have the generated version for the new package, so we end up inserting a PackageReference with an empty Version bound.

@marcpopMSFT
Copy link
Member

@baronfel in the past, we've had to merge installer first and then update stage 0. In the future, we won't have the overlay sdk and so should use the live bundled versions. At the moment, I am not sure. this may require two PRs to do it right.

@baronfel
Copy link
Member

Thanks @marcpopMSFT - so for testing purposes folks using this should add a property to their pack commands: -p MicrosoftSbomTargetsPackageVersion=2.2.8

@@ -89,6 +89,7 @@
<MicrosoftWindowsCsWin32PackageVersion>0.3.49-beta</MicrosoftWindowsCsWin32PackageVersion>
<!-- This is the version of the zip archive for the WiX toolset and is different from the NuGet package version format. -->
<WixVersion>3.14.1.8722</WixVersion>
<MicrosoftSbomTargetsPackageVersion>2.2.9</MicrosoftSbomTargetsPackageVersion>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new version was released, so I've mirrored it and bumped this

@JonDouglas
Copy link

Added #43582 for tracking some tests.

Also added proper list of future items in issues here: microsoft/sbom-tool#693

Please feel free to add/remove to them as we track.

@dsplaisted
Copy link
Member

@dsplaisted / @marcpopMSFT / @joeloff - since this is a change to GenerateBundledVersions.props, how do we get that change to be reflected on the redist SDK? Right now for simple usage (build.cmd, eng/dogfood.ps1, dotnet pack) the redist SDK doesn't have the generated version for the new package, so we end up inserting a PackageReference with an empty Version bound.

The way it currently works is that the redist project runs an OverlaySdkOnLKG target which copies a bunch of stuff from stage 0, and then copies the just-built SDK assets on top of it. The bundled versions file is handled specially in the OverrideAndCreateBundledNETCoreAppPackageVersion, where it updates the runtime version to the current version that comes from version.props. So one way of resolving that for SBOM testing would be to update that task to also handle the SBOM version.

Another way would be to use the output of the GenerateBundledVersions target that runs in the redist-installer project. That's probably what we want to do long term, but may require some more non-trivial refactoring of the build process, as I think the redist-installer project is probably built after the redist project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Infrastructure untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants