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

Accept an array of arguments when building the provenance #2519

Merged
merged 3 commits into from
Feb 4, 2022

Conversation

jul-sh
Copy link
Contributor

@jul-sh jul-sh commented Feb 4, 2022

This aligns the invocation of the provenance script with the provenance file, which stores build arguments as an array.

Previously the build_provenance script accepted exactly two arguments 1) exactly one build arg, 2) the output path.

Now it accepts 1) the output path, followed by one or multiple build args.

This aligns the invocation of the provenance script with the provenance file it produces
Copy link
Collaborator

@tiziano88 tiziano88 left a comment

Choose a reason for hiding this comment

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

LG but is there any way of testing it with the transparent release repo? Or do we postpone that anyways?

# build arguments.
shift

eval "./scripts/docker_run ${@}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm surprised this works with the the quotes, does eval do its own splitting / parsing? Or maybe docker_run does

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bash arg handling is very confusing, but: In docker_run we use $* when invoking the docker CLI. Afaik $* effectively means args a delineated by whitespace. Hence passing as args as a joined string doesn't make difference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An upside of the code I wrote in this for formatting bash args into a toml string is that it doubles as a handy tool for investigating bash arg handling :p

Copy link
Contributor Author

Choose a reason for hiding this comment

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

docker_run concats args into a single string by default. That string is passed to fix_docker_user_and_run, which executes that string using su --session-command. I think thats where the splitting happens?

@jul-sh
Copy link
Contributor Author

jul-sh commented Feb 4, 2022

@tiziano88 RE linking it with the repo, it's planned, not postponed. Here's my planned timeline:

  1. Update transparent release to use Amber SLSA files (it makes not a lot of sense to do the next steps before this, as they depend on it)
  2. Update this GitHub action to produce an Amber SLSA file
  3. Update our Kokoro CI action to consume Amber SLSA files that are pushed to the provenance branch, and verify them with the transparent release tooling.

Step 3) does two things which are pretty nice: It verifies that our SLSA files can be parsed by transparent release, B) It adds a continuous check that our builds stay reproducible. This is since the Kokoro CI action will fail if the build yields a different hash inside of GitHub actions and inside of Kokoro. (Not quite "same hash on three systems in a row", but close). 😇

cc @rbehjati on this too.

@jul-sh jul-sh merged commit 3cb2a2c into project-oak:main Feb 4, 2022
@jul-sh jul-sh deleted the prov-array branch February 4, 2022 18:57
@github-actions
Copy link

github-actions bot commented Feb 4, 2022

Reproducibility Index:

aceeff030af86c5ac6ad1bd9a9e545cd0230c751c28eefb8592a77c148ab1a22  ./bin/abitest_linear_handles.wasm
ae40c3d23f01e26cf483aceb546214ca3545f027bcd871fdf5d139ba0bc2f750  ./bin/aggregator.wasm
38ccfa3b6238de78954b1a9221d4eaca34f240f2883d9dc44907a0ac411c7baf  ./bin/chat.wasm
2d3092239b893607044288afc6ffdef8b49736d7c6d740d649d2d54b1ea48cde  ./bin/hello_world.wasm
860cd8dbdf15cd1ccfdcf3d55ae595106b87f6e589142befc984c7d970f0b367  ./bin/http_server.wasm
aa42ff2b21a7227926b75d6c180c65896b2f98bae6e3429777514faf7d992c4a  ./bin/injection.wasm
6324c477b71512cfbb14e091fd731b23d6b6882a722bb99a8008485c6a30f229  ./bin/private_set_intersection.wasm
9a3024f4f4e2cfa4161a5d767d6f138fc9617e2d94d4f1018b7ecba724165882  ./bin/private_set_intersection_handler.wasm
e9dd1228f25f49e666e2a129992856858253b27e65c4487ee474867dbe71a201  ./bin/proxy_attestation_example.wasm
c6b4ba0fde448ef7b4b51859b965272ee3683e79bd597af4e459de2eeb34ea7f  ./bin/translator.wasm
54a80b6b12e4c27e529a2fcfa57206d7af66231d4233c77e988a171c0ac7e9e8  ./bin/trusted_database.wasm
7de7bbd63bdc679eee8e065e7df444d82ab7f69e6851ce01a0538cf324da4590  ./oak_loader/bin/oak_loader
fa32cf976e6d4378e36b7c1e7a6f39fe547b6ff513e94f6fe2d13be6738486cf  ./oak_functions/loader/bin/oak_functions_loader

Reproducibility Index diff:

@rbehjati
Copy link
Contributor

rbehjati commented Feb 7, 2022

Update our Kokoro CI action to consume Amber SLSA files that are pushed to the provenance branch, and verify them with the transparent release tooling.

Just to clarify: Do you mean that the Kokoro CI action could verify all the provenances? This is what I understand from your point B on "It adds a continuous check...".

@jul-sh
Copy link
Contributor Author

jul-sh commented Feb 7, 2022

@rbehjati It would be possible, yeah. Note that provenances are only generated for commits in main, so basically it would run for each commit in main to verify the provenances.

We can tweak that if it's too much of a load, but it might not be much heavier than the Actions we already run.

@rbehjati
Copy link
Contributor

rbehjati commented Feb 7, 2022

I think that would be an interesting check :)
Should it run after each merge to main, like other actions, or can we make it a nightly check?

@jul-sh
Copy link
Contributor Author

jul-sh commented Feb 7, 2022

I'd be inclined towards running it right away like other actions. We can consider changing that approach in case of problems. It uses a different service, so I think that there would be no impact on the speed / availability of GitHub runners executing our PR CI actions.

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

Successfully merging this pull request may close these issues.

None yet

3 participants