add option to revoke certificate with private key #2242

lde · 2024-05-16T17:26:05Z

Description

Ability to revoke certificate with private_key trough endpoint revoke-with-key witch is an unprivileged operation.

Checklist

Added CHANGELOG entry (only for user-facing changes)
Acceptance tests where run against all supported Vault Versions

Output from acceptance testing:

$ make testacc TESTARGS='-v -test.run TestPkiSecretBackendCert*'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test -v -test.run TestPkiSecretBackendCert* -timeout 30m ./...
?   	github.com/hashicorp/terraform-provider-vault	[no test files]
?   	github.com/hashicorp/terraform-provider-vault/cmd/coverage	[no test files]
?   	github.com/hashicorp/terraform-provider-vault/helper	[no test files]
?   	github.com/hashicorp/terraform-provider-vault/internal/consts	[no test files]
?   	github.com/hashicorp/terraform-provider-vault/cmd/generate	[no test files]
?   	github.com/hashicorp/terraform-provider-vault/internal/identity/mfa	[no test files]
?   	github.com/hashicorp/terraform-provider-vault/internal/pki	[no test files]
?   	github.com/hashicorp/terraform-provider-vault/internal/identity/group	[no test files]
testing: warning: no tests to run
PASS
ok  	github.com/hashicorp/terraform-provider-vault/codegen	(cached) [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/hashicorp/terraform-provider-vault/internal/identity/entity	(cached) [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/hashicorp/terraform-provider-vault/internal/provider	(cached) [no tests to run]
?   	github.com/hashicorp/terraform-provider-vault/internal/sync	[no test files]
?   	github.com/hashicorp/terraform-provider-vault/schema	[no test files]
?   	github.com/hashicorp/terraform-provider-vault/util/mountutil	[no test files]
testing: warning: no tests to run
PASS
ok  	github.com/hashicorp/terraform-provider-vault/testutil	(cached) [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/hashicorp/terraform-provider-vault/util	(cached) [no tests to run]
=== RUN   TestPkiSecretBackendCert_basic
--- PASS: TestPkiSecretBackendCert_basic (5.04s)
=== RUN   TestPkiSecretBackendCert_renew
--- PASS: TestPkiSecretBackendCert_renew (8.15s)
PASS
ok  	github.com/hashicorp/terraform-provider-vault/vault	(cached)


...

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

lde · 2024-06-10T08:29:57Z

Hi i missed something to review @fairclothjm ?

stevendpclark

Hello @lde, thanks for the contribution!

Overall great but I think we'd like to have the new revoke_with_key conflict with the existing revoke key to avoid user confusion of needing to set both keys to true to get the new behavior.

vault/resource_pki_secret_backend_cert.go

stevendpclark · 2024-11-26T16:01:45Z

website/docs/r/pki_secret_backend_cert.html.md

@@ -69,6 +69,8 @@ The following arguments are supported:

 * `revoke` - If set to `true`, the certificate will be revoked on resource destruction. 

+* `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.


We also need to call out in the docs that the revoke and revoke_with_key fields can not be set simultaneously.

vault/resource_pki_secret_backend_cert.go

stevendpclark

Thanks for updating the PR with my review comments and sorry about the delay, just catching up from the holiday vacation.

This is looking great, I have two small nits I'd like to get addressed before merging this in.

stevendpclark · 2025-01-03T15:25:30Z

vault/resource_pki_secret_backend_cert.go

@@ -357,28 +365,41 @@ 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


nit: This can be simplified with just as the d.Get call will never return nil.

revokeWithKey := d.Get(consts.FieldRevokeWithKey).(bool)

stevendpclark · 2025-01-03T15:40:14Z

website/docs/r/pki_secret_backend_cert.html.md

@@ -67,7 +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`.


Needs privileged access on Vault engine.

Sorry I meant to call this out in my initial review but I seem to have missed it. This might lead to people thinking that revoke_with_key doesn't need to be authorized in Vault which is does and is up to an operator's policy decisions on making it more privileged or not.

Could we update this to

Suggested change

* `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` - If set to `true`, the certificate will be revoked on resource destruction using the `revoke` PKI API. Conflicts with `revoke_with_key`. Default `false`.

* `revoke_with_key` - If set to `true`, the certificate will be revoked on resource destruction using the `revoke-with-key` PKI API. Conflicts with `revoke`. Default `false`

stevendpclark added pki pki-enhancements labels Sep 23, 2024

stevendpclark requested changes Nov 26, 2024

View reviewed changes

lde added 2 commits December 17, 2024 13:10

add option to revoke with cert

6678869

review 1: change behaviour on revocation

a1d8ed5

lde force-pushed the revoke_cert branch from a7c8429 to a1d8ed5 Compare December 17, 2024 16:36

lde requested a review from a team as a code owner December 17, 2024 16:36

lde requested review from clemon and stevendpclark December 17, 2024 16:36

stevendpclark requested changes Jan 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add option to revoke certificate with private key #2242

add option to revoke certificate with private key #2242

lde commented May 16, 2024 •

edited

Loading

lde commented Jun 10, 2024

stevendpclark left a comment

stevendpclark Nov 26, 2024

stevendpclark left a comment

stevendpclark Jan 3, 2025

stevendpclark Jan 3, 2025

		@@ -69,6 +69,8 @@ The following arguments are supported:

		* `revoke` - If set to `true`, the certificate will be revoked on resource destruction.

		* `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` - 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` - If set to `true`, the certificate will be revoked on resource destruction using the `revoke` PKI API. Conflicts with `revoke_with_key`. Default `false`.
+* `revoke_with_key` - If set to `true`, the certificate will be revoked on resource destruction using the `revoke-with-key` PKI API. Conflicts with `revoke`. Default `false`

add option to revoke certificate with private key #2242

Are you sure you want to change the base?

add option to revoke certificate with private key #2242

Conversation

lde commented May 16, 2024 • edited Loading

Description

Checklist

Output from acceptance testing:

Community Note

lde commented Jun 10, 2024

stevendpclark left a comment

Choose a reason for hiding this comment

stevendpclark Nov 26, 2024

Choose a reason for hiding this comment

stevendpclark left a comment

Choose a reason for hiding this comment

stevendpclark Jan 3, 2025

Choose a reason for hiding this comment

stevendpclark Jan 3, 2025

Choose a reason for hiding this comment

lde commented May 16, 2024 •

edited

Loading