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 proto definition for vuln predicate type #345

Merged
merged 9 commits into from
Nov 5, 2024

Conversation

hectorj2f
Copy link
Contributor

This PR adds a proto definition for the vuln predicate type. This was pending since we added the vuln predicate type and it is needed to update the cosign vuln predicate types.

@hectorj2f
Copy link
Contributor Author

related to #268

Copy link
Contributor

@TomHennen TomHennen left a comment

Choose a reason for hiding this comment

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

Thanks for sending this.

Besides the other comments, have you by any chance generated any json with this? Could you provide a sample? it would be nice to compare it against the example https://github.com/in-toto/attestation/blob/main/spec/predicates/vuln.md#example

go/predicates/vuln/v1/vuln.pb.go Outdated Show resolved Hide resolved
protos/in_toto_attestation/predicates/vuln/v1/vuln.proto Outdated Show resolved Hide resolved
protos/in_toto_attestation/predicates/vuln/v1/vuln.proto Outdated Show resolved Hide resolved
@marcelamelara
Copy link
Contributor

Friendly ping on this PR. @hectorj2f Could you please remove the generated Go/Python/Java files and add an entry for this proto here? then I think your PR will be pretty much ready to go.

@hectorj2f
Copy link
Contributor Author

@marcelamelara Yes, I'll do it.

@TomHennen
Copy link
Contributor

I think something might have gotten a bit screwy with the git commits (I've done this myself for sure). When I look at 'Files Changed' it doesn't look like anything has changed since our earlier comments?

@hectorj2f
Copy link
Contributor Author

I'm confused now. I followed what @marcelamelara commented above remove all the generated go/python and java code and leave the entry in protos.

@marcelamelara
Copy link
Contributor

If I understand correctly, having only the .proto file is what Tom had originally requested. But there may have been some edits to that proto file that were lost when the auto-generated files were removed? I only see 1 commit in this PR right now.

Separately, I also had requested a minor documentation update to the /protos/README.md file. This way this new proto is included in that list.

Copy link
Contributor

@TomHennen TomHennen left a comment

Choose a reason for hiding this comment

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

Thanks for these changes, just a couple minor updates needed I think.

protos/in_toto_attestation/predicates/vuln/v1/vuln.proto Outdated Show resolved Hide resolved
protos/in_toto_attestation/predicates/vuln/v1/vuln.proto Outdated Show resolved Hide resolved
protos/in_toto_attestation/predicates/vuln/v1/vuln.proto Outdated Show resolved Hide resolved
protos/in_toto_attestation/predicates/vuln/v1/vuln.proto Outdated Show resolved Hide resolved
protos/in_toto_attestation/predicates/vuln/v1/vuln.proto Outdated Show resolved Hide resolved
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 @hectorj2f ! LGTM.

@marcelamelara
Copy link
Contributor

One final question (which can be addressed in a separate PR, I think) is about the predicate type URI for this predicate. Right now, in the vuln predicate spec it's listed as https"//in-toto.io/attestation/vuln_s_ rather than /vuln which is the name we use everywhere else. Does the spec have a typo? We usually want the name to match everywhere, but I'm also worried that tooling might already be using the pluralized type URL.

@hectorj2f What do you think? Is this a concern?

@hectorj2f
Copy link
Contributor Author

@marcelamelara Let's change vuln to vulns wherever we use it as part of a different PR.

@hectorj2f hectorj2f force-pushed the vuln-att-protos branch 2 times, most recently from bcbd48e to ffffbc7 Compare October 13, 2024 20:14
@hectorj2f
Copy link
Contributor Author

@marcelamelara @lumjjb I've applied some of the changes you proposed.

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 so much for the updates @hectorj2f ! At this point the changes I'm requesting are just some small documentation fixes.

spec/predicates/README.md Outdated Show resolved Hide resolved
Signed-off-by: hectorj2f <[email protected]>
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 @hectorj2f ! LGTM

@TomHennen
Copy link
Contributor

I appreciate these changes, but the one big problem I see is that it erases the old 'vuln v0.1' predicate and replaces it with this new one.

There are folks that are using the old predicate (however flawed it might be me), we should keep that copy around for them to reference.

So a couple requests:

  1. Leave the old vuln.md there (folks probably have links to it). Maybe add a note pointing them at the newer version.
  2. Update the version in the new vulns to 0.2
  3. Update the proto to match 0.2. (including it's path in the Protos directory)
  4. Rename vulns.md to vulns_02.md

Signed-off-by: hectorj2f <[email protected]>
@hectorj2f
Copy link
Contributor Author

@TomHennen I believe I've made all your requested changes.

@TomHennen
Copy link
Contributor

Thank you! And I'm so sorry it took me so long to get back to this. I missed the email about the update. :(

@TomHennen TomHennen merged commit 099c970 into in-toto:main Nov 5, 2024
2 checks passed
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.

4 participants