From 194f2c10f75fef1044683480aa16b102cc84a50a Mon Sep 17 00:00:00 2001
From: vinay-gopalan <86625824+vinay-gopalan@users.noreply.github.com>
Date: Tue, 21 Mar 2023 09:19:35 -0700
Subject: [PATCH] Avoid writing empty strings to Vault when creating managed
 keys (#1803)

---
 testutil/testutil.go                |  5 +++
 vault/resource_managed_keys.go      | 10 ++++-
 vault/resource_managed_keys_test.go | 64 +++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+), 2 deletions(-)

diff --git a/testutil/testutil.go b/testutil/testutil.go
index f7a8bb31b..8d51a14a5 100644
--- a/testutil/testutil.go
+++ b/testutil/testutil.go
@@ -196,6 +196,11 @@ func GetTestNomadCreds(t *testing.T) (string, string) {
 	return v[0], v[1]
 }
 
+func GetTestPKCSCreds(t *testing.T) (string, string, string) {
+	v := SkipTestEnvUnset(t, "PKCS_KEY_LIBRARY", "PKCS_KEY_SLOT", "PKCS_KEY_PIN")
+	return v[0], v[1], v[2]
+}
+
 func TestCheckResourceAttrJSON(name, key, expectedValue string) resource.TestCheckFunc {
 	return func(s *terraform.State) error {
 		resourceState, ok := s.RootModule().Resources[name]
diff --git a/vault/resource_managed_keys.go b/vault/resource_managed_keys.go
index 1cc36ac75..56d2d078f 100644
--- a/vault/resource_managed_keys.go
+++ b/vault/resource_managed_keys.go
@@ -371,6 +371,12 @@ func getManagedKeysConfigData(config map[string]interface{}, sm schemaMap) (stri
 
 	for blockKey := range sm {
 		if v, ok := config[blockKey]; ok {
+			// ensure empty strings are not written
+			// to vault as part of the data
+			if s, ok := v.(string); ok && s == "" {
+				continue
+			}
+
 			data[blockKey] = v
 
 			if blockKey == consts.FieldName {
@@ -605,7 +611,7 @@ func readAndSetManagedKeys(d *schema.ResourceData, client *api.Client, providerT
 }
 
 func readAWSManagedKeys(d *schema.ResourceData, client *api.Client) error {
-	redacted := []string{"access_key", "secret_key"}
+	redacted := []string{consts.FieldAccessKey, consts.FieldSecretKey}
 	if err := readAndSetManagedKeys(d, client, consts.FieldAWS,
 		map[string]string{consts.FieldUUID: "UUID"}, redacted); err != nil {
 		return err
@@ -625,7 +631,7 @@ func readAzureManagedKeys(d *schema.ResourceData, client *api.Client) error {
 }
 
 func readPKCSManagedKeys(d *schema.ResourceData, client *api.Client) error {
-	redacted := []string{"pin"}
+	redacted := []string{consts.FieldPin, consts.FieldKeyID}
 	if err := readAndSetManagedKeys(d, client, consts.FieldPKCS,
 		map[string]string{consts.FieldUUID: "UUID"}, redacted); err != nil {
 		return err
diff --git a/vault/resource_managed_keys_test.go b/vault/resource_managed_keys_test.go
index 15d615bef..4e25b9010 100644
--- a/vault/resource_managed_keys_test.go
+++ b/vault/resource_managed_keys_test.go
@@ -158,6 +158,53 @@ func TestManagedKeys(t *testing.T) {
 	})
 }
 
+// The following test requires a Vault server to be set up with a specific server configuration
+// (kms_library needs to be defined). We need not dedicate an entire server setup just for one
+// test, and hence this test is meant to be run locally
+//
+// The following test requires a PKCS#11 key to be set up and needs the following
+// environment variables to operate successfully:
+// * PKCS_KEY_LIBRARY
+// * PKCS_KEY_SLOT
+// * PKCS_KEY_PIN
+// * TF_ACC_LOCAL=1
+//
+// The final variable specifies that this test can only be run locally
+func TestManagedKeysPKCS(t *testing.T) {
+	testutil.SkipTestEnvUnset(t, "TF_ACC_LOCAL")
+
+	name := acctest.RandomWithPrefix("pkcs-keys")
+	resourceName := "vault_managed_keys.test"
+
+	library, slot, pin := testutil.GetTestPKCSCreds(t)
+
+	resource.Test(t, resource.TestCase{
+		PreCheck:          func() { testutil.TestEntPreCheck(t) },
+		ProviderFactories: providerFactories,
+		Steps: []resource.TestStep{
+			{
+				Config: testManagedKeysConfig_pkcs(name, library, slot, pin),
+				Check: resource.ComposeAggregateTestCheckFunc(
+					resource.TestCheckResourceAttr(resourceName, "pkcs.#", "1"),
+					resource.TestCheckResourceAttr(resourceName, "pkcs.0.library", library),
+					resource.TestCheckResourceAttr(resourceName, "pkcs.0.key_label", "kms-intermediate"),
+					resource.TestCheckResourceAttr(resourceName, "pkcs.0.key_id", "kms-intermediate"),
+					resource.TestCheckResourceAttr(resourceName, "pkcs.0.key_bits", "4096"),
+					resource.TestCheckResourceAttr(resourceName, "pkcs.0.slot", slot),
+					resource.TestCheckResourceAttr(resourceName, "pkcs.0.pin", pin),
+					resource.TestCheckResourceAttr(resourceName, "pkcs.0.mechanism", "1"),
+				),
+			},
+			{
+				ResourceName:            resourceName,
+				ImportState:             true,
+				ImportStateVerify:       true,
+				ImportStateVerifyIgnore: []string{"pkcs.0.pin", "pkcs.0.key_id"},
+			},
+		},
+	})
+}
+
 func testManagedKeysConfig_basic(name0, name1 string) string {
 	return fmt.Sprintf(`
 resource "vault_managed_keys" "test" {
@@ -196,3 +243,20 @@ resource "vault_managed_keys" "test" {
 }
 `, name)
 }
+
+func testManagedKeysConfig_pkcs(name, library, slot, pin string) string {
+	return fmt.Sprintf(`
+resource "vault_managed_keys" "test" {
+  pkcs {
+    name               = "%s"
+    library            = "%s"
+    key_label          = "kms-intermediate"
+    key_id             = "kms-intermediate"
+    key_bits           = "4096"
+    slot               = "%s"
+    pin                = "%s"
+    mechanism          = "0x0001"
+  }
+}
+`, name, library, slot, pin)
+}