chore: refactored security guardian tool and security-guardian action. Enables local run. #34158
Conversation
aws-cdk-automation
added
the
pr/needs-maintainer-review
.../changed_templates/tools_@aws-cdk_security-guardian_test_templates_CMCMK-Stack.template.json
Outdated
Show resolved
Hide resolved
|
can you also update the contributing readme to include the instructions to run this tool |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
There was a problem hiding this comment.
Thanks @QuantumNeuralCoder for implementing this tool. Left some comments for your consideration.
| await exec.exec('cfn-guard', [ | ||
| 'validate', |
There was a problem hiding this comment.
nit: may be good to check if the cfn-guard is installed before this execution. Given the tool can be executed locally it might be good to throw an error if it is not installed.
| let matches: Array<{ filePath: string, statements: any[] }> = []; | ||
| let totalFiles = 0; | ||
|
|
||
| function isRootPrincipal(statement: any): boolean { |
There was a problem hiding this comment.
I think it would be good to define as an interface for IAMStatement and use it here instead of using as any type.
Eg:
interface IAMStatement {
Effect: ...;
Principal?: {
AWS?: ...;
Service?: ...;
};
Action?: ...;
Resource?: ...;
}
| |------------------|------------------------------------------------------|----------|-----------------------| | ||
| | `rule_set_path` | Local path to the cfn-guard rules file | Yes | | | ||
| | `show_summary` | Show summary (`fail`, `warn`, or `none`) | No | `fail` | | ||
| | `output_format` | Output format (`single-line-summary`, `json`, etc.) | No | `single-line-summary` | |
There was a problem hiding this comment.
How does single-line-summary output look like? Will it be informative for the developer to investigate by using that ouput? I'm just thinking if it will be helpful or we need to run with json ouput to get more details for debugging.
Do you think it will be good to default output as json so it will have detailed information in the github actions workflow runs for debugging?
| | Name | Description | Required | Default | | ||
| |------------------|------------------------------------------------------|----------|-----------------------| | ||
| | `rule_set_path` | Local path to the cfn-guard rules file | Yes | | | ||
| | `show_summary` | Show summary (`fail`, `warn`, or `none`) | No | `fail` | |
There was a problem hiding this comment.
seems warn is not a valid value based on the cfn-gaurd readme. Also i think we accept other possible values right in that case should we mention that also?
From cfn-gaurd readme:
Controls if the summary table needs to be displayed. --show-summary fail (default) or --show-summary pass,fail (only show rules that did pass/fail) or --show-summary none (to turn it off) or --show-summary all (to show all the rules that pass, fail or skip) [default: fail] [possible values: none, all, pass, fail, skip]
| const message = `cfn-guard validation failed: ${(err as Error).message}`; | ||
| core.warning(message); | ||
| errors.push(message); |
There was a problem hiding this comment.
Seems like the code will continue if the cfn-guard fails. Should we throw error if the cfn-gaurd fails?
| const ruleSetPath = core.getInput('rule_set_path'); | ||
| const showSummary = core.getInput('show_summary') || 'fail'; | ||
| const outputFormat = core.getInput('output_format') || 'single-line-summary'; | ||
| const ruleSetPath = getInput('rule_set_path'); |
There was a problem hiding this comment.
i think rule_set_path is required, in that case should it be like below
getInput('rule_set_path', { required: true });
or since the rules are defined here ./tools/@aws-cdk/security-guardian/rules, we can have keep it as optional with default pointing to this path for this input parameter?
| import * as fs from 'fs'; | ||
| import * as path from 'path'; | ||
|
|
||
| export async function detectChangedTemplates(baseSha: string, headSha: string, outputDir: string): Promise<boolean> { |
There was a problem hiding this comment.
outputDir can be renamed to workingDir to be consistent.
| await exec.exec('git', ['diff', '--name-status', `${baseSha}`, `${headSha}`], { | ||
| listeners: { | ||
| stdout: (data: Buffer) => { | ||
| stdout += data.toString(); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
nit: would be better to handle execution process and file processing with try catch.
| const repoRoot = await exec.getExecOutput('git', ['rev-parse', '--show-toplevel']); | ||
| const fullPath = path.join(repoRoot.stdout.trim(), file); | ||
| console.log('fullpath:', fullPath); | ||
| if (fs.existsSync(fullPath)) { | ||
| const safeName = file.replace(/\//g, '_'); | ||
| fs.copyFileSync(fullPath, path.join(outputDir, safeName)); | ||
| core.info(`Copied: ${file}`); | ||
| } else { | ||
| core.warning(`Changed file not found: ${file}`); | ||
| } | ||
| } |
|
|
||
| async function run(): Promise<void> { | ||
| const errors: string[] = []; | ||
| let workingDir: string = './changed_templates'; |
There was a problem hiding this comment.
I understand we are using the workingDir to store all the changed templates and run the cfnguard and intrinsic checks on this directory. However, I have a concern about file traceability. Currently, when files are copied to the working directory (./changed_templates), the mapping to their original paths is lost. This might make it difficult for developers to trace back the issues found by cfn-guard and intrinsic checks to the actual source files. For example, in a PR with multiple changed template files, developers might struggle to identify which specific template in their repository needs fixing.
I'm thinking a file mapping mechanism to maintain the relationship between original file paths and their copies in the working directory might help so the validation result can show the path of the original file path in the output.
Also another approach could be avoiding copying the changed templates to a separate directory and instead process the scan directly from the original file where we could able to track. However, this might not be feasible as cfn-guard's --data option may have limitations with processing multiple template files. I'm not sure about this.
Let me know your thoughts.
Issue # (if applicable)
None
Closes #.
NA
Reason for this change
With this change, developers can locally run security guardian against committed files to detect changed .template.json and run the 2 part scanner
Please note that this will detect templates where the developer has explicitly provided broadened scope permissions like
new AccountPrincipal();
We will use this as an opportunity to review if that is really needed or can be scoped down.
Description of changes
Describe any new or updated permissions being added
Description of how you validated changes
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license