From 61f46249872e24acf207b4f6b36634207cedce37 Mon Sep 17 00:00:00 2001 From: "Alison Lomaka (from Dev Box)" Date: Tue, 14 May 2024 13:06:59 -0400 Subject: [PATCH 1/2] Previously, we have been using enums to model ExternalRepositoryType and RelationshipType in SPDX files. This worked acceptably when we only ever had to validate SPDXs that we ourselves created (so we controlled the possible range of values). However, this requires us to know and enumerate every possible legal value for each enum, and it is vulnerable to breaking changes between minor versions of SPDX when deserializing. For example, when processing 3P SBOMs that were produced by other tools, we may encounter values that are legal by the SPDX spec, but not defined in our enums; this prevents us from parsing those files successfully. It also is a factor preventing us from flexibly handling all 2.x SPDX rather than just 2.2.1, which is a feature we require for redaction. This change models RelationshipType and ExternalRepositoryType as strings, rather than enums. That allows us to tolerate a wide range of possible input values without worrying if they align with our known range. The SBOM generation code still uses the enum types to enforce expected output values, simply converting to string at serialization time. The test case that asserted known values for RelationshipType has been removed since it is no longer relevant; an unrecognized RelationshipType should not cause failure anymore. --- .../Entities/ExternalReference.cs | 2 +- .../Entities/SPDXRelationship.cs | 2 +- .../Generator.cs | 4 ++-- .../Utils/SPDXExtensions.cs | 2 +- .../Parser/SbomRelationshipParserTests.cs | 1 - .../Utils/SPDXExtensionsTest.cs | 14 ++++++++++++-- 6 files changed, 17 insertions(+), 8 deletions(-) diff --git a/src/Microsoft.Sbom.Parsers.Spdx22SbomParser/Entities/ExternalReference.cs b/src/Microsoft.Sbom.Parsers.Spdx22SbomParser/Entities/ExternalReference.cs index 328a0a1f6..07a2718ba 100644 --- a/src/Microsoft.Sbom.Parsers.Spdx22SbomParser/Entities/ExternalReference.cs +++ b/src/Microsoft.Sbom.Parsers.Spdx22SbomParser/Entities/ExternalReference.cs @@ -25,7 +25,7 @@ public class ExternalReference /// https://spdx.github.io/spdx-spec/appendix-VI-external-repository-identifiers/. /// [JsonPropertyName("referenceType")] - public ExternalRepositoryType Type { get; set; } + public string Type { get; set; } /// /// Gets or sets a unique string without any spaces that specifies a location where the package specific information diff --git a/src/Microsoft.Sbom.Parsers.Spdx22SbomParser/Entities/SPDXRelationship.cs b/src/Microsoft.Sbom.Parsers.Spdx22SbomParser/Entities/SPDXRelationship.cs index 64ae55a8f..21e28553b 100644 --- a/src/Microsoft.Sbom.Parsers.Spdx22SbomParser/Entities/SPDXRelationship.cs +++ b/src/Microsoft.Sbom.Parsers.Spdx22SbomParser/Entities/SPDXRelationship.cs @@ -15,7 +15,7 @@ public class SPDXRelationship /// Gets or sets defines the type of the relationship between the source and the target element. /// [JsonPropertyName("relationshipType")] - public SPDXRelationshipType RelationshipType { get; set; } + public string RelationshipType { get; set; } /// /// Gets or sets the id of the target element with whom the source element has a relationship. diff --git a/src/Microsoft.Sbom.Parsers.Spdx22SbomParser/Generator.cs b/src/Microsoft.Sbom.Parsers.Spdx22SbomParser/Generator.cs index d77b5af8a..ad20e23ac 100644 --- a/src/Microsoft.Sbom.Parsers.Spdx22SbomParser/Generator.cs +++ b/src/Microsoft.Sbom.Parsers.Spdx22SbomParser/Generator.cs @@ -206,7 +206,7 @@ public GenerationResult GenerateRootPackage( new ExternalReference { ReferenceCategory = ReferenceCategory.PACKAGE_MANAGER.ToNormalizedString(), - Type = ExternalRepositoryType.purl, + Type = ExternalRepositoryType.purl.ToString(), Locator = internalMetadataProvider.GetSwidTagId() } }, @@ -248,7 +248,7 @@ public GenerationResult GenerateJsonDocument(Relationship relationship) var spdxRelationship = new SPDXRelationship { SourceElementId = sourceElement, - RelationshipType = GetSPDXRelationshipType(relationship.RelationshipType), + RelationshipType = GetSPDXRelationshipType(relationship.RelationshipType).ToString(), TargetElementId = targetElement }; diff --git a/src/Microsoft.Sbom.Parsers.Spdx22SbomParser/Utils/SPDXExtensions.cs b/src/Microsoft.Sbom.Parsers.Spdx22SbomParser/Utils/SPDXExtensions.cs index 973784337..4ff91ae29 100644 --- a/src/Microsoft.Sbom.Parsers.Spdx22SbomParser/Utils/SPDXExtensions.cs +++ b/src/Microsoft.Sbom.Parsers.Spdx22SbomParser/Utils/SPDXExtensions.cs @@ -77,7 +77,7 @@ public static void AddPackageUrls(this SPDXPackage spdxPackage, SbomPackage pack var extRef = new ExternalReference { ReferenceCategory = ReferenceCategory.PACKAGE_MANAGER.ToNormalizedString(), - Type = ExternalRepositoryType.purl, + Type = ExternalRepositoryType.purl.ToString(), Locator = packageInfo.PackageUrl, }; diff --git a/test/Microsoft.Sbom.Parsers.Spdx22SbomParser.Tests/Parser/SbomRelationshipParserTests.cs b/test/Microsoft.Sbom.Parsers.Spdx22SbomParser.Tests/Parser/SbomRelationshipParserTests.cs index ca994563f..35872fb3b 100644 --- a/test/Microsoft.Sbom.Parsers.Spdx22SbomParser.Tests/Parser/SbomRelationshipParserTests.cs +++ b/test/Microsoft.Sbom.Parsers.Spdx22SbomParser.Tests/Parser/SbomRelationshipParserTests.cs @@ -73,7 +73,6 @@ public void IgnoresAdditionalPropertiesTest(string json) } [DataTestMethod] - [DataRow(RelationshipStrings.MalformedJsonRelationshipsStringBadRelationshipType)] [DataRow(RelationshipStrings.MalformedJsonRelationshipsString)] [TestMethod] public void MalformedJsonTest_Throws(string json) diff --git a/test/Microsoft.Sbom.Parsers.Spdx22SbomParser.Tests/Utils/SPDXExtensionsTest.cs b/test/Microsoft.Sbom.Parsers.Spdx22SbomParser.Tests/Utils/SPDXExtensionsTest.cs index c9263803a..8524073d9 100644 --- a/test/Microsoft.Sbom.Parsers.Spdx22SbomParser.Tests/Utils/SPDXExtensionsTest.cs +++ b/test/Microsoft.Sbom.Parsers.Spdx22SbomParser.Tests/Utils/SPDXExtensionsTest.cs @@ -44,7 +44,12 @@ public void AddPackageUrlsTest_Success() spdxPackage.AddPackageUrls(packageInfo); var externalRef = spdxPackage.ExternalReferences.First(); Assert.AreEqual(ReferenceCategory.PACKAGE_MANAGER.ToNormalizedString(), externalRef.ReferenceCategory); - Assert.AreEqual(ExternalRepositoryType.purl, externalRef.Type); + + // ExternalRepositoryTypes are deserialized as strings for portability when handling 3P SBOMs, + // but in the context of this test we expect the value to align with a known enum value. So + // convert to enum for comparison. + Enum.TryParse(externalRef.Type, out var refType); + Assert.AreEqual(ExternalRepositoryType.purl, refType); Assert.AreEqual(PackageUrl, externalRef.Locator); } @@ -84,7 +89,12 @@ public void AddPackageUrlsTest_WithSpecialCharacter_Success(string inputUrl, str var externalRef = spdxPackage.ExternalReferences.First(); Assert.AreEqual(ReferenceCategory.PACKAGE_MANAGER.ToNormalizedString(), externalRef.ReferenceCategory); - Assert.AreEqual(ExternalRepositoryType.purl, externalRef.Type); + + // ExternalRepositoryTypes are deserialized as strings for portability when handling 3P SBOMs, + // but in the context of this test we expect the value to align with a known enum value. So + // convert to enum for comparison. + Enum.TryParse(externalRef.Type, out var refType); + Assert.AreEqual(ExternalRepositoryType.purl, refType); Assert.AreEqual(expectedUrl, externalRef.Locator); } From cd5ae31a3d3ed8e94900162e880a4f11422df771 Mon Sep 17 00:00:00 2001 From: "Alison Lomaka (from Dev Box)" Date: Tue, 14 May 2024 13:14:50 -0400 Subject: [PATCH 2/2] We will no longer require license and copyright related fields on SPDXFile and SPDXPackage entities. It's not clear why we previously required them, but they are not required by the SPDX v2 spec nor by the NTIA minimum required elements for an SBOM. Loosening this constraint improves our ability to handle all 2.x SPDXs, including those produced by 3P tools that may not follow the same generation conventions as sbom-tool. In addition to removing the JsonRequired attribute from several fields, remove the test cases that check that the attribute is appropriately applied, since they are no longer relevant. --- .../Entities/SPDXPackage.cs | 3 --- .../Entities/SpdxFile.cs | 3 --- .../Parser/SbomFileParserTests.cs | 3 --- 3 files changed, 9 deletions(-) diff --git a/src/Microsoft.Sbom.Parsers.Spdx22SbomParser/Entities/SPDXPackage.cs b/src/Microsoft.Sbom.Parsers.Spdx22SbomParser/Entities/SPDXPackage.cs index c419cb26c..f12d96450 100644 --- a/src/Microsoft.Sbom.Parsers.Spdx22SbomParser/Entities/SPDXPackage.cs +++ b/src/Microsoft.Sbom.Parsers.Spdx22SbomParser/Entities/SPDXPackage.cs @@ -54,7 +54,6 @@ public class SPDXPackage /// /// Gets or sets contain the license the SPDX file creator has concluded as the package or alternative values. /// - [JsonRequired] [JsonPropertyName("licenseConcluded")] public string LicenseConcluded { get; set; } @@ -68,14 +67,12 @@ public class SPDXPackage /// /// Gets or sets contains a list of licenses the have been declared by the authors of the package. /// - [JsonRequired] [JsonPropertyName("licenseDeclared")] public string LicenseDeclared { get; set; } /// /// Gets or sets copyright holder of the package, as well as any dates present. /// - [JsonRequired] [JsonPropertyName("copyrightText")] public string CopyrightText { get; set; } diff --git a/src/Microsoft.Sbom.Parsers.Spdx22SbomParser/Entities/SpdxFile.cs b/src/Microsoft.Sbom.Parsers.Spdx22SbomParser/Entities/SpdxFile.cs index c0d8f6802..54b0ac54a 100644 --- a/src/Microsoft.Sbom.Parsers.Spdx22SbomParser/Entities/SpdxFile.cs +++ b/src/Microsoft.Sbom.Parsers.Spdx22SbomParser/Entities/SpdxFile.cs @@ -34,21 +34,18 @@ public class SPDXFile /// /// Gets or sets contain the license the SPDX file creator has concluded as the package or alternative values. /// - [JsonRequired] [JsonPropertyName("licenseConcluded")] public string LicenseConcluded { get; set; } /// /// Gets or sets contains the license information actually found in the file, if any. /// - [JsonRequired] [JsonPropertyName("licenseInfoInFiles")] public IEnumerable LicenseInfoInFiles { get; set; } /// /// Gets or sets copyright holder of the package, as well as any dates present. /// - [JsonRequired] [JsonPropertyName("copyrightText")] public string FileCopyrightText { get; set; } diff --git a/test/Microsoft.Sbom.Parsers.Spdx22SbomParser.Tests/Parser/SbomFileParserTests.cs b/test/Microsoft.Sbom.Parsers.Spdx22SbomParser.Tests/Parser/SbomFileParserTests.cs index 0525c98b1..4ca317bb9 100644 --- a/test/Microsoft.Sbom.Parsers.Spdx22SbomParser.Tests/Parser/SbomFileParserTests.cs +++ b/test/Microsoft.Sbom.Parsers.Spdx22SbomParser.Tests/Parser/SbomFileParserTests.cs @@ -104,9 +104,6 @@ public void MissingPropertiesTest_ThrowsSHA256() [DataRow(SbomFileJsonStrings.JsonWith1FileMissingNameString)] [DataRow(SbomFileJsonStrings.JsonWith1FileMissingIDString)] [DataRow(SbomFileJsonStrings.JsonWith1FileMissingChecksumsString)] - [DataRow(SbomFileJsonStrings.JsonWith1FileMissingLicenseConcludedString)] - [DataRow(SbomFileJsonStrings.JsonWith1FileMissingLicenseInfoInFilesString)] - [DataRow(SbomFileJsonStrings.JsonWith1FileMissingCopyrightString)] [DataRow(SbomFileJsonStrings.JsonWith1FileMissingCopyrightAndPathString)] [TestMethod] public void MissingPropertiesTest_Throws(string json)