From f4620fe206c4ea066dd8cb06a05c08ab91e9d359 Mon Sep 17 00:00:00 2001 From: Michael Cline Date: Fri, 1 Nov 2024 10:44:24 +0100 Subject: [PATCH 1/3] Made the Timeout in LicenseInformationService configurable via CLI argument (#584) --- src/Microsoft.Sbom.Api/Config/Args/GenerationArgs.cs | 9 +++++++++ .../Executors/ComponentDetectionBaseWalker.cs | 10 +++++++++- .../Executors/ILicenseInformationFetcher.cs | 6 +++++- .../Executors/ILicenseInformationService.cs | 2 ++ .../Executors/LicenseInformationFetcher.cs | 7 ++++++- .../Executors/LicenseInformationService.cs | 8 ++++++-- src/Microsoft.Sbom.Common/Config/Configuration.cs | 9 +++++++++ src/Microsoft.Sbom.Common/Config/IConfiguration.cs | 6 ++++++ src/Microsoft.Sbom.Common/Config/InputConfiguration.cs | 4 ++++ 9 files changed, 56 insertions(+), 5 deletions(-) diff --git a/src/Microsoft.Sbom.Api/Config/Args/GenerationArgs.cs b/src/Microsoft.Sbom.Api/Config/Args/GenerationArgs.cs index 3d87b243a..914e213f0 100644 --- a/src/Microsoft.Sbom.Api/Config/Args/GenerationArgs.cs +++ b/src/Microsoft.Sbom.Api/Config/Args/GenerationArgs.cs @@ -116,6 +116,15 @@ public class GenerationArgs : GenerationAndValidationCommonArgs [ArgDescription("If set to true, we will attempt to fetch license information of packages detected in the SBOM from the ClearlyDefinedApi.")] public bool? FetchLicenseInformation { get; set; } + /// + /// Specifies the timeout in seconds for fetching the license information. Defaults to 30 seconds. Has no effect if + /// FetchLicenseInformation (li) argument is false or not provided. + /// + [ArgShortcut("lto")] + [ArgDescription("Specifies the timeout in seconds for fetching the license information. Defaults to 30 seconds. " + + "Has no effect if the FetchLicenseInformation (li) argument is false or not provided. ")] + public int? LicenseInformationTimeout { get; set; } + /// /// If set to true, we will attempt to parse license and supplier info from the packages metadata file. /// diff --git a/src/Microsoft.Sbom.Api/Executors/ComponentDetectionBaseWalker.cs b/src/Microsoft.Sbom.Api/Executors/ComponentDetectionBaseWalker.cs index d4240c50c..e0e1ca5f0 100644 --- a/src/Microsoft.Sbom.Api/Executors/ComponentDetectionBaseWalker.cs +++ b/src/Microsoft.Sbom.Api/Executors/ComponentDetectionBaseWalker.cs @@ -137,7 +137,15 @@ async Task Scan(string path) { licenseInformationRetrieved = true; - var apiResponses = await licenseInformationFetcher.FetchLicenseInformationAsync(listOfComponentsForApi); + List apiResponses; + if (configuration.LicenseInformationTimeout is null) + { + apiResponses = await licenseInformationFetcher.FetchLicenseInformationAsync(listOfComponentsForApi); + } + else + { + apiResponses = await licenseInformationFetcher.FetchLicenseInformationAsync(listOfComponentsForApi, configuration.LicenseInformationTimeout.Value); + } foreach (var response in apiResponses) { diff --git a/src/Microsoft.Sbom.Api/Executors/ILicenseInformationFetcher.cs b/src/Microsoft.Sbom.Api/Executors/ILicenseInformationFetcher.cs index b5e7a719e..c0f28f931 100644 --- a/src/Microsoft.Sbom.Api/Executors/ILicenseInformationFetcher.cs +++ b/src/Microsoft.Sbom.Api/Executors/ILicenseInformationFetcher.cs @@ -17,12 +17,16 @@ public interface ILicenseInformationFetcher /// List ConvertComponentsToListForApi(IEnumerable scannedComponents); + /// > + Task> FetchLicenseInformationAsync(List listOfComponentsForApi); + /// /// Calls the ClearlyDefined API to get the license information for the list of components. /// /// A list of strings formatted into a list of strings that can be used to call the batch ClearlyDefined API. + /// Timeout in seconds to use when making web requests /// - Task> FetchLicenseInformationAsync(List listOfComponentsForApi); + Task> FetchLicenseInformationAsync(List listOfComponentsForApi, int timeout); /// /// Gets the dictionary of licenses that were fetched from the ClearlyDefined API. diff --git a/src/Microsoft.Sbom.Api/Executors/ILicenseInformationService.cs b/src/Microsoft.Sbom.Api/Executors/ILicenseInformationService.cs index 675c46137..2b09a65ca 100644 --- a/src/Microsoft.Sbom.Api/Executors/ILicenseInformationService.cs +++ b/src/Microsoft.Sbom.Api/Executors/ILicenseInformationService.cs @@ -9,4 +9,6 @@ namespace Microsoft.Sbom.Api.Executors; public interface ILicenseInformationService { public Task> FetchLicenseInformationFromAPI(List listOfComponentsForApi); + + public Task> FetchLicenseInformationFromAPI(List listOfComponentsForApi, int timeout); } diff --git a/src/Microsoft.Sbom.Api/Executors/LicenseInformationFetcher.cs b/src/Microsoft.Sbom.Api/Executors/LicenseInformationFetcher.cs index 47dc6f7d1..59ed5a70c 100644 --- a/src/Microsoft.Sbom.Api/Executors/LicenseInformationFetcher.cs +++ b/src/Microsoft.Sbom.Api/Executors/LicenseInformationFetcher.cs @@ -84,7 +84,12 @@ public List ConvertComponentsToListForApi(IEnumerable public async Task> FetchLicenseInformationAsync(List listOfComponentsForApi) { - return await licenseInformationService.FetchLicenseInformationFromAPI(listOfComponentsForApi); + return await FetchLicenseInformationAsync(listOfComponentsForApi, 30); + } + + public async Task> FetchLicenseInformationAsync(List listOfComponentsForApi, int timeout) + { + return await licenseInformationService.FetchLicenseInformationFromAPI(listOfComponentsForApi, timeout); } // Will attempt to extract license information from a clearlyDefined batch API response. Will always return a dictionary which may be empty depending on the response. diff --git a/src/Microsoft.Sbom.Api/Executors/LicenseInformationService.cs b/src/Microsoft.Sbom.Api/Executors/LicenseInformationService.cs index c2a2a1504..ee435f345 100644 --- a/src/Microsoft.Sbom.Api/Executors/LicenseInformationService.cs +++ b/src/Microsoft.Sbom.Api/Executors/LicenseInformationService.cs @@ -20,7 +20,6 @@ public class LicenseInformationService : ILicenseInformationService private readonly ILogger log; private readonly IRecorder recorder; private readonly HttpClient httpClient; - private const int ClientTimeoutSeconds = 30; public LicenseInformationService(ILogger log, IRecorder recorder, HttpClient httpClient) { @@ -30,6 +29,11 @@ public LicenseInformationService(ILogger log, IRecorder recorder, HttpClient htt } public async Task> FetchLicenseInformationFromAPI(List listOfComponentsForApi) + { + return await FetchLicenseInformationFromAPI(listOfComponentsForApi, 30); + } + + public async Task> FetchLicenseInformationFromAPI(List listOfComponentsForApi, int timeout) { var batchSize = 500; var responses = new List(); @@ -38,7 +42,7 @@ public async Task> FetchLicenseInformationFromAPI(List list var uri = new Uri("https://api.clearlydefined.io/definitions?expand=-files"); httpClient.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json")); - httpClient.Timeout = TimeSpan.FromSeconds(ClientTimeoutSeconds); + httpClient.Timeout = TimeSpan.FromSeconds(timeout); for (var i = 0; i < listOfComponentsForApi.Count; i += batchSize) { diff --git a/src/Microsoft.Sbom.Common/Config/Configuration.cs b/src/Microsoft.Sbom.Common/Config/Configuration.cs index b7cfd6216..3dd94ab15 100644 --- a/src/Microsoft.Sbom.Common/Config/Configuration.cs +++ b/src/Microsoft.Sbom.Common/Config/Configuration.cs @@ -47,6 +47,7 @@ public class Configuration : IConfiguration private static readonly AsyncLocal> generationTimestamp = new(); private static readonly AsyncLocal> followSymlinks = new(); private static readonly AsyncLocal> fetchLicenseInformation = new(); + private static readonly AsyncLocal> licenseInformationTimeout = new(); private static readonly AsyncLocal> enablePackageMetadataParsing = new(); private static readonly AsyncLocal> deleteManifestDirIfPresent = new(); private static readonly AsyncLocal> failIfNoPackages = new(); @@ -309,6 +310,14 @@ public ConfigurationSetting FetchLicenseInformation set => fetchLicenseInformation.Value = value; } + /// + [DefaultValue(30)] + public ConfigurationSetting LicenseInformationTimeout + { + get => licenseInformationTimeout.Value; + set => licenseInformationTimeout.Value = value; + } + /// [DefaultValue(false)] public ConfigurationSetting EnablePackageMetadataParsing diff --git a/src/Microsoft.Sbom.Common/Config/IConfiguration.cs b/src/Microsoft.Sbom.Common/Config/IConfiguration.cs index b2ecdc356..1da7b892b 100644 --- a/src/Microsoft.Sbom.Common/Config/IConfiguration.cs +++ b/src/Microsoft.Sbom.Common/Config/IConfiguration.cs @@ -194,6 +194,12 @@ public interface IConfiguration /// ConfigurationSetting FetchLicenseInformation { get; set; } + /// + /// Specifies the timeout in seconds for fetching the license information. Defaults to 30 seconds. Has no effect if + /// FetchLicenseInformation (li) argument is false or not provided. + /// + ConfigurationSetting LicenseInformationTimeout { get; set; } + /// /// If set to true, we will attempt to locate and parse package metadata files for additional information to include in the SBOM such as .nuspec/.pom files in the local package cache. /// diff --git a/src/Microsoft.Sbom.Common/Config/InputConfiguration.cs b/src/Microsoft.Sbom.Common/Config/InputConfiguration.cs index f01022296..384c0c833 100644 --- a/src/Microsoft.Sbom.Common/Config/InputConfiguration.cs +++ b/src/Microsoft.Sbom.Common/Config/InputConfiguration.cs @@ -141,6 +141,10 @@ public class InputConfiguration : IConfiguration [DefaultValue(false)] public ConfigurationSetting FetchLicenseInformation { get; set; } + /// + [DefaultValue(30)] + public ConfigurationSetting LicenseInformationTimeout { get; set; } + [DefaultValue(false)] public ConfigurationSetting EnablePackageMetadataParsing { get; set; } From e2ed187976e3df22a7c23cdac9f963382fb87dc6 Mon Sep 17 00:00:00 2001 From: Michael Cline Date: Sat, 2 Nov 2024 08:12:10 +0100 Subject: [PATCH 2/3] Added recommended fixes for PR #773 - Renamed variables to specify Seconds - Added new CLI arg -lto to docs - Added support for negative values for -lto - Added tests for limits of -lto - Fixed breaking API change by creating new Interfaces - Changed some magic numbers to constants instead - Added some Warning statements --- docs/sbom-tool-arguments.md | 2 ++ .../Config/Args/GenerationArgs.cs | 7 ++--- .../Config/ConfigSanitizer.cs | 15 +++++++++++ .../Executors/ComponentDetectionBaseWalker.cs | 10 +++++-- .../Executors/ILicenseInformationFetcher.cs | 8 ++---- .../Executors/ILicenseInformationFetcher2.cs | 20 ++++++++++++++ .../Executors/ILicenseInformationService.cs | 2 -- .../Executors/ILicenseInformationService2.cs | 12 +++++++++ .../Executors/LicenseInformationFetcher.cs | 18 ++++++++++--- .../Executors/LicenseInformationService.cs | 16 ++++++++--- .../Config/Configuration.cs | 4 +-- .../Config/IConfiguration.cs | 4 +-- .../Config/InputConfiguration.cs | 4 +-- src/Microsoft.Sbom.Targets/README.md | 1 + .../Config/ConfigSanitizerTests.cs | 27 +++++++++++++++++++ 15 files changed, 123 insertions(+), 27 deletions(-) create mode 100644 src/Microsoft.Sbom.Api/Executors/ILicenseInformationFetcher2.cs create mode 100644 src/Microsoft.Sbom.Api/Executors/ILicenseInformationService2.cs diff --git a/docs/sbom-tool-arguments.md b/docs/sbom-tool-arguments.md index 44095822a..94af3d3ac 100644 --- a/docs/sbom-tool-arguments.md +++ b/docs/sbom-tool-arguments.md @@ -66,6 +66,8 @@ Actions DeleteManifestDirIfPresent (-D) If set to true, we will delete any previous manifest directories that are already present in the ManifestDirPath without asking the user for confirmation. The new manifest directory will then be created at this location and the generated SBOM will be stored there. FetchLicenseInformation (-li) If set to true, we will attempt to fetch license information of packages detected in the SBOM from the ClearlyDefinedApi. + LicenseInformationTimeoutInSeconds (-lto) Specifies the timeout in seconds for fetching the license information. Defaults to 30 seconds. Has no effect if + FetchLicenseInformation (-li) argument is false or not provided. A negative value specifies an infinite timeout. EnablePackageMetadataParsing (-pm) If set to true, we will attempt to parse license and supplier info from the packages metadata file (RubyGems, NuGet, Maven, Npm). Verbosity (-V) Display this amount of detail in the logging output. Verbose diff --git a/src/Microsoft.Sbom.Api/Config/Args/GenerationArgs.cs b/src/Microsoft.Sbom.Api/Config/Args/GenerationArgs.cs index 914e213f0..e711c0d72 100644 --- a/src/Microsoft.Sbom.Api/Config/Args/GenerationArgs.cs +++ b/src/Microsoft.Sbom.Api/Config/Args/GenerationArgs.cs @@ -118,12 +118,13 @@ public class GenerationArgs : GenerationAndValidationCommonArgs /// /// Specifies the timeout in seconds for fetching the license information. Defaults to 30 seconds. Has no effect if - /// FetchLicenseInformation (li) argument is false or not provided. + /// FetchLicenseInformation (li) argument is false or not provided. A negative value corresponds to an infinite timeout. /// [ArgShortcut("lto")] [ArgDescription("Specifies the timeout in seconds for fetching the license information. Defaults to 30 seconds. " + - "Has no effect if the FetchLicenseInformation (li) argument is false or not provided. ")] - public int? LicenseInformationTimeout { get; set; } + "Has no effect if the FetchLicenseInformation (li) argument is false or not provided. A negative value corresponds" + + "to an infinite timeout.")] + public int? LicenseInformationTimeoutInSeconds { get; set; } /// /// If set to true, we will attempt to parse license and supplier info from the packages metadata file. diff --git a/src/Microsoft.Sbom.Api/Config/ConfigSanitizer.cs b/src/Microsoft.Sbom.Api/Config/ConfigSanitizer.cs index e457589b0..0133911c9 100644 --- a/src/Microsoft.Sbom.Api/Config/ConfigSanitizer.cs +++ b/src/Microsoft.Sbom.Api/Config/ConfigSanitizer.cs @@ -6,6 +6,7 @@ using System.IO; using System.Linq; using System.Reflection; +using Microsoft.Sbom.Api.Executors; using Microsoft.Sbom.Api.Hashing; using Microsoft.Sbom.Api.Utils; using Microsoft.Sbom.Common; @@ -81,6 +82,20 @@ public IConfiguration SanitizeConfig(IConfiguration configuration) // Set default package supplier if not provided in configuration. configuration.PackageSupplier = GetPackageSupplierFromAssembly(configuration, logger); + // Prevent null value for LicenseInformationTimeoutInSeconds. + // Values from int.MinValue to int.MaxValue are allowed. Negative values are simply treated as InfiniteTimespan in LicenseInformationService, and + // from what I can tell there are no upper limits higher than int.MaxValue in Timespan.FromSeconds or httpClient.Timeout + if (configuration.LicenseInformationTimeoutInSeconds is null) + { + configuration.LicenseInformationTimeoutInSeconds = new(LicenseInformationFetcher.DefaultTimeoutInSeconds, SettingSource.Default); + } + + // Check if arg -lto is specified but -li is not + if (configuration.FetchLicenseInformation?.Value != true && !configuration.LicenseInformationTimeoutInSeconds.IsDefaultSource) + { + logger.Warning("A license fetching timeout is specified (argument -lto), but this has no effect when FetchLicenseInfo is unspecified or false (argument -li)"); + } + // Replace backslashes in directory paths with the OS-sepcific directory separator character. PathUtils.ConvertToOSSpecificPathSeparators(configuration); diff --git a/src/Microsoft.Sbom.Api/Executors/ComponentDetectionBaseWalker.cs b/src/Microsoft.Sbom.Api/Executors/ComponentDetectionBaseWalker.cs index e0e1ca5f0..6c83b0e17 100644 --- a/src/Microsoft.Sbom.Api/Executors/ComponentDetectionBaseWalker.cs +++ b/src/Microsoft.Sbom.Api/Executors/ComponentDetectionBaseWalker.cs @@ -138,13 +138,19 @@ async Task Scan(string path) licenseInformationRetrieved = true; List apiResponses; - if (configuration.LicenseInformationTimeout is null) + var licenseInformationFetcher2 = licenseInformationFetcher as ILicenseInformationFetcher2; + if (licenseInformationFetcher2 != null && (bool)!configuration.LicenseInformationTimeoutInSeconds?.IsDefaultSource) + { + log.Warning("Timeout value is specified, but ILicenseInformationFetcher2 is not implemented for the licenseInformationFetcher"); + } + + if (licenseInformationFetcher2 is null || configuration.LicenseInformationTimeoutInSeconds is null) { apiResponses = await licenseInformationFetcher.FetchLicenseInformationAsync(listOfComponentsForApi); } else { - apiResponses = await licenseInformationFetcher.FetchLicenseInformationAsync(listOfComponentsForApi, configuration.LicenseInformationTimeout.Value); + apiResponses = await licenseInformationFetcher2.FetchLicenseInformationAsync(listOfComponentsForApi, configuration.LicenseInformationTimeoutInSeconds.Value); } foreach (var response in apiResponses) diff --git a/src/Microsoft.Sbom.Api/Executors/ILicenseInformationFetcher.cs b/src/Microsoft.Sbom.Api/Executors/ILicenseInformationFetcher.cs index c0f28f931..186338cb0 100644 --- a/src/Microsoft.Sbom.Api/Executors/ILicenseInformationFetcher.cs +++ b/src/Microsoft.Sbom.Api/Executors/ILicenseInformationFetcher.cs @@ -17,16 +17,12 @@ public interface ILicenseInformationFetcher /// List ConvertComponentsToListForApi(IEnumerable scannedComponents); - /// > - Task> FetchLicenseInformationAsync(List listOfComponentsForApi); - /// - /// Calls the ClearlyDefined API to get the license information for the list of components. + /// Calls the ClearlyDefined API to get the license information for the list of components. Uses a default timeout specified in implementation /// /// A list of strings formatted into a list of strings that can be used to call the batch ClearlyDefined API. - /// Timeout in seconds to use when making web requests /// - Task> FetchLicenseInformationAsync(List listOfComponentsForApi, int timeout); + Task> FetchLicenseInformationAsync(List listOfComponentsForApi); /// /// Gets the dictionary of licenses that were fetched from the ClearlyDefined API. diff --git a/src/Microsoft.Sbom.Api/Executors/ILicenseInformationFetcher2.cs b/src/Microsoft.Sbom.Api/Executors/ILicenseInformationFetcher2.cs new file mode 100644 index 000000000..26d305ba8 --- /dev/null +++ b/src/Microsoft.Sbom.Api/Executors/ILicenseInformationFetcher2.cs @@ -0,0 +1,20 @@ +// Copyright (c) Microsoft. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using System.Collections.Concurrent; +using System.Collections.Generic; +using System.Threading.Tasks; +using Microsoft.ComponentDetection.Contracts.BcdeModels; + +namespace Microsoft.Sbom.Api.Executors; + +public interface ILicenseInformationFetcher2: ILicenseInformationFetcher +{ + /// + /// Calls the ClearlyDefined API to get the license information for the list of components. + /// + /// A list of strings formatted into a list of strings that can be used to call the batch ClearlyDefined API. + /// Timeout in seconds to use when making web requests + /// + Task> FetchLicenseInformationAsync(List listOfComponentsForApi, int timeoutInSeconds); +} diff --git a/src/Microsoft.Sbom.Api/Executors/ILicenseInformationService.cs b/src/Microsoft.Sbom.Api/Executors/ILicenseInformationService.cs index 2b09a65ca..675c46137 100644 --- a/src/Microsoft.Sbom.Api/Executors/ILicenseInformationService.cs +++ b/src/Microsoft.Sbom.Api/Executors/ILicenseInformationService.cs @@ -9,6 +9,4 @@ namespace Microsoft.Sbom.Api.Executors; public interface ILicenseInformationService { public Task> FetchLicenseInformationFromAPI(List listOfComponentsForApi); - - public Task> FetchLicenseInformationFromAPI(List listOfComponentsForApi, int timeout); } diff --git a/src/Microsoft.Sbom.Api/Executors/ILicenseInformationService2.cs b/src/Microsoft.Sbom.Api/Executors/ILicenseInformationService2.cs new file mode 100644 index 000000000..0e6077be9 --- /dev/null +++ b/src/Microsoft.Sbom.Api/Executors/ILicenseInformationService2.cs @@ -0,0 +1,12 @@ +// Copyright (c) Microsoft. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using System.Collections.Generic; +using System.Threading.Tasks; + +namespace Microsoft.Sbom.Api.Executors; + +public interface ILicenseInformationService2 : ILicenseInformationService +{ + public Task> FetchLicenseInformationFromAPI(List listOfComponentsForApi, int timeoutInSeconds); +} diff --git a/src/Microsoft.Sbom.Api/Executors/LicenseInformationFetcher.cs b/src/Microsoft.Sbom.Api/Executors/LicenseInformationFetcher.cs index 59ed5a70c..f22c74e26 100644 --- a/src/Microsoft.Sbom.Api/Executors/LicenseInformationFetcher.cs +++ b/src/Microsoft.Sbom.Api/Executors/LicenseInformationFetcher.cs @@ -14,8 +14,9 @@ namespace Microsoft.Sbom.Api.Executors; -public class LicenseInformationFetcher : ILicenseInformationFetcher +public class LicenseInformationFetcher : ILicenseInformationFetcher2 { + public const int DefaultTimeoutInSeconds = 30; private readonly ILogger log; private readonly IRecorder recorder; private readonly ILicenseInformationService licenseInformationService; @@ -84,12 +85,21 @@ public List ConvertComponentsToListForApi(IEnumerable public async Task> FetchLicenseInformationAsync(List listOfComponentsForApi) { - return await FetchLicenseInformationAsync(listOfComponentsForApi, 30); + return await FetchLicenseInformationAsync(listOfComponentsForApi, DefaultTimeoutInSeconds); } - public async Task> FetchLicenseInformationAsync(List listOfComponentsForApi, int timeout) + public async Task> FetchLicenseInformationAsync(List listOfComponentsForApi, int timeoutInSeconds) { - return await licenseInformationService.FetchLicenseInformationFromAPI(listOfComponentsForApi, timeout); + var licenseInformationService2 = licenseInformationService as ILicenseInformationService2; + if (licenseInformationService2 is null) + { + log.Warning("Timeout is specified in License Fetcher, but licenseInformationService does not implement ILicenseInformationService2"); + return await licenseInformationService.FetchLicenseInformationFromAPI(listOfComponentsForApi); + } + else + { + return await licenseInformationService2.FetchLicenseInformationFromAPI(listOfComponentsForApi, timeoutInSeconds); + } } // Will attempt to extract license information from a clearlyDefined batch API response. Will always return a dictionary which may be empty depending on the response. diff --git a/src/Microsoft.Sbom.Api/Executors/LicenseInformationService.cs b/src/Microsoft.Sbom.Api/Executors/LicenseInformationService.cs index ee435f345..76bff9360 100644 --- a/src/Microsoft.Sbom.Api/Executors/LicenseInformationService.cs +++ b/src/Microsoft.Sbom.Api/Executors/LicenseInformationService.cs @@ -15,8 +15,9 @@ namespace Microsoft.Sbom.Api.Executors; -public class LicenseInformationService : ILicenseInformationService +public class LicenseInformationService : ILicenseInformationService2 { + private const int DefaultTimeoutInSeconds = LicenseInformationFetcher.DefaultTimeoutInSeconds; private readonly ILogger log; private readonly IRecorder recorder; private readonly HttpClient httpClient; @@ -30,10 +31,10 @@ public LicenseInformationService(ILogger log, IRecorder recorder, HttpClient htt public async Task> FetchLicenseInformationFromAPI(List listOfComponentsForApi) { - return await FetchLicenseInformationFromAPI(listOfComponentsForApi, 30); + return await FetchLicenseInformationFromAPI(listOfComponentsForApi, DefaultTimeoutInSeconds); } - public async Task> FetchLicenseInformationFromAPI(List listOfComponentsForApi, int timeout) + public async Task> FetchLicenseInformationFromAPI(List listOfComponentsForApi, int timeoutInSeconds) { var batchSize = 500; var responses = new List(); @@ -42,7 +43,14 @@ public async Task> FetchLicenseInformationFromAPI(List list var uri = new Uri("https://api.clearlydefined.io/definitions?expand=-files"); httpClient.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json")); - httpClient.Timeout = TimeSpan.FromSeconds(timeout); + if (timeoutInSeconds >= 0) + { + httpClient.Timeout = TimeSpan.FromSeconds(timeoutInSeconds); + } + else + { + httpClient.Timeout = System.Threading.Timeout.InfiniteTimeSpan; + } for (var i = 0; i < listOfComponentsForApi.Count; i += batchSize) { diff --git a/src/Microsoft.Sbom.Common/Config/Configuration.cs b/src/Microsoft.Sbom.Common/Config/Configuration.cs index 3dd94ab15..7b503ee41 100644 --- a/src/Microsoft.Sbom.Common/Config/Configuration.cs +++ b/src/Microsoft.Sbom.Common/Config/Configuration.cs @@ -310,9 +310,9 @@ public ConfigurationSetting FetchLicenseInformation set => fetchLicenseInformation.Value = value; } - /// + /// [DefaultValue(30)] - public ConfigurationSetting LicenseInformationTimeout + public ConfigurationSetting LicenseInformationTimeoutInSeconds { get => licenseInformationTimeout.Value; set => licenseInformationTimeout.Value = value; diff --git a/src/Microsoft.Sbom.Common/Config/IConfiguration.cs b/src/Microsoft.Sbom.Common/Config/IConfiguration.cs index 1da7b892b..d1f87fcf2 100644 --- a/src/Microsoft.Sbom.Common/Config/IConfiguration.cs +++ b/src/Microsoft.Sbom.Common/Config/IConfiguration.cs @@ -196,9 +196,9 @@ public interface IConfiguration /// /// Specifies the timeout in seconds for fetching the license information. Defaults to 30 seconds. Has no effect if - /// FetchLicenseInformation (li) argument is false or not provided. + /// FetchLicenseInformation (li) argument is false or not provided. Negative values correspond to an infinite timeout /// - ConfigurationSetting LicenseInformationTimeout { get; set; } + ConfigurationSetting LicenseInformationTimeoutInSeconds { get; set; } /// /// If set to true, we will attempt to locate and parse package metadata files for additional information to include in the SBOM such as .nuspec/.pom files in the local package cache. diff --git a/src/Microsoft.Sbom.Common/Config/InputConfiguration.cs b/src/Microsoft.Sbom.Common/Config/InputConfiguration.cs index 384c0c833..319a65152 100644 --- a/src/Microsoft.Sbom.Common/Config/InputConfiguration.cs +++ b/src/Microsoft.Sbom.Common/Config/InputConfiguration.cs @@ -141,9 +141,9 @@ public class InputConfiguration : IConfiguration [DefaultValue(false)] public ConfigurationSetting FetchLicenseInformation { get; set; } - /// + /// [DefaultValue(30)] - public ConfigurationSetting LicenseInformationTimeout { get; set; } + public ConfigurationSetting LicenseInformationTimeoutInSeconds { get; set; } [DefaultValue(false)] public ConfigurationSetting EnablePackageMetadataParsing { get; set; } diff --git a/src/Microsoft.Sbom.Targets/README.md b/src/Microsoft.Sbom.Targets/README.md index f89b8ce69..11382a2ed 100644 --- a/src/Microsoft.Sbom.Targets/README.md +++ b/src/Microsoft.Sbom.Targets/README.md @@ -36,6 +36,7 @@ The custom MSBuild task accepts most of the arguments available for the [SBOM CL | `` | N/A | No | | `` | N/A | No | | `` | `false` | No | +| ``| `30` | No | | `` | `false` | No | | `` | `Information` | No | | `` | `SPDX:2.2` | No | diff --git a/test/Microsoft.Sbom.Api.Tests/Config/ConfigSanitizerTests.cs b/test/Microsoft.Sbom.Api.Tests/Config/ConfigSanitizerTests.cs index 0b6ba5cb7..180117edf 100644 --- a/test/Microsoft.Sbom.Api.Tests/Config/ConfigSanitizerTests.cs +++ b/test/Microsoft.Sbom.Api.Tests/Config/ConfigSanitizerTests.cs @@ -9,6 +9,7 @@ using System.Security.Cryptography; using Microsoft.Sbom.Api.Config; using Microsoft.Sbom.Api.Exceptions; +using Microsoft.Sbom.Api.Executors; using Microsoft.Sbom.Api.Hashing; using Microsoft.Sbom.Api.Utils; using Microsoft.Sbom.Common; @@ -351,4 +352,30 @@ public void ConfigSantizer_Validate_ReplacesBackslashes_Linux(ManifestToolAction Assert.IsTrue(config.TelemetryFilePath.Value.StartsWith($"/{nameof(config.TelemetryFilePath)}/", StringComparison.Ordinal)); } } + + [TestMethod] + [DataRow(int.MinValue, DisplayName = "Minimum Value")] + [DataRow(0, DisplayName = "Zero is allowed")] + [DataRow(int.MaxValue, DisplayName = "Maximum Value")] + public void LicenseInformationTimeoutInSeconds_SanitizeMakesNoChanges(int value) + { + var config = GetConfigurationBaseObject(); + config.LicenseInformationTimeoutInSeconds = new(value, SettingSource.CommandLine); + + configSanitizer.SanitizeConfig(config); + + Assert.AreEqual(value, config.LicenseInformationTimeoutInSeconds.Value, "The value of LicenseInformationTimeoutInSeconds should remain the same through the sanitization process"); + } + + [TestMethod] + public void LicenseInformationTimeoutInSeconds_SanitizeNull() + { + var config = GetConfigurationBaseObject(); + config.LicenseInformationTimeoutInSeconds = null; + + configSanitizer.SanitizeConfig(config); + + Assert.AreEqual(LicenseInformationFetcher.DefaultTimeoutInSeconds, config.LicenseInformationTimeoutInSeconds.Value, "The value of LicenseInformationTimeoutInSeconds should be set to 30s when null"); + Assert.AreEqual(SettingSource.Default, config.LicenseInformationTimeoutInSeconds.Source, "The source of LicenseInformationTimeoutInSeconds should be set to Default when null"); + } } From e3e21ad54fd91b11cd1fe9b4802afceaf824ab88 Mon Sep 17 00:00:00 2001 From: Michael Cline Date: Tue, 5 Nov 2024 19:22:11 +0100 Subject: [PATCH 3/3] Added additional recommended fixes for PR microsoft#773 - Fixed a couple of minor bugs - Reduced valid input range for -lto to 1-86400 - Moved MaxTimeout and DefaultTimeout to Constants class --- docs/sbom-tool-arguments.md | 3 ++- .../Config/ConfigSanitizer.cs | 16 +++++++++--- .../Executors/ComponentDetectionBaseWalker.cs | 2 +- .../Executors/LicenseInformationFetcher.cs | 3 +-- .../Executors/LicenseInformationService.cs | 11 +++----- .../Config/Configuration.cs | 2 +- .../Config/IConfiguration.cs | 5 ++-- .../Config/InputConfiguration.cs | 2 +- src/Microsoft.Sbom.Common/Constants.cs | 3 +++ .../Config/ConfigSanitizerTests.cs | 26 ++++++++++++++++--- 10 files changed, 50 insertions(+), 23 deletions(-) diff --git a/docs/sbom-tool-arguments.md b/docs/sbom-tool-arguments.md index 94af3d3ac..e938ec373 100644 --- a/docs/sbom-tool-arguments.md +++ b/docs/sbom-tool-arguments.md @@ -67,7 +67,8 @@ Actions manifest directory will then be created at this location and the generated SBOM will be stored there. FetchLicenseInformation (-li) If set to true, we will attempt to fetch license information of packages detected in the SBOM from the ClearlyDefinedApi. LicenseInformationTimeoutInSeconds (-lto) Specifies the timeout in seconds for fetching the license information. Defaults to 30 seconds. Has no effect if - FetchLicenseInformation (-li) argument is false or not provided. A negative value specifies an infinite timeout. + FetchLicenseInformation (-li) argument is false or not provided. Valid values are from 1 to 86400. Negative values use the default + value and Values exceeding the maximum are truncated to the maximum. EnablePackageMetadataParsing (-pm) If set to true, we will attempt to parse license and supplier info from the packages metadata file (RubyGems, NuGet, Maven, Npm). Verbosity (-V) Display this amount of detail in the logging output. Verbose diff --git a/src/Microsoft.Sbom.Api/Config/ConfigSanitizer.cs b/src/Microsoft.Sbom.Api/Config/ConfigSanitizer.cs index 0133911c9..1ab6ccc19 100644 --- a/src/Microsoft.Sbom.Api/Config/ConfigSanitizer.cs +++ b/src/Microsoft.Sbom.Api/Config/ConfigSanitizer.cs @@ -83,11 +83,21 @@ public IConfiguration SanitizeConfig(IConfiguration configuration) configuration.PackageSupplier = GetPackageSupplierFromAssembly(configuration, logger); // Prevent null value for LicenseInformationTimeoutInSeconds. - // Values from int.MinValue to int.MaxValue are allowed. Negative values are simply treated as InfiniteTimespan in LicenseInformationService, and - // from what I can tell there are no upper limits higher than int.MaxValue in Timespan.FromSeconds or httpClient.Timeout + // Values of (0, Constants.MaxLicenseFetchTimeoutInSeconds] are allowed. Negative values are replaced with the default, and + // the higher values are truncated to the maximum of Common.Constants.MaxLicenseFetchTimeoutInSeconds if (configuration.LicenseInformationTimeoutInSeconds is null) { - configuration.LicenseInformationTimeoutInSeconds = new(LicenseInformationFetcher.DefaultTimeoutInSeconds, SettingSource.Default); + configuration.LicenseInformationTimeoutInSeconds = new(Common.Constants.DefaultLicenseFetchTimeoutInSeconds, SettingSource.Default); + } + else if (configuration.LicenseInformationTimeoutInSeconds.Value <= 0) + { + logger.Warning($"Negative and Zero Values not allowed for timeout. Using the default {Common.Constants.DefaultLicenseFetchTimeoutInSeconds} seconds instead."); + configuration.LicenseInformationTimeoutInSeconds.Value = Common.Constants.DefaultLicenseFetchTimeoutInSeconds; + } + else if (configuration.LicenseInformationTimeoutInSeconds.Value > Common.Constants.MaxLicenseFetchTimeoutInSeconds) + { + logger.Warning($"Specified timeout exceeds maximum allowed. Truncating the timeout to {Common.Constants.MaxLicenseFetchTimeoutInSeconds} seconds."); + configuration.LicenseInformationTimeoutInSeconds.Value = Common.Constants.MaxLicenseFetchTimeoutInSeconds; } // Check if arg -lto is specified but -li is not diff --git a/src/Microsoft.Sbom.Api/Executors/ComponentDetectionBaseWalker.cs b/src/Microsoft.Sbom.Api/Executors/ComponentDetectionBaseWalker.cs index 6c83b0e17..e5c1b9e5b 100644 --- a/src/Microsoft.Sbom.Api/Executors/ComponentDetectionBaseWalker.cs +++ b/src/Microsoft.Sbom.Api/Executors/ComponentDetectionBaseWalker.cs @@ -139,7 +139,7 @@ async Task Scan(string path) List apiResponses; var licenseInformationFetcher2 = licenseInformationFetcher as ILicenseInformationFetcher2; - if (licenseInformationFetcher2 != null && (bool)!configuration.LicenseInformationTimeoutInSeconds?.IsDefaultSource) + if (licenseInformationFetcher2 is null && (bool)!configuration.LicenseInformationTimeoutInSeconds?.IsDefaultSource) { log.Warning("Timeout value is specified, but ILicenseInformationFetcher2 is not implemented for the licenseInformationFetcher"); } diff --git a/src/Microsoft.Sbom.Api/Executors/LicenseInformationFetcher.cs b/src/Microsoft.Sbom.Api/Executors/LicenseInformationFetcher.cs index f22c74e26..bf9c38fe1 100644 --- a/src/Microsoft.Sbom.Api/Executors/LicenseInformationFetcher.cs +++ b/src/Microsoft.Sbom.Api/Executors/LicenseInformationFetcher.cs @@ -16,7 +16,6 @@ namespace Microsoft.Sbom.Api.Executors; public class LicenseInformationFetcher : ILicenseInformationFetcher2 { - public const int DefaultTimeoutInSeconds = 30; private readonly ILogger log; private readonly IRecorder recorder; private readonly ILicenseInformationService licenseInformationService; @@ -85,7 +84,7 @@ public List ConvertComponentsToListForApi(IEnumerable public async Task> FetchLicenseInformationAsync(List listOfComponentsForApi) { - return await FetchLicenseInformationAsync(listOfComponentsForApi, DefaultTimeoutInSeconds); + return await licenseInformationService.FetchLicenseInformationFromAPI(listOfComponentsForApi); } public async Task> FetchLicenseInformationAsync(List listOfComponentsForApi, int timeoutInSeconds) diff --git a/src/Microsoft.Sbom.Api/Executors/LicenseInformationService.cs b/src/Microsoft.Sbom.Api/Executors/LicenseInformationService.cs index 76bff9360..133ab2330 100644 --- a/src/Microsoft.Sbom.Api/Executors/LicenseInformationService.cs +++ b/src/Microsoft.Sbom.Api/Executors/LicenseInformationService.cs @@ -17,7 +17,6 @@ namespace Microsoft.Sbom.Api.Executors; public class LicenseInformationService : ILicenseInformationService2 { - private const int DefaultTimeoutInSeconds = LicenseInformationFetcher.DefaultTimeoutInSeconds; private readonly ILogger log; private readonly IRecorder recorder; private readonly HttpClient httpClient; @@ -31,7 +30,7 @@ public LicenseInformationService(ILogger log, IRecorder recorder, HttpClient htt public async Task> FetchLicenseInformationFromAPI(List listOfComponentsForApi) { - return await FetchLicenseInformationFromAPI(listOfComponentsForApi, DefaultTimeoutInSeconds); + return await FetchLicenseInformationFromAPI(listOfComponentsForApi, Common.Constants.MaxLicenseFetchTimeoutInSeconds); } public async Task> FetchLicenseInformationFromAPI(List listOfComponentsForApi, int timeoutInSeconds) @@ -43,14 +42,10 @@ public async Task> FetchLicenseInformationFromAPI(List list var uri = new Uri("https://api.clearlydefined.io/definitions?expand=-files"); httpClient.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json")); - if (timeoutInSeconds >= 0) + if (timeoutInSeconds > 0) { httpClient.Timeout = TimeSpan.FromSeconds(timeoutInSeconds); - } - else - { - httpClient.Timeout = System.Threading.Timeout.InfiniteTimeSpan; - } + } // The else cases should be sanitized in Config Sanitizer, and even if not, it'll just use httpClient's default timeout for (var i = 0; i < listOfComponentsForApi.Count; i += batchSize) { diff --git a/src/Microsoft.Sbom.Common/Config/Configuration.cs b/src/Microsoft.Sbom.Common/Config/Configuration.cs index 7b503ee41..aa5789bef 100644 --- a/src/Microsoft.Sbom.Common/Config/Configuration.cs +++ b/src/Microsoft.Sbom.Common/Config/Configuration.cs @@ -311,7 +311,7 @@ public ConfigurationSetting FetchLicenseInformation } /// - [DefaultValue(30)] + [DefaultValue(Constants.DefaultLicenseFetchTimeoutInSeconds)] public ConfigurationSetting LicenseInformationTimeoutInSeconds { get => licenseInformationTimeout.Value; diff --git a/src/Microsoft.Sbom.Common/Config/IConfiguration.cs b/src/Microsoft.Sbom.Common/Config/IConfiguration.cs index d1f87fcf2..d461ce8db 100644 --- a/src/Microsoft.Sbom.Common/Config/IConfiguration.cs +++ b/src/Microsoft.Sbom.Common/Config/IConfiguration.cs @@ -195,8 +195,9 @@ public interface IConfiguration ConfigurationSetting FetchLicenseInformation { get; set; } /// - /// Specifies the timeout in seconds for fetching the license information. Defaults to 30 seconds. Has no effect if - /// FetchLicenseInformation (li) argument is false or not provided. Negative values correspond to an infinite timeout + /// Specifies the timeout in seconds for fetching the license information. Defaults to . + /// Has no effect if FetchLicenseInformation (li) argument is false or not provided. Negative values are set to the default and values exceeding the + /// maximum are truncated to /// ConfigurationSetting LicenseInformationTimeoutInSeconds { get; set; } diff --git a/src/Microsoft.Sbom.Common/Config/InputConfiguration.cs b/src/Microsoft.Sbom.Common/Config/InputConfiguration.cs index 319a65152..151d4ca02 100644 --- a/src/Microsoft.Sbom.Common/Config/InputConfiguration.cs +++ b/src/Microsoft.Sbom.Common/Config/InputConfiguration.cs @@ -142,7 +142,7 @@ public class InputConfiguration : IConfiguration public ConfigurationSetting FetchLicenseInformation { get; set; } /// - [DefaultValue(30)] + [DefaultValue(Constants.DefaultLicenseFetchTimeoutInSeconds)] public ConfigurationSetting LicenseInformationTimeoutInSeconds { get; set; } [DefaultValue(false)] diff --git a/src/Microsoft.Sbom.Common/Constants.cs b/src/Microsoft.Sbom.Common/Constants.cs index f4fa50769..bb5182193 100644 --- a/src/Microsoft.Sbom.Common/Constants.cs +++ b/src/Microsoft.Sbom.Common/Constants.cs @@ -13,5 +13,8 @@ public static class Constants public const int DefaultParallelism = 8; public const int MaxParallelism = 48; + public const int DefaultLicenseFetchTimeoutInSeconds = 30; + public const int MaxLicenseFetchTimeoutInSeconds = 86400; + public const LogEventLevel DefaultLogLevel = LogEventLevel.Warning; } diff --git a/test/Microsoft.Sbom.Api.Tests/Config/ConfigSanitizerTests.cs b/test/Microsoft.Sbom.Api.Tests/Config/ConfigSanitizerTests.cs index 180117edf..7d56f8e97 100644 --- a/test/Microsoft.Sbom.Api.Tests/Config/ConfigSanitizerTests.cs +++ b/test/Microsoft.Sbom.Api.Tests/Config/ConfigSanitizerTests.cs @@ -354,9 +354,8 @@ public void ConfigSantizer_Validate_ReplacesBackslashes_Linux(ManifestToolAction } [TestMethod] - [DataRow(int.MinValue, DisplayName = "Minimum Value")] - [DataRow(0, DisplayName = "Zero is allowed")] - [DataRow(int.MaxValue, DisplayName = "Maximum Value")] + [DataRow(1, DisplayName = "Minimum value of 1")] + [DataRow(Common.Constants.MaxLicenseFetchTimeoutInSeconds, DisplayName = "Maximum Value of 86400")] public void LicenseInformationTimeoutInSeconds_SanitizeMakesNoChanges(int value) { var config = GetConfigurationBaseObject(); @@ -367,6 +366,21 @@ public void LicenseInformationTimeoutInSeconds_SanitizeMakesNoChanges(int value) Assert.AreEqual(value, config.LicenseInformationTimeoutInSeconds.Value, "The value of LicenseInformationTimeoutInSeconds should remain the same through the sanitization process"); } + [TestMethod] + [DataRow(int.MinValue, Common.Constants.DefaultLicenseFetchTimeoutInSeconds, DisplayName = "Negative Value is changed to Default")] + [DataRow(0, Common.Constants.DefaultLicenseFetchTimeoutInSeconds, DisplayName = "Zero is changed to Default")] + [DataRow(Common.Constants.MaxLicenseFetchTimeoutInSeconds + 1, Common.Constants.MaxLicenseFetchTimeoutInSeconds, DisplayName = "Max Value + 1 is truncated")] + [DataRow(int.MaxValue, Common.Constants.MaxLicenseFetchTimeoutInSeconds, DisplayName = "int.MaxValue is truncated")] + public void LicenseInformationTimeoutInSeconds_SanitizeExceedsLimits(int value, int expected) + { + var config = GetConfigurationBaseObject(); + config.LicenseInformationTimeoutInSeconds = new(value, SettingSource.CommandLine); + + configSanitizer.SanitizeConfig(config); + + Assert.AreEqual(expected, config.LicenseInformationTimeoutInSeconds.Value, "The value of LicenseInformationTimeoutInSeconds should be sanitized to a valid value"); + } + [TestMethod] public void LicenseInformationTimeoutInSeconds_SanitizeNull() { @@ -375,7 +389,11 @@ public void LicenseInformationTimeoutInSeconds_SanitizeNull() configSanitizer.SanitizeConfig(config); - Assert.AreEqual(LicenseInformationFetcher.DefaultTimeoutInSeconds, config.LicenseInformationTimeoutInSeconds.Value, "The value of LicenseInformationTimeoutInSeconds should be set to 30s when null"); + Assert.AreEqual( + Common.Constants.DefaultLicenseFetchTimeoutInSeconds, + config.LicenseInformationTimeoutInSeconds.Value, + $"The value of LicenseInformationTimeoutInSeconds should be set to {Common.Constants.DefaultLicenseFetchTimeoutInSeconds}s when null"); + Assert.AreEqual(SettingSource.Default, config.LicenseInformationTimeoutInSeconds.Source, "The source of LicenseInformationTimeoutInSeconds should be set to Default when null"); } }