gha: keep trusted and untrusted paths separate, and simplify actions reference #30207
Conversation
This file contains hidden or 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
bafde25 to
5d4d7a1
Compare
|
/test |
2 similar comments
|
/test |
|
/test |
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>
As an additional security measure, let's postpone the checkout of the untrusted context after the setup of the test environment. Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
02cf32c to
cd23fd5
Compare
|
Rebased onto main to fix conflicts. |
|
/test |
This was referenced Jan 22, 2024
Merged
This was referenced Feb 12, 2024
joestringer
reviewed
Aug 25, 2025
| @@ -139,6 +139,7 @@ jobs: | |||
| uses: ./.github/actions/helm-default | |||
| with: | |||
| image-tag: ${{ inputs.SHA }} | |||
| chart-dir: ./untrusted/install/kubernetes/cilium | |||
There was a problem hiding this comment.
How does this pull from untrusted/ if the untrusted path is only checked out on line 224?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.shscript 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")