Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add audit non HMAC'd request / response keys to mount resource #1194

Closed
wants to merge 1 commit into from

Conversation

npurdy-tyro
Copy link
Contributor

@npurdy-tyro npurdy-tyro commented Oct 14, 2021

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

Closes #667

Release note for CHANGELOG:

`resource/vault_mount`: Added support for `audit_non_hmac_request_keys` and `audit_non_hmac_response_keys` when mounting secret backends ([#667](https://github.com/hashicorp/terraform-provider-vault/issues/667))

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestResourceMount'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./...) -v -run=TestResourceMount -timeout 20m
?       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/cmd/generate      [no test files]
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-vault/codegen   (cached) [no tests to run]
?       github.com/hashicorp/terraform-provider-vault/generated [no test files]
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-vault/generated/datasources/transform/decode    (cached) [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-vault/generated/datasources/transform/encode    (cached) [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-vault/generated/resources/transform/alphabet    (cached) [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-vault/generated/resources/transform/role        (cached) [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-vault/generated/resources/transform/template    (cached) [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-vault/generated/resources/transform/transformation      (cached) [no tests to run]
?       github.com/hashicorp/terraform-provider-vault/helper    [no test files]
?       github.com/hashicorp/terraform-provider-vault/schema    [no test files]
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-vault/util      (cached) [no tests to run]
=== RUN   TestResourceMount
--- PASS: TestResourceMount (1.57s)
=== RUN   TestResourceMount_Local
--- PASS: TestResourceMount_Local (1.61s)
=== RUN   TestResourceMount_SealWrap
--- PASS: TestResourceMount_SealWrap (1.68s)
=== RUN   TestResourceMount_AuditNonHMACRequestKeys
--- PASS: TestResourceMount_AuditNonHMACRequestKeys (1.59s)
=== RUN   TestResourceMount_KVV2
--- PASS: TestResourceMount_KVV2 (1.66s)
=== RUN   TestResourceMount_ExternalEntropyAccess
--- PASS: TestResourceMount_ExternalEntropyAccess (3.51s)
PASS
ok      github.com/hashicorp/terraform-provider-vault/vault     (cached)

@hashicorp-cla
Copy link

hashicorp-cla commented Oct 14, 2021

CLA assistant check
All committers have signed the CLA.

@dovys
Copy link

dovys commented Nov 19, 2021

Hey, thanks for the contribution! Would really love to see this merged as we're blocked by this at @monzo too

@benashz benashz added this to the 3.1.0 milestone Dec 7, 2021
@benashz benashz self-requested a review December 16, 2021 20:35
@benashz benashz modified the milestones: 3.1.0, 3.2.0 Dec 21, 2021
Copy link
Contributor

@benashz benashz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. I made some suggestions for the tests. There was some code duplication in the tests, we also want to verify the state matches.

Comment on lines +575 to +604
func testResourceMount_UpdateAuditNonHMACRequestKeys(s *terraform.State) error {
resourceState := s.Modules[0].Resources["vault_mount.test"]
instanceState := resourceState.Primary

path := instanceState.ID

if path != instanceState.Attributes["path"] {
return fmt.Errorf("id doesn't match path")
}

if path != "remountingExample" {
return fmt.Errorf("unexpected path value")
}

mount, err := findMount(path)
if err != nil {
return fmt.Errorf("error reading back mount: %s", err)
}

if len(mount.Config.AuditNonHMACRequestKeys) < 2 || mount.Config.AuditNonHMACRequestKeys[0] != "test3request" || mount.Config.AuditNonHMACRequestKeys[1] != "test4request" {
return fmt.Errorf("audit_non_hmac_request_keys is %v; expected [\"test3request\", \"test4request\"]", mount.Config.AuditNonHMACRequestKeys)
}

if len(mount.Config.AuditNonHMACResponseKeys) < 2 || mount.Config.AuditNonHMACResponseKeys[0] != "test3response" || mount.Config.AuditNonHMACResponseKeys[1] != "test4response" {
return fmt.Errorf("audit_non_hmac_response_keys is %v; expected [\"test3response\", \"test4response\"]", mount.Config.AuditNonHMACRequestKeys)
}

return nil
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use testResourceMount_CheckAuditNonHMACRequestKeys() instead

Suggested change
func testResourceMount_UpdateAuditNonHMACRequestKeys(s *terraform.State) error {
resourceState := s.Modules[0].Resources["vault_mount.test"]
instanceState := resourceState.Primary
path := instanceState.ID
if path != instanceState.Attributes["path"] {
return fmt.Errorf("id doesn't match path")
}
if path != "remountingExample" {
return fmt.Errorf("unexpected path value")
}
mount, err := findMount(path)
if err != nil {
return fmt.Errorf("error reading back mount: %s", err)
}
if len(mount.Config.AuditNonHMACRequestKeys) < 2 || mount.Config.AuditNonHMACRequestKeys[0] != "test3request" || mount.Config.AuditNonHMACRequestKeys[1] != "test4request" {
return fmt.Errorf("audit_non_hmac_request_keys is %v; expected [\"test3request\", \"test4request\"]", mount.Config.AuditNonHMACRequestKeys)
}
if len(mount.Config.AuditNonHMACResponseKeys) < 2 || mount.Config.AuditNonHMACResponseKeys[0] != "test3response" || mount.Config.AuditNonHMACResponseKeys[1] != "test4response" {
return fmt.Errorf("audit_non_hmac_response_keys is %v; expected [\"test3response\", \"test4response\"]", mount.Config.AuditNonHMACRequestKeys)
}
return nil
}

`, path)
}

func testResourceMount_InitialCheckAuditNonHMACRequestKeys(expectedPath string) resource.TestCheckFunc {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func testResourceMount_InitialCheckAuditNonHMACRequestKeys(expectedPath string) resource.TestCheckFunc {
func testResourceMount_CheckAuditNonHMACRequestKeys(expectedPath string, expectedReqKeys, expectedRespKeys []string) resource.TestCheckFunc {

Steps: []resource.TestStep{
{
Config: testResourceMount_InitialConfigAuditNonHMACRequestKeys(path),
Check: testResourceMount_InitialCheckAuditNonHMACRequestKeys(path),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest checking the state attributes as well.

Suggested change
Check: testResourceMount_InitialCheckAuditNonHMACRequestKeys(path),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr("vault_mount.test", "path", path),
resource.TestCheckResourceAttr("vault_mount.test", "audit_non_hmac_request_keys.#", "2"),
resource.TestCheckResourceAttr("vault_mount.test", "audit_non_hmac_request_keys.0", "test1request"),
resource.TestCheckResourceAttr("vault_mount.test", "audit_non_hmac_request_keys.1", "test2request"),
resource.TestCheckResourceAttr("vault_mount.test", "audit_non_hmac_response_keys.#", "2"),
resource.TestCheckResourceAttr("vault_mount.test", "audit_non_hmac_response_keys.0", "test1response"),
resource.TestCheckResourceAttr("vault_mount.test", "audit_non_hmac_response_keys.1", "test2response"),
testResourceMount_CheckAuditNonHMACRequestKeys(
path,
[]string{"test1request", "test2request"},
[]string{"test1response", "test2response"}),
),

},
{
Config: testResourceMount_UpdateConfigAuditNonHMACRequestKeys,
Check: testResourceMount_UpdateAuditNonHMACRequestKeys,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Check: testResourceMount_UpdateAuditNonHMACRequestKeys,
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr("vault_mount.test", "path", "remountingExample"),
resource.TestCheckResourceAttr("vault_mount.test", "audit_non_hmac_request_keys.#", "2"),
resource.TestCheckResourceAttr("vault_mount.test", "audit_non_hmac_request_keys.0", "test3request"),
resource.TestCheckResourceAttr("vault_mount.test", "audit_non_hmac_request_keys.1", "test4request"),
resource.TestCheckResourceAttr("vault_mount.test", "audit_non_hmac_response_keys.#", "2"),
resource.TestCheckResourceAttr("vault_mount.test", "audit_non_hmac_response_keys.0", "test3response"),
resource.TestCheckResourceAttr("vault_mount.test", "audit_non_hmac_response_keys.1", "test4response"),
testResourceMount_CheckAuditNonHMACRequestKeys(
"remountingExample",
[]string{"test3request", "test4request"},
[]string{"test3response", "test4response"}),
),

path := "example-" + acctest.RandString(10)
resource.Test(t, resource.TestCase{
Providers: testProviders,
PreCheck: func() { testAccPreCheck(t) },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

testAccPreCheck() was just replaced by testutil.TestAccPreCheck() in 5660a05, you'll want to rebase.

Suggested change
PreCheck: func() { testAccPreCheck(t) },
PreCheck: func() { testutil.TestAccPreCheck(t) },

Comment on lines +546 to +551
if len(mount.Config.AuditNonHMACRequestKeys) < 2 || mount.Config.AuditNonHMACRequestKeys[0] != "test1request" || mount.Config.AuditNonHMACRequestKeys[1] != "test2request" {
return fmt.Errorf("audit_non_hmac_request_keys is %v; expected [\"test1request\", \"test2request\"]", mount.Config.AuditNonHMACRequestKeys)
}

if len(mount.Config.AuditNonHMACResponseKeys) < 2 || mount.Config.AuditNonHMACResponseKeys[0] != "test1response" || mount.Config.AuditNonHMACResponseKeys[1] != "test2response" {
return fmt.Errorf("audit_non_hmac_response_keys is %v; expected [\"test1response\", \"test2response\"]", mount.Config.AuditNonHMACRequestKeys)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if len(mount.Config.AuditNonHMACRequestKeys) < 2 || mount.Config.AuditNonHMACRequestKeys[0] != "test1request" || mount.Config.AuditNonHMACRequestKeys[1] != "test2request" {
return fmt.Errorf("audit_non_hmac_request_keys is %v; expected [\"test1request\", \"test2request\"]", mount.Config.AuditNonHMACRequestKeys)
}
if len(mount.Config.AuditNonHMACResponseKeys) < 2 || mount.Config.AuditNonHMACResponseKeys[0] != "test1response" || mount.Config.AuditNonHMACResponseKeys[1] != "test2response" {
return fmt.Errorf("audit_non_hmac_response_keys is %v; expected [\"test1response\", \"test2response\"]", mount.Config.AuditNonHMACRequestKeys)
if !reflect.DeepEqual(expectedReqKeys, mount.Config.AuditNonHMACRequestKeys) {
return fmt.Errorf("expected audit_non_hmac_request_keys %#v, actual %#v",
expectedReqKeys,
mount.Config.AuditNonHMACRequestKeys)
}
if !reflect.DeepEqual(expectedRespKeys, mount.Config.AuditNonHMACResponseKeys) {
return fmt.Errorf("expected audit_non_hmac_response_keys %#v, actual %#v",
expectedRespKeys,
mount.Config.AuditNonHMACResponseKeys)

@benashz
Copy link
Contributor

benashz commented Jan 11, 2022

Moving this work to #1297.

Thank you @npurdy-tyro for your contribution. We will make sure you recognized as a co-author!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request - have vault_mount resource support audit_non_hmac_response_keys
4 participants