secrets/ldap: fix bindpass getting overwritten (#1875)

* secrets/ldap: fix bindpass getting overwritten * add ad container and tests * remove privileged setting; not avail in gh actions * add comment * fix build env directive * fix build env directive again * try sleeping to wait for ad container * make url dynamic and set cap-add for gh action runner * add ad health check * increase timeout and interval on health checks * revert healthcheck * try to debug containers * add more debug * even more debug * fix docker cmd * make tests local-only :(
hashicorp · Jun 7, 2023 · 82dd646 · 82dd646
1 parent 14efbae
commit 82dd646
Show file tree

Hide file tree

Showing 3 changed files with 154 additions and 19 deletions.
diff --git a/docker-compose.yaml b/docker-compose.yaml
@@ -56,3 +56,20 @@ services:
       - LDAP_ADMIN_PASSWORD=adminpassword
       - LDAP_USERS=alice,bob,foo
       - LDAP_PASSWORDS=password1,password2,password3
+
+  # this container will take at least 20 seconds for the ad service to be
+  # available so manual test runs should take this into account
+  ad:
+    image: laslabs/alpine-samba-dc:0.1.0
+    cap_add:
+    - "SYS_ADMIN"
+    ports:
+      - '2389:389'
+      - '2636:636'
+    environment:
+      - SAMBA_DC_REALM=corp.example.net
+      - SAMBA_DC_DOMAIN=EXAMPLE
+      - SAMBA_DC_ADMIN_PASSWD=SuperSecretPassw0rd
+      - SAMBA_DC_DNS_BACKEND=SAMBA_INTERNAL
+
+
diff --git a/vault/resource_ldap_secret_backend.go b/vault/resource_ldap_secret_backend.go
@@ -187,6 +187,13 @@ func createUpdateLDAPConfigResource(ctx context.Context, d *schema.ResourceData,
 	}
 
 	for _, field := range fields {
+		// don't update bindpass unless it was changed in the config so that we
+		// don't overwrite it in the event a rotate-root operation was
+		// performed in Vault
+		if field == consts.FieldBindPass && !d.HasChange(field) {
+			continue
+		}
+
 		if v, ok := d.GetOk(field); ok {
 			data[field] = v
 		}

diff --git a/vault/resource_ldap_secret_backend_test.go b/vault/resource_ldap_secret_backend_test.go
@@ -9,6 +9,7 @@ import (
 
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/acctest"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
+	"github.com/hashicorp/terraform-plugin-sdk/v2/terraform"
 
 	"github.com/hashicorp/terraform-provider-vault/internal/consts"
 	"github.com/hashicorp/terraform-provider-vault/internal/provider"
@@ -34,7 +35,6 @@ func TestLDAPSecretBackend(t *testing.T) {
 		resourceName          = resourceType + ".test"
 		description           = "test description"
 		updatedDescription    = "new test description"
-		userDN                = "CN=Users,DC=corp,DC=example,DC=net"
 		updatedUserDN         = "CN=Users,DC=corp,DC=hashicorp,DC=com"
 	)
 	resource.Test(t, resource.TestCase{
@@ -46,10 +46,10 @@ func TestLDAPSecretBackend(t *testing.T) {
 		CheckDestroy: testCheckMountDestroyed(resourceType, consts.MountTypeLDAP, consts.FieldPath),
 		Steps: []resource.TestStep{
 			{
-				Config: testLDAPSecretBackendConfig_defaults(bindDN, bindPass),
+				Config: testLDAPSecretBackendConfig_defaults(path, bindDN, bindPass),
 				Check: resource.ComposeTestCheckFunc(
 					resource.TestCheckResourceAttr(resourceName, consts.FieldSchema, "openldap"),
-					resource.TestCheckResourceAttr(resourceName, consts.FieldPath, "ldap"),
+					resource.TestCheckResourceAttr(resourceName, consts.FieldPath, path),
 					resource.TestCheckResourceAttr(resourceName, consts.FieldDescription, description),
 					resource.TestCheckResourceAttr(resourceName, consts.FieldDefaultLeaseTTL, "0"),
 					resource.TestCheckResourceAttr(resourceName, consts.FieldMaxLeaseTTL, "0"),
@@ -62,51 +62,132 @@ func TestLDAPSecretBackend(t *testing.T) {
 				),
 			},
 			{
-				Config: testLDAPSecretBackendConfig(path, description, bindDN, bindPass, url, userDN, "ad", true),
+				Config: testLDAPSecretBackendConfig(path, updatedDescription, bindDN, bindPass, url, updatedUserDN, "openldap", false),
 				Check: resource.ComposeTestCheckFunc(
-					resource.TestCheckResourceAttr(resourceName, consts.FieldSchema, "ad"),
 					resource.TestCheckResourceAttr(resourceName, consts.FieldPath, path),
-					resource.TestCheckResourceAttr(resourceName, consts.FieldDescription, description),
+					resource.TestCheckResourceAttr(resourceName, consts.FieldSchema, "openldap"),
+					resource.TestCheckResourceAttr(resourceName, consts.FieldDescription, updatedDescription),
 					resource.TestCheckResourceAttr(resourceName, consts.FieldDefaultLeaseTTL, "3600"),
 					resource.TestCheckResourceAttr(resourceName, consts.FieldMaxLeaseTTL, "7200"),
 					resource.TestCheckResourceAttr(resourceName, consts.FieldBindDN, bindDN),
 					resource.TestCheckResourceAttr(resourceName, consts.FieldBindPass, bindPass),
 					resource.TestCheckResourceAttr(resourceName, consts.FieldURL, url),
-					resource.TestCheckResourceAttr(resourceName, consts.FieldUserDN, userDN),
-					resource.TestCheckResourceAttr(resourceName, consts.FieldInsecureTLS, "true"),
+					resource.TestCheckResourceAttr(resourceName, consts.FieldUserDN, updatedUserDN),
+					resource.TestCheckResourceAttr(resourceName, consts.FieldInsecureTLS, "false"),
 					resource.TestCheckResourceAttr(resourceName, consts.FieldConnectionTimeout, "99"),
 				),
 			},
+			testutil.GetImportTestStep(resourceName, false, nil,
+				consts.FieldBindPass, consts.FieldConnectionTimeout, consts.FieldDescription, consts.FieldDisableRemount),
+		},
+	})
+}
+
+// TestLDAPSecretBackend_SchemaAD tests vault_ldap_secret_backend for the AD
+// schema and tests that the bindpass is not overwritten unless it is
+// explicitly changed in the TF config so that we don't clobber a bindpass that
+// was changed via a rotate-root operation in Vault.
+//
+// To test, run the ad service provided in the docker-compose.yaml file:
+//
+//	docker compose up -d ad
+//
+// Then export the following environment variables:
+//
+// export AD_URL=ldaps://localhost:2636
+func TestLDAPSecretBackend_SchemaAD(t *testing.T) {
+	var (
+		path         = acctest.RandomWithPrefix("tf-test-ldap")
+		resourceType = "vault_ldap_secret_backend"
+		resourceName = resourceType + ".test"
+		userDN       = "CN=Users,DC=corp,DC=example,DC=net"
+		bindPass     = "SuperSecretPassw0rd"
+		bindDN       = "CN=Administrator,CN=Users,DC=corp,DC=example,DC=net"
+
+		url = testutil.SkipTestEnvUnset(t, "AD_URL")[0]
+	)
+	resource.Test(t, resource.TestCase{
+		ProviderFactories: providerFactories,
+		PreCheck: func() {
+			testutil.TestAccPreCheck(t)
+			SkipIfAPIVersionLT(t, testProvider.Meta(), provider.VaultVersion112)
+		},
+		CheckDestroy: testCheckMountDestroyed(resourceType, consts.MountTypeLDAP, consts.FieldPath),
+		Steps: []resource.TestStep{
 			{
-				Config: testLDAPSecretBackendConfig(path, updatedDescription, bindDN, bindPass, url, updatedUserDN, "ad", false),
+				Config: testLDAPSecretBackendConfig_ad(path, url, ""),
 				Check: resource.ComposeTestCheckFunc(
-					resource.TestCheckResourceAttr(resourceName, consts.FieldPath, path),
+					func(*terraform.State) error {
+						client := testProvider.Meta().(*provider.ProviderMeta).GetClient()
+						if _, err := client.Logical().Write(path+"/rotate-root", nil); err != nil {
+							return err
+						}
+						return nil
+					},
 					resource.TestCheckResourceAttr(resourceName, consts.FieldSchema, "ad"),
-					resource.TestCheckResourceAttr(resourceName, consts.FieldDescription, updatedDescription),
-					resource.TestCheckResourceAttr(resourceName, consts.FieldDefaultLeaseTTL, "3600"),
-					resource.TestCheckResourceAttr(resourceName, consts.FieldMaxLeaseTTL, "7200"),
+					resource.TestCheckResourceAttr(resourceName, consts.FieldPath, path),
+					resource.TestCheckResourceAttr(resourceName, consts.FieldDescription, ""),
+					resource.TestCheckResourceAttr(resourceName, consts.FieldDefaultLeaseTTL, "0"),
+					resource.TestCheckResourceAttr(resourceName, consts.FieldMaxLeaseTTL, "0"),
 					resource.TestCheckResourceAttr(resourceName, consts.FieldBindDN, bindDN),
 					resource.TestCheckResourceAttr(resourceName, consts.FieldBindPass, bindPass),
 					resource.TestCheckResourceAttr(resourceName, consts.FieldURL, url),
-					resource.TestCheckResourceAttr(resourceName, consts.FieldUserDN, updatedUserDN),
-					resource.TestCheckResourceAttr(resourceName, consts.FieldInsecureTLS, "false"),
-					resource.TestCheckResourceAttr(resourceName, consts.FieldConnectionTimeout, "99"),
+					resource.TestCheckResourceAttr(resourceName, consts.FieldUserDN, userDN),
+					resource.TestCheckResourceAttr(resourceName, consts.FieldInsecureTLS, "true"),
+					resource.TestCheckResourceAttr(resourceName, consts.FieldConnectionTimeout, "30"),
+				),
+			},
+			{
+				Config: testLDAPSecretBackendConfig_ad(path, url, `description = "new description"`),
+				Check: resource.ComposeTestCheckFunc(
+					func(*terraform.State) error {
+						client := testProvider.Meta().(*provider.ProviderMeta).GetClient()
+						if _, err := client.Logical().Write(path+"/rotate-root", nil); err != nil {
+							return err
+						}
+						return nil
+					},
+					resource.TestCheckResourceAttr(resourceName, consts.FieldSchema, "ad"),
+					resource.TestCheckResourceAttr(resourceName, consts.FieldPath, path),
+					resource.TestCheckResourceAttr(resourceName, consts.FieldDescription, "new description"),
+					resource.TestCheckResourceAttr(resourceName, consts.FieldDefaultLeaseTTL, "0"),
+					resource.TestCheckResourceAttr(resourceName, consts.FieldMaxLeaseTTL, "0"),
+					resource.TestCheckResourceAttr(resourceName, consts.FieldBindDN, bindDN),
+					resource.TestCheckResourceAttr(resourceName, consts.FieldURL, url),
+					resource.TestCheckResourceAttr(resourceName, consts.FieldUserDN, userDN),
+					resource.TestCheckResourceAttr(resourceName, consts.FieldInsecureTLS, "true"),
+					resource.TestCheckResourceAttr(resourceName, consts.FieldConnectionTimeout, "30"),
+
+					// Even though we did a rotate-root, we expect the TF state
+					// to have no change for bindpass because we did not
+					// explicitly change it in the config and it is not
+					// returned by the Vault API.
+					resource.TestCheckResourceAttr(resourceName, consts.FieldBindPass, bindPass),
+				),
+			},
+			{
+				Config: testLDAPSecretBackendConfig_ad_updated(path, url, `description = "new description"`),
+				Check: resource.ComposeTestCheckFunc(
+					// We explicitly updated the bindpass in the TF config so
+					// we expect the state to contain the new value.
+					resource.TestCheckResourceAttr(resourceName, consts.FieldBindPass, "NEW-SuperSecretPassw0rd"),
 				),
 			},
 			testutil.GetImportTestStep(resourceName, false, nil,
-				consts.FieldBindPass, consts.FieldConnectionTimeout, consts.FieldDescription, consts.FieldDisableRemount),
+				consts.FieldBindPass, consts.FieldConnectionTimeout, consts.FieldDisableRemount),
 		},
 	})
 }
 
 // testLDAPSecretBackendConfig_defaults is used to setup the backend defaults.
-func testLDAPSecretBackendConfig_defaults(bindDN, bindPass string) string {
+func testLDAPSecretBackendConfig_defaults(path, bindDN, bindPass string) string {
 	return fmt.Sprintf(`
 resource "vault_ldap_secret_backend" "test" {
+  path                      = "%s"
   description               = "test description"
   binddn                    = "%s"
   bindpass                  = "%s"
-}`, bindDN, bindPass)
+}`, path, bindDN, bindPass)
 }
 
 func testLDAPSecretBackendConfig(mount, description, bindDN, bindPass, url, userDN, schema string, insecureTLS bool) string {
@@ -126,3 +207,33 @@ resource "vault_ldap_secret_backend" "test" {
 }
 `, mount, description, bindDN, bindPass, url, userDN, insecureTLS, schema)
 }
+
+func testLDAPSecretBackendConfig_ad(path, url, extraConfig string) string {
+	return fmt.Sprintf(`
+resource "vault_ldap_secret_backend" "test" {
+  path         = "%s"
+  binddn       = "CN=Administrator,CN=Users,DC=corp,DC=example,DC=net"
+  bindpass     = "SuperSecretPassw0rd"
+  url          = "%s"
+  insecure_tls = "true"
+  userdn       = "CN=Users,DC=corp,DC=example,DC=net"
+  schema       = "ad"
+  %s
+}
+`, path, url, extraConfig)
+}
+
+func testLDAPSecretBackendConfig_ad_updated(path, url, extraConfig string) string {
+	return fmt.Sprintf(`
+resource "vault_ldap_secret_backend" "test" {
+  path         = "%s"
+  binddn       = "CN=Administrator,CN=Users,DC=corp,DC=example,DC=net"
+  bindpass     = "NEW-SuperSecretPassw0rd"
+  url          = "%s"
+  insecure_tls = "true"
+  userdn       = "CN=Users,DC=corp,DC=example,DC=net"
+  schema       = "ad"
+  %s
+}
+`, path, url, extraConfig)
+}