fix: Unpack config file in BuildDefinition #1547

rbehjati · 2023-01-17T17:13:34Z

Signed-off-by: Razieh Behjati [email protected]

To unpack the input config file, even in case of dry-run we have to fetch the source, and load the config file. Because of this, some parts of builder.go are refactored as part of this PR. The benefit of this is that now dry-run and build commands now have more in common. In particular, dry-run performs more of the checks in build.

internal/builders/docker/testdata/build-definition.json

asraa · 2023-01-18T15:36:16Z

.github/workflows/builder_docker-based_slsa3.yml

@@ -208,13 +218,13 @@ jobs:
            --build-config-path "${CONFIG_PATH}" \
            --build-definition-path build-definition.json \
            --builder-image "${BUILDER_IMAGE}@${BUILDER_DIGEST}" \
-            --git-commit-digest "sha1:${GITHUB_SHA}" \
+            --git-commit-digest "sha1:${{needs.commit.outputs.sha}}" \


Ah, are you having trouble getting GITHUB_SHA? Hmmm

Yes. In case of pull requests it is not the actual commit. I am trying to get the latest commit, but now I get the error fatal: not a git repository (or any of the parent directories): .git. This is very strange.

I thought detect-env would give the correct sha, but it gives empty string.

Ohhh gotcha, for pull_request events it does appear GITHUB_SHA is not the triggering commit (https://github.com/orgs/community/discussions/26325). But for that, wouldn't the builder need to checkout the base repository (which may be the fork?)

I thought detect-env would give the correct sha, but it gives empty string.

detect-env gives the ref of the reusable workflow, not the triggering environment. hmmm. It should give the head sha of that, let me see if I can test that out as well.

But for that, wouldn't the builder need to checkout the base repository (which may be the fork?)

Yes, but even in that case the GITHUB_SHA we provide to the command is not something that exists on the repo.

It should be possible to get the actual commit hash. I have one more idea to try :)

I think detect-env has special case for pull requests to enable pre-submit tests. We don't really support it out outside of that context. The action may need to be adjusted.

internal/builders/docker/pkg/builder.go

internal/builders/docker/commands.go

rbehjati · 2023-01-18T17:17:05Z

.github/workflows/builder_docker-based_slsa3.yml

@@ -208,13 +220,13 @@ jobs:
            --build-config-path "${CONFIG_PATH}" \
            --build-definition-path build-definition.json \
            --builder-image "${BUILDER_IMAGE}@${BUILDER_DIGEST}" \
-            --git-commit-digest "sha1:${GITHUB_SHA}" \
+            --git-commit-digest "sha1:${{ github.event.pull_request.head.sha }}" \


With this change we are getting the correct commit sha, but I think we also need to specify the correct branch in source-repo. I am not sure if that is possible. I'll give it a try.

Also, this will fail if the event is push. We should be able to handle both cases.

IIUC the head SHA will only be available for pull_request events? Are there other events you'll need to handle? I assume that the primary event you would need to handle is tag or release for when release tags are created.

Yes. We have to be able to handle all these events. I am thinking of reverting this back to use GITHUB_SHA, but make these steps conditional on events that are not pull_request, and add a separate step for pull_request.

Yeah, we have a lot of places throughout that act differently for pull requests.

I'm curious how you are planning to support pull requests? we have the problem that id-token:write isn't available on pull requests (#131) so we aren't really able to sign provenance for those events currently.

For pull-requests we only run the dry-run command. We won't actually generate the provenance or sign, but just to make sure that the inputs are correct, and that we can create a BuildDefinition.

The idea is to allow the users to verify their workflow parameters before merging.

OK so just to clarify what happens here now: actions/checkout by default checks out the github repo at the ref of the merge commit. I think that also references GITHUB_SHA, and so the comparison with the checked out repo and the GITHUB_SHA of the command line input should always match, pull request or not?

Yes. After all these experiments that is my understanding too. This behaviour makes sense, and makes things simpler :)

secondly, the new resolution-strategy will allow dry-run to create a build definition, ignoring the mismatch and using the current checkout for the source digest? if so, i think that makes sense.

True. But actually this feature is no longer required, at least not for GitHub. So we could either keep it, or drop it in favour of making the code simpler :)

what happens to the digest of the config file though? is it checked out at the current checkout or at the git-commit-digest during --resolution-strategy ignore? (It seems like ignore is like override with the current checkout so it uses the current checkout for the config file)

It is checked out at the current commit, or what I call the effective commit. The generated BuildDefinition also contains this effective commit digest, not the one given in the inputs.

So we could either keep it, or drop it in favour of making the code simpler :)

Should we keep, and file a follow-up issue to clean it if the code is complete and we find it's not useful for testing?

I can actually drop these changes easily, I just need to revert the last commit.

easy! thank you!

Done. I think this is now ready to be merged :)

asraa

one small nit, but otherwise lgtm. thank you for all the documentation!

internal/builders/docker/pkg/options.go

Signed-off-by: Razieh Behjati <[email protected]>

rbehjati requested review from asraa, ianlewis, laurentsimon and joshuagl as code owners January 17, 2023 17:13

rbehjati commented Jan 17, 2023

View reviewed changes

internal/builders/docker/testdata/build-definition.json Outdated Show resolved Hide resolved

rbehjati force-pushed the unpack branch 2 times, most recently from 6bc7a92 to d4ec45b Compare January 18, 2023 15:34

asraa reviewed Jan 18, 2023

View reviewed changes

rbehjati force-pushed the unpack branch from d4ec45b to 753f957 Compare January 18, 2023 15:45

asraa reviewed Jan 18, 2023

View reviewed changes

internal/builders/docker/pkg/builder.go Show resolved Hide resolved

internal/builders/docker/commands.go Show resolved Hide resolved

rbehjati force-pushed the unpack branch 2 times, most recently from a7771cb to 0221010 Compare January 18, 2023 17:07

rbehjati commented Jan 18, 2023

View reviewed changes

rbehjati force-pushed the unpack branch 10 times, most recently from 301fa00 to e011dda Compare January 24, 2023 14:12

asraa approved these changes Jan 24, 2023

View reviewed changes

internal/builders/docker/pkg/options.go Show resolved Hide resolved

rbehjati mentioned this pull request Jan 24, 2023

[feature] Update the provenance format in the docker-based generator #1566

Closed

rbehjati force-pushed the unpack branch 2 times, most recently from bd87ed2 to dac281f Compare January 24, 2023 15:21

rbehjati added 3 commits January 24, 2023 15:21

Unpack config file in BuildDefinition

57f1a09

Signed-off-by: Razieh Behjati <[email protected]>

Marshal command as json array

d801ff5

Signed-off-by: Razieh Behjati <[email protected]>

Minor fixes

524efd1

Signed-off-by: Razieh Behjati <[email protected]>

rbehjati force-pushed the unpack branch from dac281f to 524efd1 Compare January 24, 2023 15:22

asraa approved these changes Jan 24, 2023

View reviewed changes

asraa merged commit 7739525 into slsa-framework:main Jan 24, 2023

rbehjati deleted the unpack branch January 24, 2023 15:46

rbehjati mentioned this pull request Feb 24, 2023

[bug] Docker-based builder: The incorrect repo is checked out #1685

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Unpack config file in BuildDefinition #1547

fix: Unpack config file in BuildDefinition #1547

rbehjati commented Jan 17, 2023

asraa Jan 18, 2023

rbehjati Jan 18, 2023

rbehjati Jan 18, 2023

asraa Jan 18, 2023

rbehjati Jan 18, 2023

ianlewis Jan 20, 2023

rbehjati Jan 18, 2023 •

edited

Loading

ianlewis Jan 19, 2023

rbehjati Jan 19, 2023

ianlewis Jan 19, 2023

rbehjati Jan 19, 2023

rbehjati Jan 24, 2023

asraa Jan 24, 2023

rbehjati Jan 24, 2023

asraa Jan 24, 2023

rbehjati Jan 24, 2023

asraa left a comment

fix: Unpack config file in BuildDefinition #1547

fix: Unpack config file in BuildDefinition #1547

Conversation

rbehjati commented Jan 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbehjati Jan 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asraa left a comment

Choose a reason for hiding this comment

rbehjati Jan 18, 2023 •

edited

Loading