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

[feature] Re-generate the regression tests for docker-based builder #610

Closed
laurentsimon opened this issue May 23, 2023 · 11 comments · Fixed by #627
Closed

[feature] Re-generate the regression tests for docker-based builder #610

laurentsimon opened this issue May 23, 2023 · 11 comments · Fixed by #627

Comments

@laurentsimon
Copy link
Contributor

laurentsimon commented May 23, 2023

The attestation currently does not follow the v1.0 specs:

"buildDefinition": {
      "buildType": "https://slsa.dev/container-based-build/v0.1?draft",
      "externalParameters": {
        "source": {
          "uri": "git+https://github.com/slsa-framework/example-package@refs/heads/main",
          "digest": {
            "sha1": "79877134d7d62f3cf333ea8f41998f59c5f4d08e"
          }
        },

It should instead nest the inputs into an inputs field:

"buildDefinition": {
      "buildType": "https://slsa.dev/container-based-build/v0.1?draft",
      "externalParameters": {
        "inputs":{
            "source": {
              "uri": "git+https://github.com/slsa-framework/example-package@refs/heads/main",
              "digest": {
                "sha1": "79877134d7d62f3cf333ea8f41998f59c5f4d08e"
              }
          },
        },

I would also be useful to add workflow:

"workflow": {
          "ref": "refs/heads/main",
          "repository": "git+https://github.com/laurentsimon/slsa-delegate-project",
          "path": ".github/workflows/anchor-sbom.yml"
        }

to help during troubleshooting or incidence response.

Should be fixed in the generator before GA

/cc @asraa @rbehjati

@asraa
Copy link
Contributor

asraa commented May 23, 2023

I would also be useful to add workflow:

One thing we discussed here was that in this case workflow is not really a parameter of the build type (the build type is a docker build - which should be agnostic to the workflow that ran it) and these details belong in the system parameters and from the build invocation ID. WDYT? I agree the info is useful, but it doesn't seem like an input to the build we are describing.

It should instead nest the inputs into an inputs field:

Can do - I suppose this makes sense if it's v1.0 spec to nest inside the inputs rather than directly.

I can make a quick turn-around fix for what we decide though.

@laurentsimon
Copy link
Contributor Author

I would also be useful to add workflow:

One thing we discussed here was that in this case workflow is not really a parameter of the build type (the build type is a docker build - which should be agnostic to the workflow that ran it) and these details belong in the system parameters and from the build invocation ID. WDYT? I agree the info is useful, but it doesn't seem like an input to the build we are describing.

We need to decide where we put that data in the provenance for all the builders. Right now it's in the externalParameters in the verify-token code (I think because we copied from the slsa.dev example), but I'm fine having it internalParameters.

@ianlewis no objection to this update?

I can make a quick turn-around fix for what we decide though.

SG. Do you want to also update the verify-token Action as part of the PR?

@laurentsimon
Copy link
Contributor Author

laurentsimon commented May 23, 2023

Created #611 t update to verifier code. One complication is that depending on the builder (builder vs generator), the workflow will be either in the externalParameters or in the internalParameters. I think this means we'll need to retrieve the workflow information differently based on builder "type" (GH-runner vs re-usable workflow).

Not sure yet what's the best way to do that.

@ianlewis
Copy link
Member

ianlewis commented May 24, 2023

@ianlewis no objection to this update?

I'm not sure I totally follow. The distinction between internal and external was "does the user have control over the parameter?". In most cases the user does technically have control over the workflow which is why it's in external parameters. At least that's how I understood it.

(the build type is a docker build - which should be agnostic to the workflow that ran it)

I suppose I see the docker image itself as analogous to the "run scripts" for the Node.js builder. Yes, these can and probably should be agnostic to the actual workflow that runs them, but the boundary that matters from SLSA's perspective is still the reusable workflow boundary.

@rbehjati
Copy link

I am not sure why we need to nest the parameters under an input field. Is this a requirement from SLSA v1?

Regarding externalParameters vs internalParameters, I agree with @ianlewis. The documentation for internalParameters says: "The parameters that are under the control of the entity represented by builder.id. ...". The ones we have are provided by the user, and should be in externalParameters.

@asraa
Copy link
Contributor

asraa commented May 24, 2023

the buildType should indicate to the user what the externalParameters represent: should we then say that the buildType for a general container execution determines the externalParameters.inputs which are the container image and command / config etc?

The other externalParameters fields may just be other user inputs that are dependent on the builder (which is a little odd, but it is still a "user" parmaeter - just not buildType specific).

In this case is the resolution to revert slsa-framework/slsa-github-generator#2165 and add workflow to externalParamters for the container builder?

@asraa
Copy link
Contributor

asraa commented May 24, 2023

This is what I'm referring to https://slsa.dev/provenance/v1#buildType

The URI SHOULD resolve to a human-readable specification that includes: overall description of the build type; schema for externalParameters and internalParameters; unambiguous instructions for how to initiate the build given this BuildDefinition, and a complete example.

This means that the buildType for a container execution can't really be re-used cleanly across other build systems, unless we have a builder-specific "object" (e.g. the workflow)

I'm not arguing one way or another! It's just confusing to me.

@laurentsimon
Copy link
Contributor Author

laurentsimon commented May 24, 2023

I am not sure why we need to nest the parameters under an input field. Is this a requirement from SLSA v1?

I think so. I think it is used to avoid collision with buildType-defined custom fields. It's simpler to keep the same format across GH builders, this way the slsa-verifier has a unique function to retrieve the inputs

@asraa
Copy link
Contributor

asraa commented May 24, 2023

I think it is used to avoid collision with buildType-defined custom fields.

Hmmm this might help with my confusion above.

this way the slsa-verifier has a unique function to retrieve the inputs

The resolution could be:

  • Move workflow inputs nested under inputs, these will match the workflow inputs 1 to 1
  • Allow or repeat the resolved fields of the configuration that are important to this buildType to remain at the top-level, for uniformity. (builder-image, etc)

Let's discuss what to do about workflow face to face.

@ianlewis
Copy link
Member

This means that the buildType for a container execution can't really be re-used cleanly across other build systems, unless we have a builder-specific "object" (e.g. the workflow)

I agree that given this maybe not feasible to reuse the full buildType for other build systems. Is there some subset that could be the same though? IIUC, parameters could have any kind of structure it wants (though there is some leaning towards a more flat structure) so part of it could be a fully reusable bit. Would that achieve any of your goals?

@laurentsimon
Copy link
Contributor Author

I think it is used to avoid collision with buildType-defined custom fields.

Hmmm this might help with my confusion above.

this way the slsa-verifier has a unique function to retrieve the inputs

The resolution could be:

  • Move workflow inputs nested under inputs, these will match the workflow inputs 1 to 1
  • Allow or repeat the resolved fields of the configuration that are important to this buildType to remain at the top-level, for uniformity. (builder-image, etc)

Let's discuss what to do about workflow face to face.

SG. I think you can drop it so long as you record the GITHUB_WORKFLOW_REF, since it contains the same information. I sent #615 this morning to make the slsa-verifier use that for our builders.

ramonpetgrave64 pushed a commit to ramonpetgrave64/slsa-verifier that referenced this issue Apr 18, 2024
- Included typescript-eslint
- This is based on https://github.com/actions/typescript-action
- Fixes slsa-framework#610

Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>

Co-authored-by: Ian Lewis <ianlewis@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants