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

workflows: simplify build image CI #3031

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mtardy
Copy link
Member

@mtardy mtardy commented Oct 22, 2024

Follow security best practices for this sensitive pull_request_target workflow and simplify it for easy maintenance.

@mtardy mtardy added the release-note/ci This PR makes changes to the CI. label Oct 22, 2024
@mtardy mtardy force-pushed the pr/mtardy/build-image-ci-rewrite branch 3 times, most recently from 403e90a to effc224 Compare October 22, 2024 17:54
Since the Image CI Build is the most sensitive of our workflows since it
runs in the context of pull_request_target, let's make it simple as
nobody currently properly maintains the signing and the BOM. I don't
think anybody expect the CI builds to be fully shipped with BOM and
signing anyway.

Signed-off-by: Mahe Tardy <[email protected]>
@mtardy mtardy force-pushed the pr/mtardy/build-image-ci-rewrite branch 2 times, most recently from 5c61f49 to 059348e Compare October 23, 2024 08:56
Copy link

netlify bot commented Oct 23, 2024

Deploy Preview for tetragon ready!

Name Link
🔨 Latest commit 059348e
🔍 Latest deploy log https://app.netlify.com/sites/tetragon/deploys/6718ba55a5e5650008680657
😎 Deploy Preview https://deploy-preview-3031--tetragon.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Follow security best practices for this sensitive pull_request_target
workflow and simplify it for easy maintenance.

Signed-off-by: Mahe Tardy <[email protected]>
@mtardy mtardy force-pushed the pr/mtardy/build-image-ci-rewrite branch from 059348e to 7cdcad7 Compare October 23, 2024 09:01
@mtardy mtardy marked this pull request as ready for review October 23, 2024 09:02
@mtardy mtardy requested review from willfindlay and a team as code owners October 23, 2024 09:02
@kkourt kkourt self-requested a review October 23, 2024 09:08
@mtardy
Copy link
Member Author

mtardy commented Oct 23, 2024

cc @piergiorgioladisa if you want to take a look at this simplification. I believe it's not fixing any actual security issue, but this would make the workflow more resilient for future evolution given the sensitivity since it's running in the context of pull_request_target and pulling untrusted code and potentially untrusted git metadata.

@piergiorgioladisa
Copy link

cc @piergiorgioladisa if you want to take a look at this simplification. I believe it's not fixing any actual security issue, but this would make the workflow more resilient for future evolution given the sensitivity since it's running in the context of pull_request_target and pulling untrusted code and potentially untrusted git metadata.

really great job in simplifying and securing the workflow. I can't see any issue, so everything looks good to me

Copy link
Contributor

@kkourt kkourt left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

run: |
cosign sign -y quay.io/${{ github.repository_owner }}/${{ matrix.name }}-ci@${{ steps.docker_build_ci_main.outputs.digest }}

- name: Generate SBOM
Copy link
Contributor

Choose a reason for hiding this comment

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

A counter argument for removing this might be that we will get a chance to notice 'SBOM' breaking before an actual release. I don't mind that, and we can always try to do -rc prereleases to catch such issues.

Copy link
Member Author

Choose a reason for hiding this comment

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

yep that's true

Copy link
Member Author

Choose a reason for hiding this comment

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

However one of my arguments would be that maybe we should remove it everywhere since nobody's maintaining it, and if somebody actually needsit, maybe we should dedicate time and effort.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/ci This PR makes changes to the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants