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.

Sign up for GitHub

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

Fix bug with CSEK where the key stored in state might be associated with the wrong disk #327

Merged
merged 4 commits into from
Sep 7, 2017
Merged

Fix bug with CSEK where the key stored in state might be associated with the wrong disk #327

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e96a3a7
Fix bug with CSEK where the key stored in state might be associated w…
danawillow Aug 15, 2017
6f7bf98
preserve original order of attached disks
danawillow Aug 16, 2017
a990b20
use the disk index to figure out the raw key
danawillow Aug 16, 2017
7ec1c94
Merge branch 'master' into encryption-key
danawillow Sep 7, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 30 additions & 14 deletions google/resource_compute_instance.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 {
Expand Down Expand Up @@ -1003,16 +1006,15 @@ func resourceComputeInstanceRead(d *schema.ResourceData, meta interface{}) error
return fmt.Errorf("Expected %d disks, API returned %d", expectedDisks, len(instance.Disks))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be outside the scope of this PR but if we add or remove a disk outside of Terraform, then this line fails preventing the user from updating the instance.

Is this a behavior we want to fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not right now- because we're relying so much on what's written in state, we can't really support that case without a fair amount of effort. It'll probably be easier once disks is gone for good, so it'll probably be worth a shot then.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think even with a fair amount of effort, we might not be able to support that case. :/ I think we may just have to accept it for a couple more weeks as a price for getting to a saner model of 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
adIndex := 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 {
Expand All @@ -1035,24 +1037,29 @@ 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 {
adIndex := attachedDiskSources[disk.Source]
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,
}
if disk.DiskEncryptionKey != nil && disk.DiskEncryptionKey.Sha256 != "" {
di["disk_encryption_key_sha256"] = disk.DiskEncryptionKey.Sha256
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 = append(attachedDisks, di)
adIndex++
attachedDisks[adIndex] = di
}
}

d.Set("disk", disks)
d.Set("attached_disk", attachedDisks)
d.Set("scratch_disk", scratchDisks)
Expand Down Expand Up @@ -1451,3 +1458,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
}
141 changes: 124 additions & 17 deletions google/resource_compute_instance_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"fmt"
"os"
"regexp"
"strconv"
"strings"
"testing"

Expand Down Expand Up @@ -224,21 +225,34 @@ 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) },
Providers: testAccProviders,
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),
),
},
},
Expand Down Expand Up @@ -910,24 +924,66 @@ 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 {
return fmt.Errorf("Not found: %s", n)
}

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: <empty>", 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: <empty>", 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)]
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
Expand Down Expand Up @@ -1352,13 +1408,44 @@ 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_disk" "foobar4" {
name = "%s"
size = 10
type = "pd-ssd"
zone = "us-central1-a"
}

resource "google_compute_instance" "foobar" {
Expand All @@ -1370,11 +1457,26 @@ 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.foobar4.self_link}"
}

attached_disk {
source = "${google_compute_disk.foobar3.self_link}"
disk_encryption_key_raw = "%s"
}

network_interface {
Expand All @@ -1385,7 +1487,12 @@ 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-testd-"+acctest.RandString(10),
instance, bootEncryptionKey,
diskNameToEncryptionKey[diskNames[0]].RawKey, diskNameToEncryptionKey[diskNames[1]].RawKey, diskNameToEncryptionKey[diskNames[2]].RawKey)
}

func testAccComputeInstance_attachedDisk(disk, instance string) string {
Expand Down