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

feat(ci): Set token permissions to read only #11442

Merged
merged 1 commit into from Mar 16, 2022
Merged

feat(ci): Set token permissions to read only #11442

merged 1 commit into from Mar 16, 2022

Conversation

spikey979
Copy link
Contributor

@spikey979 spikey979 commented Feb 7, 2022

Signed-off-by: Kristijan spikey979@gmail.com

feat(ci): Set token permissions to read only

Summary

The OpenSSF Scorecard flagged a number of Dangerous-Workflow items. This check "Determines if the project's GitHub Action workflows avoid dangerous patterns." For detailed background see https://github.com/ossf/scorecard/blob/5d3f19838078ad86630f1f80c5ad051249594a1a/docs/checks.md#token-permissions
There were 42 instances of non read-only tokens detected in GitHub workflows. See
token-permissions.txt

In all files I added top level permissions set to read-all. I also used online tool https://app.stepsecurity.io/

Additional Information

This is for issue: https://github.com/ospoco/magma-issue-tracker/issues/54

@spikey979 spikey979 self-assigned this Feb 7, 2022
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines. label Feb 7, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Feb 7, 2022

Thanks for opening a PR! 💯

A couple initial guidelines

Howto

  • Reviews. The "Reviewers" listed for this PR are the Magma maintainers who will shepherd it.
  • Checks. All required CI checks must pass before merge.
  • Merge. Once approved and passing CI checks, use the ready2merge label to indicate the maintainers can merge your PR.

More info

Please take a moment to read through the Magma project's

If this is your first Magma PR, also consider reading

@github-actions github-actions bot added the component: ci All updates on CI (Jenkins/CircleCi/Github Action) label Feb 7, 2022
@mstre123 mstre123 marked this pull request as ready for review February 11, 2022 21:46
@mstre123 mstre123 requested a review from a team as a code owner February 11, 2022 21:46
@spikey979 spikey979 requested review from a team and electronjoe February 15, 2022 08:33
@pull-request-size pull-request-size bot added size/XL Denotes a Pull Request that changes 500-999 lines. and removed size/XS Denotes a PR that changes 0-9 lines. labels Feb 15, 2022
Copy link
Member

@electronjoe electronjoe left a comment

Choose a reason for hiding this comment

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

To my eye, this PR may be doing a couple different things (and it might be easier to review / roll back / etc if they were separated).

One large function of this PR appears to be the pinning of ~all GitHub Action external workflow references to a particular git hash. I suspect this is more secure (preventing upstream tampering of what v2 means) - but the PR description as it stands now does not discuss this large set of changes specifically (and they seem not necessary for the PR topic at hand).

Can we split these out into their own PR with a description / rationale?

@varunsh-coder
Copy link

Happy to see you used https://app.stepsecurity.io/. The code for that is at https://github.com/step-security/secure-workflows. Please do create an issue if you see areas of improvement. Also, I am working on automating scorecard requirements, so if you have pain points etc, please let me know. Thanks!

@pull-request-size pull-request-size bot added size/L Denotes a Pull Request that changes 100-499 lines. size/M Denotes a PR that changes 30-99 lines. and removed size/XL Denotes a Pull Request that changes 500-999 lines. size/L Denotes a Pull Request that changes 100-499 lines. labels Feb 17, 2022
@spikey979
Copy link
Contributor Author

@electronjoe, yes you're right. I made the changes you suggested, so this PR only contains the changes described here.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 17, 2022

feg-workflow

    2 files  202 suites   33s ⏱️
363 tests 363 ✔️ 0 💤 0
377 runs  377 ✔️ 0 💤 0

Results for commit ccac5a0.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 17, 2022

nms-workflow

0 files  0 suites   0s ⏱️
0 tests 0 ✔️ 0 💤 0

Results for commit ccac5a0.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 17, 2022

cloud-workflow

    7 files  351 suites   2m 30s ⏱️
960 tests 960 ✔️ 0 💤 0

Results for commit ccac5a0.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 17, 2022

agw-workflow

  50 files    77 suites   4m 55s ⏱️
896 tests 887 ✔️ 9 💤 0
897 runs  888 ✔️ 9 💤 0

Results for commit ccac5a0.

♻️ This comment has been updated with latest results.

@spikey979 spikey979 closed this Feb 28, 2022
@spikey979 spikey979 reopened this Mar 10, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Mar 10, 2022

Oops! Looks like you failed the Python Format Check.

Howto

♻️ Updated: ✅ The check is passing the Python Format Check after the last commit.

@spikey979 spikey979 marked this pull request as draft March 10, 2022 07:41
@spikey979 spikey979 marked this pull request as ready for review March 10, 2022 09:54
@spikey979
Copy link
Contributor Author

I don't think error "undefined: os.ReadFile / os.WriteFile" has anything to do with the changes I made. I haven't changed file magma/src/go/capture/gen/main.go and everything seems ok to me in the file.

@spikey979 spikey979 added the ready2merge This PR is ready to be merged (is approved and passes all required checks) label Mar 10, 2022
@themarwhal
Copy link
Member

@mstre123 I seem to also see this error when running locally inside the Magma VM. Any clue why it is failing?
@quentinDERORY Please review.

@mstre123
Copy link
Contributor

@mstre123 I seem to also see this error when running locally inside the Magma VM. Any clue why it is failing? @quentinDERORY Please review.

looks like the error is being thrown here
https://github.com/magma/magma/blob/master/src/go/capture/gen/main.go#L100
so that is trying to read https://github.com/magma/magma/blob/master/src/go/capture/gen/main.go#L40
"/home/vagrant/magma/lte/gateway/python/integ_tests/defs.mk"
which is still on the branch
https://github.com/spikey979/magma/blob/token_permissions/lte/gateway/python/integ_tests/defs.mk

i wonder if the permission change in this diff breaks how we do os reads and writes.

@mstre123
Copy link
Contributor

oh actually took a closer look

note: module requires Go 1.17

@spikey979 spikey979 requested a review from markjen as a code owner March 11, 2022 07:40
@pull-request-size pull-request-size bot added size/L Denotes a Pull Request that changes 100-499 lines. and removed size/M Denotes a PR that changes 30-99 lines. labels Mar 11, 2022
@magma magma deleted a comment from github-actions bot Mar 11, 2022
@magma magma deleted a comment from github-actions bot Mar 11, 2022
@magma magma deleted a comment from github-actions bot Mar 11, 2022
Signed-off-by: Kristijan <spikey979@gmail.com>
@spikey979
Copy link
Contributor Author

@mstre123, regarding the error in file src/go/capture/gen/main.go -> undefined: os.ReadFile / os.WriteFile…
The test that failed referred to the .github/workflows/golang-build-test.yml file, so I first removed the changes I made in that file, however the test still failed with the same error.
After that, in file src/go/capture/gen/main.go I replaced os.WriteFile and os.ReadFile commands with ioutil.WriteFile and ioutil.ReadFile commands and now the test pass.
According to the Go documentation, commands os.ReadFile / os.WriteFile should be identical to ioutil.WriteFile / ioutil.ReadFile, so I have no idea why the os. commands failed the test.

@themarwhal themarwhal requested a review from tmdzk March 14, 2022 12:47
@themarwhal
Copy link
Member

@tmdzk / @quentinDERORY ?

@themarwhal themarwhal merged commit 3f81d14 into magma:master Mar 16, 2022
@spikey979 spikey979 deleted the token_permissions branch March 17, 2022 06:24
spikey979 added a commit that referenced this pull request Mar 22, 2022
tmdzk pushed a commit that referenced this pull request Mar 22, 2022
vikramgreddy pushed a commit to vikramgreddy/magma that referenced this pull request Mar 23, 2022
Signed-off-by: Kristijan <spikey979@gmail.com>
ardzoht pushed a commit that referenced this pull request Mar 30, 2022
Signed-off-by: Kristijan <spikey979@gmail.com>
ardzoht pushed a commit that referenced this pull request Mar 30, 2022
emakeev pushed a commit to emakeev/magma that referenced this pull request Aug 5, 2022
Signed-off-by: Kristijan <spikey979@gmail.com>
emakeev pushed a commit to emakeev/magma that referenced this pull request Aug 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: ci All updates on CI (Jenkins/CircleCi/Github Action) ready2merge This PR is ready to be merged (is approved and passes all required checks) size/L Denotes a Pull Request that changes 100-499 lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants