-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
gha: keep trusted and untrusted paths separate, and simplify actions reference #30207
Merged
pchaigno
merged 2 commits into
main
from
pr/giorio94/main/gha-dont-hardcode-actions-version
Jan 16, 2024
Merged
gha: keep trusted and untrusted paths separate, and simplify actions reference #30207
pchaigno
merged 2 commits into
main
from
pr/giorio94/main/gha-dont-hardcode-actions-version
Jan 16, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
giorio94
added
area/CI-improvement
Topic or proposal to improve the Continuous Integration workflow
release-note/ci
This PR makes changes to the CI.
labels
Jan 11, 2024
giorio94
force-pushed
the
pr/giorio94/main/gha-dont-hardcode-actions-version
branch
from
January 11, 2024 14:39
bafde25
to
5d4d7a1
Compare
giorio94
changed the title
gha: keep trusted and untrusted paths separate, and simplify actions ref
gha: keep trusted and untrusted paths separate, and simplify actions reference
Jan 11, 2024
/test |
2 similar comments
/test |
/test |
giorio94
added
needs-backport/1.12
needs-backport/1.13
This PR / issue needs backporting to the v1.13 branch
needs-backport/1.14
This PR / issue needs backporting to the v1.14 branch
needs-backport/1.15
This PR / issue needs backporting to the v1.15 branch
labels
Jan 12, 2024
giorio94
added
the
backport/author
The backport will be carried out by the author of the PR.
label
Jan 12, 2024
thorn3r
approved these changes
Jan 12, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work 👍
youngnick
approved these changes
Jan 15, 2024
brlbil
approved these changes
Jan 15, 2024
A few GHA workflows got recently modified to hardcode the repository and branch of the actions hosted locally (e.g., [1]). This was a security measure, as they are triggered after checking out the untrusted context (i.e., PR branch), and thus it would be possible for an external PR to inject malicious code. Yet, at the same time, this change mostly defeats the smooth development process enabled by ariane (which automatically uses the workflow and context from the PR for trusted branches -- i.e., in cilium/cilium), requiring again to manually modify those references for testing purposes. Similarly, it also requires manual adaptations when changes are backported to stable branches, or to allow running them from forks, which are easy to overlook. As an alternative solution, let's only check out the helm chart from the untrusted context in a separate directory, without overriding any of the trusted files (i.e., from the target branch) retrieved initially. This way, we are guaranteed that the local github actions are always trusted (as we are not overriding them, nor we are executing any script which could modify them), and can be invoked directly, without any additional constraint. A key aspect for this is that helm charts cannot execute arbitrary code in the client host. Another difference, compared to the previous approach, is that now we also execute the `./contrib/scripts/kind.sh` script from the trusted context (i.e., target branch) instead of the PR context. However, this file is effectively part of the workflow definition, and this change brings consistency with the rest of it. The same also applies for the Gateway API conformance tests. [1]: 654d92f ("ci-e2e: Use lvh-kind in secure way") Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
giorio94
removed
the
backport/author
The backport will be carried out by the author of the PR.
label
Jan 22, 2024
giorio94
added
backport-pending/1.15
The backport for Cilium 1.15.x for this PR is in progress.
and removed
needs-backport/1.15
This PR / issue needs backporting to the v1.15 branch
labels
Jan 22, 2024
maintainer-s-little-helper
bot
moved this from Needs backport from main
to Backport pending to v1.15
in v1.15.0-rc.1
Jan 22, 2024
giorio94
added
backport-pending/1.14
The backport for Cilium 1.14.x for this PR is in progress.
and removed
needs-backport/1.14
This PR / issue needs backporting to the v1.14 branch
labels
Jan 22, 2024
This was referenced Jan 22, 2024
Merged
giorio94
added
backport-pending/1.13
The backport for Cilium 1.13.x for this PR is in progress.
and removed
needs-backport/1.13
This PR / issue needs backporting to the v1.13 branch
labels
Jan 23, 2024
github-actions
bot
added
backport-done/1.14
The backport for Cilium 1.14.x for this PR is done.
backport-done/1.12
The backport for Cilium 1.12.x for this PR is done.
backport-done/1.13
The backport for Cilium 1.13.x for this PR is done.
and removed
backport-pending/1.14
The backport for Cilium 1.14.x for this PR is in progress.
backport-pending/1.12
backport-pending/1.13
The backport for Cilium 1.13.x for this PR is in progress.
labels
Jan 25, 2024
giorio94
added
backport-done/1.15
The backport for Cilium 1.15.x for this PR is done.
and removed
backport-pending/1.15
The backport for Cilium 1.15.x for this PR is in progress.
labels
Jan 29, 2024
maintainer-s-little-helper
bot
moved this from Backport pending to v1.15
to Backport done to v1.15
in v1.15.0-rc.1
Jan 29, 2024
michi-covalent
moved this from Needs backport from main
to Backport done to v1.12
in 1.12.19
Feb 12, 2024
This was referenced Feb 12, 2024
michi-covalent
moved this from Needs backport from main
to Backport done to v1.13
in 1.13.12
Feb 13, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
area/CI-improvement
Topic or proposal to improve the Continuous Integration workflow
backport-done/1.12
The backport for Cilium 1.12.x for this PR is done.
backport-done/1.13
The backport for Cilium 1.13.x for this PR is done.
backport-done/1.14
The backport for Cilium 1.14.x for this PR is done.
backport-done/1.15
The backport for Cilium 1.15.x for this PR is done.
ready-to-merge
This PR has passed all tests and received consensus from code owners to merge.
release-note/ci
This PR makes changes to the CI.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A few GHA workflows got recently modified to hardcode the repository and branch of the actions hosted locally (e.g., [1]). This was a security measure, as they are triggered after checking out the untrusted context (i.e., PR branch), and thus it would be possible for an external PR to inject malicious code.
Yet, at the same time, this change mostly defeats the smooth development process enabled by ariane (which automatically uses the workflow and context from the PR for trusted branches -- i.e., in cilium/cilium), requiring again to manually modify those references for testing purposes. Similarly, it also requires manual adaptations when changes are backported to stable branches, or to allow running them from forks, which are easy to overlook.
As an alternative solution, let's only check out the helm chart from the untrusted context in a separate directory, without overriding any of the trusted files (i.e., from the target branch) retrieved initially. This way, we are guaranteed that the local github actions are always trusted (as we are not overriding them, nor we are executing any script which could modify them), and can be invoked directly, without any additional constraint. A key aspect for this is that helm charts cannot execute arbitrary code in the client host.
Another difference, compared to the previous approach, is that now we also execute the
./contrib/scripts/kind.sh
script from the trusted context (i.e., target branch) instead of the PR context. However, this file is effectively part of the workflow definition, and this change brings consistency with the rest of it. The same also applies for the Gateway API conformance tests.As an additional security measure, let's also postpone the checkout of the context after the setup of the test environment.
[1]: 654d92f ("ci-e2e: Use lvh-kind in secure way")