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

JSON Schemas for attestors with generation scripts #197

Merged
merged 12 commits into from
May 10, 2024

Conversation

ChaosInTheCRD
Copy link
Collaborator

No description provided.

Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
@matglas
Copy link
Contributor

matglas commented Apr 30, 2024

@ChaosInTheCRD the idea is to have this schema be published into a readable form for https://witness.dev for example so people can understand what the data is they will get correct?

@ChaosInTheCRD
Copy link
Collaborator Author

@matglas that is correct! Probably would be helpful to expose these on the CLI as well. I need to circle back on this at some point and get it ready for merge 😄

Signed-off-by: John Kjell <john@testifysec.com>
Copy link
Member

@jkjell jkjell left a comment

Choose a reason for hiding this comment

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

Looks good and very straight forward. 👍

Two things this made me think about when reviewing:

  • How are nested attestors represented (i.e. GitHub/GitLab using the JWT attestor)?
  • For the (near-term) future: How do we handle schema versioning?

There's a few things that broke from the VSA stuff (and it'll break again after the Link/SLSA stuff too 😅).

…on-schema

Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
…tness into attestor-json-schema

Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
schemagen/schema.go Outdated Show resolved Hide resolved
Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
@ChaosInTheCRD
Copy link
Collaborator Author

* How are nested attestors represented (i.e. GitHub/GitLab using the JWT attestor)?

@jkjell good catch, it is represented like:

      "properties": {
        "jwt": {
          "$ref": "#/$defs/Attestor"
        },

so yeah, the nesting seems to be an issue. I'm not sure what the tm correct way of fixing this is (besides doing a version of what I did for the SLSA, Link and VSA attestors.

*For the (near-term) future: How do we handle schema versioning?

Ah, of course we handle versioning with predicate type, but this isn't included in the JSON schema...

Copy link
Member

@jkjell jkjell left a comment

Choose a reason for hiding this comment

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

Everything looks good other than the material and product attestor generation. If those are resolved we'll be good to go. ✅

schemagen/material.json Outdated Show resolved Hide resolved
schemagen/product.json Outdated Show resolved Hide resolved
@ChaosInTheCRD
Copy link
Collaborator Author

* How are nested attestors represented (i.e. GitHub/GitLab using the JWT attestor)?

@jkjell good catch, it is represented like:

      "properties": {
        "jwt": {
          "$ref": "#/$defs/Attestor"
        },

so yeah, the nesting seems to be an issue. I'm not sure what the tm correct way of fixing this is (besides doing a version of what I did for the SLSA, Link and VSA attestors.

*For the (near-term) future: How do we handle schema versioning?

Ah, of course we handle versioning with predicate type, but this isn't included in the JSON schema...

@jkjell with respect to this, should I try and find a solution or do you think it's fine as is for now?

@jkjell
Copy link
Member

jkjell commented May 10, 2024

should I try and find a solution or do you think it's fine as is for now?

It's good for now. This is WAY better than what we have now... i.e. the source code. 😂

Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
@jkjell jkjell merged commit f346f85 into in-toto:main May 10, 2024
12 checks passed
@colek42
Copy link
Member

colek42 commented May 11, 2024

This is a great step forward. Aligning with upstream using protos would be nice, but a lot more work

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