Skip to content
This repository has been archived by the owner on Sep 3, 2024. It is now read-only.

S3 Block public access rule doesn't work when aws_s3_bucket_public_access_block is used for conditional resource or in the module #295

Open
marioerceg opened this issue Jan 28, 2022 · 26 comments
Labels
bug Something isn't working

Comments

@marioerceg
Copy link

Hi,

It seems that S3 Block public access rule (https://github.com/fugue/regula/blob/master/rego/rules/tf/aws/s3/block_public_access.rego) doesn't work when aws_s3_bucket_public_access_block is used for conditional resource OR within the module.

Example file with working and not working resources:

`terraform {

backend "local" {
path = "../terraform.tfstate.27.01.2022"
}

}

provider "aws" {
version = "~> 2.0"
region = var.aws_region
allowed_account_ids = [var.aws_account_id]
}

resource "aws_s3_bucket" "working_1" {
bucket = "working-1-random-string"
}
resource "aws_s3_bucket_public_access_block" "working_1_block" {
bucket = aws_s3_bucket.working_1.id

block_public_acls = true
block_public_policy = true
ignore_public_acls = true
restrict_public_buckets = true
}

resource "aws_s3_bucket" "not_working_1" {
count = var.aws_region == "eu-west-1" ? 1 : 0
bucket = "not-working-1-random-string"
acl = "private"
}
resource "aws_s3_bucket_public_access_block" "not_working_1_block" {
count = var.aws_region == "eu-west-1" ? 1 : 0
bucket = aws_s3_bucket.not_working_1[count.index].id
block_public_acls = true
block_public_policy = true
ignore_public_acls = true
restrict_public_buckets = true
}

module "working_2" {
source = "[email protected]:rackspace-infrastructure-automation/aws-terraform-s3//?ref=v0.12.6"
name = "working-2-random-string"
}
resource "aws_s3_bucket_public_access_block" "working_2_block" {
bucket = module.working_2.bucket_id

block_public_acls = true
block_public_policy = true
ignore_public_acls = true
restrict_public_buckets = true
}

module "not_working_2" {
source = "[email protected]:rackspace-infrastructure-automation/aws-terraform-s3//?ref=v0.12.6"

block_public_access = true
block_public_access_acl = true
block_public_access_ignore_acl = true
block_public_access_policy = true
block_public_access_restrict_bucket = true
name = "not-working-2-random-string"
}`

@ameliafugue ameliafugue added the bug Something isn't working label Jan 28, 2022
@ameliafugue
Copy link
Member

Hi @marioerceg

Thank you for reporting this. The team is investigating this issue.

@marioerceg
Copy link
Author

Hi @ameliafugue . Is there any update on this? Thanks!

@jaspervdj-luminal
Copy link
Member

@marioerceg Are you using Regula against .tf files or a JSON plan?

We need to improve our count support in the HCL view a little bit to get aws_s3_bucket.not_working_1[count.index].id to work -- currently the HCL processor constructs the resource without the extra index. I'll see if I can get some time to work on that.

I took at the look at the source code of the module you are using, and they are using the same count pattern, so the problem is likely this.

@marioerceg
Copy link
Author

Thanks @jaspervdj-luminal !

I am using json plan for this.

@marioerceg
Copy link
Author

Hi @jaspervdj-luminal . Did you have time to check this? Thanks!

@jaspervdj-luminal
Copy link
Member

Hey @marioerceg, sorry for the delay! It was a bit chaotic the past few weeks because we were acquired by Snyk. I should have some time to look into this today or tomorrow though. I'll let you know when I have a branch you can test!

@marioerceg
Copy link
Author

Congrats @jaspervdj-luminal !
Deal. Waiting for your reply.

@jaspervdj-luminal
Copy link
Member

I've started a count-improvements branch, however there's some work left. This turned out to be significantly more difficult than I thought, since it requires an update to our dependency analysis, as well as the way we pass resources as values into the HCL evaluator. I expect to finish this by the end of this week.

@marioerceg
Copy link
Author

Great! Thanks @jaspervdj-luminal !

@jaspervdj-luminal
Copy link
Member

@marioerceg I think the count-improvements branch should work now. I need to test it a bit more before we merge it, but feel free to give it a try for your use case.

@marioerceg
Copy link
Author

Thanks @jaspervdj-luminal ! Will try it tomorrow

@marioerceg
Copy link
Author

Hi @jaspervdj-luminal it seems it doesn't work, or I am not testing well. It still works for 2 working examples.

I am using "opa eval .... 'data.fugue.regula.report'"

Tested with TF versions 0.14.11 and 1.0.11.

Tested with the last opa as well as with 0.30.1

@jaspervdj-luminal
Copy link
Member

@marioerceg Oh, I see. Unfortunately improving the count attribute turned it to be relatively complex, so I don't think it's feasible to do in pure Rego. We do the transformation in the hcl preprocessor in the Regula binary. Since we are able to be a bit more flexible there, it has a number of other improvements over the pure Rego library as well.

You can get similar output to opa eval using something like: regula run -f json. If for some reason, it's not possible to use a regula invocation (e.g. you are using conftest), you can still use the preprocessor like this:

regula show input . >regula_input.json  # read in TF files and preprocess
opa run -d regula/rego/lib -d regula/rego/rules -i regula_input.json 'data.fugue.regula.report'  # use preprocessed input

Does that help?

@marioerceg
Copy link
Author

marioerceg commented Mar 9, 2022

Hi @jaspervdj-luminal . After I added comment, I tried with the last regula version "regula run" and got the same results (but again from terraform generated json plan)
Will it give different results if I download the new branch and try with commands you specified?
Thanks!

@jaspervdj-luminal
Copy link
Member

@marioerceg Yeah, it will give different results if you point it at the folder with your .tf files. That way regula will be able to parse and preprocess it all from source, which as a nice side effect gives you some other things like file names and line numbers.

@marioerceg
Copy link
Author

@jaspervdj-luminal Let me try it tomorrow. I hope it will work and that the code and all the modules are processed well

@marioerceg
Copy link
Author

Hi @jaspervdj-luminal . I tried with the last available prebuilt binary version of regula, and it worked for not_working_2, but it didn't recognize not_working_1 bucket at all - not showing it.

@marioerceg
Copy link
Author

Just small update:

It is recognized if not using variable, like this:

count = "eu-west-1" == "eu-west-1" ? 1 : 0

but block access settings is not linked with it even if not using variable.

With terraform json it is recognized.

@jaspervdj-luminal
Copy link
Member

@marioerceg The PR hasn't merged yet, it's here: #321

If you have Go installed, it should be relatively easy to build a binary using make binary. I can also provide you with one if necessary (or I expect we will get this into a release sometime next week).

@marioerceg
Copy link
Author

Hi @jaspervdj-luminal . Tried but can't get not_working_1 recognized at all.

One more question: is it possible to have full description in "table output" of regula run, and not only the short summary one?
Thanks!

@jaspervdj-luminal
Copy link
Member

@marioerceg Can you try this again with Regula v2.6.1?

I think having the full description in the table output would be hard to fit on a screen nicely for most of users? Some descriptions are quite long, and this output is meant to be human-readable first.

@marioerceg
Copy link
Author

Hi @jaspervdj-luminal .
Tested with v2.6.1, build 0d40e66 (built with OPA v0.37.2), downloaded the latest verion of the count-improvement branch as well, and still doesn't recognize not_working_1 at all (not showing that resource in the output at all)

Thanks,
Mario

@marioerceg
Copy link
Author

Hi @jaspervdj-luminal . Regarding adding descriptions to the table it would be good to be optional, or at least to have another field where link to description could be added.
I have some custom rules, and want to give description what should be done to fix the issue.
Thanks!

@marioerceg
Copy link
Author

Hi @jaspervdj-luminal !
Any update on this?
Thanks

@cxystras-xm
Copy link

Hi @jaspervdj-luminal , can we get some traction here. This issue affects us in our daily operations. thanks

@marioerceg
Copy link
Author

Hi @jaspervdj-luminal . Could you share if there is any ETA to fix this? Thanks

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants