From e96a3a72491c8928c6d8c3a0b9076ca004940035 Mon Sep 17 00:00:00 2001 From: Dana Hoffman Date: Tue, 15 Aug 2017 14:59:25 -0700 Subject: [PATCH 1/3] Fix bug with CSEK where the key stored in state might be associated with the wrong disk --- google/resource_compute_instance.go | 62 +++++++++-- google/resource_compute_instance_test.go | 127 ++++++++++++++++++++--- 2 files changed, 162 insertions(+), 27 deletions(-) diff --git a/google/resource_compute_instance.go b/google/resource_compute_instance.go index 6f723d23626..f98399369f7 100644 --- a/google/resource_compute_instance.go +++ b/google/resource_compute_instance.go @@ -1,15 +1,18 @@ package google import ( + "crypto/sha256" + "encoding/base64" "fmt" "log" "strings" + "regexp" + "github.com/hashicorp/terraform/helper/schema" "github.com/hashicorp/terraform/helper/validation" "google.golang.org/api/compute/v1" "google.golang.org/api/googleapi" - "regexp" ) func stringScopeHashcode(v interface{}) int { @@ -1009,7 +1012,6 @@ func resourceComputeInstanceRead(d *schema.ResourceData, meta interface{}) error } dIndex := 0 - adIndex := 0 sIndex := 0 disks := make([]map[string]interface{}, 0, disksCount) attachedDisks := make([]map[string]interface{}, 0, attachedDisksCount) @@ -1035,24 +1037,55 @@ func resourceComputeInstanceRead(d *schema.ResourceData, meta interface{}) error "device_name": d.Get(fmt.Sprintf("disk.%d.device_name", dIndex)), "disk_encryption_key_raw": d.Get(fmt.Sprintf("disk.%d.disk_encryption_key_raw", dIndex)), } - if disk.DiskEncryptionKey != nil && disk.DiskEncryptionKey.Sha256 != "" { - di["disk_encryption_key_sha256"] = disk.DiskEncryptionKey.Sha256 + if d.Get(fmt.Sprintf("disk.%d.disk_encryption_key_raw", dIndex)) != "" { + sha, err := hash256(d.Get(fmt.Sprintf("disk.%d.disk_encryption_key_raw", dIndex)).(string)) + if err != nil { + return err + } + di["disk_encryption_key_sha256"] = sha } disks = append(disks, di) dIndex++ } else { + if disk.DiskEncryptionKey != nil { + // we'll handle these specifically later + continue + } di := map[string]interface{}{ - "source": disk.Source, - "device_name": disk.DeviceName, - "disk_encryption_key_raw": d.Get(fmt.Sprintf("attached_disk.%d.disk_encryption_key_raw", adIndex)), + "source": disk.Source, + "device_name": disk.DeviceName, + } + attachedDisks = append(attachedDisks, di) + } + } + + allEncryptedDisks := make(map[string]*compute.AttachedDisk) + for _, disk := range instance.Disks { + if disk.DiskEncryptionKey != nil { + allEncryptedDisks[disk.DiskEncryptionKey.Sha256] = disk + } + } + + for i := 0; i < attachedDisksCount; i++ { + if v := d.Get(fmt.Sprintf("attached_disk.%d.disk_encryption_key_raw", i)); v != "" { + hash, err := hash256(v.(string)) + if err != nil { + return err } - if disk.DiskEncryptionKey != nil && disk.DiskEncryptionKey.Sha256 != "" { - di["disk_encryption_key_sha256"] = disk.DiskEncryptionKey.Sha256 + disk := allEncryptedDisks[hash] + if disk == nil { + return fmt.Errorf("Could not find disk with encryption key hash %q", hash) + } + di := map[string]interface{}{ + "source": disk.Source, + "device_name": disk.DeviceName, + "disk_encryption_key_raw": v.(string), + "disk_encryption_key_sha256": hash, } attachedDisks = append(attachedDisks, di) - adIndex++ } } + d.Set("disk", disks) d.Set("attached_disk", attachedDisks) d.Set("scratch_disk", scratchDisks) @@ -1451,3 +1484,12 @@ func getProjectFromSubnetworkLink(subnetwork string) string { return r.FindStringSubmatch(subnetwork)[1] } + +func hash256(raw string) (string, error) { + decoded, err := base64.StdEncoding.DecodeString(raw) + if err != nil { + return "", err + } + h := sha256.Sum256(decoded) + return base64.StdEncoding.EncodeToString(h[:]), nil +} diff --git a/google/resource_compute_instance_test.go b/google/resource_compute_instance_test.go index 31c81144e6c..65cdde199ec 100644 --- a/google/resource_compute_instance_test.go +++ b/google/resource_compute_instance_test.go @@ -4,6 +4,7 @@ import ( "fmt" "os" "regexp" + "strconv" "strings" "testing" @@ -224,7 +225,22 @@ func TestAccComputeInstance_deprecated_disksWithAutodelete(t *testing.T) { func TestAccComputeInstance_diskEncryption(t *testing.T) { var instance compute.Instance var instanceName = fmt.Sprintf("instance-test-%s", acctest.RandString(10)) - var diskName = fmt.Sprintf("instance-testd-%s", acctest.RandString(10)) + bootEncryptionKey := "SGVsbG8gZnJvbSBHb29nbGUgQ2xvdWQgUGxhdGZvcm0=" + bootEncryptionKeyHash := "esTuF7d4eatX4cnc4JsiEiaI+Rff78JgPhA/v1zxX9E=" + diskNameToEncryptionKey := map[string]*compute.CustomerEncryptionKey{ + fmt.Sprintf("instance-testd-%s", acctest.RandString(10)): { + RawKey: "Ym9vdDU2Nzg5MDEyMzQ1Njc4OTAxMjM0NTY3ODkwMTI=", + Sha256: "awJ7p57H+uVZ9axhJjl1D3lfC2MgA/wnt/z88Ltfvss=", + }, + fmt.Sprintf("instance-testd-%s", acctest.RandString(10)): { + RawKey: "c2Vjb25kNzg5MDEyMzQ1Njc4OTAxMjM0NTY3ODkwMTI=", + Sha256: "7TpIwUdtCOJpq2m+3nt8GFgppu6a2Xsj1t0Gexk13Yc=", + }, + fmt.Sprintf("instance-testd-%s", acctest.RandString(10)): { + RawKey: "dGhpcmQ2Nzg5MDEyMzQ1Njc4OTAxMjM0NTY3ODkwMTI=", + Sha256: "b3pvaS7BjDbCKeLPPTx7yXBuQtxyMobCHN1QJR43xeM=", + }, + } resource.Test(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, @@ -232,13 +248,11 @@ func TestAccComputeInstance_diskEncryption(t *testing.T) { CheckDestroy: testAccCheckComputeInstanceDestroy, Steps: []resource.TestStep{ resource.TestStep{ - Config: testAccComputeInstance_disks_encryption(diskName, instanceName), + Config: testAccComputeInstance_disks_encryption(bootEncryptionKey, diskNameToEncryptionKey, instanceName), Check: resource.ComposeTestCheckFunc( testAccCheckComputeInstanceExists( "google_compute_instance.foobar", &instance), - testAccCheckComputeInstanceDisk(&instance, instanceName, true, true), - testAccCheckComputeInstanceDisk(&instance, diskName, true, false), - testAccCheckComputeInstanceDiskEncryptionKey("google_compute_instance.foobar", &instance), + testAccCheckComputeInstanceDiskEncryptionKey("google_compute_instance.foobar", &instance, bootEncryptionKeyHash, diskNameToEncryptionKey), ), }, }, @@ -910,7 +924,7 @@ func testAccCheckComputeInstanceScratchDisk(instance *compute.Instance, interfac } } -func testAccCheckComputeInstanceDiskEncryptionKey(n string, instance *compute.Instance) resource.TestCheckFunc { +func testAccCheckComputeInstanceDiskEncryptionKey(n string, instance *compute.Instance, bootDiskEncryptionKey string, diskNameToEncryptionKey map[string]*compute.CustomerEncryptionKey) resource.TestCheckFunc { return func(s *terraform.State) error { rs, ok := s.RootModule().Resources[n] if !ok { @@ -918,16 +932,56 @@ func testAccCheckComputeInstanceDiskEncryptionKey(n string, instance *compute.In } for i, disk := range instance.Disks { - attr := rs.Primary.Attributes[fmt.Sprintf("disk.%d.disk_encryption_key_sha256", i)] - if attr == "" && disk.Boot { - attr = rs.Primary.Attributes["boot_disk.0.disk_encryption_key_sha256"] + if disk.Boot { + attr := rs.Primary.Attributes["boot_disk.0.disk_encryption_key_sha256"] + if attr == "" { + attr = rs.Primary.Attributes[fmt.Sprintf("disk.%d.disk_encryption_key_sha256", i)] + } + if attr != bootDiskEncryptionKey { + return fmt.Errorf("Boot disk has wrong encryption key in state.\nExpected: %s\nActual: %s", bootDiskEncryptionKey, attr) + } + if disk.DiskEncryptionKey == nil && attr != "" { + return fmt.Errorf("Disk %d has mismatched encryption key.\nTF State: %+v\nGCP State: ", i, attr) + } + if disk.DiskEncryptionKey != nil && attr != disk.DiskEncryptionKey.Sha256 { + return fmt.Errorf("Disk %d has mismatched encryption key.\nTF State: %+v\nGCP State: %+v", + i, attr, disk.DiskEncryptionKey.Sha256) + } + } else { + if disk.DiskEncryptionKey != nil { + sourceUrl := strings.Split(disk.Source, "/") + expectedKey := diskNameToEncryptionKey[sourceUrl[len(sourceUrl)-1]].Sha256 + if disk.DiskEncryptionKey.Sha256 != expectedKey { + return fmt.Errorf("Disk %d has unexpected encryption key in GCP.\nExpected: %s\nActual: %s", i, expectedKey, disk.DiskEncryptionKey.Sha256) + } + } } - if disk.DiskEncryptionKey == nil && attr != "" { - return fmt.Errorf("Disk %d has mismatched encryption key.\nTF State: %+v\nGCP State: ", i, attr) + } + + numDisks, err := strconv.Atoi(rs.Primary.Attributes["disk.#"]) + if err != nil { + return fmt.Errorf("Error converting value of disk.#") + } + for i := 0; i < numDisks; i++ { + diskName := rs.Primary.Attributes[fmt.Sprintf("disk.%d.disk", i)] + encryptionKey := rs.Primary.Attributes[fmt.Sprintf("disk.%d.disk_encryption_key_sha256", i)] + expectedEncryptionKey := diskNameToEncryptionKey[diskName].Sha256 + if encryptionKey != expectedEncryptionKey { + return fmt.Errorf("Disk %d has unexpected encryption key in state.\nExpected: %s\nActual: %s", i, expectedEncryptionKey, encryptionKey) } - if disk.DiskEncryptionKey != nil && attr != disk.DiskEncryptionKey.Sha256 { - return fmt.Errorf("Disk %d has mismatched encryption key.\nTF State: %+v\nGCP State: %+v", - i, attr, disk.DiskEncryptionKey.Sha256) + } + + numAttachedDisks, err := strconv.Atoi(rs.Primary.Attributes["attached_disk.#"]) + if err != nil { + return fmt.Errorf("Error converting value of attached_disk.#") + } + for i := 0; i < numAttachedDisks; i++ { + diskSourceUrl := strings.Split(rs.Primary.Attributes[fmt.Sprintf("attached_disk.%d.source", i)], "/") + diskName := diskSourceUrl[len(diskSourceUrl)-1] + encryptionKey := rs.Primary.Attributes[fmt.Sprintf("attached_disk.%d.disk_encryption_key_sha256", i)] + expectedEncryptionKey := diskNameToEncryptionKey[diskName].Sha256 + if encryptionKey != expectedEncryptionKey { + return fmt.Errorf("Attached disk %d has unexpected encryption key in state.\nExpected: %s\nActual: %s", i, expectedEncryptionKey, encryptionKey) } } return nil @@ -1352,13 +1406,37 @@ resource "google_compute_instance" "foobar" { `, disk, instance, autodelete) } -func testAccComputeInstance_disks_encryption(disk, instance string) string { +func testAccComputeInstance_disks_encryption(bootEncryptionKey string, diskNameToEncryptionKey map[string]*compute.CustomerEncryptionKey, instance string) string { + diskNames := []string{} + for k, _ := range diskNameToEncryptionKey { + diskNames = append(diskNames, k) + } return fmt.Sprintf(` resource "google_compute_disk" "foobar" { name = "%s" size = 10 type = "pd-ssd" zone = "us-central1-a" + + disk_encryption_key_raw = "%s" +} + +resource "google_compute_disk" "foobar2" { + name = "%s" + size = 10 + type = "pd-ssd" + zone = "us-central1-a" + + disk_encryption_key_raw = "%s" +} + +resource "google_compute_disk" "foobar3" { + name = "%s" + size = 10 + type = "pd-ssd" + zone = "us-central1-a" + + disk_encryption_key_raw = "%s" } resource "google_compute_instance" "foobar" { @@ -1370,11 +1448,22 @@ resource "google_compute_instance" "foobar" { initialize_params{ image = "debian-8-jessie-v20160803" } - disk_encryption_key_raw = "SGVsbG8gZnJvbSBHb29nbGUgQ2xvdWQgUGxhdGZvcm0=" + disk_encryption_key_raw = "%s" } disk { disk = "${google_compute_disk.foobar.name}" + disk_encryption_key_raw = "%s" + } + + attached_disk { + source = "${google_compute_disk.foobar2.self_link}" + disk_encryption_key_raw = "%s" + } + + attached_disk { + source = "${google_compute_disk.foobar3.self_link}" + disk_encryption_key_raw = "%s" } network_interface { @@ -1385,7 +1474,11 @@ resource "google_compute_instance" "foobar" { foo = "bar" } } -`, disk, instance) +`, diskNames[0], diskNameToEncryptionKey[diskNames[0]].RawKey, + diskNames[1], diskNameToEncryptionKey[diskNames[1]].RawKey, + diskNames[2], diskNameToEncryptionKey[diskNames[2]].RawKey, + instance, bootEncryptionKey, + diskNameToEncryptionKey[diskNames[0]].RawKey, diskNameToEncryptionKey[diskNames[1]].RawKey, diskNameToEncryptionKey[diskNames[2]].RawKey) } func testAccComputeInstance_attachedDisk(disk, instance string) string { From 6f7bf98ebdd3c773651f361607450371af864380 Mon Sep 17 00:00:00 2001 From: Dana Hoffman Date: Wed, 16 Aug 2017 12:58:23 -0700 Subject: [PATCH 2/3] preserve original order of attached disks --- google/resource_compute_instance.go | 11 ++++++----- google/resource_compute_instance_test.go | 20 +++++++++++++++++--- 2 files changed, 23 insertions(+), 8 deletions(-) diff --git a/google/resource_compute_instance.go b/google/resource_compute_instance.go index f98399369f7..b1bf7ee07cc 100644 --- a/google/resource_compute_instance.go +++ b/google/resource_compute_instance.go @@ -1006,15 +1006,15 @@ func resourceComputeInstanceRead(d *schema.ResourceData, meta interface{}) error return fmt.Errorf("Expected %d disks, API returned %d", expectedDisks, len(instance.Disks)) } - attachedDiskSources := make(map[string]struct{}, attachedDisksCount) + attachedDiskSources := make(map[string]int, attachedDisksCount) for i := 0; i < attachedDisksCount; i++ { - attachedDiskSources[d.Get(fmt.Sprintf("attached_disk.%d.source", i)).(string)] = struct{}{} + attachedDiskSources[d.Get(fmt.Sprintf("attached_disk.%d.source", i)).(string)] = i } dIndex := 0 sIndex := 0 disks := make([]map[string]interface{}, 0, disksCount) - attachedDisks := make([]map[string]interface{}, 0, attachedDisksCount) + attachedDisks := make([]map[string]interface{}, attachedDisksCount) scratchDisks := make([]map[string]interface{}, 0, scratchDisksCount) for _, disk := range instance.Disks { if _, ok := d.GetOk("boot_disk"); ok && disk.Boot { @@ -1055,7 +1055,8 @@ func resourceComputeInstanceRead(d *schema.ResourceData, meta interface{}) error "source": disk.Source, "device_name": disk.DeviceName, } - attachedDisks = append(attachedDisks, di) + adIndex := attachedDiskSources[disk.Source] + attachedDisks[adIndex] = di } } @@ -1082,7 +1083,7 @@ func resourceComputeInstanceRead(d *schema.ResourceData, meta interface{}) error "disk_encryption_key_raw": v.(string), "disk_encryption_key_sha256": hash, } - attachedDisks = append(attachedDisks, di) + attachedDisks[i] = di } } diff --git a/google/resource_compute_instance_test.go b/google/resource_compute_instance_test.go index 65cdde199ec..5fe94bd57ab 100644 --- a/google/resource_compute_instance_test.go +++ b/google/resource_compute_instance_test.go @@ -979,9 +979,11 @@ func testAccCheckComputeInstanceDiskEncryptionKey(n string, instance *compute.In diskSourceUrl := strings.Split(rs.Primary.Attributes[fmt.Sprintf("attached_disk.%d.source", i)], "/") diskName := diskSourceUrl[len(diskSourceUrl)-1] encryptionKey := rs.Primary.Attributes[fmt.Sprintf("attached_disk.%d.disk_encryption_key_sha256", i)] - expectedEncryptionKey := diskNameToEncryptionKey[diskName].Sha256 - if encryptionKey != expectedEncryptionKey { - return fmt.Errorf("Attached disk %d has unexpected encryption key in state.\nExpected: %s\nActual: %s", i, expectedEncryptionKey, encryptionKey) + if key, ok := diskNameToEncryptionKey[diskName]; ok { + expectedEncryptionKey := key.Sha256 + if encryptionKey != expectedEncryptionKey { + return fmt.Errorf("Attached disk %d has unexpected encryption key in state.\nExpected: %s\nActual: %s", i, expectedEncryptionKey, encryptionKey) + } } } return nil @@ -1439,6 +1441,13 @@ resource "google_compute_disk" "foobar3" { disk_encryption_key_raw = "%s" } +resource "google_compute_disk" "foobar4" { + name = "%s" + size = 10 + type = "pd-ssd" + zone = "us-central1-a" +} + resource "google_compute_instance" "foobar" { name = "%s" machine_type = "n1-standard-1" @@ -1461,6 +1470,10 @@ resource "google_compute_instance" "foobar" { disk_encryption_key_raw = "%s" } + attached_disk { + source = "${google_compute_disk.foobar4.self_link}" + } + attached_disk { source = "${google_compute_disk.foobar3.self_link}" disk_encryption_key_raw = "%s" @@ -1477,6 +1490,7 @@ resource "google_compute_instance" "foobar" { `, diskNames[0], diskNameToEncryptionKey[diskNames[0]].RawKey, diskNames[1], diskNameToEncryptionKey[diskNames[1]].RawKey, diskNames[2], diskNameToEncryptionKey[diskNames[2]].RawKey, + "instance-testd-"+acctest.RandString(10), instance, bootEncryptionKey, diskNameToEncryptionKey[diskNames[0]].RawKey, diskNameToEncryptionKey[diskNames[1]].RawKey, diskNameToEncryptionKey[diskNames[2]].RawKey) } From a990b201bca7a405bd4e33ba9b53ba3e508ee34f Mon Sep 17 00:00:00 2001 From: Dana Hoffman Date: Wed, 16 Aug 2017 15:30:43 -0700 Subject: [PATCH 3/3] use the disk index to figure out the raw key --- google/resource_compute_instance.go | 37 ++++------------------------- 1 file changed, 5 insertions(+), 32 deletions(-) diff --git a/google/resource_compute_instance.go b/google/resource_compute_instance.go index b1bf7ee07cc..eda65ad4522 100644 --- a/google/resource_compute_instance.go +++ b/google/resource_compute_instance.go @@ -1047,43 +1047,16 @@ func resourceComputeInstanceRead(d *schema.ResourceData, meta interface{}) error disks = append(disks, di) dIndex++ } else { - if disk.DiskEncryptionKey != nil { - // we'll handle these specifically later - continue - } + adIndex := attachedDiskSources[disk.Source] di := map[string]interface{}{ "source": disk.Source, "device_name": disk.DeviceName, } - adIndex := attachedDiskSources[disk.Source] - attachedDisks[adIndex] = di - } - } - - allEncryptedDisks := make(map[string]*compute.AttachedDisk) - for _, disk := range instance.Disks { - if disk.DiskEncryptionKey != nil { - allEncryptedDisks[disk.DiskEncryptionKey.Sha256] = disk - } - } - - for i := 0; i < attachedDisksCount; i++ { - if v := d.Get(fmt.Sprintf("attached_disk.%d.disk_encryption_key_raw", i)); v != "" { - hash, err := hash256(v.(string)) - if err != nil { - return err - } - disk := allEncryptedDisks[hash] - if disk == nil { - return fmt.Errorf("Could not find disk with encryption key hash %q", hash) - } - di := map[string]interface{}{ - "source": disk.Source, - "device_name": disk.DeviceName, - "disk_encryption_key_raw": v.(string), - "disk_encryption_key_sha256": hash, + if key := disk.DiskEncryptionKey; key != nil { + di["disk_encryption_key_raw"] = d.Get(fmt.Sprintf("attached_disk.%d.disk_encryption_key_raw", adIndex)) + di["disk_encryption_key_sha256"] = key.Sha256 } - attachedDisks[i] = di + attachedDisks[adIndex] = di } }