aws_s3_bucket - Fix policy normalization to match AWS API

[vancluever]

There are a couple of situations I have encountered where a S3 bucket policy that is technically correct gets changed by AWS:

• When a policy statement has no Sid specified, it will create a Sid entry with blank data.
• When a policy statement only has one action, but is specified in an array, it will truncate the array and use a single element string action instead. (ie: "Action": [ "s3:ListBucket" ] becomes "Action": "s3:ListBucket")
• Multiple actions will be sorted alphabetically.

This PR expands on the current normalizeJson functionality that exists as a StateFunc hook and adds features to correct this:

• sortJSONArray will recursively search for JSON arrays in the policy and sort them
• normalizePolicyStatements sets the empty Sid when one does not exist and also truncates single-element Action entries.

Related to #3634, #3124, #4245, and possibly others, basically there seems to be a lot of other situations where this is an issue, each with possibly different quirks.

[catsby]

Hey @vancluever sorry this has taken so long to get to, this sounds really promising. Curious, do you think this applies to other resources (like most IAM things) that could benefit from this as well? If so, maybe we could put this in structure.go you think?

[vancluever]

Hey @catsby, definitely - if the logic for normalization is common across the board for all policy documents then this definitely should go in there. I'm not too sure if I saw that file or not, but possibly I refrained from putting it in there because I was not too sure of the scope at the time.

I can probably do the work to move this later today or early this weekend. Should I keep the existing StateFunc as normalizeJson local to the resource file or encapsulate everything in the new function (so JSON in, JSON out)? I just briefly glanced over the other functions so I haven't had time to fully assess a pattern.

[vancluever]

@catsby, check out the code I just re-pushed/squashed. This code is a mix if the sorting and "flattening" that I was doing before, and some of the stuff that was hashed out by @radeksimko in #3124, namely the struct for the policy layout. I didn't use the full depth of the struct layout as things seem to get murky when you are dealing with elements that could have either a string value or an array value, or sub-elements when the entire parent element could be missing (ie: NotPrincipal, for example). So I instead decided to handle that part in code.

This should cover a lot of the cases and gets us to almost all of the way there. As I'm typing this I'm thinking that principals might be an issue if they are sorted on the AWS side, we might not be able to get away with just having them in a map like they are right now and may have to stage them in a struct that is separate from the main data structure. However I have tested it on a slightly more complex S3 policy than what I was using before, with things like both actions and conditions and things seem to get sorted and flattened properly, so with a little more work and maybe some test cases we might be good to merge this.

Also, I have swapped out normalizeJson in the ES resource, I intend to test this soon, maybe tomorrow, to see if it corrects the policy shown in #3634.

[vancluever]

More updates:

• Principal stuff now supported, added in @radeksimko's struct for this as well and am parsing it using mapstructure. So things are a bit more explicit now and multiple principals in a single statement should be ordered.
• Also - tests!

[leeprovoost]

@vancluever I noticed another issue with the Principal (tested against 0.6.12). When you're creating an S3 bucket policy for ELB logs and follow the AWS documentation (http://docs.aws.amazon.com/ElasticLoadBalancing/latest/DeveloperGuide/enable-access-logs.html), it asks you to use the ELB Region ID like this:

      "Principal": {
        "AWS": [
          "797873946194"
        ]
      }

This gets marked as changed all the time. It's only when you use the following format that it is not marked anymore:

      "Principal": {
        "AWS": "arn:aws:iam::797873946194:root"
      }

So you need to normalise it, as well as use the full ARN.

[coen-hyde]

@leeprovoost I'm not sure it's a good idea to normalize data, as opposed to structure. In the scenario you describe I think the user should update the principle to be the arn.

[vancluever]

Guys,

I can see points in both. I actually never knew the bareword account ID was legit as an ARN. That's quite silly IMO, but if that's what happens and it's documented like that by Amazon then it might be something that might need to be done. On the other hand, I think being as explicit as possible is optimal here.

I'm torn, but if I revisit this soon then I'll try and get an opinion from Amazon on it, via ticket or whatever. Maybe they can give some insight on what else we can expect too.

Otherwise, not too sure what the status of this is, it's been open for a while - paging @radeksimko @catsby

[coen-hyde]

I think this should be shipped now, and re-evaluate the data normalization after. Getting the structure normalized will provide a lot of value to people who use Terraform. If it is decided that data normalization is a good idea, then it can be added. In the mean time we have 80% of silly policy conflicts covered.

[vancluever]

Yeah, it would be nice to see it merged. I'm still using it in our own custom fork. On that note, I've given this branch a rebase so that it's easier to merge. Would love to see it upstream!

[leeprovoost]

I don't feel very strongly about one or the other. Maybe we can resolve it by adding it in the Terraform documentation to avoid that future users are bumping into the same problem?

[vancluever]

Everyone - seeing as #6881 is nearing merge, I'm thinking about closing this (would be one less branch I'd need to maintain). Before I do that and drop it though is there anything that we need out of it? One thing I did take a quick look at and noticed is that there does not seem to be any logic for handling single-element actions or principals (things that AWS will convert to strings versus keeping them as single element arrays).

[stack72]

@jen20 thoughts on the questions @vancluever has above? It would be good to be able to close this PR

[vancluever]

@stack72 - Just noticed this, wow!

Anyway, if any PR should be merged, I'd like it to be #6956, since this fixes it in a Terraform-ish way, and is easier to manage (in comparison, this PR is kind of hacky and we might be playing catch-up with cases that we have not 100% caught).

[stack72]

ok @vancluever

thanks! shall we close this one out then?

[vancluever]

@stack72 Sure! I'll do the honors ;)

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.