search

hashicorp / terraform-provider-google

Terraform Provider for Google Cloud Platform
https://registry.terraform.io/providers/hashicorp/google/latest/docs
Mozilla Public License 2.0
2.33k stars 1.73k forks source link

Bug in google_binary_authorization_policy can silently disable binauthz #14929

Open benhoskings opened 1 year ago

benhoskings commented 1 year ago

Community Note

Terraform Version

Terraform v1.4.2
on darwin_amd64
+ provider registry.terraform.io/datadog/datadog v3.24.0
+ provider registry.terraform.io/hashicorp/archive v2.3.0
+ provider registry.terraform.io/hashicorp/aws v4.65.0
+ provider registry.terraform.io/hashicorp/google v4.63.1
+ provider registry.terraform.io/hashicorp/google-beta v4.63.1
+ provider registry.terraform.io/hashicorp/local v2.4.0
+ provider registry.terraform.io/hashicorp/null v3.2.1
+ provider registry.terraform.io/hashicorp/random v3.5.1

Affected Resource(s)

google_binary_authorization_policy

Terraform Configuration Files

resource "google_binary_authorization_policy" "policy" {
  project = module.project.project_id
  description = "${module.project.project_id} binary authorization policy"
  global_policy_evaluation_mode = "ENABLE"

  default_admission_rule {
    enforcement_mode = "ENFORCED_BLOCK_AND_AUDIT_LOG"
    evaluation_mode = "REQUIRE_ATTESTATION"
    require_attestations_by = [
      "projects/binauthz-credentials/attestors/a",
      "projects/binauthz-credentials/attestors/b",
    ]
  }
}

The issue

We've hit a bug with the google_binary_authorization_policy resource. When adding entries to default_admission_rule.require_attestations_by, we've found that they replace existing entries rather than being created alongside them. This causes the existing binary authorization protections to be removed without any indication that it's happening.

This is a serious security issue. Applying a plan that appears correct will have the effect of silently disabling binary authorisation until the target is applied again.

This issue has been reported before here: https://github.com/hashicorp/terraform-provider-google/issues/9216 but it wasn't addressed or noticed as far as I can see, so I'm creating a fresh ticket with current details, logs, etc.

Reproduction

Specifically, we started with a policy containing two attestors, a and b, as shown above. When we add a third, c, to the list, we get the following plan, which is correct:

Terraform will perform the following actions:

  # module.runtime.google_binary_authorization_policy.policy will be updated in-place
  ~ resource "google_binary_authorization_policy" "policy" {
        id                            = "runtime-project"
        # (3 unchanged attributes hidden)

      ~ default_admission_rule {
          ~ require_attestations_by = [
              + "projects/credentials-project/attestors/c",
                # (2 unchanged elements hidden)
            ]
            # (2 unchanged attributes hidden)
        }
    }

Plan: 0 to add, 1 to change, 0 to destroy.

However, applying that plan generates the following request (provider.terraform-provider-google_v4.63.1_x5):

---[ REQUEST ]---------------------------------------
PUT /v1/projects/runtime-project/policy?alt=json HTTP/1.1
Host: binaryauthorization.googleapis.com
User-Agent: Terraform/1.4.2 (+https://www.terraform.io) Terraform-Plugin-SDK/2.10.1 terraform-provider-google/4.63.1
Content-Length: 368
Content-Type: application/json
Accept-Encoding: gzip
{
"clusterAdmissionRules": {},
"defaultAdmissionRule": {
 "enforcementMode": "ENFORCED_BLOCK_AND_AUDIT_LOG",
 "evaluationMode": "REQUIRE_ATTESTATION",
 "requireAttestationsBy": [
  "projects/credentials-project/attestors/c"
 ]
},
"description": "runtime-project binary authorization policy",
"globalPolicyEvaluationMode": "ENABLE"
}
-----------------------------------------------------

After applying this plan, the cloud console will show c has been added to the list but a and b have been removed.

(The resource names above are simulated but the details are accurate otherwise.)

edwardmedia commented 1 year ago

https://github.com/hashicorp/terraform-provider-google/issues/9216

ScottSuarez commented 1 year ago

Hey @melinath, passing this along. I tried to reproduce but couldn't the the deployment of google_container_analysis_note to work no matter what I tried.

I kept receiving the following error durring deployment

Error: Error creating Note: googleapi: Error 403: permission "containeranalysis.notes.create" denied for project "my-project", entity ID ""
│ 
│   with google_container_analysis_note.note,
│   on main.tf line 21, in resource "google_container_analysis_note" "note":
│   21: resource "google_container_analysis_note" "note" {

provider "google-local" {
 project        = "my-project"
}

resource "google_container_analysis_note" "note" {
  provider = google-local
  name = "tf-test-1"
  attestation_authority {
    hint {
      human_readable_name = "My Attestor"
    }
  }
}

resource "google_binary_authorization_attestor" "attestor" {
  provider = google-local
  name = "tf-test-11"
  attestation_authority_note {
    note_reference = google_container_analysis_note.note.name
  }
}

resource "google_binary_authorization_policy" "policy" {
  provider = google-local
  admission_whitelist_patterns {
    name_pattern = "gcr.io/google_containers/*"
  }

  default_admission_rule {
    evaluation_mode  = "ALWAYS_ALLOW"
    enforcement_mode = "ENFORCED_BLOCK_AND_AUDIT_LOG"
  }

  cluster_admission_rules {
    cluster                 = "us-central1-a.prod-cluster"
    evaluation_mode         = "REQUIRE_ATTESTATION"
    enforcement_mode        = "ENFORCED_BLOCK_AND_AUDIT_LOG"
    require_attestations_by = [google_binary_authorization_attestor.attestor.name]
  }

  global_policy_evaluation_mode = "ENABLE"
}
edwardmedia commented 1 year ago

@benhoskings using below config, I tested by adding more entries in require_attestations_by. It seems working fine. Can you try my config to see if that works for you? Also mind sharing your full debug log so I can take a closer look?

resource "google_binary_authorization_policy" "policy" {
  admission_whitelist_patterns {
    name_pattern = "gcr.io/google_containers/*"
  }

  default_admission_rule {
    evaluation_mode  = "ALWAYS_ALLOW"
    enforcement_mode = "ENFORCED_BLOCK_AND_AUDIT_LOG"
  }

  cluster_admission_rules {
    cluster                 = "us-central1-a.prod-cluster"
    evaluation_mode         = "REQUIRE_ATTESTATION"
    enforcement_mode        = "ENFORCED_BLOCK_AND_AUDIT_LOG"
    require_attestations_by = [google_binary_authorization_attestor.attestor2.name, 
                               google_binary_authorization_attestor.attestor.name,
                               google_binary_authorization_attestor.attestor3.name,
                            ]
  }
}

resource "google_container_analysis_note" "note" {
  name = "issue9216"
  attestation_authority {
    hint {
      human_readable_name = "My attestor"
    }
  }
}

resource "google_binary_authorization_attestor" "attestor" {
  name = "issue9216"
  attestation_authority_note {
    note_reference = google_container_analysis_note.note.name
  }
}

resource "google_binary_authorization_attestor" "attestor2" {
  name = "issue9216-2"
  attestation_authority_note {
    note_reference = google_container_analysis_note.note.name
  }
}

resource "google_binary_authorization_attestor" "attestor3" {
  name = "issue9216-3"
  attestation_authority_note {
    note_reference = google_container_analysis_note.note.name
  }
}

In my case, I see below in the plan which is different from yours. 

      - cluster_admission_rules {
          - cluster                 = "us-central1-a.prod-cluster" -> null
          - enforcement_mode        = "ENFORCED_BLOCK_AND_AUDIT_LOG" -> null
          - evaluation_mode         = "REQUIRE_ATTESTATION" -> null
          - require_attestations_by = [
              - "projects/myproject/attestors/issue9216",
              - "projects/myproject/attestors/issue9216-2",
            ] -> null
        }
      + cluster_admission_rules {
          + cluster                 = "us-central1-a.prod-cluster"
          + enforcement_mode        = "ENFORCED_BLOCK_AND_AUDIT_LOG"
          + evaluation_mode         = "REQUIRE_ATTESTATION"
          + require_attestations_by = [
              + "issue9216",
              + "issue9216-2",
              + "issue9216-3",
            ]
        }

I have tested the same version of the provider as what you have (v4.63.1), same result.

Even if I use direct attestor id (below) instead of the reference, I still see the similar plan which is replacement on cluster_admission_rules

    require_attestations_by = ["projects/myproject/attestors/issue9216-2", 
                             "projects/myproject/attestors/issue9216",
                              "projects/myproject/attestors/issue9216-3",
                            ]

I wonder how to repro your plan

edwardmedia commented 1 year ago

@benhoskings using your config, it doesn't work for me as the cluster is required. Did you hit that error?

edwardmedia commented 1 year ago

@benhoskings my terraform version was 1.5.1. I have also tried v1.4.5. The result is the same in which replacement is planned on cluster_admission_rules

Can you share your debug log?

benhoskings commented 1 year ago

@edwardmedia Our config uses the default_admission_rule block; perhaps the bug is present there but not in cluster_admission_rules.

it doesn't work for me as the cluster is required

That's a required field within the cluster_admission_rules block but the block itself is optional.

melinath commented 1 year ago

I can confirm that this behavior exists. Specifically, adding an attestor to require_attestations_by works fine inside cluster_admission_rules but suppresses the previous value for default_admission_rule. Sample API request:

PUT /v1/projects/project-id/policy?alt=json HTTP/1.1

{
 "admissionWhitelistPatterns": [
  {
   "namePattern": "gcr.io/google_containers/*"
  }
 ],
 "clusterAdmissionRules": {
  "us-central1-a.prod-cluster": {
   "enforcementMode": "ENFORCED_BLOCK_AND_AUDIT_LOG",
   "evaluationMode": "REQUIRE_ATTESTATION",
   "requireAttestationsBy": [
    "projects/project-id/attestors/tf-test-myqv9m2flk",
    "projects/project-id/attestors/tf-test-myqv9m2flk2"
   ]
  }
 },
 "defaultAdmissionRule": {
  "enforcementMode": "ENFORCED_BLOCK_AND_AUDIT_LOG",
  "evaluationMode": "REQUIRE_ATTESTATION",
  "requireAttestationsBy": [
   "projects/project-id/attestors/tf-test-myqv9m2flk2"
  ]
 },
 "globalPolicyEvaluationMode": "DISABLE"
}
trodge commented 1 year ago

I found that the resource's update method is getting called with a set built from here, which only contains the new elements and not all elements in the config. I think this is a bug with the SDK.

It's also possible that https://github.com/hashicorp/terraform-plugin-sdk/issues/157 is related.

jeespers commented 10 months ago

I can also confirm the behaviour.

Starting with a policy requiring the foo-attestor and trying to add two more bar-attestor and baz-attestor removed the previously existing foo-attestor from the policy.

resource "google_binary_authorization_policy" "policy" {

  default_admission_rule {
    evaluation_mode         = "REQUIRE_ATTESTATION"
    enforcement_mode        = "ENFORCED_BLOCK_AND_AUDIT_LOG"
    require_attestations_by = [
      "projects/sdbx/attestors/foo-attestor", 

      "projects/sdbx/attestors/bar-attestor",    # added this attestor
      "projects/sdbx-2/attestors/baz-attestor",  # added this attestor
    ]
  }

  project = var.project_id
}

But the plan looks like this

Terraform will perform the following actions:

  # google_binary_authorization_policy.policy will be updated in-place
  ~ resource "google_binary_authorization_policy" "policy" {
        id      = "projects/sdbx"
        # (1 unchanged attribute hidden)

      ~ default_admission_rule {
          ~ require_attestations_by = [
              - "projects/sdbx/attestors/foo-attestor",
              + "projects/sdbx/attestors/bar-attestor",
              + "projects/sdbx-2/attestors/baz-attestor",
            ]
            # (2 unchanged attributes hidden)
        }
    }

Plan: 0 to add, 1 to change, 0 to destroy.

Updating the resource to look like this (missing the foo-attestor)

$ gcloud container binauthz policy export                

defaultAdmissionRule:
  enforcementMode: ENFORCED_BLOCK_AND_AUDIT_LOG
  evaluationMode: REQUIRE_ATTESTATION
  requireAttestationsBy:
  - projects/sdbx-2/attestors/baz-attestor
  - projects/sdbx/attestors/bar-attestor