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

Reduce generated permissions to read-all at top level for generated workflow file #3026

Open
andrewvaughan opened this issue Oct 20, 2023 · 5 comments · May be fixed by #3032
Open

Reduce generated permissions to read-all at top level for generated workflow file #3026

andrewvaughan opened this issue Oct 20, 2023 · 5 comments · May be fixed by #3032
Labels
bug Something isn't working

Comments

@andrewvaughan
Copy link
Contributor

andrewvaughan commented Oct 20, 2023

Describe the bug
Checkov (rightfully) complains about CKV2_GHA_1 that the workflow file leaves permissions to the default write-all in .mega-linter.yml file. This line should be added to the top level:

permissions: read-all

The one build step permissions shouldn't have to change. It's a slightly pedantic issue, but it does occur on the default setup. It will also help prevent any mistakes if users add a second step beyond build.

@andrewvaughan andrewvaughan added the bug Something isn't working label Oct 20, 2023
@nvuillam
Copy link
Member

@andrewvaughan to post comments with results or push new commits, read-all won't be enough :)

But MegaLinter users are free to not use these features and set manually permissions to read-all :)

@andrewvaughan
Copy link
Contributor Author

andrewvaughan commented Oct 21, 2023

Correct! But you should be able to put these permissions per-job. The github-actions linter is complaining because permissions: read-only is not set at the root. This requires each job to be explicit in setting its permissions.

So to be clear, this isn't replacing the existing permissions block, this is adding an additional permissions: read-only in the same level as the on clause to set read-only as a build default. The existing permission block will overwrite this.

As mentioned, it's pedantic, but it does enforce any extra build steps to require explicit permission building per-job, which is best practice. That is what Checkov errors about in the existing workflow.

Edit:

So it would look something like this:

name: MegaLinter

on:
  push:
    branches:
      - main
      - production
      - staging

  pull_request:

##
# Set any build permissions to `read-all` unless explicitly overridden. Important, as
# GitHub defaults to `write-all` which is a security risk.
#
permissions: read-all     # <---- Line to add, prevents `CKV2_GHA_1` linting error

## ... env, concurrency, etc ...


jobs:
  build:
    name: MegaLinter
    runs-on: ubuntu-latest

    ##
    # Override the permissions from the default `read-only` set above to what this job needs
    # to function. Each job should set their own permissions, accordingly.
    #
    permissions:
      contents: write
      issues: write
      pull-requests: write

      // ...

A way to look at this is "hey GitHub, by default, only give any jobs read-all permissions (instead of the default write-all) unless that job explicitly overrides the read-all setting.

You could imagine a developer adding a job parallel to build in the script, not realizing that they unexpectedly are running that new job with write-all capabilities. By setting a read-all default, it ensures that permissions are white-listed, by default, for each job.

Finally, adding this line adds zero change to the existing functionality for MegaLinter, which is nice. It's simply an additional layer of security. I've formatted my generated file a little more verbosely with explanatory comments and fully complying with default MegaLinter configurations, if the project wishes to steal any parts of it - feel free:

https://github.com/andrewvaughan/template-core/blob/main/.github/workflows/mega-linter.yml

@nvuillam
Copy link
Member

@andrewvaughan i fully agree with your clear explanations and will of course accept an incoming PR :)

@andrewvaughan
Copy link
Contributor Author

@andrewvaughan i fully agree with your clear explanations and will of course accept an incoming PR :)

I got you fam.

Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity.
It will be closed in 14 days if no further activity occurs.
Thank you for your contributions.

If you think this issue should stay open, please remove the O: stale 🤖 label or comment on the issue.

@github-actions github-actions bot added the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Nov 21, 2023
@echoix echoix removed the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Nov 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants