99designs / iamy

A cli tool for importing and exporting AWS IAM configuration to YAML files
MIT License
238 stars 24 forks source link

Skip resources tagged with aws:cloudformation:stack-name #73

Closed stevehodgkiss closed 3 years ago

stevehodgkiss commented 4 years ago

CloudFormation automatically tags resources with default tags including the stack name, id and logical id. We can infer that the resource is managed by CloudFormation by looking for these tags and ignoring them.

This makes it possible for iamy to ignore bucket policies for buckets created with CFN, which was the main driver for this PR - We had to move bucket policies out of CFN to iamy to avoid both of them trying to manage them.

I've also added this functionality to users & roles since tags are available in the existing api call responses and it makes sense to be consistent. This should make it possible for iamy to ignore CFN created users or roles that specify RoleName or UserName.

iamy pull --delete --debug output:

DEBUG 2020/08/02 09:58:53 CloudFormation generated resource my-s3-bucket in stack my-stack
stevehodgkiss commented 4 years ago

One caveat with the bucket policy filtering in this change is that it uses tags from the bucket itself to determine if the bucket policy should be ignored. So if the bucket was created with CFN, its policy should also be managed with CFN. iamy will ignore the policy regardless of whether it was created with CFN or not, because the bucket was created with CFN. Unfortunately tags aren't available on the bucket policy itself.

patrobinson commented 4 years ago

I like this idea, however I think I'd consider this a breaking change. We'd need to be explicit and careful about making this change.

mtibben commented 4 years ago

This flag overlaps with the purpose of --accurate-cfn.

  1. Is using tags always an accurate way to determine if a resource if owned by CFN? If so, should we remove --accurate-cfn?

  2. Or is using tags a better heuristic than our current approach. If so, should we remove the current heuristic approach?

  3. Or if not and we really do need 3 strategies to determine whether a resource is owned by CFN, why not consolidate into a single flag, for example --skip-cfn-strategy=heuristic|tagged|accurate, to specify the strategy used to skip CFN resources (however will this require further API calls to fetch tags?)

/cc @vektah

patrobinson commented 4 years ago

@mtibben great question, I think it's useful to break up the approaches into these 3 options:

heuristic: This approach is OK, it's been working fine for a number of years, but it's imperfect. It's possible to miss resources that are not cloudformation resources, for instance we have some buckets named <name>-<account id> which don't appear in iamy because that format matches the regex of this. It also doesn't work for Bucket Policies or IAM Roles that have a user defined name in CloudFormation.

accurate: I have not tried this approach but I'm skeptical as to how it scales across dozens of regions with hundreds of stacks. It also only currently applies to the default region so it's still not accurate.

tagged: I think this is the best of the 3. It's more accurate than heuristic but faster and more scalable than "accurate". There's some caveats because CloudFormation will silently fail to tag IAM Roles/Policies if the user doesn't have permission to, which would result in inconsistent results.

Overall I'm inclined to move towards a strategy of tagged and remove accurate and heuristic. I'm hesitant though to move from heuristic to tagged immediately, as it's going to give different results that will take people time to adjust to. I think we should add tagged, removing accurate and look at swapping the default from heuristic to tagged in a major version release.

stevehodgkiss commented 4 years ago
  1. Is using tags always an accurate way to determine if a resource if owned by CFN? If so, should we remove --accurate-cfn?

I think it's an accurate way to determine that CFN created a given resource. I'm not aware of a way to prevent CFN from tagging resources with these tags. https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-resource-tags.html

The one caveat with S3 bucket policies is that they can't themselves be tagged, so we're looking at the S3 bucket instead and if tagged by CFN, skipping the bucket policy. I think this is a reasonable thing to assume & enforce - why create a bucket with CFN and manage it's policy with a different tool? It also solves the issue we currently have where we're forced to manage all bucket policies with iamy.

  1. Or is using tags a better heuristic than our current approach. If so, should we remove the current heuristic approach?

The tag based approach is faster and I think it would be more accurate. IAM resources are global and --accurate-cfn appears to list stacks in the current region, so it could end up not skipping resources were created in a different region.

iamy pull timings for a test account:

  1. Or if not and we really do need 3 strategies to determine whether a resource is owned by CFN, why not consolidate into a single flag, for example --skip-cfn-strategy=heuristic|tagged|accurate, to specify the strategy used to skip CFN resources (however will this require further API calls to fetch tags?)

+1 to consolidating and using the tag based approach over --accurate-cfn.

mtibben commented 4 years ago

OK great. Sounds like tagged is an excellent default option.

Given that heuristic and accurate are already in use by users at the moment, I'd propose that we

  1. implement a --cfn-skip-strategy=tagged|heuristic|accurate flag with tagged being the default strategy (and perhaps we can improve the names of these strategies)
  2. Remove --skip-cfn-tagged and --accurate-cfn

Then we can get further feedback from existing users whether heuristic and accurate are actually required, and we can deprecate in a later release

vektah commented 4 years ago

Aw, I was really excited for this.

With --accurate-cfn

iamy pull --accurate-cfn 1.72s user 0.31s system 8% cpu 25.006 total

no changes

With --skip-cfn-tagged

iamy pull --skip-cfn-tagged 1.28s user 0.23s system 9% cpu 15.376 total

lots of new policies and roles picked up that are managed by CF

I don't think these two are doing the same thing, running this branch adds a bunch of iam roles that dont get filtered.

Update: After a little spelunking I think these stacks that contain those roles have all been updated over time, Perhaps stack tags arent propagated to resources that were created as stack updates?

vektah commented 4 years ago

Another thought is that heuristic may work for people using terraform, as all it requires is some random junk on the end, but perhaps theres an argument to be made that they can tag with their own custom stack tag. Writing aws:cloudformation:stack-name from TF would be weird, perhaps it should be more configurable?

stevehodgkiss commented 4 years ago

Yeah, unfortunately it looks like IAM Roles aren't being created with the default CFN tags as documented. I've submitted a support ticket to AWS to find out if this is expected or if it's a bug that can be fixed.

It's not possible to tag resources with aws: as the prefix - the error returned is UPDATE_FAILED aws: prefixed tag key names are not allowed for external use..

Yeah, making the tag(s) to filter by configurable sounds like a good idea and something I was tempted to implement when writing the --skip-cfn-tagged flag. How about --skip-tagged iamy-ignore (where iamy-ignore is the tag key)?

simpsora commented 4 years ago

It feels like each of these solutions has edge cases that aren't being (or can't be) worked around:

I wonder if effort would be better spent implementing a way to include/exclude resource types (as described in https://github.com/99designs/iamy/pull/39#issuecomment-341582450). That approach, at least, would allow any user to specify exactly what they wanted and presumably wouldn't suffer from any of the above edge cases.

stevehodgkiss commented 4 years ago

I wonder if effort would be better spent implementing a way to include/exclude resource types (as described in #39 (comment)). That approach, at least, would allow any user to specify exactly what they wanted and presumably wouldn't suffer from any of the above edge cases.

Excluding a resource type (IAM::Role, BucketPolicy) would mean that we wouldn't be able to manage any of that resource type with iamy. It would fix the bucket policy issue (where right now we must manage them with iamy), but I think the automatic option by looking at the bucket's tags is better. The ability to exclude specific resources might be useful, but it's not as flexible as using tags (i.e. a list of each resource would need to be added to iamy config before applying with CFN - not ideal).

I think a combination of heuristic + tag filtering would be an improvement. Until AWS fix this issue [1] we'd need to add a specific tag (i.e. IamyIgnore: true in the CFN template) to named IAM entities that we want iamy to skip over, and then exclude them when using iamy using --skip-tagged IamyIgnore. As @vektah pointed out the more flexible tag approach would make iamy easier to use alongside terraform without needing to add random strings to the end of role names.

[1] https://github.com/aws-cloudformation/aws-cloudformation-coverage-roadmap/issues/277

mipearson commented 4 years ago

Out of curiosity: what do 99designs do here? terraform? cfn? iamy-only? etc

mtibben commented 4 years ago

Hey @mipearson we use a combination of CFN and iamy

mipearson commented 4 years ago

Do you guys run into the same problem with bucket policies?

stevehodgkiss commented 4 years ago

I've pushed an update to add a --skip-tagged my-tag option. Here's a summary of how the changes in this PR can be used:

When/if the CFN default tag propagation to iam resources issue is fixed, the tag based approach could likely replace the current heuristic name checks and --accurate-cfn.

patrobinson commented 4 years ago

@stevehodgkiss what do you think about @mtibben's recommendation?

implement a --cfn-skip-strategy=tagged|heuristic|accurate flag with tagged being the default strategy (and perhaps we can improve the names of these strategies)

(I'd recommend not making it the default for now, so we can test it out before switching it on)

stevehodgkiss commented 4 years ago

I don't see much point in using the tagged approach on it's own right now. CFN isn't propagating the default CFN tags to IAM entities, so we'd need to add a custom tag to all IAM entities created via CFN (and exclude with --skip-tagged), otherwise all CFN created entities would show up in iamy. The combination of heuristic & tagged would be my ideal approach until the default CFN tag propagation issue is resolved.

patrobinson commented 4 years ago

@mtibben

I think Steve's point is valid, that we would want to use Tagged and Heuristic together. Does adding --skip-tagged for now and waiting to see if AWS fix the issue with cloudformation tags not propogating sound reasonable? If they do fix it, we can make it the default.

mtibben commented 3 years ago

@stevehodgkiss @patrobinson Given that the intended behaviour of iamy is to always skip CFN resources, do we want the new CLI flags at all? If the heuristic isn't doing the job and you think we need to use both strategies together, I'd be happy to add this as the default behaviour rather than adding more options, as long as it plays well with the alternative "accurate" strategy and we document the strategies in the readme

stevehodgkiss commented 3 years ago

@mtibben yeah I agree this should probably be the default. My preference would be to have a release where the new functionality is opt-in first though, to aid in gradual migration of bucket policies (where the bucket is CFN-managed) back to CFN. Perhaps we only add the --skip-tagged flag as part of this change and remove --skip-cfn-tagged. --skip-tagged still makes sense until the CFN->IAM tagging issue is resolved - to be able to create named IAM resources with CFN a custom tag needs to be used (i.e. IamyIgnore) in CFN and then tell iamy to ignore it with --skip-tagged IamyIgnore. A release containing --skip-tagged would give us the ability to migrate gradually and not have to deprecate/remove a flag when we make --skip-cfn-tagged (--skip-tagged aws:cloudformation:stack-name) the default in a subsequent release.

stevehodgkiss commented 3 years ago

@mtibben any thoughts on this? We're keen to start using this functionality.