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

Incorrect logic in the ARN matching code #134

Open
dgubitosi opened this issue Jul 1, 2020 · 4 comments
Open

Incorrect logic in the ARN matching code #134

dgubitosi opened this issue Jul 1, 2020 · 4 comments

Comments

@dgubitosi
Copy link
Contributor

dgubitosi commented Jul 1, 2020

The is_arn_match code is incorrect. An empty string for the Partition, Service, Region, and Account is always assumed to be correct regardless of the resource type.

Example:

{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Effect": "Allow",
            "Action": "cloudtrail:*",
            "Resource": "arn:::::trail/my-trail*"
        }
    ]
}

Clearly the ARN above is incorrect, but Parliament will only produce RESOURCE_MISMATCH findings for those actions that require resource *.

I double checked that the AWS documentation is correct and that Parliament produces a correct ARN definition:

    "resources": [
      {
        "arn": "arn:${Partition}:cloudtrail:${Region}:${Account}:trail/${TrailName}",
        "condition_keys": [],
        "resource": "trail"
      }

The offending code is here:
https://github.com/duo-labs/parliament/blob/main/parliament/__init__.py#L130-L138

I'm thinking about how to correct this. I may have a PR in a day or two.

@0xdabbad00
Copy link
Collaborator

I believe an empty string for those segments is correct in at least in some cases. For example, the ARN for an S3 bucket is arn:aws:s3:::mybucket. I'd want to ensure that cases like that are taken into consideration. The ARN definition for an S3 bucket though is arn:${Partition}:s3:::${BucketName} so I guess we'd just have to ensure that if there is a variable defined for those segments that it is minimally non-empty.

@dgubitosi
Copy link
Contributor Author

dgubitosi commented Jul 1, 2020

I know. There are three cases that I know from experience:

  • Resources that use an empty string for the region -- iam, organizations, cloudfront, waf.
  • Resources that use an empty string for the account -- apigateway
  • Resources that use an empty string for both the region and account -- s3, route53

I've already updated the logic as follows and am testing those conditions above now.

    for position in range(0, 5):
        if arn_parts[position] == "*" and resource_parts[position] != "":
            continue
        elif resource_parts[position] == "*":
            continue
        elif arn_parts[position] == resource_parts[position]:
            continue
        else:
            return False

If the arn part is star, then the resource part cannot be an empty string.
Always pass if the resource part is star.
Last ensure that the arn part matches the resource part, and this would include empty strings on both sides.

@dgubitosi
Copy link
Contributor Author

The arns_parts[] == resource_parts[] portion is technically incomplete according to the documentation:
https://docs.aws.amazon.com/quicksight/latest/APIReference/qs-arn-format.html

"You can use wildcard characters (* and ?) within any ARN segment ."

@dgubitosi
Copy link
Contributor Author

dgubitosi commented Jul 1, 2020

PR for discussion: #135

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

No branches or pull requests

2 participants