review 1: change behaviour on revocation

hashicorp · Dec 17, 2024 · a1d8ed5 · a1d8ed5
1 parent 6678869
commit a1d8ed5
Show file tree

Hide file tree

Showing 3 changed files with 39 additions and 23 deletions.
diff --git a/vault/resource_pki_secret_backend_cert.go b/vault/resource_pki_secret_backend_cert.go
@@ -166,16 +166,18 @@ func pkiSecretBackendCertResource() *schema.Resource {
 					"the expiration is less than min_seconds_remaining in the future.",
 			},
 			consts.FieldRevoke: {
-				Type:        schema.TypeBool,
-				Optional:    true,
-				Default:     false,
-				Description: "Revoke the certificate upon resource destruction.",
+				Type:          schema.TypeBool,
+				Optional:      true,
+				Default:       false,
+				Description:   "Revoke the certificate upon resource destruction.",
+				ConflictsWith: []string{consts.FieldRevokeWithKey},
 			},
 			consts.FieldRevokeWithKey: {
-				Type:        schema.TypeBool,
-				Optional:    true,
-				Default:     false,
-				Description: "Revoke the certificate with private key method",
+				Type:          schema.TypeBool,
+				Optional:      true,
+				Default:       false,
+				Description:   "Revoke the certificate with private key method upon resource destruction.",
+				ConflictsWith: []string{consts.FieldRevoke},
 			},
 			consts.FieldIssuerRef: {
 				Type:        schema.TypeString,
@@ -363,26 +365,27 @@ func pkiSecretBackendCertUpdate(ctx context.Context, d *schema.ResourceData, m i
 }
 
 func pkiSecretBackendCertDelete(_ context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics {
-	if d.Get(consts.FieldRevoke).(bool) {
+	var revokeWithKey bool
+	if d.Get(consts.FieldRevokeWithKey) != nil {
+		revokeWithKey = d.Get(consts.FieldRevokeWithKey).(bool)
+	} else {
+		revokeWithKey = false
+	}
+	if d.Get(consts.FieldRevoke).(bool) || revokeWithKey {
 		client, e := provider.GetClient(d, meta)
 		if e != nil {
 			return diag.FromErr(e)
 		}
 
 		backend := d.Get(consts.FieldBackend).(string)
-
-		privateKey := d.Get(consts.FieldPrivateKey).(string)
 		serialNumber := d.Get(consts.FieldSerialNumber).(string)
 		commonName := d.Get(consts.FieldCommonName).(string)
-		revokeWithKey := d.Get(consts.FieldRevokeWithKey).(bool)
 		data := map[string]interface{}{
 			consts.FieldSerialNumber: serialNumber,
 		}
-		if revokeWithKey {
-			data["private_key"] = privateKey
-		}
 		var path string
 		if revokeWithKey {
+			data["private_key"] = d.Get(consts.FieldPrivateKey).(string)
 			path = strings.Trim(backend, "/") + "/revoke-with-key"
 		} else {
 			path = strings.Trim(backend, "/") + "/revoke"
@@ -391,7 +394,7 @@ func pkiSecretBackendCertDelete(_ context.Context, d *schema.ResourceData, meta
 		log.Printf("[DEBUG] Revoking certificate %q with serial number %q on PKI secret backend %q",
 			commonName, serialNumber, backend)
 		_, err := client.Logical().Write(path, data)
-		
+
 		if err != nil {
 			return diag.Errorf("error revoking certificate %q with serial number %q for PKI secret backend %q: %s",
 				commonName, serialNumber, backend, err)

diff --git a/vault/resource_pki_secret_backend_cert_test.go b/vault/resource_pki_secret_backend_cert_test.go
@@ -98,7 +98,7 @@ func TestPkiSecretBackendCert_basic(t *testing.T) {
 				Config: testPkiSecretBackendCertConfig_basic(rootPath, intermediatePath, true, true, true),
 				Check: resource.ComposeTestCheckFunc(
 					append(checks,
-						resource.TestCheckResourceAttr(resourceName, "revoke", "true"),
+						resource.TestCheckResourceAttr(resourceName, "revoke_with_key", "true"),
 						testPKICertRevocation(intermediatePath, store),
 						testCapturePKICert(resourceName, store),
 					)...,
@@ -179,20 +179,33 @@ resource "vault_pki_secret_backend_role" "test" {
 `, rootPath, intermediatePath),
 	}
 
-	if withCert {
+	if withCert && !revokeWithKey {
 		fragments = append(fragments, fmt.Sprintf(`
 resource "vault_pki_secret_backend_cert" "test" {
   backend               = vault_pki_secret_backend_role.test.backend
   name                  = vault_pki_secret_backend_role.test.name
   common_name           = "cert.test.my.domain"
   uri_sans              = ["spiffe://test.my.domain"]
-	user_ids              = ["foo", "bar"]
+  user_ids              = ["foo", "bar"]
   ttl                   = "720h"
   min_seconds_remaining = 60
   revoke                = %t
-  revoke_with_key       = %t
 }
-`, revoke, revokeWithKey))
+`, revoke))
+	}
+	if revokeWithKey && withCert {
+		fragments = append(fragments, `
+resource "vault_pki_secret_backend_cert" "test" {
+  backend               = vault_pki_secret_backend_role.test.backend
+  name                  = vault_pki_secret_backend_role.test.name
+  common_name           = "cert.test.my.domain"
+  uri_sans              = ["spiffe://test.my.domain"]
+  user_ids              = ["foo", "bar"]
+  ttl                   = "720h"
+  min_seconds_remaining = 60
+  revoke_with_key       = true
+}
+`)
 	}
 
 	return strings.Join(fragments, "\n")

diff --git a/website/docs/r/pki_secret_backend_cert.html.md b/website/docs/r/pki_secret_backend_cert.html.md
@@ -67,9 +67,9 @@ The following arguments are supported:
 
 * `auto_renew` - (Optional) If set to `true`, certs will be renewed if the expiration is within `min_seconds_remaining`. Default `false`
 
-* `revoke` - If set to `true`, the certificate will be revoked on resource destruction. 
+* `revoke` - If set to `true`, the certificate will be revoked on resource destruction. Needs privileged access on Vault engine. Conflicts with `revoke_with_key`. Default `false`.
 
-* `revoke_with_key` - if set to `true`, use method `revoke-with-key` to revoke the certificate on resource destruction. Used to revoke certificate without using privileged operation.
+* `revoke_with_key` - If set to `true`, use method `revoke-with-key` to revoke the certificate on resource destruction. Used to revoke certificate without using privileged operation. Conflicts with `revoke`. Default `false`
 
 ## Attributes Reference