From ec08fd9ebdd797c4f16e869205d77990384330eb Mon Sep 17 00:00:00 2001 From: Ben Ash Date: Fri, 29 Jul 2022 16:43:29 -0400 Subject: [PATCH 1/2] Make password_policy conflict with the formatter and length fields Previously it was possible to configure password_policy together with formatter or length in the vault_ad_secret_backend resource. These fields should now be mutually exclusive. --- .github/workflows/build.yml | 7 ++ testutil/testutil.go | 14 ++- vault/resource_ad_secret_backend.go | 22 ++-- vault/resource_ad_secret_backend_test.go | 125 ++++++++++++++++------- 4 files changed, 119 insertions(+), 49 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 3dcd043f2..68781fca5 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -82,6 +82,10 @@ jobs: --health-interval 1s --health-timeout 5s --health-retries 60 + openldap: + image: docker.io/bitnami/openldap:latest + ports: + - 1389:1389 steps: - uses: actions/checkout@v3 - name: Acceptance Tests @@ -98,6 +102,9 @@ jobs: COUCHBASE_HOST: couchbase COUCHBASE_USERNAME: Administrator COUCHBASE_PASSWORD: password + AD_URL: "ldap://openldap:1389" + AD_BINDDN: "cn=admin,dc=example,dc=com" + AD_BINDPASS: "adminpassword" run: | export TF_VAULT_VERSION=$(echo ${{ matrix.image }} | \ awk -F: '{match($NF, /^([0-9]+\.[0-9]+\.[0-9]+)/); printf("%s", substr($NF, RSTART, RLENGTH))}') diff --git a/testutil/testutil.go b/testutil/testutil.go index 1d1244b4a..21472992c 100644 --- a/testutil/testutil.go +++ b/testutil/testutil.go @@ -392,7 +392,7 @@ func (v *VaultStateTest) String() string { } // TransformVaultValue function to be used for a value from vault into a form that can be ccmpared to a value from -// from the TF state. +// the TF state. type TransformVaultValue func(st *VaultStateTest, resp *api.Secret) (interface{}, error) func SplitVaultValueString(st *VaultStateTest, resp *api.Secret) (interface{}, error) { @@ -649,3 +649,15 @@ func CheckJSONData(resourceName, attr, expected string) resource.TestCheckFunc { return nil } } + +// GetImportTestStep for resource name. Optionally include field names that should be ignored during the import +// verification, typically ignore fields should only be provided for values that are not returned from the +// provisioning API. +func GetImportTestStep(resourceName string, skipVerify bool, ignoreFields ...string) resource.TestStep { + return resource.TestStep{ + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: !skipVerify, + ImportStateVerifyIgnore: ignoreFields, + } +} diff --git a/vault/resource_ad_secret_backend.go b/vault/resource_ad_secret_backend.go index 80a269cf0..bfc44f2ec 100644 --- a/vault/resource_ad_secret_backend.go +++ b/vault/resource_ad_secret_backend.go @@ -84,11 +84,12 @@ func adSecretBackendResource() *schema.Resource { Description: `Use anonymous bind to discover the bind DN of a user.`, }, "formatter": { - Type: schema.TypeString, - Optional: true, - Computed: true, - Deprecated: `Formatter is deprecated and password_policy should be used with Vault >= 1.5.`, - Description: `Text to insert the password into, ex. "customPrefix{{PASSWORD}}customSuffix".`, + Type: schema.TypeString, + Optional: true, + Computed: true, + Deprecated: `Formatter is deprecated and password_policy should be used with Vault >= 1.5.`, + Description: `Text to insert the password into, ex. "customPrefix{{PASSWORD}}customSuffix".`, + ConflictsWith: []string{"password_policy"}, }, "groupattr": { Type: schema.TypeString, @@ -117,11 +118,12 @@ func adSecretBackendResource() *schema.Resource { Description: `The number of seconds after a Vault rotation where, if Active Directory shows a later rotation, it should be considered out-of-band.`, }, "length": { - Type: schema.TypeInt, - Optional: true, - Computed: true, - Deprecated: `Length is deprecated and password_policy should be used with Vault >= 1.5.`, - Description: `The desired length of passwords that Vault generates.`, + Type: schema.TypeInt, + Optional: true, + Computed: true, + Deprecated: `Length is deprecated and password_policy should be used with Vault >= 1.5.`, + Description: `The desired length of passwords that Vault generates.`, + ConflictsWith: []string{"password_policy"}, }, "local": { Type: schema.TypeBool, diff --git a/vault/resource_ad_secret_backend_test.go b/vault/resource_ad_secret_backend_test.go index 1b45c8557..381b1afb1 100644 --- a/vault/resource_ad_secret_backend_test.go +++ b/vault/resource_ad_secret_backend_test.go @@ -2,6 +2,7 @@ package vault import ( "fmt" + "regexp" "strings" "testing" @@ -17,6 +18,7 @@ func TestADSecretBackend(t *testing.T) { backend := acctest.RandomWithPrefix("tf-test-ad") bindDN, bindPass, url := testutil.GetTestADCreds(t) + resourceName := "vault_ad_secret_backend.test" resource.Test(t, resource.TestCase{ Providers: testProviders, PreCheck: func() { testutil.TestAccPreCheck(t) }, @@ -26,29 +28,44 @@ func TestADSecretBackend(t *testing.T) { { Config: testADSecretBackend_initialConfig(backend, bindDN, bindPass, url), Check: resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttr("vault_ad_secret_backend.test", "backend", backend), - resource.TestCheckResourceAttr("vault_ad_secret_backend.test", "description", "test description"), - resource.TestCheckResourceAttr("vault_ad_secret_backend.test", "default_lease_ttl_seconds", "3600"), - resource.TestCheckResourceAttr("vault_ad_secret_backend.test", "max_lease_ttl_seconds", "7200"), - resource.TestCheckResourceAttr("vault_ad_secret_backend.test", "binddn", bindDN), - resource.TestCheckResourceAttr("vault_ad_secret_backend.test", "bindpass", bindPass), - resource.TestCheckResourceAttr("vault_ad_secret_backend.test", "url", url), - resource.TestCheckResourceAttr("vault_ad_secret_backend.test", "insecure_tls", "true"), - resource.TestCheckResourceAttr("vault_ad_secret_backend.test", "userdn", "CN=Users,DC=corp,DC=example,DC=net"), + resource.TestCheckResourceAttr(resourceName, "backend", backend), + resource.TestCheckResourceAttr(resourceName, "description", "test description"), + resource.TestCheckResourceAttr(resourceName, "default_lease_ttl_seconds", "3600"), + resource.TestCheckResourceAttr(resourceName, "max_lease_ttl_seconds", "7200"), + resource.TestCheckResourceAttr(resourceName, "binddn", bindDN), + resource.TestCheckResourceAttr(resourceName, "bindpass", bindPass), + resource.TestCheckResourceAttr(resourceName, "url", url), + resource.TestCheckResourceAttr(resourceName, "insecure_tls", "true"), + resource.TestCheckResourceAttr(resourceName, "userdn", "CN=Users,DC=corp,DC=example,DC=net"), ), }, + testutil.GetImportTestStep(resourceName, false, "bindpass", "description"), + { + Config: testADSecretBackendConflictsConfig( + resourceName, bindDN, bindPass, url, "length", 12), + ExpectError: regexp.MustCompile(`.*"length": conflicts with password_policy.*`), + + PlanOnly: true, + }, + { + Config: testADSecretBackendConflictsConfig( + resourceName, bindDN, bindPass, url, "formatter", "{{foo}}"), + ExpectError: regexp.MustCompile(`.*"formatter": conflicts with password_policy.*`), + + PlanOnly: true, + }, { Config: testADSecretBackend_updateConfig(backend, bindDN, bindPass, url), Check: resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttr("vault_ad_secret_backend.test", "backend", backend), - resource.TestCheckResourceAttr("vault_ad_secret_backend.test", "description", "test description"), - resource.TestCheckResourceAttr("vault_ad_secret_backend.test", "default_lease_ttl_seconds", "7200"), - resource.TestCheckResourceAttr("vault_ad_secret_backend.test", "max_lease_ttl_seconds", "14400"), - resource.TestCheckResourceAttr("vault_ad_secret_backend.test", "binddn", bindDN), - resource.TestCheckResourceAttr("vault_ad_secret_backend.test", "bindpass", bindPass), - resource.TestCheckResourceAttr("vault_ad_secret_backend.test", "url", url), - resource.TestCheckResourceAttr("vault_ad_secret_backend.test", "insecure_tls", "false"), - resource.TestCheckResourceAttr("vault_ad_secret_backend.test", "userdn", "CN=Users,DC=corp,DC=hashicorp,DC=com"), + resource.TestCheckResourceAttr(resourceName, "backend", backend), + resource.TestCheckResourceAttr(resourceName, "description", "test description"), + resource.TestCheckResourceAttr(resourceName, "default_lease_ttl_seconds", "7200"), + resource.TestCheckResourceAttr(resourceName, "max_lease_ttl_seconds", "14400"), + resource.TestCheckResourceAttr(resourceName, "binddn", bindDN), + resource.TestCheckResourceAttr(resourceName, "bindpass", bindPass), + resource.TestCheckResourceAttr(resourceName, "url", url), + resource.TestCheckResourceAttr(resourceName, "insecure_tls", "false"), + resource.TestCheckResourceAttr(resourceName, "userdn", "CN=Users,DC=corp,DC=hashicorp,DC=com"), ), }, }, @@ -85,29 +102,61 @@ func testAccADSecretBackendCheckDestroy(s *terraform.State) error { func testADSecretBackend_initialConfig(backend, bindDN, bindPass, url string) string { return fmt.Sprintf(` resource "vault_ad_secret_backend" "test" { - backend = "%s" - description = "test description" - default_lease_ttl_seconds = "3600" - max_lease_ttl_seconds = "7200" - binddn = "%s" - bindpass = "%s" - url = "%s" - insecure_tls = "true" - userdn = "CN=Users,DC=corp,DC=example,DC=net" -}`, backend, bindDN, bindPass, url) + backend = "%s" + description = "test description" + default_lease_ttl_seconds = "3600" + max_lease_ttl_seconds = "7200" + binddn = "%s" + bindpass = "%s" + url = "%s" + insecure_tls = "true" + userdn = "CN=Users,DC=corp,DC=example,DC=net" +} +`, backend, bindDN, bindPass, url) } func testADSecretBackend_updateConfig(backend, bindDN, bindPass, url string) string { return fmt.Sprintf(` resource "vault_ad_secret_backend" "test" { - backend = "%s" - description = "test description" - default_lease_ttl_seconds = "7200" - max_lease_ttl_seconds = "14400" - binddn = "%s" - bindpass = "%s" - url = "%s" - insecure_tls = "false" - userdn = "CN=Users,DC=corp,DC=hashicorp,DC=com" -}`, backend, bindDN, bindPass, url) + backend = "%s" + description = "test description" + default_lease_ttl_seconds = "7200" + max_lease_ttl_seconds = "14400" + binddn = "%s" + bindpass = "%s" + url = "%s" + insecure_tls = "false" + userdn = "CN=Users,DC=corp,DC=hashicorp,DC=com" +} +`, backend, bindDN, bindPass, url) +} + +func testADSecretBackendConflictsConfig(backend, bindDN, bindPass, url, conflict string, conflictVal interface{}) string { + var cVal string + switch v := conflictVal.(type) { + case string: + cVal = fmt.Sprintf(`"%s"`, v) + case int: + cVal = fmt.Sprintf("%d", v) + default: + panic(fmt.Sprintf("unsupprted type %T", v)) + } + + config := fmt.Sprintf(` +resource "vault_ad_secret_backend" "test" { + backend = "%s" + description = "test description" + default_lease_ttl_seconds = "7200" + max_lease_ttl_seconds = "14400" + binddn = "%s" + bindpass = "%s" + url = "%s" + insecure_tls = "false" + userdn = "CN=Users,DC=corp,DC=hashicorp,DC=com" + password_policy = "foo" + %s = %s +} +`, backend, bindDN, bindPass, url, conflict, cVal) + + return config } From 969a4dc45b3e8fa88dc725ebe40982f49a16170b Mon Sep 17 00:00:00 2001 From: Ben Ash Date: Fri, 29 Jul 2022 17:58:58 -0400 Subject: [PATCH 2/2] Hold off on making length conflict with password_policy - revert openldap addition until we have a proper image test against. --- .github/workflows/build.yml | 7 ------- vault/resource_ad_secret_backend.go | 11 +++++------ vault/resource_ad_secret_backend_test.go | 16 +++++++++------- website/docs/r/ad_secret_backend.html.md | 1 + 4 files changed, 15 insertions(+), 20 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 68781fca5..3dcd043f2 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -82,10 +82,6 @@ jobs: --health-interval 1s --health-timeout 5s --health-retries 60 - openldap: - image: docker.io/bitnami/openldap:latest - ports: - - 1389:1389 steps: - uses: actions/checkout@v3 - name: Acceptance Tests @@ -102,9 +98,6 @@ jobs: COUCHBASE_HOST: couchbase COUCHBASE_USERNAME: Administrator COUCHBASE_PASSWORD: password - AD_URL: "ldap://openldap:1389" - AD_BINDDN: "cn=admin,dc=example,dc=com" - AD_BINDPASS: "adminpassword" run: | export TF_VAULT_VERSION=$(echo ${{ matrix.image }} | \ awk -F: '{match($NF, /^([0-9]+\.[0-9]+\.[0-9]+)/); printf("%s", substr($NF, RSTART, RLENGTH))}') diff --git a/vault/resource_ad_secret_backend.go b/vault/resource_ad_secret_backend.go index bfc44f2ec..8fee129ab 100644 --- a/vault/resource_ad_secret_backend.go +++ b/vault/resource_ad_secret_backend.go @@ -118,12 +118,11 @@ func adSecretBackendResource() *schema.Resource { Description: `The number of seconds after a Vault rotation where, if Active Directory shows a later rotation, it should be considered out-of-band.`, }, "length": { - Type: schema.TypeInt, - Optional: true, - Computed: true, - Deprecated: `Length is deprecated and password_policy should be used with Vault >= 1.5.`, - Description: `The desired length of passwords that Vault generates.`, - ConflictsWith: []string{"password_policy"}, + Type: schema.TypeInt, + Optional: true, + Computed: true, + Deprecated: `Length is deprecated and password_policy should be used with Vault >= 1.5.`, + Description: `The desired length of passwords that Vault generates.`, }, "local": { Type: schema.TypeBool, diff --git a/vault/resource_ad_secret_backend_test.go b/vault/resource_ad_secret_backend_test.go index 381b1afb1..3f5678617 100644 --- a/vault/resource_ad_secret_backend_test.go +++ b/vault/resource_ad_secret_backend_test.go @@ -40,13 +40,15 @@ func TestADSecretBackend(t *testing.T) { ), }, testutil.GetImportTestStep(resourceName, false, "bindpass", "description"), - { - Config: testADSecretBackendConflictsConfig( - resourceName, bindDN, bindPass, url, "length", 12), - ExpectError: regexp.MustCompile(`.*"length": conflicts with password_policy.*`), - - PlanOnly: true, - }, + // TODO: on vault-1.11+ length should conflict with password_policy + // We should re-enable this check when we have the adaptive version support. + //{ + // Config: testADSecretBackendConflictsConfig( + // resourceName, bindDN, bindPass, url, "length", 12), + // ExpectError: regexp.MustCompile(`.*"length": conflicts with password_policy.*`), + + // PlanOnly: true, + //}, { Config: testADSecretBackendConflictsConfig( resourceName, bindDN, bindPass, url, "formatter", "{{foo}}"), diff --git a/website/docs/r/ad_secret_backend.html.md b/website/docs/r/ad_secret_backend.html.md index 84f11b0fc..347a0eb4b 100644 --- a/website/docs/r/ad_secret_backend.html.md +++ b/website/docs/r/ad_secret_backend.html.md @@ -86,6 +86,7 @@ Defaults to `false`. shows a later rotation, it should be considered out-of-band * `length` - (Optional) **Deprecated** use `password_policy`. The desired length of passwords that Vault generates. + *Mutually exclusive with `password_policy` on vault-1.11+* * `local` - (Optional) Mark the secrets engine as local-only. Local engines are not replicated or removed by replication.Tolerance duration to use when checking the last rotation time.