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 Protobuf and Go generated code for CycloneDX 1.5 #296

Closed

Conversation

trishankatdatadog
Copy link
Member

IDK if there's any interest for the Attestation maintainers to keep track of the CycloneDX Protobuf and the resulting Go generated code, but I figured it'd be useful for downstream consumers.

The biggest change here that results in backwards-incompatibility is the different predicateType which I thought is more accurate in terms of actually pointing to the JSON schema. Please advise.

@trishankatdatadog trishankatdatadog requested a review from a team as a code owner November 3, 2023 02:17
@trishankatdatadog trishankatdatadog changed the title Add Go generated code for CycloneDX 1.5 Add Protobuf and Go generated code for CycloneDX 1.5 Nov 3, 2023
@TomHennen
Copy link
Contributor

Since we just delegate to the cyclonedx spec this seems reasonable?

Do you know of anyone that actually wants to use this right now?

@trishankatdatadog
Copy link
Member Author

Do you know of anyone that actually wants to use this right now?

Yes, us (Datadog) 🙂

@trishankatdatadog
Copy link
Member Author

trishankatdatadog commented Nov 3, 2023

Do you know of anyone that actually wants to use this right now?

Also, do you mean specifically CycloneDX 1.5, or just the CycloneDX Protobuf Go generated code in general?

@TomHennen
Copy link
Contributor

Do you know of anyone that actually wants to use this right now?

Also, do you mean specifically CycloneDX 1.5, or just the CycloneDX Protobuf Go generated code in general?

The protobuf Go generated code.

I think if there's actually someone that wants to use it this sounds like a good idea?

@trishankatdatadog
Copy link
Member Author

The protobuf Go generated code.

Let me double-check our internal use and get back to you...

@mlieberman85
Copy link

I would be curious if folks are more interested in the json schema or xsd over the protobuf or at least want the options. I say this because the json schema and xsd also include additional type information, regex validations, etc. whereas the protobuf only has basic types like strings. I noticed the generated code from json schema and xsd will include those validations in the deserialization logic.

@trishankatdatadog
Copy link
Member Author

@mlieberman85 Agree, but pretty sure we don't (yet) specify Predicates beyond Protobuf right now

Copy link
Contributor

@marcelamelara marcelamelara left a comment

Choose a reason for hiding this comment

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

Thanks @trishankatdatadog for this PR! LGTM, just a couple minor changes requested.

spec/predicates/cyclonedx.md Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this should be within the scope of this PR: How tough would it be to add a proto validator for this predicate like: https://github.com/in-toto/attestation/blob/main/go/predicates/provenance/v1/provenance.go ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Shouldn't be hard, but I'll need to get back to you...

Copy link
Member

@pxp928 pxp928 left a comment

Choose a reason for hiding this comment

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

LGTM in terms of the proto and code gen perspective. Thanks!

@trishankatdatadog
Copy link
Member Author

Thanks! I wonder if it's worth making a protobom attestation? But that's a separate issue and PR...

@mikhailswift
Copy link
Member

After a discussion in last week's maintainers meeting, we have concerns about vendoring additional dependencies and think that it would be ideal to keep this repo lightweight and specific to in-toto

@trishankatdatadog
Copy link
Member Author

After a discussion in last week's maintainers meeting, we have concerns about vendoring additional dependencies and think that it would be ideal to keep this repo lightweight and specific to in-toto

Understand where you're coming from, but also respectfully disagree.

How should downstream end-users use this repo? The CycloneDX predicate is loosely defined in as Markdown in this repo, but there is no protobuf or generated Go code for it, unlike with other predicates. What are users supposed to do in this case?

This is not to mention that there are no extra utilities for doing repetitive tasks such as signing and storing attestations. There is a stronger argument that those functions are beyond the scope of this repo, but I would like to know how end-users should actually use this repo for at least standard predicates.

@marcelamelara
Copy link
Contributor

With protobom beginning to mature, I also wonder how that project plays into this effort.

@trishankatdatadog
Copy link
Member Author

With protobom beginning to mature, I also wonder how that project plays into this effort.

Agreed. My recommendation is to somehow resolve the inconsistency that I pointed out above: if you're not going to go beyond simply saying that it's possible to sign SBOMs as in-toto attestations, then at least explain to users why you're not able to help them actually do so with this repo.

@marcelamelara
Copy link
Contributor

marcelamelara commented Jan 19, 2024

My recommendation is to somehow resolve the inconsistency that I pointed out above: if you're not going to go beyond simply saying that it's possible to sign SBOMs as in-toto attestations, then at least explain to users why you're not able to help them actually do so with this repo.

@trishankatdatadog Point taken, and the sooner we resolve this the better.

I think there are 2 things that need to happen:

(1) We need to decide more broadly how to handle providing the language bindings for externally defined, but in-toto supported, predicates like SLSA Provenance, CycloneDX etc.

So far, we've decided on a case-by-case basis. After a discussion with the SLSA community, they decided that in-toto should host the protos and language bindings because there was precedent for that. The SBOM ecosystem is trickier because they've developed a fair amount of tooling independently already.

One of the main concerns with this PR that came up during one of our maintainer's discussions was having to add and maintain submodules for the SBOM protos and in some ways possibly duplicate work. Which leads me to point (2).

(2) Clearly document that this repo only provides data structures, and point to the in-toto Go/Python/Java etc. APIs and tools that do handle attestation generation.

Especially for externally defined standards, I don't see a conflict between clearly signaling that these are supported in-toto predicates, and pointing to the corresponding in-toto implementation that actually generates attestations with them.

Now, this is currently challenging because this is contingent on having a "canonical" in-toto APIs for each ecosystem. But with the upcoming in-toto Go consolidation, for instance, there's a chance we'll end up moving code out of this repo and into the Go implementation, so the intent truly is to keep the code in this repo as lightweight as possible.

While there doesn't exist a single "canonical" in-toto Go implementation, there is something to be said about adding the language bindings here to import a single dependency in an attestation generator. So, I can be convinced to add the protos submodule for CycloneDX here as a stop-gap measure, especially if it is possible to automate the process of maintaining the submodule with something like dependabot.

On the flip side, if Cyclone and SPDX also already provide language bindings, then it should really be the corresponding "canonical" in-toto APIs/implementation that import those bindings directly. So, should we decide to merge this PR, once the Go consolidation and Python implementations are updated to fully support ITEs 6 and 9, my take would be to shift towards supporting signed SBOM generation directly within those implementations, and deprecating support here.

I'd love to hear others' thoughts on this: @TomHennen @mikhailswift @jkjell @adityasaky

@pxp928
Copy link
Member

pxp928 commented Jan 22, 2024

Correct me if I am wrong but is the thought process to move https://github.com/in-toto/in-toto-golang/tree/master/in_toto from in-toto-golang into another in-toto Go implementation library? This would also house other Go implementations of the various predicates we have defined here.

@marcelamelara
Copy link
Contributor

Correct me if I am wrong but is the thought process to move https://github.com/in-toto/in-toto-golang/tree/master/in_toto from in-toto-golang into another in-toto Go implementation library? This would also house other Go implementations of the various predicates we have defined here.

My understanding from the in-toto-go-consolidation discussions so far has been that we'll refactor some pieces from in-toto-golang into go-witness, and eventually deprecate in-toto-golang. The language bindings for the predicates defined in here would then be imported directly into go-witness. When this all will happen is less clear.

@trishankatdatadog
Copy link
Member Author

Will revisit later when need be. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants