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

chore(ci): enforce top-level permission to minimum fail-safe permission as per openssf #2638

Conversation

step-security-bot
Copy link
Contributor

@step-security-bot step-security-bot commented Jul 4, 2023

Issue number: #2203

Summary

OSSF Scorecard rates workflows based on top-level permission as either read-all, : read. Step-Security recommends the bare minimum for a fail-safe run with contents: read`.

Doesn't that break our workflows that need additional permissions?

No. Job-level permissions can still increase a given scope. This changes merely enforces that new jobs will have contents: read permission if no explicit scope is defined.

In which scenario this could break our workflows?

Nested workflows via workflow_call, where parent workflow's permissions cannot be overridden to prevent privilege escalation.

Notes

This pull request is created by Secure Repo at the request of @heitorlessa. Please merge the Pull Request to incorporate the requested changes. Please tag @heitorlessa on your message if you have any questions related to the PR. You can also engage with the StepSecurity team by tagging @step-security-bot.

Changes

Security Fixes

Least Privileged GitHub Actions Token Permissions

The GITHUB_TOKEN is an automatically generated secret to make authenticated calls to the GitHub API. GitHub recommends setting minimum token permissions for the GITHUB_TOKEN.

Add Dependency Review Workflow

The Dependency Review Workflow enforces dependency reviews on your pull requests. The action scans for vulnerable versions of dependencies introduced by package version changes in pull requests, and warns you about the associated security vulnerabilities. This gives you better visibility of what's changing in a pull request, and helps prevent vulnerabilities being added to your repository.

Feedback

For bug reports, feature requests, and general feedback; please create an issue in step-security/secure-repo. To create such PRs, please visit https://app.stepsecurity.io/securerepo.

Signed-off-by: StepSecurity Bot [email protected]

@step-security-bot step-security-bot requested a review from a team as a code owner July 4, 2023 08:24
@boring-cyborg boring-cyborg bot added the github-actions Pull requests that update Github_actions code label Jul 4, 2023
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 4, 2023
@heitorlessa heitorlessa self-requested a review July 4, 2023 08:25
@github-actions github-actions bot added the internal Maintenance changes label Jul 4, 2023
Copy link
Contributor

@heitorlessa heitorlessa left a comment

Choose a reason for hiding this comment

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

fix suggestions to not break all workflows that require additional permissions at job-level

.github/workflows/build_changelog.yml Show resolved Hide resolved
.github/workflows/label_pr_on_title.yml Show resolved Hide resolved
.github/workflows/on_closed_issues.yml Show resolved Hide resolved
.github/workflows/on_label_added.yml Show resolved Hide resolved
.github/workflows/on_merged_pr.yml Show resolved Hide resolved
.github/workflows/run-e2e-tests.yml Show resolved Hide resolved
.github/workflows/secure_workflows.yml Show resolved Hide resolved
@heitorlessa
Copy link
Contributor

@varunsh-coder following up on this with a proper review this time - I've noticed Step-Security tool adds contents: read in any workflow at the top-level as suggested by OSSF, however this will break workflows that need additional permissions at per job-level --- is that correct?

My previous experience doing this led to actions failing as top-level permissions cannot be increased at the job-level.

Practical example: we have certain workflows that need to create temporary branches and create PRs from them.

@heitorlessa
Copy link
Contributor

just read all issues open/closed in OSSF Scorecard, and specifically this ossf/scorecard#1128.

I'll create a separate repo to test one of our workflows with a read-only top-level permission that increases its scope at the job-level. I suspect I might be misremembering a nested workflow increasing the scope and failing vs top-level minimum with some increased scope at job-level.

@heitorlessa
Copy link
Contributor

Confirmed, reverting suggested changes and keeping contents: read minimal. Workflows only fail when nested workflows try to escalate permissions, which we already addressed at job-level.

Test: https://github.com/heitorlessa/aws-lambda-powertools-test/actions/runs/5453114771/workflow#L16

@heitorlessa heitorlessa changed the title chore(ci): openssf remediation for GH Actions chore(ci): enforce top-level permission to minimum fail-safe permission as per openssf Jul 4, 2023
Copy link
Contributor

@heitorlessa heitorlessa left a comment

Choose a reason for hiding this comment

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

Approving after clarifying top-level vs job-level permissions potential breakage.

PR body updated to help review in the future.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 4, 2023

No acknowledgement section found. Please make sure you used the template to open a PR and didn't remove the acknowledgment section. Check the template here: https://github.com/aws-powertools/powertools-lambda-python/blob/develop/.github/PULL_REQUEST_TEMPLATE.md#acknowledgment

@github-actions
Copy link
Contributor

github-actions bot commented Jul 4, 2023

No related issues found. Please ensure there is an open issue related to this change to avoid significant delays or closure.

@github-actions github-actions bot added do-not-merge need-license-agreement-acknowledge PRs that are missing acknowledgement section need-issue PRs that are missing related issues labels Jul 4, 2023
@heitorlessa heitorlessa added do-not-merge and removed do-not-merge need-issue PRs that are missing related issues need-license-agreement-acknowledge PRs that are missing acknowledgement section labels Jul 4, 2023
@heitorlessa heitorlessa merged commit cdd28fe into aws-powertools:develop Jul 4, 2023
@boring-cyborg
Copy link

boring-cyborg bot commented Jul 4, 2023

Awesome work, congrats on your first merged pull request and thank you for helping improve everyone's experience!

@varunsh-coder
Copy link

@varunsh-coder following up on this with a proper review this time - I've noticed Step-Security tool adds contents: read in any workflow at the top-level as suggested by OSSF, however this will break workflows that need additional permissions at per job-level --- is that correct?

My previous experience doing this led to actions failing as top-level permissions cannot be increased at the job-level.

Practical example: we have certain workflows that need to create temporary branches and create PRs from them.

Hi @heitorlessa, the Step-Security tool only adds the contents: read top-level permission, if all jobs already have job level permission. If all jobs do not have job level permission, it tries to figure out the job level permission using a knowledge base of permissions for GitHub Actions used by that job. If it cannot figure it out, it will not add the contents: read top-level permission. https://github.com/step-security/secure-repo/blob/d9d6d7e778b117a718e621e7aa9b2cf068c7865d/remediation/workflow/secureworkflow.go#L43-L50

If a job has job level permission, those job level permissions are used. Top level permissions are used if a job does not define any permissions.

@heitorlessa
Copy link
Contributor

if all jobs already have job level permission

Gotcha, that explains it. Thank you! Everything went smoothly, only one small breakage after merging due to nested workflows not getting contents: read propagated (TIL)

Thank you once again for the awesome tool!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
github-actions Pull requests that update Github_actions code internal Maintenance changes size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants