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: add ASH, Bandit and CDK Nag #530

Merged
merged 7 commits into from
Sep 13, 2023
Merged

feat: add ASH, Bandit and CDK Nag #530

merged 7 commits into from
Sep 13, 2023

Conversation

jaidisido
Copy link
Contributor

Feature or Bugfix

  • Feature

Detail

Add ASH, Bandit and CDK Nag security checks.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link
Contributor Author

@jaidisido jaidisido left a comment

Choose a reason for hiding this comment

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

Some initial comments, still WIP

.github/workflows/bandit.yml Show resolved Hide resolved
.github/workflows/cdk-nag.yml Show resolved Hide resolved
@jaidisido jaidisido marked this pull request as ready for review June 22, 2023 14:44
@jaidisido jaidisido requested a review from dlpzx June 22, 2023 14:45
@jaidisido jaidisido self-assigned this Jun 22, 2023
@dbalintx
Copy link
Contributor

The new Bandit job fails due to 2 occurences of Popen calls with the argument "shell=True" which basically makes the process to be executed via shell.
We have a couple of other occurences in the repo where this finding is suppressed with the "# nosec" tag, eg. in the cdk_cli_wrapper

The bandit guideline suggests that the input which is executed in the shell should be properly sanitized / checked to mitigate the risk.

@dlpzx How do you want to proceed? Should we simply suppress this warning to get this PR merged, or do you want to have first some input sanitization steps to be implemented, everywhere where this "shell=True" option is used?

@jaidisido
Copy link
Contributor Author

The new Bandit job fails due to 2 occurences of Popen calls with the argument "shell=True" which basically makes the process to be executed via shell. We have a couple of other occurences in the repo where this finding is suppressed with the "# nosec" tag, eg. in the cdk_cli_wrapper

The bandit guideline suggests that the input which is executed in the shell should be properly sanitized / checked to mitigate the risk.

@dlpzx How do you want to proceed? Should we simply suppress this warning to get this PR merged, or do you want to have first some input sanitization steps to be implemented, everywhere where this "shell=True" option is used?

@dbalintx thanks for the input. Agreed - the # nosec comments should be removed and some time should be spent by the team implementing input sanitization for all subprocess calls. semgrep has a guide with mitigation suggestions for example

@dlpzx
Copy link
Contributor

dlpzx commented Jul 4, 2023

I agree @dbalintx , we should fix and not ignore the issues. In this other PR #536 I added semgrep and it points out to the same issue

@dlpzx dlpzx changed the base branch from main to main-v2 September 4, 2023 11:58
@dlpzx dlpzx changed the base branch from main-v2 to main September 4, 2023 11:59
@dlpzx dlpzx requested a review from noah-paige September 13, 2023 13:07
@dlpzx
Copy link
Contributor

dlpzx commented Sep 13, 2023

@noah-paige @jaidisido, I added some #nosec comments for the shell-true findings. There is a separate issue to track the fix, which has been added to milestone 2.1.0

Copy link
Contributor

@noah-paige noah-paige left a comment

Choose a reason for hiding this comment

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

Looks good - approving this PR

@dlpzx dlpzx merged commit 13c1baf into main Sep 13, 2023
9 checks passed
@dlpzx dlpzx deleted the feat/github-security-checks branch November 8, 2023 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants