cyclonedx-dotnet-library icon indicating copy to clipboard operation
cyclonedx-dotnet-library copied to clipboard

support cdx 1.6

Open mtsfoni opened this issue 1 year ago • 1 comments

mtsfoni avatar May 05 '24 18:05 mtsfoni

@mtsfoni Do you have an understanding which features/changes still need to be added to support 1.6 (https://github.com/CycloneDX/specification/releases/tag/1.6)? From what I can see your changes are mostly related to general revisioning and CBOM. This means we are missing at least:

  • Attestation
  • Several smaller features (like tags, swhid, etc.)
  • Json and Protobuf tests

andreas-hilti avatar Aug 19 '24 20:08 andreas-hilti

  • The CBOM part probably still needs adaption for JSON. It seems to be a good idea to start with JSON and add XML after - unfortunately I did it the other way. This will be my next step.

  • Attestation is missing.

  • Smaller Features might or might not be added, needs checking.

  • I added the test. But many tests were generated outputting an empty BOM, so before releasing the round-trip-test have to be reviewed, that in- and output are actually equal. (For example the Attestation tests)

I hope we managed to fix or at least work around the already existing problem enough now. Those quite threw me off, when I started implementing this months ago. Thank you for your help with those!

mtsfoni avatar Aug 24 '24 16:08 mtsfoni

@andreas-hilti

On another note, why did you introduce additional fields like CipherSuite.Algorithms_XML?

I think, that was my first attempt to solve the extra nesting level of lists in xml. It seems to also work with XmlArray/XmlArrayItem - I willl change it.

One issue which fails several of the tests is that Evidence.Identity can now also be an array of EvidenceIdentities.

Looking into that now.

mtsfoni avatar Aug 25 '24 09:08 mtsfoni

Looks like you already fixed Evidence.Identity?

mtsfoni avatar Aug 25 '24 09:08 mtsfoni

Looks like you already fixed Evidence.Identity?

Yes, at least I tried.

andreas-hilti avatar Aug 25 '24 09:08 andreas-hilti

@andreas-hilti just so that we dont make things double, because i see you doing a lot right now: I am working on attestations right now.

mtsfoni avatar Aug 25 '24 11:08 mtsfoni

I did Declarations/Attestations. I see you did some smaller things.

Do you know ad hoc what is still open?

mtsfoni avatar Aug 25 '24 14:08 mtsfoni

I did Declarations/Attestations. I see you did some smaller things.

Do you know ad hoc what is still open?

From the "Added" section of the release notes:

  • [x] Core enhancement: Cryptography Bill of Materials — CBOM (https://github.com/CycloneDX/specification/issues/171, https://github.com/CycloneDX/specification/issues/291 via https://github.com/CycloneDX/specification/pull/347)
  • [x] Core enhancement: Attestation — CDXA (https://github.com/CycloneDX/specification/issues/192 via https://github.com/CycloneDX/specification/pull/348)
  • [x] Feature to express the URL to source distribution (https://github.com/CycloneDX/specification/issues/98 via https://github.com/CycloneDX/specification/pull/269)
  • [x] Feature to express the URL to RFC 9116 compliant documents (https://github.com/CycloneDX/specification/issues/380 via https://github.com/CycloneDX/specification/pull/381)
  • [x] Feature to express tags/keywords for services and components (via https://github.com/CycloneDX/specification/pull/383)
  • [x] Feature to express details for component authors (https://github.com/CycloneDX/specification/issues/335 via https://github.com/CycloneDX/specification/pull/379)
  • [x] Feature to express details for component and BOM manufacturer (https://github.com/CycloneDX/specification/issues/346 via https://github.com/CycloneDX/specification/pull/379)
  • [x] Feature to express communicate concluded values from observed evidences (https://github.com/CycloneDX/specification/issues/411 via https://github.com/CycloneDX/specification/pull/412)
  • [x] Features to express license acknowledgement (https://github.com/CycloneDX/specification/issues/407 via https://github.com/CycloneDX/specification/pull/408)
  • [x] Feature to express environmental consideration information for model cards (https://github.com/CycloneDX/specification/issues/396 via https://github.com/CycloneDX/specification/pull/395)
  • [x] Feature to express the address of organizational entities (via https://github.com/CycloneDX/specification/pull/395)
  • [x] Feature to express additional component identifiers: Universal Bill Of Receipts Identifier and Software Heritage persistent IDs (https://github.com/CycloneDX/specification/issues/413 via https://github.com/CycloneDX/specification/pull/414)

andreas-hilti avatar Aug 25 '24 14:08 andreas-hilti

In addition, a couple of things that are wrong in the current implementation:

  • [x] Fix de-/serialization of ImplementationPlatform (because both "-" and "_" are used)
  • [x] Fix JSON de-/serialization of DatasetChoices (see e.g. valid-machine-learning-1.6.json)
  • [ ] Add deprecations for some entries in the context of component/BOM authors/manufacturer
  • [x] There are v1.5 snapshots in the Json\v1.6 folder
  • [x] Definitions/Standards are still missing (see e.g. valid-standard-1.6.json)
  • [ ] Fix merging new top level bom elements (Declarations, Definitions)
  • [ ] EnumerateAllComponents needs to be updated as there are more places where components can be used (Formulation>Components, Declarations>Target>Components, Vulnerabilities>Tools>Components, Annotations>Annotator>Component etc.) Maybe similar for services.
  • [x] Fix LifecyclePhase (No Null value, design starts at 0)

andreas-hilti avatar Aug 25 '24 14:08 andreas-hilti

Environmental Consideration and the Postal Address are added.

Somethink that I just stumbled over: the outputs in the Protobuf-Tests have other enum values than expected. I am not sure if this is an actual problem or just a problem with how the tests work. Generally I don't like the protobuf test. Maybe we should make an extended json/xml roundtrip, that includes a seralization to and a deserialization from protobuf in the middle.

Maybe the problem is just the order in the enum, need to check that tomorrow.

mtsfoni avatar Aug 25 '24 22:08 mtsfoni

Somethink that I just stumbled over: the outputs in the Protobuf-Tests have other enum values than expected. I am not sure if this is an actual problem or just a problem with how the tests work. Generally I don't like the protobuf test. Maybe we should make an extended json/xml roundtrip, that includes a seralization to and a deserialization from protobuf in the middle.

Maybe the problem is just the order in the enum, need to check that tomorrow.

In general, if it is like this, it is an actual issue; in most cases, I think it goes back to a wrong order and a missing null value. The protobuf field number is based on the integer value of the enum (unless you use ProtoMember to overwrite it), which by default is based on the order starting at value 0.

I was also wondering how to improve the tests, in particular whether one can test consistency more. However, if you just serialize to protobuf and deserialize using the same implementation, such things as wrong protobuf enum values will stay undetected.

andreas-hilti avatar Aug 26 '24 04:08 andreas-hilti

Given that the official samples are there in all three formats, one could try the following: Deserialize them in two formats, say JSON and protobuf, and test the resulting in-memory BOMs for equality. However, this will only work if

  • the samples are actually identical in the different formats
  • the three different formats are identical (at least for the relevant content) (this is unfortunately not 100% true)

It also poses the question on how to compare the resulting BOMs, but maybe the JSON-based equality check is good enough for starters, except to compare actual differences.

andreas-hilti avatar Aug 26 '24 06:08 andreas-hilti

As a side-note: when doing the comparison, I noticed that all DateTime fields are incorrectly serialized to protobuf. adding: [ProtoMember(7, DataFormat = global::ProtoBuf.DataFormat.WellKnown)] public DateTime? LastRenewal

seems to fix it, unfortunately, it is marked as deprecated. In the CycloneDX proto definition, it is using google.protobuf.Timestamp, but this is not respected otherwise. Compare also https://stackoverflow.com/questions/51271719/decoded-timestamp-is-not-the-same-date-as-original-datetime-value

Edit: Seems like this would be the way to fix it: [ProtoMember(7)] [CompatibilityLevel(CompatibilityLevel.Level240)] public DateTime? LastRenewal

Edit 2: Doesn't seem to work in all cases, but I don't understand why... Edit 3: Maybe I just confused myself because the examples are inconsistent. And it seems to be enough to specify it once on module level: [module: CompatibilityLevel(CompatibilityLevel.Level300)]

andreas-hilti avatar Aug 27 '24 21:08 andreas-hilti

You say this is an already existing bug? Nobody complained yet - but it's possible that only a few people actually use proto for sboms.

I understand when we place [module: CompatibilityLevel(CompatibilityLevel.Level300)] anywhere in the .Core project and it will work correctly? It would however be a breaking change, and proto boms that were created in an older version of the library will have an incorrect date when deserialized again?

mtsfoni avatar Aug 28 '24 21:08 mtsfoni

You say this is an already existing bug? Nobody complained yet - but it's possible that only a few people actually use proto for sboms.

I understand when we place [module: CompatibilityLevel(CompatibilityLevel.Level300)] anywhere in the .Core project and it will work correctly? It would however be a breaking change, and proto boms that were created in an older version of the library will have an incorrect date when deserialized again?

I agree with all of the above. Just to add: if the old proto boms were deserizalized with a different tool (conforming to the specification), then it would lead to wrong values or fail as well.

andreas-hilti avatar Aug 28 '24 21:08 andreas-hilti

So we put the change in, but add a big note to the changelogs

mtsfoni avatar Aug 29 '24 18:08 mtsfoni