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 check for 403 during rollback to prevent deletion spam #97

Merged
merged 1 commit into from
Aug 28, 2020

Conversation

Valarissa
Copy link
Contributor

@Valarissa Valarissa commented Aug 28, 2020

Overview

This change addresses an issue where the client will continue to attempt a deletion rollback when the create fails with a 403. A check was implemented in the rollback behavior to check for a 403 response. When a 403 is observed, the WAL is removed to prevent repeated attempts at rollback of a non-existent entity.

Test Plan

  1. Create a GCP service account with weak permissions
  2. Enable GCP secrets engine with: vault secrets enable gcp
  3. Run the following to attempt to create a roleset (which should fail with a 403):
vault write gcp/roleset/my-token-roleset project="my-project" secret_type="access_token"  token_scopes="https://www.googleapis.com/auth/cloud-platform" \
bindings=-<<EOF
resource "//cloudresourcemanager.googleapis.com/projects/my-project" {
  roles = ["roles/viewer"]
}
EOF
  1. Wait a few minutes
  2. Head to the GCP API overview page and check your traffic to see that only one google.iam.admin.v1.IAM.DeleteServiceAccount call comes through

Contributor Checklist

[x] Add output for any tests not ran in CI to the PR description (eg, acceptance tests)

GOROOT=/usr/local/go #gosetup
GOPATH=/Users/lvoswinkel/go #gosetup
/usr/local/go/bin/go test -json ./...
=== RUN   TestParseBindings
--- PASS: TestParseBindings (0.00s)
=== RUN   TestParseBindingsB64
--- PASS: TestParseBindingsB64 (0.00s)
PASS
ok  	github.com/hashicorp/vault-plugin-secrets-gcp/plugin/util	(cached)
=== RUN   TestIamResource_ServiceAccount
--- PASS: TestIamResource_ServiceAccount (1.56s)
=== RUN   TestPolicyToDataset
--- PASS: TestPolicyToDataset (0.00s)
=== RUN   TestDatasetToPolicy
--- PASS: TestDatasetToPolicy (0.00s)
=== RUN   TestDatasetResource
--- PASS: TestDatasetResource (0.00s)
=== RUN   TestConditionalDatasetResource
--- PASS: TestConditionalDatasetResource (0.00s)
=== RUN   TestIamResource
--- PASS: TestIamResource (0.00s)
=== RUN   TestConditionalIamResource
--- PASS: TestConditionalIamResource (0.00s)
=== RUN   TestEnabledIamResources_RelativeName
--- PASS: TestEnabledIamResources_RelativeName (0.00s)
=== RUN   TestEnabledIamResources_FullName
--- PASS: TestEnabledIamResources_FullName (0.01s)
=== RUN   TestEnabledIamResources_SelfLink
--- PASS: TestEnabledIamResources_SelfLink (0.01s)
=== RUN   TestIamEnabledResources_ValidateGeneratedConfig
--- PASS: TestIamEnabledResources_ValidateGeneratedConfig (0.00s)
PASS
ok  	github.com/hashicorp/vault-plugin-secrets-gcp/plugin/iamutil	1.816s
=== RUN   TestConfigRotateRootUpdate
=== PAUSE TestConfigRotateRootUpdate
=== CONT  TestConfigRotateRootUpdate
--- PASS: TestConfigRotateRootUpdate (0.00s)
=== RUN   TestConfigRotateRootUpdate/no_configuration
=== PAUSE TestConfigRotateRootUpdate/no_configuration
=== CONT  TestConfigRotateRootUpdate/no_configuration
    --- PASS: TestConfigRotateRootUpdate/no_configuration (0.00s)
=== RUN   TestConfigRotateRootUpdate/config_with_no_credentials
=== PAUSE TestConfigRotateRootUpdate/config_with_no_credentials
=== CONT  TestConfigRotateRootUpdate/config_with_no_credentials
    --- PASS: TestConfigRotateRootUpdate/config_with_no_credentials (0.00s)
=== RUN   TestConfigRotateRootUpdate/config_with_invalid_credentials
=== PAUSE TestConfigRotateRootUpdate/config_with_invalid_credentials
=== CONT  TestConfigRotateRootUpdate/config_with_invalid_credentials
    --- PASS: TestConfigRotateRootUpdate/config_with_invalid_credentials (0.00s)
=== RUN   TestConfigRotateRootUpdate/rotate
=== PAUSE TestConfigRotateRootUpdate/rotate
=== CONT  TestConfigRotateRootUpdate/rotate
    --- PASS: TestConfigRotateRootUpdate/rotate (3.58s)
=== RUN   TestConfig
=== PAUSE TestConfig
=== CONT  TestConfig
--- PASS: TestConfig (0.00s)
=== RUN   TestPathRoleSet_Basic
    path_role_set_test.go:788: [WARNING] found test service account projects/lvoswinkel-test/serviceAccounts/vaulttest-basicrs-1598483254@lvoswinkel-test.iam.gserviceaccount.com that should have been deleted, did test fail? Manually deleting...
--- PASS: TestPathRoleSet_Basic (5.08s)
=== RUN   TestPathRoleSet_UpdateKeyRoleSet
--- PASS: TestPathRoleSet_UpdateKeyRoleSet (7.10s)
=== RUN   TestPathRoleSet_RotateKeyRoleSet
    path_role_set_test.go:788: [WARNING] found test service account projects/lvoswinkel-test/serviceAccounts/vaulttest-rotatekey-1598483257@lvoswinkel-test.iam.gserviceaccount.com that should have been deleted, did test fail? Manually deleting...
--- PASS: TestPathRoleSet_RotateKeyRoleSet (9.40s)
=== RUN   TestPathRoleSet_UpdateTokenRoleSetScopes
    path_role_set_test.go:788: [WARNING] found test service account projects/lvoswinkel-test/serviceAccounts/vaulttest-updatetok-1598483259@lvoswinkel-test.iam.gserviceaccount.com that should have been deleted, did test fail? Manually deleting...
--- PASS: TestPathRoleSet_UpdateTokenRoleSetScopes (7.54s)
=== RUN   TestPathRoleSet_UpdateTokenRoleSet
    path_role_set_test.go:788: [WARNING] found test service account projects/lvoswinkel-test/serviceAccounts/vaulttest-updatetok-1598483260@lvoswinkel-test.iam.gserviceaccount.com that should have been deleted, did test fail? Manually deleting...
--- PASS: TestPathRoleSet_UpdateTokenRoleSet (14.44s)
=== RUN   TestPathRoleSet_RotateTokenRoleSet
    path_role_set_test.go:788: [WARNING] found test service account projects/lvoswinkel-test/serviceAccounts/vaulttest-rotatetok-1598483362@lvoswinkel-test.iam.gserviceaccount.com that should have been deleted, did test fail? Manually deleting...
    path_role_set_test.go:791: [WARNING] Disregard previous warning - manual delete returned 404, probably IAM eventual consistency
    path_role_set_test.go:788: [WARNING] found test service account projects/lvoswinkel-test/serviceAccounts/vaulttest-rotatetok-1598483366@lvoswinkel-test.iam.gserviceaccount.com that should have been deleted, did test fail? Manually deleting...
    path_role_set_test.go:791: [WARNING] Disregard previous warning - manual delete returned 404, probably IAM eventual consistency
--- PASS: TestPathRoleSet_RotateTokenRoleSet (14.46s)
=== RUN   TestSecrets_GenerateAccessToken
--- PASS: TestSecrets_GenerateAccessToken (10.26s)
=== RUN   TestSecrets_GenerateKeyConfigTTL
    path_role_set_test.go:788: [WARNING] found test service account projects/lvoswinkel-test/serviceAccounts/vaulttest-genkey-1598483268@lvoswinkel-test.iam.gserviceaccount.com that should have been deleted, did test fail? Manually deleting...
    path_role_set_test.go:788: [WARNING] found test service account projects/lvoswinkel-test/serviceAccounts/vaulttest-genkey-1598483266@lvoswinkel-test.iam.gserviceaccount.com that should have been deleted, did test fail? Manually deleting...
    path_role_set_test.go:788: [WARNING] found test service account projects/lvoswinkel-test/serviceAccounts/vaulttest-genkey-1598483265@lvoswinkel-test.iam.gserviceaccount.com that should have been deleted, did test fail? Manually deleting...
    path_role_set_test.go:788: [WARNING] found test service account projects/lvoswinkel-test/serviceAccounts/vaulttest-genkey-1598483387@lvoswinkel-test.iam.gserviceaccount.com that should have been deleted, did test fail? Manually deleting...
    path_role_set_test.go:791: [WARNING] Disregard previous warning - manual delete returned 404, probably IAM eventual consistency
--- PASS: TestSecrets_GenerateKeyConfigTTL (13.80s)
=== RUN   TestSecrets_GenerateKeyTTLOverride
--- PASS: TestSecrets_GenerateKeyTTLOverride (7.70s)
=== RUN   TestSecrets_GenerateKeyMaxTTLCheck
--- PASS: TestSecrets_GenerateKeyMaxTTLCheck (10.00s)
PASS
ok  	github.com/hashicorp/vault-plugin-secrets-gcp/plugin	103.674s
?   	github.com/hashicorp/vault-plugin-secrets-gcp/plugin/iamutil/internal	[no test files]

Process finished with exit code 0

@Valarissa Valarissa changed the title Add check for 403 during rollback to prevent spam Add check for 403 during rollback to prevent deletion spam Aug 28, 2020
@Valarissa Valarissa merged commit 42e2f83 into master Aug 28, 2020
@kalafut kalafut deleted the address_403_during_rollback branch September 15, 2020 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants