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] randomize binary names #500

Closed
laurentsimon opened this issue Jul 7, 2022 · 13 comments
Closed

[feature] randomize binary names #500

laurentsimon opened this issue Jul 7, 2022 · 13 comments
Assignees
Labels
area:generic Issue with the generic generator area:go Issue related to the Go ecosystem type:feature New feature or request

Comments

@laurentsimon
Copy link
Collaborator

laurentsimon commented Jul 7, 2022

We currently use hard-coded names for builder binaries and project binaries we compile. This has some downsides:

  1. If a user calls our builder multiple times in the same workflow, they will need to make the build sequential or it will likely fail. (I've seen one user do this already)
  2. If user uses multiple builder calls in the same workflow, the workflow will race to download the binary and it could be corrupt and the run will fail
  3. If the generated binary is the same as a file in the repo, it could overwrite it. (not sure if this could lead to problems or not)

So I propose randomizing the names of the generated binaries. Anyone OK with that?

I will add an e2e tests as well.

@laurentsimon laurentsimon added type:feature New feature or request status:triage Issue that has not been triaged labels Jul 7, 2022
@laurentsimon laurentsimon self-assigned this Jul 7, 2022
@ianlewis
Copy link
Member

I think randomizing the names will be the simplest approach.

I think that the user will use the outputs to determine the right file name for each individual run, right?

@laurentsimon
Copy link
Collaborator Author

laurentsimon commented Jul 11, 2022

Internally, we'll use random names. The final output will still be determined by the developer. For example, in the case of Go builders, the .slsa-goreleaser defines the output name.

You're right, the developer can still use the outputs field.

Wdut?

@laurentsimon
Copy link
Collaborator Author

laurentsimon commented Jul 11, 2022

follow-up: I think the builder names need to be randomized. The generated binaries only need to be randomized if a hardcoded name is used to upload / download them between jobs (we used to do that but it's been changed, so the user should be responsible for naming its binaries to avoid name collision)

@ianlewis
Copy link
Member

SG. I think the generic workflow will need a way to either set the name of the provenance file or randomize the name since it currently uses a static name.

@ianlewis ianlewis removed the status:triage Issue that has not been triaged label Jul 12, 2022
@laurentsimon laurentsimon reopened this Jul 12, 2022
@laurentsimon
Copy link
Collaborator Author

laurentsimon commented Jul 12, 2022

Re-opening because @ianlewis is correct about generic workflow attestation name.
Changes were made for the Go builder to randomize the builder binaries. Since the project binaries are set by the user, we don't randomize them and rely on the user to provide non-colliding names.

@ianlewis ianlewis added area:go Issue related to the Go ecosystem area:generic Issue with the generic generator labels Jul 13, 2022
@ianlewis ianlewis assigned ianlewis and unassigned laurentsimon Jul 13, 2022
@laurentsimon
Copy link
Collaborator Author

laurentsimon commented Jul 14, 2022

Something to add is presubmits to test multiple runs of the builders in the same workflow

@laurentsimon
Copy link
Collaborator Author

I added support for randomized builder binaries for the generic workflow. Last AI is to add a pre-submit if we think it's necessary

@ianlewis ianlewis added this to the GA for generic generator milestone Jul 20, 2022
@ianlewis
Copy link
Member

I added support for randomized builder binaries for the generic workflow. Last AI is to add a pre-submit if we think it's necessary

What would you test in the pre-submit? Maybe failure if two calls to the workflow have the same attestation-name?

@ianlewis
Copy link
Member

Hmm. actions/upload-artifact doesn't have an option to fail if the artifact already exists so two calls to the generic workflow with the same attestation-name will clobber each other.
https://github.com/slsa-framework/slsa-github-generator/actions/runs/2716814219

@laurentsimon
Copy link
Collaborator Author

laurentsimon commented Jul 22, 2022

Good catch. We rely on the user to give to provide non-colliding names for the result. The attestations overwrite each other, but the verification will fail for one of the artifacts, so we fail close ,not open.

Maybe we don't need adversarial tests the, and just use a workflow similar to https://github.com/slsa-framework/example-package/blob/main/.github/workflows/e2e.generic.schedule.main.multi-subjects.slsa3.yml to catch errors early.

@ianlewis
Copy link
Member

I added some tests in slsa-framework/example-package#90

Do you think that will be enough?

@laurentsimon
Copy link
Collaborator Author

definitely.

@ianlewis
Copy link
Member

Tests in slsa-framework/example-package#90 were merged and ran successfully.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:generic Issue with the generic generator area:go Issue related to the Go ecosystem type:feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants