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

Merging tekton-provenance and slsa-provenance predicate formats #176

Closed
3 of 4 tasks
priyawadhwa opened this issue Oct 6, 2021 · 19 comments · Fixed by #179
Closed
3 of 4 tasks

Merging tekton-provenance and slsa-provenance predicate formats #176

priyawadhwa opened this issue Oct 6, 2021 · 19 comments · Fixed by #179
Labels
provenance Applies to SLSA provenance spec

Comments

@priyawadhwa
Copy link
Collaborator

priyawadhwa commented Oct 6, 2021

I'm opening a tracking issue where we can keep track of progress on this and consolidate opinions. Some things to consider:

Feel free to add anything I forgot!

cc @loosebazooka @MarkLodato @inferno-chromium @bobcatwilson @TomHennen @msuozzo

@dlorenc
Copy link

dlorenc commented Oct 7, 2021

The other biggest one IMO is the definedInMaterial pointer. This is covered in bad API design smells, and seems hard to actually use correctly.

I'd strongly prefer the recipe URI approach used in Tekton Chains. The system can refer directly to where it fetched the recipe at a URI. That may or may not be in a material.

@msuozzo
Copy link
Contributor

msuozzo commented Oct 7, 2021

Yep +1 dan. This is one action items I think we should add for revising the recipe schema. Correlating the URI with materials then needs to have careful consideration but I think that's tractable.

@trishankatdatadog
Copy link
Member

Seems fine to me.

@joshuagl
Copy link
Member

joshuagl commented Oct 7, 2021

Agree with the intent, it would be good to have one provenance predicate if we can.

For others less familiar with Tekton, here's their current provenance predicate definition: https://github.com/tektoncd/chains/blob/61ebcf67bad289b60d4a184b62309ccd44a5559e/PROVENANCE_SPEC.md

@joshuagl joshuagl closed this as completed Oct 7, 2021
@joshuagl joshuagl reopened this Oct 7, 2021
@MarkLodato MarkLodato added the provenance Applies to SLSA provenance spec label Oct 7, 2021
@dlorenc
Copy link

dlorenc commented Oct 7, 2021

Cc @mattmoor is there anything else we need changed in this predicate to get rid of the Tekton one?

I think my big 3 issues were:

  • Separate materials from steps
  • Recipe URI instead of the pointer
  • Inline steps information (command, entry point, arguments, environment) where present

I think that all of these are mostly captured in the bullets Priya outlined here.

@trishankatdatadog
Copy link
Member

  • Separate materials from steps

Agree with the rest, but hold on, why separate materials from steps? Don't you want to associate materials/input and products/output with each step? Or am I misunderstanding what you mean here?

@dlorenc
Copy link

dlorenc commented Oct 7, 2021

Agree with the rest, but hold on, why separate materials from steps? Don't you want to associate materials/input and products/output with each step? Or am I misunderstanding what you mean here?

Sorry I explained poorly. This is exactly the goal. The current SLSA predicate treats the "step" environment (for example, which container image a step ran in ) the same as the "inputs" (for example, which git repository was cloned at the start). The idea is just to model those concepts separately rather than as one flat list of "materials".

@trishankatdatadog
Copy link
Member

Sorry I explained poorly. This is exactly the goal. The current SLSA predicate treats the "step" environment (for example, which container image a step ran in ) the same as the "inputs" (for example, which git repository was cloned at the start). The idea is just to model those concepts separately rather than as one flat list of "materials".

Ah, I see, I think I get it now. Ok, let me review the PRs to make sure we are all on the same page 👍🏽

@joshuagl
Copy link
Member

joshuagl commented Oct 7, 2021

@dlorenc or @priyawadhwa could you link to/paste an example of a populated Tekton predicate? Might make this easier to think about.

@dlorenc
Copy link

dlorenc commented Oct 7, 2021

There should be a couple here: https://security.googleblog.com/2021/09/distroless-builds-are-now-slsa-2.html

I think @priyawadhwa made a couple small tweaks/bug fixes after this though.

@dlorenc
Copy link

dlorenc commented Oct 7, 2021

Here's my rough mental model for these concepts:

image

The invocation contains all information a system needs to kick off a build. The recipe and materials are specified by URI.

The build system:

  • resolves the actual recipe from the URI reference
  • fetches materials (inputs)
  • prepares the execution environment (usually container images)
  • executes the build
  • creates the subject(s)

The recipe may or may not be in a material. The recipe URI can be easily compared to the material URIs to determine this, or policy can be directly applied against the recipe URI itself. This handles cases where builds are completely specified inline (gcloud container builds submit), cases where the recipe is defined in source control, or cases where the recipe has been defined via some other artifact reference (OCI, etc.)

@mattmoor
Copy link
Contributor

mattmoor commented Oct 7, 2021

It has been a while since I carefully examined either, and Tekton's hermeticity options leave something to be desired, so I would say something else that might be worth looking at is how hermeticity is expressed.

I think that generally the goal of SLSA 4 is (or should be) to make sure that any step that can executes user-sourced code is executed hermetically. So for example, it may make sense for a fetch step (e.g. git, kaniko's warmer) to execute prior to the network jail, but for user-sourced "builds (e.g. make, kaniko) to execute within the network jail, and finally for publish steps (e.g. docker push) to execute outside of the network jail.

Tekton's API for this (today) is too coarse (and I have several TEPs open with proposals), but we should make sure that the predicate is sufficiently fine-grained to capture the intent here.

@MarkLodato
Copy link
Member

Let's move the discussion around definedInMaterial to #178, and reserve this top-level issue to discuss the overall idea or propose new sub-bugs. Otherwise the thread will be difficult to follow.

Thanks @priyawadhwa for filing this!

@slsa-framework slsa-framework deleted a comment from joshuagl Oct 7, 2021
@slsa-framework slsa-framework deleted a comment from dlorenc Oct 7, 2021
@inferno-chromium
Copy link
Contributor

Yes i agree, lets file sub-bugs and we can update c#0 to account for it. Thank you everyone for the productive discussion.

@dlorenc
Copy link

dlorenc commented Oct 10, 2021

From the feedback we're receiving it seems like there's not really appetite to merge the changes from the Tekton format back in here. I agree it would be nice to have one format, but the goals might be different enough that it's better to just have multiple.

Since adoption is still so early on both, I'll suggest we just wait until things mature and we have a better idea of how these are actually used end to end before making any changes.

@inferno-chromium
Copy link
Contributor

From the feedback we're receiving it seems like there's not really appetite to merge the changes from the Tekton format back in here. I agree it would be nice to have one format, but the goals might be different enough that it's better to just have multiple.

Since adoption is still so early on both, I'll suggest we just wait until things mature and we have a better idea of how these are actually used end to end before making any changes.

I do agree with your sentiment, but please note that SLSA steering committee has not made their decision on this, so we will give the benefit of doubt and resolve any conflicts with discussions in SLSA biweekly. I also think there should be multiple formats allowed, but what we say here is SLSA provenance format should be simplest and easiest to the use for the community for easy uptake.

@dlorenc
Copy link

dlorenc commented Oct 10, 2021

but what we say here is SLSA provenance format should be simplest and easiest to the use for the community for easy uptake.

Big +1!

I think the best way to do this is to iterate quickly, prototype, see what works, see what people actually want to use these payloads for and how they use them.

I don't think this is the type of thing that can be designed in a committee or a vacuum without first getting a lot more data and adoption. Arguing over the names of fields is missing the forest for the trees.

My focus in the Tekton predicate is on making it the most useful format for Tekton users, for whatever use cases they have. That means getting it in the hands of users.

@mattmoor
Copy link
Contributor

Just to echo the sentiment about experimentation. I was actually surprised to find that SLSA had already defined its own predicate type when I was first going through this because it’s very existence will preclude some amount of experimentation in the community.

@dlorenc
Copy link

dlorenc commented Oct 10, 2021

Just to echo the sentiment about experimentation. I was actually surprised to find that SLSA had already defined its own predicate type when I was first going through this because it’s very existence will preclude some amount of experimentation in the community.

I think the history here was actually in line with this sentiment, the "SLSA" format was initially defined in the in-toto attestation repo: https://github.com/in-toto/attestation. The in-toto attestation model is designed to support many predicate types, so we decided to move it from there to help make it clear that this wasn't the only supported in-toto predicate format.

It seems to have just moved the problem down a level though :)

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

Successfully merging a pull request may close this issue.

8 participants