-
Notifications
You must be signed in to change notification settings - Fork 4.3k
chore: new github action to run security-guardian #34115
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This review is outdated)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #34115 +/- ##
==========================================
+ Coverage 83.98% 84.00% +0.01%
==========================================
Files 120 121 +1
Lines 6976 6984 +8
Branches 1178 1179 +1
==========================================
+ Hits 5859 5867 +8
Misses 1005 1005
Partials 112 112
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
733b2ef
to
9467c57
Compare
Exemption Request since github workflow separately tested on personal fork. |
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
required: false | ||
default: "single-line-summary" | ||
|
||
runs: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since cfn-guard is required, I believe it worth adding a step to install cfn-guard, so we do not need every time we use this GH action to add a step to install it
if (!rulePathToUse && ruleSetUrl) { | ||
const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'rules-')); | ||
rulePathToUse = path.join(tempDir, 'rules.guard'); | ||
await downloadFile(ruleSetUrl, rulePathToUse); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the rules set file will be always a local file in the repo, why shall we download it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed all in latest commit
76b0552
to
b52d3d3
Compare
# Triggered from a separate job when a review is added | ||
workflow_run: | ||
workflows: [PR Linter Trigger] | ||
types: | ||
- completed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not really following, based on lines 2,3
on:
pull_request: {}
This action will be invoked whenever the PR got created, synchronized, reopened, updated. So, why we still need to link this action to another flow, what will be the added value
permissions: | ||
contents: read | ||
pull-requests: read | ||
statuses: read | ||
issues: read | ||
checks: read |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at least from my understanding, I do not see that this action do any updates to the PR, so why do we need these permissions. Also, I think the comment on line 20 is not aligning withe the given permissions, since all permissions are read
# Security Guardian | ||
|
||
A GitHub Action tool designed to validate changed AWS CloudFormation templates against custom [cfn-guard](https://github.com/aws-cloudformation/cloudformation-guard) rules. Supports both local paths and remote URLs for rule sets. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this tool is planned to do more checks than running cfn-guard. As I understood, for now it will run cfn-guard against one rule, with a plan to extend these rules.
Also, there is some static check logic is planned to be done on the snapshot templates, that will be done in the tool, and not by cfn-guard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If my understanding is not correct, so I think we do not need to implement this tool in TS, it can be as simple as running a bash script.
permissions: | ||
contents: read | ||
pages: write | ||
id-token: write |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why we these permissions are needed ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For codecov for this PR. On side note, pull_request_target will run in main branch context. We can only see the gh action once its merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Indranil. I am approving the PR, but as we agreed, we need a follow up PR to allow contributors to execute the security guardian tool locally.
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
None
Closes #.
None
Reason for this change
Description of changes
Refer README.md
Describe any new or updated permissions being added
N/A
Description of how you validated changes
Tested on personal fork. Refer QuantumNeuralCoder#6
PR Linter output shows test results.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license