Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GCP subnet share conditions not working correctly #152

Closed
sandhyajob opened this issue Feb 27, 2019 · 7 comments
Closed

GCP subnet share conditions not working correctly #152

sandhyajob opened this issue Feb 27, 2019 · 7 comments
Assignees
Labels
bug Something isn't working

Comments

@sandhyajob
Copy link

Based on the default 3 conditions

  1. No shared_vpc and no subnets => no grants
  2. shared_vpc and no subnets => grant to the project (all networks/subnets)
  3. shared_vpc and subnets => grant to the subnets (and not to the project)

If the subnet id get messed up ( fat finger) while attempting case 3, project factory ends up sharing the entire set of networks in the host project VPC to the service project. Which is a serious security issue. Ideally it should follow step 1 and should not grant any thing and error out.

@leone145
Copy link

When variable vpc_networks is incorrectly formed the failure happens and leaves all subnets shared
Correct formatting and functional
shared_vpc_subnets: '["projects/vpc-abc/regions/europe-west3/subnetworks/subnet1"]'

Incorrect formatting and error observed below
shared_vpc_subnets: '["projects/vpc-abc/regions/europe-west3/subnet1"]'

* google_compute_subnetwork_iam_member.service_account_role_to_vpc_subnets: Error retrieving IAM policy for Compute Subnetwork non-prod-host/europe-west3/projects: googleapi: Error 404: The resource 'projects/non-prod-host/regions/europe-west3/subnetworks/projects' was not found, notFound

* module.core-factory.google_compute_subnetwork_iam_member.apis_service_account_role_to_vpc_subnets: 1 error(s) occurred:

* google_compute_subnetwork_iam_member.apis_service_account_role_to_vpc_subnets: Error retrieving IAM policy for Compute Subnetwork non-prod-host/europe-west3/projects: googleapi: Error 404: The resource 'projects/non-prod-host/regions/europe-west3/subnetworks/projects' was not found, notFound

@morgante
Copy link
Contributor

#97 is related.

@glarizza
Copy link
Contributor

glarizza commented Mar 4, 2019

To add some more context here, these IAM resources are using a split() on the shared_vpc_subnets array to populate the subnetwork attribute on the subnet IAM member resources: https://github.com/terraform-google-modules/terraform-google-project-factory/blob/master/modules/core_project_factory/main.tf#L219-L265 If you pass in an element to that array that is't properly formatted with projects/<project_name>/regions/<region>/subnetworks/<subnetwork> then the element() function will return an empty string, and that's what gets passed to the IAM membership resource. Passing an empty string is the same as if you didn't pass it, and so that's why permissions are granted at the project and not the subnetwork level.

This will also happen if you pass the subnets_self_links output to var.shared_vpc_subnets (which is what this PR is referencing: terraform-google-modules/terraform-google-network#33 ).

If we're going to continue to use the projects/<project_name>/regions/<region>/subnetworks/<subnetwork> format we might want to check for the strings projects, regions, and subnetworks being in the correct places for each element in the array as something of a preflight check?

@kopachevsky kopachevsky self-assigned this May 1, 2019
@kopachevsky
Copy link

kopachevsky commented May 6, 2019

@aaron-lane @morgante
how do you think what is the best option for validation, we have 2 possible points:

  1. terraform-google-project-factory/modules/core_project_factory/scripts/preconditions/preconditions.py, which is not good option as for me
  2. do it in terraform module, here is the code I would solve this:
variable "shared_vpc_subnets" {
  description = "List of subnets fully qualified subnet IDs (ie. projects/$project_id/regions/$region/subnetworks/$subnet_id)"
  type        = "list"
  default     = [
    "http://projects/base-project-196723/regions/us-east1/subnetworks/default",
    "projects/base-project-196723/regions/us-central1/subnetworks/default",
    "XXXX/base-project-196723/regions/us-central1/subnetworks/subnet-1",
  ]
}

resource "null_resource" "invalid_subnets" {
  count = "${length(var.shared_vpc_subnets)}"
  triggers = {
    subnet  = "${replace(var.shared_vpc_subnets[count.index],
    "/(https://www.googleapis.com/compute/v1/)?projects/[a-z0-9-]+/regions/[a-z0-9-]+/subnetworks/[a-z0-9-]+/", "") == "" ? false : true}"
  }
}

locals {
    invalid_subnets  = "${null_resource.invalid_subnets.*.triggers.subnet}"
}

resource "null_resource" "valid_subnet" {
  count = "${length(var.shared_vpc_subnets)}"
  triggers = {
    subnet = "${local.invalid_subnets[count.index] ? "dummy-subnet" : var.shared_vpc_subnets[count.index]}"
  }
}

output "valid_subnets" {
  value       = "${null_resource.valid_subnet.*.triggers.subnet}"
}

It will give following output:

valid_subnets = [
    dummy-subnet,
    projects/base-project-196723/regions/us-central1/subnetworks/default,
    dummy-subnet
]

which cause fail for incorrect subnet item

@aaron-lane aaron-lane added the bug Something isn't working label May 7, 2019
@aaron-lane
Copy link
Contributor

@kopachevsky this should indeed be solved in the Terraform configuration; preconditions.py is used to verify the presence of required permissions.

If we are relying on the google_subnet_iam_member resources to throw errors when invalid subnets are provided then we should choose a more descriptive string than simply "dummy-subnet". Alternatively, we could use one of the solutions from this thread to raise a descriptive error.

@kopachevsky
Copy link

kopachevsky commented May 8, 2019

@aaron-lane I agree "dummy-subnet" not good way to point user with the problem it his config, I've tried options form issue you mentioned above initially, for some reason it wont work for me, I even raised stackoverflow question https://stackoverflow.com/questions/56042077/terraform-v0-11-xx-null-resource-not-always-works-as-assertion

kopachevsky pushed a commit to kopachevsky/terraform-google-project-factory that referenced this issue May 8, 2019
…rking correctly

- added regexp based subnet name validation
- added relative region and subnet values extractions from source string
kopachevsky pushed a commit to kopachevsky/terraform-google-project-factory that referenced this issue May 8, 2019
kopachevsky pushed a commit to kopachevsky/terraform-google-project-factory that referenced this issue May 9, 2019
…rking correctly. - fixed subnet region to available
kopachevsky pushed a commit to kopachevsky/terraform-google-project-factory that referenced this issue May 10, 2019
…rking correctly

- removed typos
- removed meta programming from tests
kopachevsky pushed a commit to kopachevsky/terraform-google-project-factory that referenced this issue May 10, 2019
…rking correctly

- fixed local variables initialization from attributes
@morgante
Copy link
Contributor

@kopachevsky As an incremental fix, please provide a PR which only automatically strips the https://www.googleapis.com/compute/v1/ prefix. This should be very straightforward and would immediately fix terraform-google-modules/terraform-google-network#33

kopachevsky pushed a commit to kopachevsky/terraform-google-project-factory that referenced this issue May 14, 2019
…rking correctly

- temporary commit to check early fail if shared vpc subnet name not valid (HTTPS changed to HTTP)
- revert after check
kopachevsky pushed a commit to kopachevsky/terraform-google-project-factory that referenced this issue May 14, 2019
…rking correctly

- revert: temporary commit to check early fail if shared vpc subnet name not valid (HTTPS changed to HTTP)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

6 participants