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

Add support for IAM UniqueID as policy Principal #36

Open
Cerebus opened this issue Dec 10, 2019 · 5 comments · May be fixed by #110
Open

Add support for IAM UniqueID as policy Principal #36

Cerebus opened this issue Dec 10, 2019 · 5 comments · May be fixed by #110

Comments

@Cerebus
Copy link

Cerebus commented Dec 10, 2019

IAM entities can be referred to via IAM UniqueIDs to prevent name reuse collisions for critical policy objects. Policy assumes all Principals are ARNs--most processing loops over the principals property and parses each as an ARN.

Ref: https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_identifiers.html#identifiers-unique-ids

@scriptsrc
Copy link
Contributor

The library doesn't fail on role IDs or user IDs. What kind of support are you looking for?

@Cerebus
Copy link
Author

Cerebus commented Jul 6, 2020

UniqueIDs are a different format and can be used in Principals in Policy objects. Follow the link in the OP.

We write critical policies using UniqueIDs so if a deleted group, role, or user is recreated we don't get unexpected results. When parsing a policy w/ policyuniverse, most methods error out as unable to recognize the principals.

@tweedge
Copy link
Contributor

tweedge commented Apr 22, 2021

Confirming, I do observe this behavior - arn.py does not support Unique IDs from my testing:

from policyuniverse.policy import Policy

policy = {
    "Version": "2012-10-17",
    "Statement": [
        {
            "Effect": "Allow",
            "Principal": {
                "AWS": "AROAXXXXXXXXXXXXXXXXX"
            },
            "Action": "kms:Decrypt",
            "Resource": "*"
        }
    ]
}

print(Policy(policy).is_internet_accessible())

Running this:

% python3 test.py
WARNING:policyuniverse:ARN Could not parse [AROAXXXXXXXXXXXXXXXXX].
WARNING:policyuniverse:Auditor could not parse ARN AROAXXXXXXXXXXXXXXXXX.
False

@tweedge
Copy link
Contributor

tweedge commented May 6, 2021

@patricksanders in addition to adding this, I'd like to propose a small refactor. I think the ARN class is best-suited to managing both Unique IDs in Principals and the userid internal value in statement.py here. The reason I suggest this is due to the example IAM policies in this post: https://aws.amazon.com/blogs/security/how-to-restrict-amazon-s3-bucket-access-to-a-specific-iam-role/

...
            "Condition": {
                "StringNotLike": {
                    "aws:userId": [
                        "AROAEXAMPLEID:*",
                        "AIDAEXAMPLEID",
                        "111111111111"
                    ]
                }
            }
...

So it seems that the Unique IDs are just another form of an ARN, and it may not make sense to split them out from other ARNs.

If that is acceptable to you, would it also make sense to remove the condition_userids property from the Statement class? Alternatively, we could duplicate userids as to not change properties, but in some cases it seems that non-userid-looking objects such as AWS account IDs could show up in that set.

@tweedge tweedge linked a pull request May 7, 2021 that will close this issue
@patricksanders
Copy link
Collaborator

I have some hesitation about using the ARN class to support unique IDs. In general, you can make some assumptions about ARNs, like being able to determine the service to which they refer (and potentially account ID, region, resource name, etc). It seems like the "right" solution here would be to support particular ID prefixes where they're supported in policy statements, but that might pose a complexity (and thus maintenance) burden. A middle ground could be to create a new class for unique IDs to provide validation, then refactor to support this new class alongside ARNs. This would allow for the flexibility to perform checks in policy validations that are specific to the unique ID type without overloading the ARN class.

Curious to hear any thoughts @scriptsrc has on this.

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 a pull request may close this issue.

4 participants