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

Stop more checks if invalid resources are found #159

Merged
merged 3 commits into from
Dec 7, 2020
Merged

Stop more checks if invalid resources are found #159

merged 3 commits into from
Dec 7, 2020

Conversation

KevinHock
Copy link
Contributor

@KevinHock KevinHock commented Dec 3, 2020

Analyzing a policy with an INVALID_ARN gives exceptions from all of the community auditors

e.g.

{
  "Version":"2012-10-17",
  "Statement":[
    {
      "Sid":"AddPerm",
      "Effect":"Allow",
      "Principal": "*",
      "Action":["ssm:PutParameter"],
      "Resource":[{"Fn::Sub": "arn:aws:ssm:*:${AWS::AccountId}:*"}]
    }
  ]
}

from test_resources.py

Also:

  • Update managed policies repo link to @z0ph's

  • Fix some spelling

@CLAassistant
Copy link

CLAassistant commented Dec 3, 2020

CLA assistant check
All committers have signed the CLA.

@KevinHock
Copy link
Contributor Author

Great project by the way!

Glad I could try to contribute like @danielpops and @piax93, small world.

@@ -41,7 +41,7 @@ This example is showing that the action s3:GetObject requires a resource matchin
The different input types allowed include:
- --file: Filename
- --directory: A directory path, for exmaple: `--directory . --include_policy_extension json --exclude_pattern ".*venv.*"`
- --aws-managed-policies: For use specifically with the repo https://github.com/SummitRoute/aws_managed_policies
- --aws-managed-policies: For use specifically with the repo https://github.com/z0ph/aws_managed_policies
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good call since I don't update my repo anymore.

for setting, settting_value in settings.items():
config[finding_type][setting] = settting_value
for setting, setting_value in settings.items():
config[finding_type][setting] = setting_value
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice catch on the spelling issue

@@ -641,7 +642,7 @@ def analyze_statement(self):
"""
Given a statement, look for problems and extract out the parts.

If it is maformed, return False
If it is malformed, return False
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice spelling catch again

@@ -955,4 +955,4 @@ def analyze_statement(self):
"RESOURCE_STAR", detail=sorted(self.resource_star), location=self.stmt
)

return True
return not has_malformed_resource
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this line is the only change the PR mentioned?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, I added a couple of lines about the 2 other commits now, let me know if you'd prefer I mention it in the PR title.

@0xdabbad00
Copy link
Collaborator

Thank you @KevinHock! Looks like your PR includes a lot of additional changes. I like the spelling changes and the link change, but I'd rather keep my for loop logic as is, as opposed to using any and all. Also I don't know what the difference is for the defaultdict changes, but I'd rather keep those as is as well. I'm guessing some sort of automation made some of those changes? Can you back out those other changes, or send a PR with only the desired change (I think it is only this line: https://github.com/duo-labs/parliament/pull/159/files#diff-1e5beac911c9f6fdd5a9abd17a5b929690ac686ce3491a571fd0baa109388df8R958 ) and those spelling and link corrections?

@KevinHock
Copy link
Contributor Author

sgtm @0xdabbad00 :) Made the changes

@0xdabbad00
Copy link
Collaborator

LGTM. This can be merged @steiza

@steiza steiza merged commit 66c6588 into duo-labs:main Dec 7, 2020
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.

None yet

4 participants