Adding s3 encryption

[jaustinpage]

Add optional s3 encryption

[jaustinpage]

output works:

$ terraform output -module='remote_state'
dynamodb_lock_stream_arn = [
    arn:aws:dynamodb:us-west-2:201770704483:table/terraform-lock/stream/2018-11-12T21:27:17.753
]
dynamodb_lock_stream_label = [
    2018-11-12T21:27:17.753
]
dynamodb_lock_table_arn = [
    arn:aws:dynamodb:us-west-2:201770704483:table/terraform-lock
]
dynamodb_lock_table_name = [
    terraform-lock
]
s3_backend_bucket_name = pwpwn-v2-tfstate

[jaustinpage]

also, when https://github.com/hashicorp/terraform/issues/14037 lands, we should adjust this to avoid the count="{$var.turns_on_off}" hacks.

[jaustinpage]

addresses: https://github.com/samstav/terraform-aws-backend/issues/8

[stavxyz]

@jaustinpage thank you! This is going to be a great addition.

I see that this defines a separate/different resource in terraform if encryption is enabled. I can't think of a way around this, and I suspect you already considered it.

I would love to get this feature added, and for any existing projects using this module to be able to switch to using encryption. I'll try it with this branch, but I assume it won't work, at least not easily-- unless users terraform import aws_s3_bucket.tf_backend_bucket_encrypted <existing_bucket> ?

If the unencrypted bucket and corresponding terraform resource already exist, and a user starts using the version of this module which enables encryption by default, I think terraform plan will result in a plan that needs to delete the existing bucket and create the new one with the same name. That won't end well-- in addition to that, the existing unencrypted bucket has prevent_destroy. Even if tf could delete the bucket, the global namespace of s3 won't "free up" that name for a while. I think it normally takes an hour or more.

Basically, with this approach, I think users would end up needing migration instructions....

Thinking........

The only thing I can think of, which I am actually 100% cool with, is adding always-on (cannot be disabled) encryption configuration to the existing bucket resource definition. Though it is always-on it could at least be made as configurable as possible: Might be good to make sse_algorithm a variable so that users could specify AES256 if they wanted. Then, if sse_algorithm is not aws:kms then kms_master_key_id is set to null?

It would also be good if it was possible to allow this to happen:

Specifically this part

The default aws/s3 AWS KMS master key is used if this element is absent while the sse_algorithm is aws:kms.

[jaustinpage]

@samstav yes, your points are well received. Until we see examples of https://github.com/hashicorp/terraform/issues/14037 being fixed, i will implement this so that encryption is enabled by default.

This will have a net effect of costing operators $1 per month (unless they run terraform more than ~10k times in a month), but, this seems like a reasonable tradeoff compared to having the s3 bucket with the terraform.

I will implement this with encryption set to default, will create a new issue to track implementing the optional behavior when terraform implements something useful in a clean way.

I will also expose the kms_master_key_id.

might take me a few days to get back to you on this.

[stavxyz]

@jaustinpage Brilliant!

[stavxyz]

https://github.com/jaustinpage/terraform-aws-backend/pull/1

[stavxyz]

This is looking really good. I'll want to test this out manually now

https://github.com/samstav/terraform-aws-backend/pull/7#discussion_r244792765

[jaustinpage]

manual testing has revealed a bug when doing the terraform init reconfigure step.

[stavxyz]

Unless the kms key variable is required for basic functionality, I think it would be good to leave the examples/docs without its use and simply document the availability of the option/variable.

[jaustinpage]

Unless the kms key variable is required for basic functionality, I think it would be good to leave the examples/docs without its use and simply document the availability of the option/variable.

so, we have a choice. If we set https://github.com/samstav/terraform-aws-backend/pull/7/files#diff-7a370d8342e7203b805911c92454f0f4R127 to AES256 by default, then we do not have to set the kms key variable, as with encrypt = true in the conf.tf file, aes256 will be satisfied.

however, with the current default of aws:kms, both encrypt = true and the kms key id are required in the conf.tf file in order for s3 to not reject the s3 bucket update.

Note, that this is independent of https://github.com/samstav/terraform-aws-backend/pull/7/files#diff-7a370d8342e7203b805911c92454f0f4R82 ; which will allow either method (or none), but will turn unencrypted updates into encrypted updates.

to me, it basically feels like we either have to split the sse_algorithm variable, to allow for splits, OR we have to manually specify the kms key in the conf.tf, even if we are using the master one. Putting the kms key (with assistance in the output vars), even though it is more cumbersome, allows for a single set of instructions about how to use the module, while also providing the ease of specifying your own kms key if you choose.

One other approach would be to, if the sse_algorithm var is not specified, have it default to aws:kms in the https://github.com/samstav/terraform-aws-backend/pull/7/files#diff-7a370d8342e7203b805911c92454f0f4R82 , and then default to aes256 for https://github.com/samstav/terraform-aws-backend/pull/7/files#diff-7a370d8342e7203b805911c92454f0f4R127, but this seemed less transparent to an end user, if more convenient.

I dont have strong feelings either way...

Because we have decided that S3 encryption is good, and should always be turned on, I put my thinking hat on and simplified the interface to just do the correct thing whenever you specify a KMS key, or whenever you dont specify a kms key.

[jaustinpage]

I have manually verified the "Commands are the fun part" https://github.com/jaustinpage/terraform-aws-backend#commands-are-the-fun-part section, steps 1-5, but using a terraform conf.tf file, and modifying step 3 to be terraform plan -out=backend.plan -target=module.backend. I did this both specifying a kms key (and specifying the full arn for the kms_key_id in the conf.tf file), and not specifying a kms key (and not specifying a kms_key_id in the conf.tf file). Both times, I was able to successfully finish with a terraform init -reconfigure, saying yes to updating the state, and getting a successful terraform exit.

TLDR: "works on my machine"

Here is the conf.tf that i was using with kms key specified:

terraform {
  backend "s3" {
    region         = "us-west-2"
    bucket         = "pwpwn-v6-tfstate"
    key            = "states/terraform.tfstate"
    encrypt        = "true"
    dynamodb_table = "pwpwn-v6-tflock"
    kms_key_id     = "arn:aws:kms:us-west-2:000000000000:key/00000000-0000-0000-0000-000000000000"
  }
}

and here is the conf.tf that i was using when not specifying the kms key:

terraform {
  backend "s3" {
    region         = "us-west-2"
    bucket         = "pwpwn-v6-tfstate"
    key            = "states/terraform.tfstate"
    encrypt        = "true"
    dynamodb_table = "pwpwn-v6-tflock"
  }
}

[stavxyz]

I only just realized that the kms_key_id must be specified in the terraform block if it is used!

https://www.terraform.io/docs/backends/types/s3.html#kms_key_id

Now your story makes sense. Thank you!!

[jaustinpage]

anything i can help test at this point?