-
Notifications
You must be signed in to change notification settings - Fork 104
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
Only inline policies are considered, attached policies are ignored #33
Comments
Hey @morganchristiansson yeah, definitely. We haven't done it yet but it should be a fairly easy change. I'll mark this as an enhancement. |
Can we discuss the scope of this enhancement? This would add definite value to the product for my team and we're considering if we have the capacity to build it out because we're tired of waiting. :) Here are a few of my initial questions/thoughts:
Thoughts? I'm still familiarizing myself with the project, so there may be elements I've missed. |
Hey, thanks for the interest in taking this up!
We'll also want to add a flag to enable/disable this feature, probably in |
So, looks like this is fairly stale, but I'm gonna jump in to add support for non-inline policies as I intend to use Aardvark/RepoKid to clean up an IAM setup at my current position. So far, I just added a quick I'll keep updating as I go! |
@premature-optimization thank you! Please let me know if I can help. I'm happy to jump on a call, bounce ideas, or anything else useful. |
@mcpeak That might be super useful! I'm in the networktocode Slack workspace as unironicGUIfan if you want to asynchronously help me out (I work strange hours due to sleep disorders) Basically, it looks like all of the permissions of non-inline policies still show up in |
So all of the policy data (including managed policies) is in the functions up to |
@mcpeak @curtis-turner ATM, I got as far as including the additional Right now, I'd imagine that the Feel free to commit/branch whatever you like in that repo, please be sure not to open a PR against master until we've completed troubleshooting and testing :) I'll look into this in a couple days, but swamped with other work atm so I appreciate any assistance! |
Hey @premature-optimization pease join our Gitter and we can start a group chat for those interested. I took a peak at the code this morning. I think it makes sense to convert "update_role_cache" to more generic "update_data". This could handle roles, policies, and anything else in the future. The current "get_account_authorization_details" can easily return managed policy info too. Aardvark already supports. managed policies. There will be some logical differences in the way we actually repo, but they should be easy to overcome.
We'll probably need to change the Dynamo table or add a separate one for managed policies.
…On Wed, Aug 21, 2019 at 5:31 AM, premature-optimization < ***@***.*** > wrote:
@ mcpeak ( https://github.com/mcpeak ) @ curtis-turner (
https://github.com/curtis-turner )
Added you both as collaborators to my fork, where I'm posting my
debugging/code changes in order to collaborate.
ATM, I got as far as including the additional get_role_managed_policies function
to update_role_data and should add the policies along with the inline
policies to the current_policies dict (to avoid renaming all variable
calls), but looks like it's still not getting the managed policy
information correctly.
Right now, I'd imagine that the get_role_managed_policies call is unhappy
with the format of the request, or the reply is in a different format than
the inline function and it doesn't want to merge the two happily.
Feel free to commit/branch whatever you like in that repo, please be sure
not to open a PR against master until we've completed troubleshooting and
testing :)
I'll look into this in a couple days, but swamped with other work atm so I
appreciate any assistance!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub (
#33?email_source=notifications&email_token=ACBG7IGTXC3HBP2C2A3DSDDQFUYQLA5CNFSM4DUPVPW2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4ZPZDY#issuecomment-523435151
) , or mute the thread (
https://github.com/notifications/unsubscribe-auth/ACBG7IEILGLGYOX5D2HX72LQFUYQLANCNFSM4DUPVPWQ
).
|
What's the status on this one? Are you currently working on it? I might have some time end of the month to take a look into it, if there is nobody working on it. |
I don't think anybody is working on it, despite lots of interest. |
we would be happy to help testing if needed |
I’d like to work on this, but I need a little help getting started. |
Repokid has gone through some refactoring, so here's some updated info on how this implementation could go. This is based on the approach of detaching a managed policy from a role iff none of the permissions in the policy were used by the principal in the evaluation window. I've broken this down into two main chunks that could be implemented in serial. Retrieve and store managed policies
role_info: IAMEntry = get_role(role, flags=FLAGS.INLINE_POLICIES | FLAGS.MANAGED_POLICIES, **conn)
Updated schema for
Update permissions calculations
|
So we just tried to run aardvark and repokid on our roles but several of them had no suggested policies.
After debugging repokid for a while it seems like role.policies that is checked only contains inline policies. Attached policies are ignored.
Can attached policies be used as source aswell?
As a workaround we're copying all attached policies to inline policies.
@KorhanOzturk90
The text was updated successfully, but these errors were encountered: