From 85c7f467b560d5140eccc68f325ddbe14465df05 Mon Sep 17 00:00:00 2001 From: Kate Sugden <107400614+ksugden@users.noreply.github.com> Date: Tue, 10 Sep 2024 12:57:32 +0100 Subject: [PATCH] feat: DBTP-1301 - provide cross account s3 to s3 migration permissions (#220) Co-authored-by: Tony Griffin Co-authored-by: tony griffin <54268925+tony-griffin@users.noreply.github.com> Co-authored-by: Will Gibson <8738245+WillGibson@users.noreply.github.com> --- README.md | 125 +++++++++++++++++---------- application-load-balancer/main.tf | 2 + data-migration/locals.tf | 4 + data-migration/main.tf | 93 ++++++++++++++++++++ data-migration/outputs.tf | 4 + data-migration/providers.tf | 9 ++ data-migration/tests/unit.tftest.hcl | 62 +++++++++++++ data-migration/variables.tf | 23 +++++ example/extensions.yml | 8 +- s3/locals.tf | 2 + s3/main.tf | 16 ++++ s3/tests/unit.tftest.hcl | 39 +++++++++ s3/variables.tf | 10 +++ 13 files changed, 350 insertions(+), 47 deletions(-) create mode 100644 data-migration/locals.tf create mode 100644 data-migration/main.tf create mode 100644 data-migration/outputs.tf create mode 100644 data-migration/providers.tf create mode 100644 data-migration/tests/unit.tftest.hcl create mode 100644 data-migration/variables.tf diff --git a/README.md b/README.md index 9d75043dd..0da6c093b 100644 --- a/README.md +++ b/README.md @@ -20,14 +20,14 @@ Alternative installation methods [here](https://github.com/trufflesecurity/truff Various quality checks are run in AWS Codebuild in the `platform-tools` account for any push to a pull request branch: -* [Checkov](https://www.checkov.io/) -* [terraform fmt](https://developer.hashicorp.com/terraform/cli/commands/fmt) -* [terraform validate](https://developer.hashicorp.com/terraform/cli/commands/validate) -* [tflint](https://github.com/terraform-linters/tflint]) -* [terraform test](https://developer.hashicorp.com/terraform/cli/commands/test) - plan style -* python tests - unit tests for python code within the terraform modules +- [Checkov](https://www.checkov.io/) +- [terraform fmt](https://developer.hashicorp.com/terraform/cli/commands/fmt) +- [terraform validate](https://developer.hashicorp.com/terraform/cli/commands/validate) +- [tflint](https://github.com/terraform-linters/tflint]) +- [terraform test](https://developer.hashicorp.com/terraform/cli/commands/test) - plan style +- python tests - unit tests for python code within the terraform modules -* Todo: [terraform test](https://developer.hashicorp.com/terraform/cli/commands/test) - end to end tests which do an apply and actually provision infrastructure +- Todo: [terraform test](https://developer.hashicorp.com/terraform/cli/commands/test) - end to end tests which do an apply and actually provision infrastructure ### Running the terraform unit tests locally @@ -43,8 +43,7 @@ The faster, but less comprehensive, tests that run against the `terraform plan` terraform test ``` -To run the longer end-to-end tests that actually deploy the module (via `terraform apply`), perform assertions and tear back down are run from the -same directory as follows: +To run the longer end-to-end tests that actually deploy the module (via `terraform apply`), perform assertions and tear back down are run from the same directory as follows: ```shell terraform test -test-directory e2e-tests @@ -52,24 +51,28 @@ terraform test -test-directory e2e-tests ### Running the python unit tests locally -The Lambda provisioned by the terraform postgres module uses python 3.11 at runtime. Tests should be executed locally, using python 3.11. From the root directory, check which python version the poetry environment is using: +The Lambda provisioned by the terraform postgres module uses python 3.11 at runtime. Tests should be executed locally, using python 3.11. From the root directory, check which python version the poetry environment is using: ```shell poetry run python --version ``` -If it is not 3.11, run +If it is not 3.11, run + ```shell -poetry env use python3.11 +poetry env use python3.11 ``` + (python 3.11 must be installed) Install dependencies: + ```shell -poetry install +poetry install ``` Execute the tests: + ```shell poetry run pytest ``` @@ -82,7 +85,7 @@ This module is configured by a YAML file and two simple args: locals { args = { application = "my-app-tf" - services = yamldecode(file("extensions.yml")) + services = yamldecode(file("platform-config.yml")) } } @@ -97,35 +100,35 @@ module "extensions" { ## Opensearch module configuration options -The options available for configuring the opensearch module should be applied in the `extensions.yml` file. They +The options available for configuring the opensearch module should be applied in the `platform-config.yml` file. They should look something like this: ```yaml my-opensearch: type: opensearch environments: - "*": # Default configuration values + '*': # Default configuration values plan: small engine: '2.11' - ebs_volume_type: gp3 # Optional. Must be one of: standard, gp2, gp3, io1, io2, sc1 or st1. Defaults to gp2. - ebs_throughput: 500 # Optional. Throughput in MiB/s. Only relevant for volume type gp3. Defaults to 250 MiB/s. - index_slow_log_retention_in_days: 3 # Optional. Valid values can be found here: https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/cloudwatch_log_group#retention_in_days + ebs_volume_type: gp3 # Optional. Must be one of: standard, gp2, gp3, io1, io2, sc1 or st1. Defaults to gp2. + ebs_throughput: 500 # Optional. Throughput in MiB/s. Only relevant for volume type gp3. Defaults to 250 MiB/s. + index_slow_log_retention_in_days: 3 # Optional. Valid values can be found here: https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/cloudwatch_log_group#retention_in_days search_slow_log_retention_in_days: 14 # Optional. As above. - es_app_log_retention_in_days: 30 # Optional. As above. - audit_log_retention_in_days: 1096 # Optional. As above. + es_app_log_retention_in_days: 30 # Optional. As above. + audit_log_retention_in_days: 1096 # Optional. As above. # The following are derived from the plan. DBTP-841 will allow them to be overriden here. # volume_size: 1000 # instances: 1 # master: false # instance: m6g.xlarge.search - env-one: # Per-environment overrides for any of the defaults in the previous section - plan: large # Override the plan. - engine: '2.7' # Downgrade the engine. + env-one: # Per-environment overrides for any of the defaults in the previous section + plan: large # Override the plan. + engine: '2.7' # Downgrade the engine. ``` ## Application Load Balancer module -This module will create a ALB that lets you specify multiple domain names for use in the HTTPS listener rule. In addition it will create the required certificates for all the domains specified. +This module will create a ALB that lets you specify multiple domain names for use in the HTTPS listener rule. In addition it will create the required certificates for all the domains specified. The primary domain will always follow the pattern: @@ -133,21 +136,21 @@ For non-production: `internal..uktrade.digital` For production: `internal..prod.uktrade.digital` -If there are multiple web services on the application, you can add the additional domain to your certificate by adding the prefix name (eg. `internal.static`) to the variable `additional_address_list` see extension.yml example below. `Note: this is just the prefix, no need to add env.uktrade.digital` +If there are multiple web services on the application, you can add the additional domain to your certificate by adding the prefix name (eg. `internal.static`) to the variable `additional_address_list` see extension.yml example below. `Note: this is just the prefix, no need to add env.uktrade.digital` `cdn_domains_list` and `additional_address_list` are optional. ### Route 53 record creation -The R53 domains for non-production and production are stored in different AWS accounts. The last half of the Terraform code needs to be able to run in the correct AWS account. This is determined by the provider passed in from the `-deploy` `aws-domain` alias. +The R53 domains for non-production and production are stored in different AWS accounts. The last half of the Terraform code needs to be able to run in the correct AWS account. This is determined by the provider passed in from the `-deploy` `aws-domain` alias. -example `extensions.yml` config. +example `platform-config.yml` config. ```yaml my-application-alb: type: alb environments: - dev: + dev: additional_address_list: - internal.my-web-service-2 ``` @@ -157,47 +160,53 @@ my-application-alb: This module will create the CloudFront (CDN) endpoints for the application if enabled. `cdn_domains_list` is a map of the domain names that will be configured in CloudFront. -* the key is the fully qualified domain name -* the value is an array containing the internal prefix and the base domain (the application's Route 53 zone). + +- the key is the fully qualified domain name. +- the value is an array containing the internal prefix and the base domain (the application's Route 53 zone). ### Optional settings: -To create a R53 record pointing to the CloudFront endpoint, set this to true. If not set, in non production this is set to true by default and set to false in production. +To create a R53 record pointing to the CloudFront endpoint, set this to true. If not set, in non production this is set to true by default and set to false in production. + - enable_cdn_record: true To turn on CloudFront logging to a S3 bucket, set this to true. + - enable_logging: true -example `extensions.yml` config. +example `platform-config.yml` config. ```yaml my-application-alb: type: alb environments: - dev: + dev: cdn_domains_list: - - dev.my-application.uktrade.digital: [ "internal", "my-application.uktrade.digital" ] - - dev.my-web-service-2.my-application.uktrade.digital: [ "internal.my-web-service-2", "my-application.uktrade.digital" ] + - dev.my-application.uktrade.digital: + ['internal', 'my-application.uktrade.digital'] + - dev.my-web-service-2.my-application.uktrade.digital: + ['internal.my-web-service-2', 'my-application.uktrade.digital'] additional_address_list: - internal.my-web-service-2 enable_cdn_record: false enable_logging: true prod: - cdn_domains_list: - - my-application.prod.uktrade.digital: [ "internal", "my-application.prod.uktrade.digital" ] + cdn_domains_list: + - my-application.prod.uktrade.digital: + ['internal', 'my-application.prod.uktrade.digital'] ``` ## Monitoring This will provision a CloudWatch Compute Dashboard and Application Insights for `-`. -Example usage in `extensions.yml`... +Example usage in `platform-config.yml`... ```yaml demodjango-monitoring: type: monitoring environments: - "*": + '*': enable_ops_center: false prod: enable_ops_center: true @@ -205,23 +214,23 @@ demodjango-monitoring: ## S3 bucket -An s3 bucket can be added by configuring the `extensions.yml` file. Below is an example configuration, showing the available options: +An s3 bucket can be added by configuring the `platform-config.yml` file. Below is an example configuration, showing the available options: ```yaml my-s3-bucket: type: s3 readonly: false # Optional services: # Optional - - "web" + - 'web' environments: - "*": # Default configuration values + '*': # Default configuration values bucket_name: my-bucket-dev # Mandatory retention_policy: # Optional mode: COMPLIANCE # GOVERNANCE" or "COMPLIANCE" days: 10 # Integer value. Alternatively years may be specified. versioning: true # Optional lifecycle_rules: # Optional. If present, contains a list of rules. - - filter_prefix: "bananas/" # Optional. If none, the rule applies to all objects. Use an empty string for a catch-all rule. + - filter_prefix: 'bananas/' # Optional. If none, the rule applies to all objects. Use an empty string for a catch-all rule. expiration_days: 10 # Integer value enabled: true # Mandatory flag objects: # Optional. If present, contains a list of objects @@ -232,14 +241,14 @@ my-s3-bucket: ## Postgres database -A postgres database can be added by configuring the `extensions.yml` file. Below is a simple example configuration, showing some of the available options: +A postgres database can be added by configuring the `platform-config.yml` file. Below is a simple example configuration, showing some of the available options: ```yaml my-postgres-db: type: postgres version: 16.2 environments: - "*": + '*': plan: tiny backup_retention_days: 1 # Optional. Must be between 1 and 35. If none, defaults to 7. prod: @@ -247,6 +256,30 @@ my-postgres-db: deletion_policy: Retain # Optional: Delete or Retain ``` +## S3 to S3 data migration module + +This module will create cross account permissions to write to your S3 bucket, allowing the copying of files from a source S3 bucket to your destination S3 bucket. Any cross account data migration **must be approved by cyber**. Please see the [S3 to S3 data migration documentation](https://platform.readme.trade.gov.uk/reference/cross-account-s3-to-s3-data-migration/) for further details on using this module. + +Most users will not have permissions to apply the following configuration. In this case, an SRE team member or someone from the DBT-platform team will be able to help once cyber has approved the request. + +S3 data migration can be enabled by adding the `data_migration` parameter along with the `import` parameter and its mandatory configuration to the S3 extension in your `platform-config.yml` file. The `source_kms_key_arn` is optional as it depends on whether the source bucket has KMS key encryption on it. + +```yaml +extensions: + my-s3-bucket: + type: s3 + services: + - web + environments: + "*": + bucket_name: bucket_name: my-bucket-dev # Mandatory + data_migration: # Optional + import: # Mandatory if data_migration is present + source_kms_key_arn: arn:aws:kms::123456789:my-source-key # Optional + source_bucket_arn: arn:aws:s3::123456789:my-source-bucket # Mandatory if data_migration is present + worker_role_arn: arn:aws:iam::123456789:my-migration-worker-arn # Mandatory if data_migration is present +``` + ## Using our `demodjango` application for testing See [instructions in the demodjango-deploy repository](https://github.com/uktrade/demodjango-deploy/tree/main#deploying-a-new-environment). diff --git a/application-load-balancer/main.tf b/application-load-balancer/main.tf index 3227cfcee..ecba5da87 100644 --- a/application-load-balancer/main.tf +++ b/application-load-balancer/main.tf @@ -13,6 +13,7 @@ data "aws_subnets" "public-subnets" { } resource "aws_lb" "this" { + # checkov:skip=CKV2_AWS_20: Redirects for HTTP requests into HTTPS happens on the CDN # checkov:skip=CKV2_AWS_28: WAF is outside of terraform-platform-modules name = "${var.application}-${var.environment}" load_balancer_type = "application" @@ -53,6 +54,7 @@ resource "aws_lb_listener" "alb-listener" { resource "aws_security_group" "alb-security-group" { # checkov:skip=CKV2_AWS_5: False Positive in Checkov - https://github.com/bridgecrewio/checkov/issues/3010 + # checkov:skip=CKV_AWS_260: Ingress traffic from 0.0.0.0:0 is necessary to enable connecting to web services for_each = local.protocols name = "${var.application}-${var.environment}-alb-${each.key}" description = "Managed by Terraform" diff --git a/data-migration/locals.tf b/data-migration/locals.tf new file mode 100644 index 000000000..7c17a4bc4 --- /dev/null +++ b/data-migration/locals.tf @@ -0,0 +1,4 @@ +locals { + role_name = "${substr(var.destination_bucket_identifier, 0, 48)}-S3MigrationRole" + policy_name = "${substr(var.destination_bucket_identifier, 0, 46)}-S3MigrationPolicy" +} diff --git a/data-migration/main.tf b/data-migration/main.tf new file mode 100644 index 000000000..772af3e80 --- /dev/null +++ b/data-migration/main.tf @@ -0,0 +1,93 @@ +resource "aws_iam_role" "s3_migration_role" { + name = local.role_name + assume_role_policy = data.aws_iam_policy_document.allow_assume_role.json +} + +data "aws_iam_policy_document" "allow_assume_role" { + statement { + sid = "AllowAssumeWorkerRole" + effect = "Allow" + + principals { + type = "AWS" + identifiers = [var.config.worker_role_arn] + } + + actions = ["sts:AssumeRole"] + } +} + +data "aws_iam_policy_document" "s3_migration_policy_document" { + statement { + sid = "AllowReadOnSourceBucket" + effect = "Allow" + + actions = [ + "s3:ListBucket", + "s3:GetObject", + "s3:GetObjectTagging", + "s3:GetObjectVersion", + "s3:GetObjectVersionTagging" + ] + + resources = [ + var.config.source_bucket_arn, + "${var.config.source_bucket_arn}/*" + ] + } + + statement { + sid = "AllowWriteOnDestinationBucket" + effect = "Allow" + + actions = [ + "s3:ListBucket", + "s3:PutObject", + "s3:PutObjectAcl", + "s3:PutObjectTagging", + "s3:GetObjectTagging", + "s3:GetObjectVersion", + "s3:GetObjectVersionTagging" + ] + + resources = [ + var.destination_bucket_arn, + "${var.destination_bucket_arn}/*" + ] + } + + statement { + sid = "AllowDestinationKMSEncryption" + effect = "Allow" + + actions = [ + "kms:Encrypt", + "kms:ReEncrypt*", + "kms:GenerateDataKey*", + ] + + resources = [var.destination_kms_key_arn] + } + + dynamic "statement" { + for_each = var.config.source_kms_key_arn != null ? [var.config.source_kms_key_arn] : [] + + content { + sid = "AllowSourceKMSDecryption" + effect = "Allow" + + actions = [ + "kms:Decrypt", + "kms:DescribeKey" + ] + + resources = [var.config.source_kms_key_arn] + } + } +} + +resource "aws_iam_role_policy" "s3_migration_policy" { + name = local.policy_name + role = aws_iam_role.s3_migration_role.name + policy = data.aws_iam_policy_document.s3_migration_policy_document.json +} diff --git a/data-migration/outputs.tf b/data-migration/outputs.tf new file mode 100644 index 000000000..a52e08aaa --- /dev/null +++ b/data-migration/outputs.tf @@ -0,0 +1,4 @@ +# output needed for the s3 tests asserting creation of submodule (asserting creation directly results in a failure during the codebuild job) +output "module_exists" { + value = true +} diff --git a/data-migration/providers.tf b/data-migration/providers.tf new file mode 100644 index 000000000..631e33505 --- /dev/null +++ b/data-migration/providers.tf @@ -0,0 +1,9 @@ +terraform { + required_version = "~> 1.7" + required_providers { + aws = { + source = "hashicorp/aws" + version = "~> 5" + } + } +} diff --git a/data-migration/tests/unit.tftest.hcl b/data-migration/tests/unit.tftest.hcl new file mode 100644 index 000000000..547812220 --- /dev/null +++ b/data-migration/tests/unit.tftest.hcl @@ -0,0 +1,62 @@ +variables { + config = { + "source_bucket_arn" = "test-source-bucket-arn" + "source_kms_key_arn" = "test-source-kms-key-arn" + "worker_role_arn" = "test-role-arn" + } + destination_bucket_arn = "test-destination-bucket-arn" + destination_bucket_identifier = "test-destination-bucket-name" +} + + +run "data_migration_unit_test" { + command = plan + + assert { + condition = aws_iam_role.s3_migration_role.name == "test-destination-bucket-name-S3MigrationRole" + error_message = "Should be: test-destination-bucket-name-S3MigrationRole" + } + + assert { + condition = aws_iam_role.s3_migration_role.assume_role_policy != null + error_message = "Role should have an assume role policy" + } + + assert { + condition = aws_iam_role_policy.s3_migration_policy.name == "test-destination-bucket-name-S3MigrationPolicy" + error_message = "Should be: test-destination-bucket-name-S3MigrationPolicy" + } + + assert { + condition = aws_iam_role_policy.s3_migration_policy.role == "test-destination-bucket-name-S3MigrationRole" + error_message = "Should be: test-destination-bucket-name-S3MigrationRole" + } + + assert { + condition = strcontains(aws_iam_role_policy.s3_migration_policy.policy, "test-destination-bucket-arn") + error_message = "Statement should contain resource arn: test-destination-bucket-arn" + } + + assert { + condition = strcontains(aws_iam_role_policy.s3_migration_policy.policy, "kms:Decrypt") + error_message = "Statement should contain kms:Decrypt" + } +} + +run "data_migration_without_source_kms_key" { + command = plan + + variables { + config = { + "source_bucket_arn" = "test-source-bucket-arn" + "worker_role_arn" = "test-role-arn" + } + destination_bucket_arn = "test-destination-bucket-arn" + destination_bucket_identifier = "test-destination-bucket-name" + } + + assert { + condition = strcontains(aws_iam_role_policy.s3_migration_policy.policy, "kms:Decrypt") == false + error_message = "Statement should not contain kms:Decrypt" + } +} diff --git a/data-migration/variables.tf b/data-migration/variables.tf new file mode 100644 index 000000000..4086e2897 --- /dev/null +++ b/data-migration/variables.tf @@ -0,0 +1,23 @@ + +variable "config" { + type = object({ + source_bucket_arn = string + source_kms_key_arn = optional(string) + worker_role_arn = string + }) +} + +variable "destination_bucket_arn" { + type = string + default = "" +} + +variable "destination_bucket_identifier" { + type = string + default = "" +} + +variable "destination_kms_key_arn" { + type = string + default = "" +} diff --git a/example/extensions.yml b/example/extensions.yml index fc0c834bc..83ddf8567 100644 --- a/example/extensions.yml +++ b/example/extensions.yml @@ -41,6 +41,11 @@ dw-s3-bucket: environments: dev: bucket_name: digital-workspace-v2-dev + data_migration: + import: + source_bucket_arn: "arn:aws:s3:::my-application-test" + source_kms_key_arn: "arn:aws:kms:eu-west-2:123456789:key/1234-1334-1234-1234" + worker_role_arn: "arn:aws:iam::987654321:role/service-role" lifecycle_rules: - filter_prefix: "logs/" expiration_days: 1 @@ -55,9 +60,10 @@ dw-s3-bucket: staging: bucket_name: xyz-test-acme-widgets-ltd versioning: false - training: bucket_name: digital-workspace-v2-training + my-environment: + bucket_name: digital-workspace-v2-my-environment objects: - key: healthcheck.txt body: S3 Proxy is working. diff --git a/s3/locals.tf b/s3/locals.tf index abeb70990..35317ad1b 100644 --- a/s3/locals.tf +++ b/s3/locals.tf @@ -8,4 +8,6 @@ locals { } kms_alias_name = strcontains(var.config.bucket_name, "pipeline") ? "${var.config.bucket_name}-key" : "${var.application}-${var.environment}-${var.config.bucket_name}-key" + + has_data_migration_import_enabled = try(var.config.data_migration.import != null, false) } diff --git a/s3/main.tf b/s3/main.tf index 9b42ddba3..1d40da497 100644 --- a/s3/main.tf +++ b/s3/main.tf @@ -148,3 +148,19 @@ resource "aws_s3_bucket_public_access_block" "public_access_block" { ignore_public_acls = true restrict_public_buckets = true } + +module "data_migration" { + count = local.has_data_migration_import_enabled ? 1 : 0 + source = "../data-migration" + + depends_on = [ + aws_s3_bucket.this, + aws_kms_key.kms-key + ] + + config = var.config.data_migration.import + + destination_bucket_identifier = aws_s3_bucket.this.id + destination_kms_key_arn = aws_kms_key.kms-key.arn + destination_bucket_arn = aws_s3_bucket.this.arn +} diff --git a/s3/tests/unit.tftest.hcl b/s3/tests/unit.tftest.hcl index a6265e458..7c6759727 100644 --- a/s3/tests/unit.tftest.hcl +++ b/s3/tests/unit.tftest.hcl @@ -223,6 +223,45 @@ run "aws_s3_bucket_lifecycle_configuration_no_prefix_unit_test" { } } +run "aws_s3_bucket_data_migration_unit_test" { + command = plan + + variables { + config = { + "bucket_name" = "dbt-terraform-test-s3-cross-account", + "type" = "s3", + "data_migration" = { + "import" = { + "worker_role_arn" = "arn:aws:iam::1234:role/service-role/my-privileged-arn", + "source_kms_key_arn" = "arn:aws:iam::1234:my-external-kms-key-arn", + "source_bucket_arn" = "arn:aws:s3::1234:my-source-bucket" + } + } + } + } + + assert { + condition = module.data_migration[0].module_exists + error_message = "data migration module should be created" + } +} + +run "aws_s3_bucket_not_data_migration_unit_test" { + command = plan + + variables { + config = { + "bucket_name" = "dbt-terraform-test-s3-not-x-account", + "type" = "s3", + } + } + + assert { + condition = length(module.data_migration) == 0 + error_message = "data migration module should not be created" + } +} + run "aws_s3_bucket_object_lock_configuration_governance_unit_test" { command = plan diff --git a/s3/variables.tf b/s3/variables.tf index 81a85060e..6f1cfc090 100644 --- a/s3/variables.tf +++ b/s3/variables.tf @@ -42,5 +42,15 @@ variable "config" { body = string key = string }))) + + # S3 to S3 data migration + data_migration = optional(object({ + import = optional(object({ + source_bucket_arn = string + source_kms_key_arn = optional(string) + worker_role_arn = string + })) + }) + ) }) }