-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
ci: few security/permissions improvements #8681
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
Conversation
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.
Thanks a lot @bluetech for tackling this!
If the second commits looks good, we should change the default permissions in the repo settings from read-write to read-only.
Mostly on what I understand from that second commit is that it disables all permissions by default, granting adequate permissions on a step basis, which looks great.
If the third commit looks good, we should remove the chatops and release_notes secrets in the repo settings.
My question is what you mean by "default permissions in repo settings", and how is that settings related to the above?
👍
scripts/upload-coverage.sh
Outdated
curl --silent --show-error --location --connect-timeout 5 --retry 6 -o codecov https://codecov.io/bash | ||
VERSION=$(grep --only-matching 'VERSION=\"[0-9\.]*\"' codecov | cut -d'"' -f2) | ||
do | ||
sha256sum --check --ignore-missing <(curl -s "https://raw.githubusercontent.com/codecov/codecov-bash/${VERSION}/SHA256SUM") |
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. 👍
Thanks for taking a look.
The setting I'm referring to is under Settings -> Actions -> Workflow permissions. It would act as an extra bit of safety when a new workflow is added but forgets to set the permissions. |
Got it, thanks! |
I did some testing on my fork for what I could, and what I was able to test seems good (after a syntax fix). So marking as ready. |
Trying a fix for ancient macos bash version. |
Good enough for me! As you said, we can always fix things later when/if they come up. 👍 |
Argh, |
@bluetech @nicoddemus by now im in favour of just dropping codecov until they get their act together and just providing a small minimal github action one can use instead of this fetch shell from web mess they have |
Mostly, verify the bash uploader hash and make it more strict and verbose.
Decrease security exposure by restricting what the code executing in the actions is allowed to do (in terms of GitHub operations).
…rets It seems more secure to use the controlled & limited token than an ambient secret.
@bluetech i just stumbled on https://github.com/codecov/codecov-action - i wonder if we could/should use it |
That actions seems to embed the bash script. Since we fetch the SHA from their github as well, it ends up at the same level of security, so I think it should be fine to use it. However since we already have a standalone script, which works and is nice to have, I wouldn't really bother with it. BTW now the CI failed because it looks like github is having problems. I won't pile on to their troubles and restart it later... |
ci: few security/permissions improvements (cherry picked from commit ff6d297) Conflicts: .github/workflows/main.yml .github/workflows/prepare-release-pr.yml .github/workflows/update-plugin-list.yml scripts/prepare-release-pr.py scripts/report-coverage.sh
This fixes #8599, and couple other improvements -- please see the commits.
This is entirely untested, and probably would require debugging until nothing breaks, so marking as draft for now, but would appreciate others taking a look.
If the second commits looks good, we should change the default permissions in the repo settings from read-write to read-only.
If the third commit looks good, we should remove the
chatops
andrelease_notes
secrets in the repo settings.