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

Wonk should support writing least privileged policies not strip hints about privilege problems silently escalate privs. #10

Open
donnoman opened this issue Aug 17, 2022 · 0 comments

Comments

@donnoman
Copy link

donnoman commented Aug 17, 2022

Or it should at least support a mode that it can be configured to do so.

For example from "The Details"

It removes all "shadowed" actions. For instance, if a statement has actions Foo:SomeAction and Foo:, it discards Foo:SomeAction because Foo: already has it covered. Similarly, Foo:Get* will shadow Foo:GetSomething, so Foo:GetSomething will be removed.

If you are trying to create least privileged policies Foo: should issue a warning, since it and Foo:Get* would shadow. The "shadow" should be cast on the least specific instructions because this is a signal we are trying to be more specific not less specific.

Under Limitations

Wonk doesn't consider action shadowing when one statement has restrictions but another doesn't. For example, given two statement blocks:

{
    "Statement": [
        {
            "Effect": "Allow",
            "Action": "Foo:Something",
            "Resource": "arn:aws:service::my_resource"
        },
        {
            "Effect": "Allow",
            "Action": "Foo:Something"
        }
    ]
}

This is a good thing that that it doesn't do this, and I wouldn't want it to. It's suggesting you add the inherent escalating of the privileges unbeknownst to the policy writer. Anybody serious about writing IAM policy shouldn't want this behavior.

If you have something restricted by resource and something comes along later with this case or with a "Resource": "*" the proper thing to do is collapse this into a statement WITH those resources. If we don't want the * we can remove it. In the particular case above it should probably issue a warning, but the course it should take is producing:

{
    "Statement": [
        {
            "Effect": "Allow",
            "Action": "Foo:Something",
            "Resource": "arn:aws:service::my_resource"
        }
   ]
}

in the input case of

{
    "Statement": [
        {
            "Effect": "Allow",
            "Action": "Foo:Something",
            "Resource": "arn:aws:service::my_resource"
        },
        {
            "Effect": "Allow",
            "Action": "Foo:Something",
            "Resource": "*"
        }
    ]
}

I would expect it to collapse to:

{
    "Statement": [
        {
            "Effect": "Allow",
            "Action": "Foo:Something",
            "Resource": [
                 "*",
                 "arn:aws:service::my_resource"
            ]
        }
    ]
}

and issue a warning; because it is elevating privileges. Either you meant to do that and you should remove "arn:aws:service::my_resource" to fix the warning, or you should remove "*" and add the additional cases you meant to add if there are any.

@donnoman donnoman changed the title Wonk should support writing least privileged policies not strip hints about privilege problems and take the more generic Wonk should support writing least privileged policies not strip hints about privilege problems silently escalate privs. Aug 17, 2022
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

1 participant