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

Harden security of GitHub Actions CI/CD #202

Merged
merged 4 commits into from
Oct 23, 2021
Merged

Conversation

vlmutolo
Copy link
Contributor

@vlmutolo vlmutolo commented May 17, 2021

This commit introduces two changes. First, the actions are changed to only have
read access to repositories. Second, we specify that GitHub should not persist the
authorization token for write access to a repository on disk (see the option
persist-credentials: false).

@vlmutolo vlmutolo changed the title [Draft] restrict permissions for GitHub actions restrict permissions for GitHub actions May 17, 2021
@vlmutolo
Copy link
Contributor Author

vlmutolo commented May 17, 2021

For reference, I'm pinning all actions to their SHA1 hash. I investigated how often releases are cut for the various actions we use, and it's no more frequent than about once per month on average. That seems acceptable, even if we start failing CI when a new release is detected for a depended-on action cutting a new release.

We're pinning:

  • actions/checkout
  • EmbarkStudios/cargo-deny-action
  • actions-rs/tarpaulin
  • actions-rs/cargo
  • actions-rs/toolchain
  • codecov/codecov-action

@vlmutolo vlmutolo changed the title restrict permissions for GitHub actions Harden security of GitHub Actions CI/CD May 17, 2021
@vlmutolo vlmutolo linked an issue May 17, 2021 that may be closed by this pull request
1 task
@brycx
Copy link
Member

brycx commented May 27, 2021

This looks great!

I don't know if it's always been there and when I made the original issue, but I just found an option to set the default permissions for workflows in the settings of the repository. It was read+write and changed it to read-only. I think it's fine to still explicitly state that we want read-only in each workflow. Having the default just adds the extra fail-safe in case we forget to add it to new ones.

Second, we specify that GitHub should persist the authorization token for write access to a repository on disk (see the option
persist-credentials: false)

Did you mean should not persist authorization token for write access? I think the idea should be to avoid having it persist such that another step from a workflow can read a potential write-access token, even though the step should only be read. (Ref. https://securitylab.github.com/research/github-actions-preventing-pwn-requests/)

The rest looks perfect. And if the Dependabot updates for actions can be added (I mentioned in the issue) then we don't have to worry about how often they cut releases and check are just automatic for updates.

@vlmutolo
Copy link
Contributor Author

Oops! Good catch with the commit message. I reworded it. And like I said on the issue comment, Dependabot seems like a great solution for this. I'll look into it over the weekend.

.github/workflows/lints.yml Outdated Show resolved Hide resolved
@brycx
Copy link
Member

brycx commented Oct 11, 2021

I've taken a look through this PR. Except for maybe updating the SHA commit refs for the Actions, is there anything blocking this from moving out of draft and being ready to merge @vlmutolo? I'd be happy to update the remaining parts and get this merged.

The remaining part is AFAIU adding the Dependabot update-check for Action as described herein: https://docs.github.com/en/code-security/supply-chain-security/keeping-your-dependencies-updated-automatically/keeping-your-actions-up-to-date-with-dependabot

I think our dependabot.yml file could end up looking like (the commit prefixes were just nice-to-haves):

version: 2
updates:
  - package-ecosystem: "cargo"
    directory: "/" # Location of package manifests
    schedule:
      interval: "daily"
    labels:
      - "dependencies"
    commit-message:
      # Prefix all commit messages with "deps:"
      prefix: "deps:"

  - package-ecosystem: "github-actions"
    directory: "/"
    schedule:
      interval: "daily"
    labels:
      - "dependencies"
    commit-message:
      # Prefix all commit messages with "ci:"
      prefix: "ci:"

Depending on how often these Dependabot updates are trigged with SHA-pinned Actions, we might want to change the interval from daily to weekly for those, later on.

@vlmutolo vlmutolo force-pushed the cicd-harden branch 2 times, most recently from 6634051 to 1423d6c Compare October 23, 2021 17:42
Vincent Mutolo added 4 commits October 23, 2021 13:43
This commit introduces two changes. First, the actions are changed to only have
read access to repositories. Second, we specify that GitHub should not persist
the authorization token for write access to a repository on disk (see the
option `persist-credentials: false`).
It shouldn't be necessary for public repositories.
@vlmutolo vlmutolo marked this pull request as ready for review October 23, 2021 17:43
@vlmutolo vlmutolo requested a review from brycx October 23, 2021 18:30
Copy link
Member

@brycx brycx left a comment

Choose a reason for hiding this comment

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

LGTM, really cool stuff!

@brycx brycx merged commit bed9f4f into orion-rs:master Oct 23, 2021
@vlmutolo vlmutolo deleted the cicd-harden branch October 27, 2021 23:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Harden security of GitHub Actions CI/CD
2 participants