PKI: Add support for CPS URL in custom policy identifiers #1495

swenson · 2022-06-09T00:52:29Z

Update the vault_resource_pki_secret_backend_role to support
specifying the CPS URL when specifying policy identifiers in line with
the recent changes to the PKI Secrets Engine in Vault 1.11:
hashicorp/vault#15751

We do this by deprecating the existing policy_identifiers argument and
creating a new block, policy_identifier, which can be specified
multiple times.

If both policy_identifiers and policy_identifier blocks are present,
then policy_identifier is ignored. (Otherwise, refreshing would delete
one or the other, and the state wouldn't have round trip stability.)

This was also tested locally with, for example, a terraform file like:

provider "vault" {
}

resource "vault_mount" "pki" {
  path                      = "pki"
  type                      = "pki"
  default_lease_ttl_seconds = 3600
  max_lease_ttl_seconds     = 86400
}

resource "vault_pki_secret_backend_role" "role" {
  name = "example-dot-com"
  backend = vault_mount.pki.path
  allowed_domains = ["example.com"]
  allow_subdomains = true
  allow_bare_domains = true
  allow_glob_domains = true
  allow_ip_sans = true
  allow_localhost = "true"
  generate_lease = true
  organization = ["Hashi test"]
  country = ["USA"]
  locality = ["Area 51"]
  province = ["NV"]
  max_ttl = "720h"
  policy_identifiers = ["2.5.29.32","1.2.3"]
  // or
  policy_identifier {
    oid = "2.5.29.32"
    cps = "https://example.com/cps"
    notice = "Some notice"
  }
  policy_identifier {
    oid = "1.2.3"
  }
}

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

Release note for CHANGELOG:

`resource/pki_secret_backend_role`: Add support for `policy_identifier` blocks, deprecate `policy_identifiers`

Output from acceptance testing:

$ make testacc TESTARGS="-v ./vault -run .*TestPkiSecretBackendRole.*"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test -v -v ./vault -run .*TestPkiSecretBackendRole.* -timeout 30m ./...
=== RUN   TestPkiSecretBackendRole_basic
--- PASS: TestPkiSecretBackendRole_basic (4.75s)
PASS
ok  	github.com/hashicorp/terraform-provider-vault/vault	5.093s
...

Update the `vault_resource_pki_secret_backend_role` to support specifying the CPS URL when specifying policy identifiers in line with the recent changes to the PKI Secrets Engine in Vault 1.11: hashicorp/vault#15751 We do this by deprecating the existing `policy_identifiers` argument and creating a new block, `policy_identifier`, which can be specified multiple times. If both `policy_identifiers` and `policy_identifier` blocks are present, then `policy_identifier` is ignored. (Otherwise, refreshing would delete one or the other, and the state wouldn't have round trip stability.) This was also tested locally with, for example, a terraform file like: ```hcl provider "vault" { } resource "vault_mount" "pki" { path = "pki" type = "pki" default_lease_ttl_seconds = 3600 max_lease_ttl_seconds = 86400 } resource "vault_pki_secret_backend_role" "role" { name = "example-dot-com" backend = vault_mount.pki.path allowed_domains = ["example.com"] allow_subdomains = true allow_bare_domains = true allow_glob_domains = true allow_ip_sans = true allow_localhost = "true" generate_lease = true organization = ["Hashi test"] country = ["USA"] locality = ["Area 51"] province = ["NV"] max_ttl = "720h" policy_identifiers = ["2.5.29.32","1.2.3"] // or policy_identifier { oid = "2.5.29.32" cps = "https://example.com/cps" notice = "Some notice" } policy_identifier { oid = "1.2.3" } } ```

vinay-gopalan

PR is looking great! Had some comments/questions

vault/resource_pki_secret_backend_role.go

vault/resource_pki_secret_backend_role_test.go

benashz

Looking good! I left some initial feedback/suggestions.

benashz · 2022-06-09T15:30:18Z

website/docs/r/pki_secret_backend_role.html.md

@@ -79,7 +79,7 @@ The following arguments are supported:

 * `email_protection_flag` - (Optional) Flag to specify certificates for email protection use

-* `key_type` - (Optional) The generated key type, choices: `rsa`, `ec`, `ed25519`, `any`  
+* `key_type` - (Optional) The generated key type, choices: `rsa`, `ec`, `ed25519`, `any`


I believe the trailing double space denotes a line beak in Markdown. I think this was intentional.

Oh. Weird. My text editor isn't going to like that :) I'll see if I can add it back.

vault/pki.go

benashz · 2022-06-09T15:34:34Z

vault/pki.go

+// into a list of strings (the OIDs) or the JSON serialization of the `policy_identifier` blocks,
+// respectively.
+func readPolicyIdentifiers(d *schema.ResourceData) interface{} {
+	policyIdentifiersList := d.Get("policy_identifiers").([]interface{})


If we me make the two fields mutually exclusive by setting ConflictsWith on their schema.Schema, then we can update this function to only have to support the Set type.

benashz · 2022-06-09T15:44:46Z

vault/pki.go

@@ -0,0 +1,79 @@
+package vault


It would be great if we could make this the beginning of a new package under internal/.

Can do. I was mostly following the pattern I saw of other utilities, e.g., gcp.go

benashz · 2022-06-09T16:10:51Z

website/docs/r/pki_secret_backend_role.html.md

@@ -112,7 +112,14 @@ The following arguments are supported:

 * `require_cn` - (Optional) Flag to force CN usage

-* `policy_identifiers` - (Optional) Specify the list of allowed policies IODs
+* `policy_identifiers` - (Optional) Specify the list of allowed policies OIDs; Deprecated: use `policy_identifier` blocks instead


I don't believe we have deprecated this field yet. We probably just want to say that it is used for Vault 1.10 and below, or something like that.

benashz · 2022-06-09T16:13:39Z

vault/resource_pki_secret_backend_role_test.go

@@ -12,6 +12,17 @@ import (
 	"github.com/hashicorp/terraform-provider-vault/testutil"
 )

+var legacyPolicyIdentifiers = `policy_identifiers = ["1.2.3.4"]`


Since the vault project is already so massive, I'd suggest that we either move theses vars within the test's scope, or include test in their names.

vinay-gopalan · 2022-06-09T21:27:57Z

internal/pki.go

@@ -0,0 +1,64 @@
+package internal


I think it might be better to instead make this live under internal/pki and call it something like package pki. We did something similar for packages like internal/consts and internal/identity/entity. Importing might make things more explictly clear as well with things like pki.ReadPolicyIdentifierBlocks

👍 makes sense

benashz

Nice update! I have a few more comments/suggestions.

benashz · 2022-06-09T19:20:03Z

internal/pki.go

@@ -0,0 +1,64 @@
+package internal


oh, I was thinking more of moving this code to a package called pki under internal/, sorry if I wasn't clear.

benashz · 2022-06-09T19:27:02Z

vault/resource_pki_secret_backend_role_test.go

+						resource.TestCheckTypeSetElemNestedAttrs(resourceName, "policy_identifier.*", map[string]string{"oid": "1.2.3.4.5.6"}),
+					)...,
+				),
+			},


It would be good to add an import TestStep after each Config step. This will ensure that a terraform import will work.

Similar to:

terraform-provider-vault/vault/resource_database_secrets_mount_test.go

Lines 64 to 69 in 187d3e4

{

ResourceName: resourcePath,

ImportState: true,

ImportStateVerify: true,

ImportStateVerifyIgnore: importIgnoreKeys,

},

vault/resource_pki_secret_backend_role_test.go

benashz · 2022-06-09T21:42:53Z

vault/resource_pki_secret_backend_role.go

-	policyIdentifiers := make([]string, 0, len(iPolicyIdentifiers))
-	for _, iIdentifier := range iPolicyIdentifiers {
-		policyIdentifiers = append(policyIdentifiers, iIdentifier.(string))
+	legacyPolicyIdentifiers, newPolicyIdentifiers, err := internal.MakePkiPolicyIdentifiersListOrSet(secret.Data["policy_identifiers"].([]interface{}))


I think this might panic here on the type assertion in the case of a value other than []interface{}. We probably want to call MakePkiPolicyIdentifiersListOrSet() conditionally when policy_identifiers in the resp.Data, and guard against the type assert panic.

benashz · 2022-06-09T21:48:44Z

vault/resource_pki_secret_backend_role.go

@@ -413,8 +439,10 @@ func pkiSecretBackendRoleCreate(d *schema.ResourceData, meta interface{}) error
 		data["ext_key_usage"] = extKeyUsage
 	}

-	if len(policyIdentifiers) > 0 {
-		data["policy_identifiers"] = policyIdentifiers
+	if len(policyIdentifiersList) > 0 {


I think calling d.GetOk() on each field would probably do the same, and then we could avoid the extra variable assignments above.

benashz · 2022-06-09T21:56:22Z

vault/resource_pki_secret_backend_role.go

+	policyIdentifiersList := d.Get("policy_identifiers").([]interface{})
+	policyIdentifierBlocks := internal.ReadPolicyIdentifierBlocks(d.Get("policy_identifier").(*schema.Set))


We can probably drop these with the suggestion below.

Suggested change

policyIdentifiersList := d.Get("policy_identifiers").([]interface{})

policyIdentifierBlocks := internal.ReadPolicyIdentifierBlocks(d.Get("policy_identifier").(*schema.Set))

benashz · 2022-06-09T22:03:07Z

vault/resource_pki_secret_backend_role.go

+	if len(policyIdentifiersList) > 0 {
+		data["policy_identifiers"] = policyIdentifiersList
+	} else if policyIdentifierBlocks != "" {
+		data["policy_identifiers"] = policyIdentifierBlocks


Same suggestion as in the Read() method. Ideally we would merge the two and rely on d.HasChange(), but I think that is probably out of scope for this PR.

Suggested change

if len(policyIdentifiersList) > 0 {

data["policy_identifiers"] = policyIdentifiersList

} else if policyIdentifierBlocks != "" {

data["policy_identifiers"] = policyIdentifierBlocks

if v, ok := d.GetOk("policy_identifiers"); ok {

data["policy_identifiers"] = v

} else if v, ok := d.GetOk("policy_identifier"); ok {

data["policy_identifiers"] = internal.ReadPolicyIdentifierBlocks(v.(*schema.Set))

}

benashz · 2022-06-09T22:03:29Z

vault/resource_pki_secret_backend_role.go

+	policyIdentifiersList := d.Get("policy_identifiers").([]interface{})
+	policyIdentifierBlocks := internal.ReadPolicyIdentifierBlocks(d.Get("policy_identifier").(*schema.Set))


Suggested change

policyIdentifiersList := d.Get("policy_identifiers").([]interface{})

policyIdentifierBlocks := internal.ReadPolicyIdentifierBlocks(d.Get("policy_identifier").(*schema.Set))

benashz · 2022-06-09T22:05:26Z

website/docs/r/pki_secret_backend_role.html.md

-* `policy_identifiers` - (Optional) Specify the list of allowed policies IODs
+* `policy_identifiers` - (Optional) Specify the list of allowed policies OIDs. Use with Vault 1.10 or before. For Vault 1.11+, use `policy_identifier` blocks instead
+
+* `policy_identifier` - (Optional) (Vault 1.11+ only) A block for specifying policy identifers. The `policy_identifier` block can be repeated, and supports the following arguments:


It might be worth adding a new example for this new feature.

benashz · 2022-06-09T22:10:04Z

vault/resource_pki_secret_backend_role_test.go

+					)...,
+				),
+			},
+		},


We might want test that the two fields are mutually exclusive by adding a ExpectError TestStep similar to:

terraform-provider-vault/vault/data_source_gcp_auth_backend_role_test.go

Lines 99 to 104 in 70a1872

{

Config: testAccGCPAuthBackendRoleDataSourceConfig(backend, name),

ExpectError: regexp.MustCompile(

fmt.Sprintf("role not found at %q", gcpRoleResourcePath(backend, name)),

),

},

Ah, perfect. I had wanted to do that, but didn't quite know how. That helps.

benashz · 2022-06-10T00:36:52Z

vault/resource_pki_secret_backend_role.go

@@ -9,7 +9,7 @@ import (

 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
-
+	"github.com/hashicorp/terraform-provider-vault/internal/pki"


benashz

LGTM!

swenson · 2022-06-10T20:59:24Z

Thanks for all of the helpful feedback!

…1495) PKI: Add support for CPS URL in custom policy identifiers Update the `vault_resource_pki_secret_backend_role` to support specifying the CPS URL when specifying policy identifiers in line with the recent changes to the PKI Secrets Engine in Vault 1.11: hashicorp/vault#15751 We do this by deprecating the existing `policy_identifiers` argument and creating a new block, `policy_identifier`, which can be specified multiple times. If both `policy_identifiers` and `policy_identifier` blocks are present, then `policy_identifier` is ignored. (Otherwise, refreshing would delete one or the other, and the state wouldn't have round trip stability.) This was also tested locally with, for example, a terraform file like: ```hcl provider "vault" { } resource "vault_mount" "pki" { path = "pki" type = "pki" default_lease_ttl_seconds = 3600 max_lease_ttl_seconds = 86400 } resource "vault_pki_secret_backend_role" "role" { name = "example-dot-com" backend = vault_mount.pki.path allowed_domains = ["example.com"] allow_subdomains = true allow_bare_domains = true allow_glob_domains = true allow_ip_sans = true allow_localhost = "true" generate_lease = true organization = ["Hashi test"] country = ["USA"] locality = ["Area 51"] province = ["NV"] max_ttl = "720h" policy_identifiers = ["2.5.29.32","1.2.3"] // or policy_identifier { oid = "2.5.29.32" cps = "https://example.com/cps" notice = "Some notice" } policy_identifier { oid = "1.2.3" } } ```

swenson requested review from benashz and vinay-gopalan June 9, 2022 00:52

github-actions bot added documentation size/L labels Jun 9, 2022

vinay-gopalan reviewed Jun 9, 2022

View reviewed changes

vault/resource_pki_secret_backend_role.go Show resolved Hide resolved

vault/resource_pki_secret_backend_role_test.go Outdated Show resolved Hide resolved

benashz reviewed Jun 9, 2022

View reviewed changes

Address review comments

99d3530

vinay-gopalan reviewed Jun 9, 2022

View reviewed changes

Rename internal/pki.go to internal/pki/policy_identifier.go

cfd52c9

benashz reviewed Jun 9, 2022

View reviewed changes

Address review comments

5c4d3a7

github-actions bot added size/XL and removed size/L labels Jun 9, 2022

benashz reviewed Jun 10, 2022

View reviewed changes

benashz approved these changes Jun 10, 2022

View reviewed changes

swenson merged commit 85ebb88 into release/vault-next Jun 10, 2022

swenson deleted the vault-5965/pki-cps-url branch June 10, 2022 20:59

benashz added this to the 3.8.0 milestone Jun 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PKI: Add support for CPS URL in custom policy identifiers #1495

PKI: Add support for CPS URL in custom policy identifiers #1495

swenson commented Jun 9, 2022

vinay-gopalan left a comment

benashz left a comment

benashz Jun 9, 2022

swenson Jun 9, 2022

benashz Jun 9, 2022

swenson Jun 9, 2022

benashz Jun 9, 2022

swenson Jun 9, 2022

benashz Jun 9, 2022

swenson Jun 9, 2022

benashz Jun 9, 2022

swenson Jun 9, 2022

vinay-gopalan Jun 9, 2022

swenson Jun 9, 2022

benashz left a comment

benashz Jun 9, 2022

benashz Jun 9, 2022

swenson Jun 9, 2022

benashz Jun 9, 2022

swenson Jun 9, 2022

benashz Jun 9, 2022

benashz Jun 9, 2022

benashz Jun 9, 2022

benashz Jun 9, 2022

benashz Jun 9, 2022

swenson Jun 9, 2022

benashz Jun 9, 2022

swenson Jun 9, 2022

benashz Jun 10, 2022

benashz left a comment

swenson commented Jun 10, 2022

	{
	ResourceName: resourcePath,
	ImportState: true,
	ImportStateVerify: true,
	ImportStateVerifyIgnore: importIgnoreKeys,
	},

		policyIdentifiersList := d.Get("policy_identifiers").([]interface{})
		policyIdentifierBlocks := internal.ReadPolicyIdentifierBlocks(d.Get("policy_identifier").(*schema.Set))

	{
	Config: testAccGCPAuthBackendRoleDataSourceConfig(backend, name),
	ExpectError: regexp.MustCompile(
	fmt.Sprintf("role not found at %q", gcpRoleResourcePath(backend, name)),
	),
	},

PKI: Add support for CPS URL in custom policy identifiers #1495

PKI: Add support for CPS URL in custom policy identifiers #1495

Conversation

swenson commented Jun 9, 2022

Community Note

vinay-gopalan left a comment

Choose a reason for hiding this comment

benashz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benashz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benashz left a comment

Choose a reason for hiding this comment

swenson commented Jun 10, 2022