Skip to content

Fix(check122): Error when policy name contains commas - #1067

Merged
jfagoagas merged 2 commits into
prowler-cloud:masterfrom
plarso:master
Mar 16, 2022
Merged

Fix(check122): Error when policy name contains commas#1067
jfagoagas merged 2 commits into
prowler-cloud:masterfrom
plarso:master

Conversation

@plarso

@plarso plarso commented Mar 14, 2022

Copy link
Copy Markdown
Contributor

Context

AWS IAM policy names allow commas (','), but check 122 does not support the case where there is a comma in the policy name.

Description

  • Change the awk command for the POLICY_VERSION variable in the script to get the version number from the last column rather than the second column.
  • Change the awk command for the POLICY_ARN variable in the script to get the policy ARN from all columns except the last instead of just the first column.

License

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

@plarso
plarso requested review from a team, jfagoagas, n4ch04 and toniblyx March 14, 2022 21:10

@jfagoagas jfagoagas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @plarso, please review comments above to merge this PR!

Comment thread checks/check122 Outdated
Comment thread checks/check122 Outdated
@jfagoagas jfagoagas added status/awaiting-reponse Waiting response from owner severity/high Bug capable of collapsing large parts of the execution. labels Mar 15, 2022
@lazize

lazize commented Mar 15, 2022

Copy link
Copy Markdown
Contributor

If it has comma, it will break csv output.
If it is allowed by AWS, we should replace it by something else.

@jfagoagas

Copy link
Copy Markdown
Member

If it has comma, it will break csv output. If it is allowed by AWS, we should replace it by something else.

Good point! I'm going to test a initial approach to solve this.

@jfagoagas

jfagoagas commented Mar 15, 2022

Copy link
Copy Markdown
Member

Hi @plarso could you include the following as a replace in line 42?

textFail "$REGION: Policy ${policy//,/[comma]} allows \"*:*\"" "$REGION" "$policy"

This is a valid approach but we are modifying role name in the output.

  1. AWS Test Roles

Screenshot 2022-03-15 at 17 55 30

2. Prowler Result

Screenshot 2022-03-15 at 18 08 43

Also @lazize please check this.

Thanks!

@plarso

plarso commented Mar 15, 2022

Copy link
Copy Markdown
Contributor Author

I've added the requested changes.

@lazize

lazize commented Mar 16, 2022

Copy link
Copy Markdown
Contributor

Based on my recent experience, I believe this kind of variable expansion will not be supported by old version of bash, like the one original from macOS.
I will test it tomorrow.

@jfagoagas

Copy link
Copy Markdown
Member

I've added the requested changes.

Cool, thank you @plarso !

@jfagoagas

Copy link
Copy Markdown
Member

Based on my recent experience, I believe this kind of variable expansion will not be supported by old version of bash, like the one original from macOS.

I will test it tomorrow.

All of this changes has been tested with Bash 3.15, this kind of variable expansion is supported in old versions of Bash.

@jfagoagas jfagoagas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good job @plarso !!

Thanks 👏

@jfagoagas
jfagoagas merged commit c526c61 into prowler-cloud:master Mar 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

severity/high Bug capable of collapsing large parts of the execution. status/awaiting-reponse Waiting response from owner

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants