frontend icon indicating copy to clipboard operation
frontend copied to clipboard

GitHub Workflows security hardening

Open sashashura opened this issue 1 year ago • 6 comments

This PR adds explicit permissions section to workflows. This is a security best practice because by default workflows run with extended set of permissions (except from on: pull_request from external forks). By specifying any permission explicitly all others are set to none. By using the principle of least privilege the damage a compromised workflow can do (because of an injection or compromised third party tool or action) is restricted. It is recommended to have most strict permissions on the top level and grant write permissions on job level case by case.

sashashura avatar Sep 20 '22 11:09 sashashura

It looks that this has prevented the Typescript and TeamCity… which means that we have no way of testing this PR against our CI/CD pipeline.

mxdvl avatar Sep 20 '22 16:09 mxdvl

It looks that this has prevented the Typescript and TeamCity… which means that we have no way of testing this PR against our CI/CD pipeline.

I believe we've configured our GHA to require an approval to run if the PR is from a fork. I'm not sure how Teamcity is configured though.

AshCorr avatar Sep 20 '22 16:09 AshCorr

Hey @sashashura,

Had a quick peek at your Github history, wow so many PR's! Are you using some kind of script to do this? Awesome!

Just wondering, why is this permission only added to 2 of our actions? Is it not needed for the other ones? e.g this action runs on PR, does it not need a permission section too? EDIT: I've just seen your comment on https://github.com/git/git/pull/1335#issuecomment-1250919707 explaining that it's not needed for actions triggered by pull_request events.

AshCorr avatar Sep 20 '22 16:09 AshCorr

Hi @AshCorr, Yes I have automated as much as possible. Still I review all the changes before the script creates a PR. Though it is impossible to read all contribution guides, so I apologize if my PR didn't follow a check list in case you have one. Let me know if you have any questions about the PR.

sashashura avatar Sep 20 '22 17:09 sashashura

I suspect we might want to review whether we can enable the org-wide setting for this (read-only by default), rather than harden individual workflows. Happy to suggest that to the DevX team and explore.

jfsoul avatar Sep 23 '22 15:09 jfsoul

Yes, and I encourage you doing so. Just keep in mind that any workflow that needs write permissions (like stale.yml for example), but do not have permissions explicitly specified will stop working after the org-wide switch. You might need to re-run and check if any write permission needs to be granted.

sashashura avatar Sep 23 '22 16:09 sashashura

@jfsoul - maybe we could:

  • switch it on for this repo directly
  • test which actions need changing
  • switch back to unsafer mode
  • change them and then switch back

This would mean we're ready for a org wide switch, and might be a way to do it more incrementally.

jamesgorrie avatar Oct 04 '22 07:10 jamesgorrie

"This PR is stale because it has been open 30 days with no activity. Unless a comment is added or the “stale” label removed, this will be closed in 3 days"

github-actions[bot] avatar Nov 07 '22 06:11 github-actions[bot]

As suggested by @akash1810, we managed to run this in TeamCity with the following script from Grid:

#!/usr/bin/env bash

set -e


if [ -z $1 ]
then
  echo "Please run this script with a PR number as its argument."
  exit 1
fi

PR=$1

echo "Pushing PR merge commit for ${PR} to remote pr${PR} branch"
git fetch -f origin pull/${PR}/merge:pr${PR}
git push -f origin pr${PR}:pr${PR}

mxdvl avatar Nov 07 '22 10:11 mxdvl

Seen on PROD (created by @sashashura and merged by @mxdvl 18 minutes and 21 seconds ago)

prout-bot avatar Nov 07 '22 11:11 prout-bot