Add support for PKI User ID's #1936

czembower · 2023-07-11T16:22:13Z

Updates vault_pki_secret_backend_role and vault_pki_secret_backend_cert resources to support User ID feature available in Vault 1.13. This PR replaces a previous version (now closed) that required extensive rebasing.

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

Add PKI resource support for User IDs

fairclothjm

Thanks! Can we also add test coverage for the newly added fields?

vault/resource_pki_secret_backend_cert.go

czembower · 2023-07-12T17:05:47Z

@fairclothjm Test added for the role configuration.

fairclothjm · 2023-07-12T19:48:58Z

@czembower Thanks, I am getting the following test failure:

--- FAIL: TestPkiSecretBackendRole_basic (7.82s)
    resource_pki_secret_backend_role_test.go:180: ImportStateVerify attributes not equivalent. Difference is shown below. Top is actual, bottom is expected.

        (map[string]string) {
        }


        (map[string]string) (len=2) {
         (string) (len=18) "allowed_user_ids.#": (string) (len=1) "1",
         (string) (len=18) "allowed_user_ids.0": (string) (len=4) "test"
        }
FAIL
FAIL	github.com/hashicorp/terraform-provider-vault/vault	101.462s

Also, it looks like we only added test coverage for the new field in resource_pki_secret_backend_role.go but not resource_pki_secret_backend_cert.go.

czembower · 2023-07-13T16:49:52Z

@fairclothjm, thank you. Is it possible that you're running the test against a non-current version of Vault? I get that error on 1.12.x, but not 1.13.0+ (when UID support was added to core).

Regarding the certificate creation test, I don't see precedent in the code for extensively testing certificate creation with all optional parameters, nor do I see conditional tests based on the Vault version, as we have with the role resource. I might need some guidance here if this test is indeed required.

fairclothjm · 2023-07-20T19:53:46Z

@czembower Yes, you are correct. Sorry about that! Also, looks like I somehow force pushed to your branch. I didn't expect that since your PR is coming from a Fork.

Anyway, the following patch should fix your issues. It looks like allowed_user_ids and user_ids are only available in PKI since Vault v1.13:

diff --git a/vault/resource_pki_secret_backend_cert.go b/vault/resource_pki_secret_backend_cert.go
index 9ef67851..c73499b5 100644
--- a/vault/resource_pki_secret_backend_cert.go
+++ b/vault/resource_pki_secret_backend_cert.go
@@ -234,7 +234,7 @@ func pkiSecretBackendCertCreate(ctx context.Context, d *schema.ResourceData, met
 	}
 
 	// add UID if supported
-	if provider.IsAPISupported(meta, provider.VaultVersion111) {
+	if provider.IsAPISupported(meta, provider.VaultVersion113) {
 		if userIds, ok := d.GetOk(consts.FieldUserIds); ok {
 			m := util.ToStringArray(userIds.([]interface{}))
 			if len(m) > 0 {
diff --git a/vault/resource_pki_secret_backend_cert_test.go b/vault/resource_pki_secret_backend_cert_test.go
index 5f176eb1..d3de3f8b 100644
--- a/vault/resource_pki_secret_backend_cert_test.go
+++ b/vault/resource_pki_secret_backend_cert_test.go
@@ -81,6 +81,17 @@ func TestPkiSecretBackendCert_basic(t *testing.T) {
 					testPKICertRevocation(intermediatePath, store),
 				),
 			},
+			{
+				SkipFunc: func() (bool, error) {
+					meta := testProvider.Meta().(*provider.ProviderMeta)
+					return !meta.IsAPISupported(provider.VaultVersion113), nil
+				},
+				Config: testPkiSecretBackendCertConfig_basic(rootPath, intermediatePath, true, false),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr(resourceName, "user_ids.0", "foo"),
+					resource.TestCheckResourceAttr(resourceName, "user_ids.1", "bar"),
+				),
+			},
 		},
 	})
 }
@@ -149,6 +160,7 @@ resource "vault_pki_secret_backend_role" "test" {
   allowed_domains  = ["test.my.domain"]
   allow_subdomains = true
   allowed_uri_sans = ["spiffe://test.my.domain"]
+  allowed_user_ids = ["foo", "bar"]
   max_ttl          = "3600"
   key_usage        = ["DigitalSignature", "KeyAgreement", "KeyEncipherment"]
 }
@@ -162,6 +174,7 @@ resource "vault_pki_secret_backend_cert" "test" {
   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                = %t
diff --git a/vault/resource_pki_secret_backend_role.go b/vault/resource_pki_secret_backend_role.go
index e6468858..50e21bea 100644
--- a/vault/resource_pki_secret_backend_role.go
+++ b/vault/resource_pki_secret_backend_role.go
@@ -487,7 +487,7 @@ func pkiSecretBackendRoleCreate(ctx context.Context, d *schema.ResourceData, met
 		}
 	}
 
-	if provider.IsAPISupported(meta, provider.VaultVersion111) {
+	if provider.IsAPISupported(meta, provider.VaultVersion113) {
 		if allowedUserIds, ok := d.GetOk(consts.FieldAllowedUserIds); ok {
 			ifcList := allowedUserIds.([]interface{})
 			list := make([]string, 0, len(ifcList))
diff --git a/vault/resource_pki_secret_backend_role_test.go b/vault/resource_pki_secret_backend_role_test.go
index af968571..daf8581a 100644
--- a/vault/resource_pki_secret_backend_role_test.go
+++ b/vault/resource_pki_secret_backend_role_test.go
@@ -253,7 +253,7 @@ func TestPkiSecretBackendRole_basic(t *testing.T) {
 			{
 				SkipFunc: func() (bool, error) {
 					meta := testProvider.Meta().(*provider.ProviderMeta)
-					return !meta.IsAPISupported(provider.VaultVersion111), nil
+					return !meta.IsAPISupported(provider.VaultVersion113), nil
 				},
 				Config: testPkiSecretBackendRoleConfig_basic(name, backend, 3600, 7200, `allowed_user_ids = ["test"]`),
 				Check:  resource.TestCheckResourceAttr(resourceName, "allowed_user_ids.0", "test"),

czembower · 2023-07-24T15:11:22Z

@fairclothjm Whoops, updated to 1.13 for the API check. Thank you for that!

fairclothjm · 2023-07-25T15:00:56Z

@czembower Thanks! That diff I included above also has a test for resource_pki_secret_backend_cert_test.go. It would be appreciated if you could add that. We are trying to add more code coverage as we add new fields.

czembower · 2023-07-25T19:30:33Z

@fairclothjm thanks for this. I'm worried that adding the UID params to the terraform resources in the cert test also needs to be conditional based on the API version, right? Wouldn't we need to add a testPkiSecretBackendCertConfig_after113 or something? This is why I didn't add cert tests originally - because I didn't see a precedent in the code for this type of thing.

What do you recommend?

fairclothjm · 2023-07-25T21:58:20Z

Assuming the source code is correctly guarding those fields, then the Vault version checks should make sure those are ignored.

I just ran your branch against 1.12.2 and added a print:

=== RUN   TestPkiSecretBackendCert_basic
>>>>> skipping user_ids for 1.12.2
--- PASS: TestPkiSecretBackendCert_basic (4.71s)
=== RUN   TestPkiSecretBackendCert_renew
>>>>> skipping user_ids for 1.12.2
>>>>> skipping user_ids for 1.12.2
>>>>> skipping user_ids for 1.12.2
--- PASS: TestPkiSecretBackendCert_renew (9.61s)
PASS
ok  	github.com/hashicorp/terraform-provider-vault/vault	14.874s

fairclothjm · 2023-07-26T19:31:57Z

Most recent build from a duplicate branch: https://github.com/hashicorp/terraform-provider-vault/actions/runs/5672094453 ✅

fairclothjm

Thanks!

fairclothjm · 2023-07-26T19:35:51Z

@czembower I forgot to ask that you update the changelog. Something like:

diff --git a/CHANGELOG.md b/CHANGELOG.md
index f125236e..c3e6a376 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,3 +1,7 @@
+## Unreleased
+FEATURES:
+* Add support for User ID configuration for PKI Secrets Engine: ([#1936](https://github.com/hashicorp/terraform-provider-vault/pull/1936))
+
 ## 3.18.0 (Jul 12, 2023)
 FEATURES:
 * Add support to set default issuers configuration for PKI Secrets Engine: ([#1937](https://github.com/hashicorp/terraform-provider-vault/pull/1937))

Co-authored-by: Mason-776 <mh776f@att.com>

github-actions bot added documentation size/M labels Jul 11, 2023

fairclothjm reviewed Jul 11, 2023

View reviewed changes

Mason-776 reviewed Jul 12, 2023

View reviewed changes

vault/resource_pki_secret_backend_cert.go Outdated Show resolved Hide resolved

fairclothjm force-pushed the pki-uids branch from 5ab398d to f94405e Compare July 14, 2023 16:22

fairclothjm approved these changes Jul 26, 2023

View reviewed changes

fairclothjm added this to the 3.19.0 milestone Jul 26, 2023

czembower and others added 8 commits July 26, 2023 16:12

apply changes with updated codebase

8fc2e2e

ForceNew for UID parameter

7f39b63

remove tests

6eed3bb

add role test

5838afe

Update vault/resource_pki_secret_backend_cert.go

19d72f5

Co-authored-by: Mason-776 <mh776f@att.com>

Update Vault API version constraint

b5e696a

certificate tests

0d89010

changelog update

c470391

fairclothjm force-pushed the pki-uids branch from ea2ac35 to c470391 Compare July 26, 2023 21:13

fairclothjm merged commit 9f1b371 into hashicorp:main Jul 26, 2023

fairclothjm mentioned this pull request Jul 27, 2023

Update resource_azure_secret_backend_role to include permanently_delete arg #1958

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for PKI User ID's #1936

Add support for PKI User ID's #1936

czembower commented Jul 11, 2023

fairclothjm left a comment

czembower commented Jul 12, 2023

fairclothjm commented Jul 12, 2023

czembower commented Jul 13, 2023

fairclothjm commented Jul 20, 2023

czembower commented Jul 24, 2023

fairclothjm commented Jul 25, 2023

czembower commented Jul 25, 2023

fairclothjm commented Jul 25, 2023

fairclothjm commented Jul 26, 2023

fairclothjm left a comment

fairclothjm commented Jul 26, 2023

Add support for PKI User ID's #1936

Add support for PKI User ID's #1936

Conversation

czembower commented Jul 11, 2023

Community Note

fairclothjm left a comment

Choose a reason for hiding this comment

czembower commented Jul 12, 2023

fairclothjm commented Jul 12, 2023

czembower commented Jul 13, 2023

fairclothjm commented Jul 20, 2023

czembower commented Jul 24, 2023

fairclothjm commented Jul 25, 2023

czembower commented Jul 25, 2023

fairclothjm commented Jul 25, 2023

fairclothjm commented Jul 26, 2023

fairclothjm left a comment

Choose a reason for hiding this comment

fairclothjm commented Jul 26, 2023